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.

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

Ignore bookmark icon url when browser gets bookmark form sync cloud #4420

Merged
merged 1 commit into from
Jan 27, 2020

Conversation

AlexeyBarabash
Copy link
Contributor

Intended t o fix brave/brave-browser#7893

Submitter Checklist:

Test Plan:

See steps in brave/brave-browser#7893

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.

@AlexeyBarabash
Copy link
Contributor Author

Some details.
Seeing

[9788:9788:0124/132232.884549:ERROR:bookmark_specifics_conversions.cc(295)] Invalid bookmark: specifics cannot have an icon_url without having a favicon.

which get us into https://source.chromium.org/chromium/chromium/src/+/79.0.3945.130:components/sync_bookmarks/bookmark_specifics_conversions.cc;l=295

@AlexeyBarabash AlexeyBarabash marked this pull request as ready for review January 24, 2020 18:51
@AlexeyBarabash AlexeyBarabash changed the title Ignore bookmark url when browser gets bookmark form sync cloud Ignore bookmark шсщт url when browser gets bookmark form sync cloud Jan 24, 2020
@AlexeyBarabash AlexeyBarabash changed the title Ignore bookmark шсщт url when browser gets bookmark form sync cloud Ignore bookmark icon url when browser gets bookmark form sync cloud Jan 24, 2020
@AlexeyBarabash
Copy link
Contributor Author

Chromium sends both icon_url and icon_data in the same time and expects to have them either both or either none of them.

Here is a place where Chromium creates BookmarkSpecifics with icon data.

sync_pb::EntitySpecifics CreateSpecificsFromBookmarkNode(
    const bookmarks::BookmarkNode* node,
    bookmarks::BookmarkModel* model,
    bool force_favicon_load)

https://source.chromium.org/chromium/chromium/src/+/79.0.3945.130:components/sync_bookmarks/bookmark_specifics_conversions.cc;bpv=1;bpt=1;l=134

force_favicon_load is set to false only in tests.

It was a time when we put bookmark icon data into SyncRecord metadata to allow to get the boookmark instantly.
We reverted that at the that time because:

  1. S3 sync records for bookmarks with icons became too big as for a simple sync record;
  2. There were a cases when client tried to sending update record and that mixed with newly came sync record making mess;
  3. Mobile clients in anyway does not send icon bitmap, as it is not in our spec.

I marking this PR as ready for review to allow issue be fixed asap. For the matter of icon, I creating a new issue just found brave/brave-browser#4095 (comment), we are good to load icon once the page will be visited.

@bsclifton
Copy link
Member

Merging as this is required to unblock some items for 1.3. CI has passed and PR has been peer reviewed 😄👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some bookmarks are not synced in 1.5.x
3 participants