-
Notifications
You must be signed in to change notification settings - Fork 888
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.
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
Replace com.apple.ReadingList by Reading List #16784
Conversation
for (auto& item : bookmarks) { | ||
for (auto& folder : item.path) { | ||
if (folder == kSafariReadingListPath) { | ||
folder = l10n_util::GetStringUTF16(IDS_BOOKMARKS_READING_LIST_GROUP); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: don't need return;
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im not sure that we can have only one path
#include "src/chrome/utility/importer/safari_importer.mm" | ||
#undef IDS_BOOKMARK_GROUP_FROM_SAFARI | ||
#define IDS_BOOKMARK_GROUP_FROM_SAFARI GetBookmarkGroupFromSafariID() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need this define here? I think it doesn't have any meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src
reviewers require to restore definitions state usually
@@ -22,6 +22,9 @@ | |||
<message name="IDS_BOOKMARK_GROUP" desc="The group name of bookmarks imported from a file"> | |||
Imported | |||
</message> | |||
<message name="IDS_BOOKMARKS_READING_LIST_GROUP" desc="The title of reading list folder in imported bookmarks."> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<!-- Bookmarks specific strings (included from generated_resources.grd). -->
<!--This file is created by l10nUtil.js. Do not edit manually.-->
Resolves brave/brave-browser#27738
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run lint
,npm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: