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 bookmark manager empty list string #4356

Merged
merged 1 commit into from Jan 16, 2020
Merged

Replace bookmark manager empty list string #4356

merged 1 commit into from Jan 16, 2020

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Jan 10, 2020 •
edited

fix brave/brave-browser#2494

Submitter Checklist:

Screen Shot 2020-01-10 at 10 50 49 AM

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 added this to the 1.5.x - Nightly milestone Jan 10, 2020
@simonhong simonhong self-assigned this Jan 10, 2020
simonhong
simonhong commented Jan 10, 2020
View reviewed changes
patches/chrome-browser-ui-webui-bookmarks-bookmarks_ui.cc.patch Outdated
content::WebUIDataSource::Create(chrome::kChromeUIBookmarksHost);
@@ -174,6 +174,7 @@ content::WebUIDataSource* CreateBookmarksUIHTMLSource(Profile* profile) {

source->UseStringsJs();

+ BraveCustomizeBookmarksDataSource(source);
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved BraveCustomizeBookmarksDataSource(source) to the last of CreateBookmarksUIHTMLSource() to get a chance to replace already added strings.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

can you convert this to a define since you're changing it anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@simonhong
Copy link
Member Author

Only unrelated steps (network-audit and test-install) were failed.

petemill
petemill approved these changes Jan 14, 2020
View reviewed changes
Copy link
Member

@petemill petemill left a comment •
edited

Choose a reason for hiding this comment

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

Nice and clean 👍. Failed build result seems unrelated (network audit fail and some error starting)

@simonhong
Copy link
Member Author

@bridiver ping for owners review :)

@simonhong simonhong force-pushed the update_empty_bookmark_list_sentence branch 3 times, most recently from 2c943d7 to 7eade77 Compare January 15, 2020 22:38
@simonhong simonhong force-pushed the update_empty_bookmark_list_sentence branch from 7eade77 to f2cb533 Compare January 15, 2020 23:49
@simonhong simonhong added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels Jan 16, 2020
@simonhong simonhong merged commit 6c346f1 into master Jan 16, 2020
@simonhong simonhong deleted the update_empty_bookmark_list_sentence branch January 16, 2020 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bridiver bridiver bridiver approved these changes

@petemill petemill petemill approved these changes

Assignees

@simonhong simonhong

Labels
CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS
Projects
None yet
Milestone
1.5.x - Release
3 participants
@simonhong @bridiver @petemill