brave-core icon indicating copy to clipboard operation
brave-core copied to clipboard

Refactor patch for optimize_webui.py build script to use brave-specific file

Open petemill opened this issue 6 years ago • 4 comments

There are additions coming to this file to route more webui pages (such as for the chrome://history page). This refactor allows those additions to modify the brave version of this script instead of further modifying the patch to the chromium version of the script.

Fix https://github.com/brave/brave-browser/issues/5233

Submitter Checklist:

  • [x] Submitted a ticket for my issue if one did not already exist.
  • [x] Used Github auto-closing keywords in the commit message.
  • [ ] Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • [ ] Android
    • [ ] iOS
    • [ ] Linux
    • [ ] macOS
    • [ ] Windows
  • Verified that these changes pass automated tests (unit, browser, security tests) on
    • [ ] iOS
    • [ ] Linux
    • [x] macOS
    • [ ] Windows
  • [x] Verified that all lint errors/warnings are resolved (npm run lint)
  • [x] Ran git rebase master (if needed).
  • [x] Ran git rebase -i to squash commits (if needed).
  • [x] Tagged reviewers and labelled the pull request as needed.
  • [ ] Request a security/privacy review as needed.
  • [x] Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone
  • [ ] Public documentation has been updated as necessary. For instance:
    • [ ] https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
    • [ ] https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
    • [ ] https://github.com/brave/brave-browser/wiki/Fingerprinting-Protection-Mode
    • [ ] https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
    • [ ] https://github.com/brave/brave-browser/wiki/Custom-Headers
    • [ ] https://github.com/brave/brave-browser/wiki/Web-compatibility-issues-with-tracking-protection
    • [ ] https://github.com/brave/brave-browser/wiki/QA-Guide

Test Plan:

  • chrome://settings still works and uses brave fonts

Reviewer Checklist:

  • [ ] New files have MPL-2.0 license header.
  • [ ] Request a security/privacy review as needed.
  • [ ] Adequate test coverage exists to prevent regressions
  • [ ] Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • [ ] The associated issue milestone is set to the smallest version that the changes has landed on.
  • [ ] All relevant documentation has been updated.

petemill avatar Jul 12 '19 17:07 petemill

@mkarolin thanks for approval - I added an addition to make sure that dependent builds re-build when this new brave-specific file is modified. I'd appreciate a re-review when you get a chance 😄

petemill avatar Jul 12 '19 21:07 petemill

Is there any reason why this PR might make it so you can't clear your history? I built this one working on my little icon side project and I can't seem to get the clear browsing data modal to appear in settings.

Edit: This might help: https://d.pr/i/YH7kSL

rossmoody avatar Jul 15 '19 21:07 rossmoody

@rossmoody Clear Browsing Data dialog got broken by the c76 bump. The fix is pending approval here: https://github.com/brave/brave-core/pull/2903/commits/4f8e2b27f553dfad670f2cd42b0653c7f752359a

mkarolin avatar Jul 15 '19 23:07 mkarolin

@petemill is this one we still want to do? Or should we close the issue / this PR?

bsclifton avatar Aug 16 '22 18:08 bsclifton

Closing as stale

bsclifton avatar Jan 31 '23 06:01 bsclifton