GitLab

As of today, BookmarkModelObserver does not distinguish two very
different events:
1. A favicon has actually changed for a bookmark.
2. A favicon for a bookmark has been loaded.

They both get reported identically and in sync's case it needs to know
whether a bookmark should be reuploaded (committed) or not. Treating all
favicon-just-loaded events as local changes, as prior to this patch, is
prone to undesired uploads that can lead to resurrected bookmarks and
other sync conflicts.

Before this patch, sync could identify some trivial cases, based on the
fact that BookmarkSpecifics (hashed) hasn't changed at all. In such
cases, BookmarkModelObserverImpl::BookmarkNodeFaviconChanged() didn't
make a distinction, but the commit was later optimized away and never
made it to the protocol.

This mechanism is notably fragile in case BookmarkSpecifics go through
code changes, such as new proto fields being introduced (unrelated to
favicons).

With this patch, the underlying design flaw is addressed and instead
the two scenarios (favicon-just-loaded vs actual favicon change) are
distinguished more reliably. A new hash is introduced in per-bookmark
sync metadata that represents the last known content of the image,
which allows determining whether the image has changed and a commit
is needed.

Change-Id: Ib88bed201b3f95be083026073a3c9422ab403b2b
Bug: 1064945
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2141985Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#757861}