Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace chrome link with brave link from "chrome://flags #5526

Merged
merged 2 commits into from
May 14, 2020
Merged

Replace chrome link with brave link from "chrome://flags #5526

merged 2 commits into from
May 14, 2020

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented May 12, 2020 •
edited
Loading

Resolves brave/brave-browser#1422

Submitter Checklist:

Test Plan:

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.

@simonhong simonhong self-assigned this May 12, 2020
@simonhong
Revert "Revert "Use brave download link and Brave brand in brave://fl…
a60358f 
…ags for download""

This reverts commit 3e277d1.
@simonhong
Use RefCountedString to return modifed flags.js resources
f7b1bc2 
new flags.js includes original flags.js and our flags.js file.
@simonhong simonhong marked this pull request as ready for review May 13, 2020 03:33
@simonhong simonhong added this to the 1.10.x - Nightly milestone May 13, 2020
simonhong
simonhong commented May 13, 2020
View reviewed changes
common/brave_content_client.cc
resource_bundle.LoadDataResourceString(
IDR_FLAGS_UI_BRAVE_FLAGS_OVERRIDES_JS);
base::RefCountedString* bytes = new base::RefCountedString();
bytes->data().assign(flags_js.data(), flags_js.length());
Copy link
Member Author

@simonhong simonhong May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes previous bug - RefCountedString owns string data(flags_js).
RefCountedStaticMemory just store data address (only useful for static data).
I think it was luck to load properly on macOS with RefCountedStaticMemory.

@bsclifton
Copy link
Member

Previously implemented in #4601 (in case folks wanted to see review or comments)

bsclifton
bsclifton approved these changes May 14, 2020
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on Windows; works great 😄

As channel is Developer, I went into devtools and made the promo element show to verify text is correct and link works as expected

@simonhong simonhong merged commit 288a91c into master May 14, 2020
@simonhong simonhong deleted the simon_flags_js branch May 14, 2020 10:52
@simonhong simonhong changed the title Remove chrome link from "chrome://flags Replace chrome link with brave link from "chrome://flags May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsclifton bsclifton bsclifton approved these changes

@bridiver bridiver Awaiting requested review from bridiver

@bbondy bbondy Awaiting requested review from bbondy

@petemill petemill Awaiting requested review from petemill

Assignees

@simonhong simonhong

Labels
None yet
Projects
None yet
Milestone
1.10.x - Release
Development

Successfully merging this pull request may close these issues.

Use brave download link and brave link from "brave://flags" for stable/beta release
2 participants
@simonhong @bsclifton