Sync the bookmark's icon URL

Sync the bookmark's icon URL.

BUG=160726
Test=TwoClientBookmarksSyncTest.*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170326

[pkotwicz, 2012-11-25 23:28:03]

Nicolas, can you please take a look?
This CL depends on https://codereview.chromium.org/11413153/

[Nicolas Zea, 2012-11-27 18:56:09]

https://codereview.chromium.org/11428004/diff/1/chrome/browser/bookmarks/book...
File chrome/browser/bookmarks/bookmark_model.cc (right):

https://codereview.chromium.org/11428004/diff/1/chrome/browser/bookmarks/book...
chrome/browser/bookmarks/bookmark_model.cc:897:
node->set_icon_url(image_result.icon_url);
similar to my other comment, is there a possibility that this will treat our
hacky "bookmark url as favicon icon" url as the actual icon url?

https://codereview.chromium.org/11428004/diff/1/chrome/browser/bookmarks/book...
File chrome/browser/bookmarks/bookmark_model.h (right):

https://codereview.chromium.org/11428004/diff/1/chrome/browser/bookmarks/book...
chrome/browser/bookmarks/bookmark_model.h:82: // Returns and the favicon's URL.
Returns an empty URL if there is no
remove and

https://codereview.chromium.org/11428004/diff/1/chrome/browser/sync/glue/book...
File chrome/browser/sync/glue/bookmark_change_processor.cc (right):

https://codereview.chromium.org/11428004/diff/1/chrome/browser/sync/glue/book...
chrome/browser/sync/glue/bookmark_change_processor.cc:760:
updated_specifics.set_icon_url(bookmark_node->icon_url().spec());
I'm a bit worried about this. Is there a possibility that we'll wind up
overwriting a valid favicon url with a legacy favicon url (i.e. one that's the
same as the bookmark url?)

https://codereview.chromium.org/11428004/diff/1/chrome/browser/sync/test/inte...
File chrome/browser/sync/test/integration/bookmarks_helper.h (right):

https://codereview.chromium.org/11428004/diff/1/chrome/browser/sync/test/inte...
chrome/browser/sync/test/integration/bookmarks_helper.h:92: // (of type
BookmarkNode::URL) in the bookmark model of profile |profile|.
move "(of type BookmarkNode::URL) to right after |icon_url|, which it refers to.

https://codereview.chromium.org/11428004/diff/1/chrome/browser/sync/test/inte...
File chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc
(right):

https://codereview.chromium.org/11428004/diff/1/chrome/browser/sync/test/inte...
chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc:174:
SetFavicon(0, bookmark0, icon_url1, CreateFavicon(SK_ColorWHITE));
are any of the favicons in this test actually HiDPI favicons?

[pkotwicz, 2012-11-27 22:50:39]

https://codereview.chromium.org/11428004/diff/1/chrome/browser/sync/glue/book...
File chrome/browser/sync/glue/bookmark_change_processor.cc (right):

https://codereview.chromium.org/11428004/diff/1/chrome/browser/sync/glue/book...
chrome/browser/sync/glue/bookmark_change_processor.cc:760:
updated_specifics.set_icon_url(bookmark_node->icon_url().spec());
I'm not sure I completely understand your concern.
The bookmark node's icon URL reflects the icon URL as per the history database.
I know for a fact that by design we overwrite whatever icon URL the history
service has with the one gotten from sync when MergeFavicon() is called.
So, if a new bookmark is created on a legacy client, the bookmark will be
created on clients that the legacy client is synced with with a fake icon URL.
I do not know how we would get into a situation where we wind up overwriting a
valid icon url with a legacy icon url.

[Nicolas Zea, 2012-11-27 23:42:18]

On 2012/11/27 22:50:39, pkotwicz wrote:
>
https://codereview.chromium.org/11428004/diff/1/chrome/browser/sync/glue/book...
> File chrome/browser/sync/glue/bookmark_change_processor.cc (right):
> 
>
https://codereview.chromium.org/11428004/diff/1/chrome/browser/sync/glue/book...
> chrome/browser/sync/glue/bookmark_change_processor.cc:760:
> updated_specifics.set_icon_url(bookmark_node->icon_url().spec());
> I'm not sure I completely understand your concern.
> The bookmark node's icon URL reflects the icon URL as per the history
database.
> I know for a fact that by design we overwrite whatever icon URL the history
> service has with the one gotten from sync when MergeFavicon() is called.
> So, if a new bookmark is created on a legacy client, the bookmark will be
> created on clients that the legacy client is synced with with a fake icon URL.
> I do not know how we would get into a situation where we wind up overwriting a
> valid icon url with a legacy icon url.

I guess I just want to understand the behavior of how this field will be filled.

On a client that upgrades (and therefore doesn't have favicon urls in sync),
when would the history service's icon url become the real url? When the user
navigates to that page and triggers a favicon refresh? Which would immediately
post the notification task that updates the bookmark model's favicon url? Does
this post the notification task even if the favicon image remained the same?

What happens if the local client has a real url, but sync receives a bookmark
node without a real url. Will sync's fake url clobber the real url?

[pkotwicz, 2012-11-28 00:01:44]

On a client that upgrades (and therefore doesn't have favicon urls in sync),
when would the history service's icon url become the real url? When the user
navigates to that page and triggers a favicon refresh? Which would immediately
post the notification task that updates the bookmark model's favicon url? Does
this post the notification task even if the favicon image remained the same?
The icon URL will become the real URL when the user navigates to the page. (No
refresh is required because the history service considers favicons which come in
from sync as out of date). When the icon URL is updated, this does post a
notification task even if the pixels are the same.

What happens if the local client has a real url, but sync receives a bookmark
node without a real url. Will sync's fake url clobber the real url?
Yes. This keeps the logic in HistoryBackend simpler.

[Nicolas Zea, 2012-11-28 00:17:34]

On 2012/11/28 00:01:44, pkotwicz wrote:
> On a client that upgrades (and therefore doesn't have favicon urls in sync),
> when would the history service's icon url become the real url? When the user
> navigates to that page and triggers a favicon refresh? Which would immediately
> post the notification task that updates the bookmark model's favicon url? Does
> this post the notification task even if the favicon image remained the same?
> The icon URL will become the real URL when the user navigates to the page. (No
> refresh is required because the history service considers favicons which come
in
> from sync as out of date). When the icon URL is updated, this does post a
> notification task even if the pixels are the same.
> 
> What happens if the local client has a real url, but sync receives a bookmark
> node without a real url. Will sync's fake url clobber the real url?
> Yes. This keeps the logic in HistoryBackend simpler.

I see. Ideally I'd prefer if we could preserve the real url (perhaps have the
MergeFavicon
method have special functionality if passed an empty icon url?). It should not
be
common for the bookmark to push a fake url, but it will happen. That doesn't
need to happen in this patch though.

[pkotwicz, 2012-11-28 01:40:53]

Added comments as requested. Sorry about the ugly diff. If you look at the diff
between revision 1 and 5 it looks much more reasonable.

[Nicolas Zea, 2012-11-28 02:00:33]

sync LGTM

https://codereview.chromium.org/11428004/diff/9005/chrome/browser/sync/test/i...
File chrome/browser/sync/test/integration/bookmarks_helper.h (right):

https://codereview.chromium.org/11428004/diff/9005/chrome/browser/sync/test/i...
chrome/browser/sync/test/integration/bookmarks_helper.h:93: // |node|'s type
should be BookmarkNode::URL.
actually, the comment in the last line doesn't really make sense. Probably
better to just remove it.

[pkotwicz, 2012-11-28 15:09:52]

Scott, can you please take a look at the bookmarks/ changes?

[sky, 2012-11-28 15:50:37]

LGTM

[commit-bot: I haz the power, 2012-11-29 22:32:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/11428004/4028

[commit-bot: I haz the power, 2012-11-30 01:24:48]

Change committed as 170326