[Sync] Add favicon datatype proto support

[Sync] Add favicon datatype proto support 

Introduces support for syncing favicons in their own datatype.

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

[Nicolas Zea, 2013-02-05 02:11:00]

+Peter. I believe this should be enough to properly handle support for multiple
high-resolution types. Whether we wind up using that approach or simply
downscaling the highest resolution is something we can adjust later. 

One question: do you think it will be useful to keep the width stored
separately? Or should we just get the width from the actual image bytes?

[pkotwicz, 2013-02-07 02:15:47]

I'm not an owner, but LGTM

https://codereview.chromium.org/12089103/diff/2001/sync/protocol/favicon_spec...
File sync/protocol/favicon_specifics.proto (right):

https://codereview.chromium.org/12089103/diff/2001/sync/protocol/favicon_spec...
sync/protocol/favicon_specifics.proto:17: // history_types.cc
Nit: history_types.h

https://codereview.chromium.org/12089103/diff/2001/sync/protocol/favicon_spec...
sync/protocol/favicon_specifics.proto:23: // A precomposed touch favicon (max
resolution xxx).
It makes sense to cap the size of both TOUCH_ICON and TOUCH_PRECOMPOSED_ICON.
As far as I know, Chrome uses TOUCH_ICON and TOUCH_PRECOMPOSED_ICON icons
interchangeably.
Differentiating between the two is useful for validating the favicons from the
history service against the favicons that are provided by the site when a user
visits the site.

https://codereview.chromium.org/12089103/diff/2001/sync/protocol/favicon_spec...
sync/protocol/favicon_specifics.proto:29: optional int64 width = 3;
We should sync both the width and height.

I think that we will want to sync the unresized favicon over sync.
If we do not, we will have to sync bitmap data resized to each of
16 * |screen_densities|
Where |screen_densities| is the set of all the supported screen densities by
Chromium. Android has way too many supported screen densities.

https://codereview.chromium.org/12089103/diff/2001/sync/protocol/favicon_spec...
sync/protocol/favicon_specifics.proto:35: optional string favicon_source = 1;
Nit: Name this parameter |favicon_url| / |icon_url| / |url| ?

https://codereview.chromium.org/12089103/diff/2001/sync/protocol/proto_value_...
File sync/protocol/proto_value_conversions.cc (right):

https://codereview.chromium.org/12089103/diff/2001/sync/protocol/proto_value_...
sync/protocol/proto_value_conversions.cc:359: DictionaryValue*
FaviconDataToValue(
Nit: put this method into an anonymous namespace?

[Nicolas Zea, 2013-02-14 02:23:28]

Based on offline discussions with the mobile and server folk, I've updated the
patch to split the support into two types, one for the image data itself, the
other for the tracking information. This means that as we update the tracking
information, we aren't also unnecessarily touching the image data.

Additionally, I've moved away from the repeated field and instead have several
optional fields for the various types. This is easier to deal with both locally
and in the server. As we support higher resolutions, we can add more fields.

PTAL.

[Nicolas Zea, 2013-02-14 18:47:56]

+albert, FYI for proto changes

[albertb, 2013-02-14 19:07:57]

https://codereview.chromium.org/12089103/diff/14001/sync/protocol/favicon_ima...
File sync/protocol/favicon_image_specifics.proto (right):

https://codereview.chromium.org/12089103/diff/14001/sync/protocol/favicon_ima...
sync/protocol/favicon_image_specifics.proto:19: optional int64 height = 3;
1) Do you really need to keep track of both dimensions? Aren't favicons always
square?

2) In fact, doesn't PNG keep track of it's own resolution?

3) Are you really expecting the height of a favicon to ever be more than 2^32
pixels?

https://codereview.chromium.org/12089103/diff/14001/sync/protocol/favicon_ima...
sync/protocol/favicon_image_specifics.proto:30: optional FaviconData favicon_web
= 2;
Since the size is already encoded in FaviconData, why have separate fields for
each resolution?

Note that since you've split the favicons into two separate datatypes, the
server-side favicon extraction no longer applies. I can just store the full
specifics in blobstore, regardless of it's layout.

https://codereview.chromium.org/12089103/diff/14001/sync/protocol/favicon_tra...
File sync/protocol/favicon_tracking_specifics.proto (right):

https://codereview.chromium.org/12089103/diff/14001/sync/protocol/favicon_tra...
sync/protocol/favicon_tracking_specifics.proto:21: optional int64
last_visit_time_ms = 3;
Is this sane time? Might want to add that detail to the comment.

https://codereview.chromium.org/12089103/diff/14001/sync/protocol/favicon_tra...
sync/protocol/favicon_tracking_specifics.proto:23: optional bool is_bookmarked =
4;
I'm confused by this field. Isn't it trivial for the client to figure out which
URLs are bookmarked and thus which favicons should never be evicted? Why sync
this fact?

Is it to signal to non-bookmark syncing clients to not evict those favicons?

[Nicolas Zea, 2013-02-14 19:45:51]

Comments addressed.

https://codereview.chromium.org/12089103/diff/14001/sync/protocol/favicon_ima...
File sync/protocol/favicon_image_specifics.proto (right):

https://codereview.chromium.org/12089103/diff/14001/sync/protocol/favicon_ima...
sync/protocol/favicon_image_specifics.proto:19: optional int64 height = 3;
On 2013/02/14 19:07:57, albertb wrote:
> 1) Do you really need to keep track of both dimensions? Aren't favicons always
> square?
> 
> 2) In fact, doesn't PNG keep track of it's own resolution?
> 
> 3) Are you really expecting the height of a favicon to ever be more than 2^32
> pixels?

1) From speaking with Peter I don't think so. Can you confirm whether this is
the case or not Peter? Originally I had only one dimension.

2) It's still useful to have the dimension so that you don't need to load and
parse the entire PNG image to find out it's a resolution you don't care about.

3) Fixed.

https://codereview.chromium.org/12089103/diff/14001/sync/protocol/favicon_ima...
sync/protocol/favicon_image_specifics.proto:30: optional FaviconData favicon_web
= 2;
On 2013/02/14 19:07:57, albertb wrote:
> Since the size is already encoded in FaviconData, why have separate fields for
> each resolution?
> 
> Note that since you've split the favicons into two separate datatypes, the
> server-side favicon extraction no longer applies. I can just store the full
> specifics in blobstore, regardless of it's layout.

Because we want to be able to store multiple favicons with different
resolutions, but want to limit it to 1 per scale type. The other option is a
repeated field, like before, but that means the client has to scan to find the
particular scale it wants, and it doesn't enforce the 1 per scale type as well.
This just seems more straightforward. 

Note that these are max resolutions. There are valid resolutions that don't fit
the 16, 32, 64, 128 standards (e.g. 48x48, 72x72, 114x114). If a website has one
of those, you just store the largest resolution that fits into the specific
field. I'll add a comment about that to make it more clear.

Also, I believe it's possible for favicons to be non-square. Peter, is that
right, or did I misunderstand you?

Lastly, isn't there still some benefit in extracting the individual images
server-side for de-duping? Although I guess theoretically, since this proto is
keyed off an icon url, all versions should be the same, once you account for the
catchup time during favicon changes. But, future versions of this proto might
support more (higher) resolutions, which would then conflict with those from old
version clients, so there might still be some benefit.

https://codereview.chromium.org/12089103/diff/14001/sync/protocol/favicon_tra...
File sync/protocol/favicon_tracking_specifics.proto (right):

https://codereview.chromium.org/12089103/diff/14001/sync/protocol/favicon_tra...
sync/protocol/favicon_tracking_specifics.proto:21: optional int64
last_visit_time_ms = 3;
On 2013/02/14 19:07:57, albertb wrote:
> Is this sane time? Might want to add that detail to the comment.

It's kind of orthogonal I think. Either way we have to trust this value, since
we have no alternative.

https://codereview.chromium.org/12089103/diff/14001/sync/protocol/favicon_tra...
sync/protocol/favicon_tracking_specifics.proto:23: optional bool is_bookmarked =
4;
On 2013/02/14 19:07:57, albertb wrote:
> I'm confused by this field. Isn't it trivial for the client to figure out
which
> URLs are bookmarked and thus which favicons should never be evicted? Why sync
> this fact?
> 
> Is it to signal to non-bookmark syncing clients to not evict those favicons?

Yes, it's primarily for non-bookmarking clients. Additionally, if someone just
added a bookmark, and the favicon reaches another client first, we don't want to
delete the favicon unnecessarily before the bookmark arrives. This will involve
a bit of logic client-side though, so that we don't wind up with a bunch of
favicons that we think are bookmarked but aren't actually (e.g. conflict
resolution somehow kept them around).

[albertb, 2013-02-14 20:31:49]

https://codereview.chromium.org/12089103/diff/14001/sync/protocol/favicon_ima...
File sync/protocol/favicon_image_specifics.proto (right):

https://codereview.chromium.org/12089103/diff/14001/sync/protocol/favicon_ima...
sync/protocol/favicon_image_specifics.proto:30: optional FaviconData favicon_web
= 2;
On 2013/02/14 19:45:51, Nicolas Zea wrote:
> On 2013/02/14 19:07:57, albertb wrote:
> > Since the size is already encoded in FaviconData, why have separate fields
for
> > each resolution?
> > 
> > Note that since you've split the favicons into two separate datatypes, the
> > server-side favicon extraction no longer applies. I can just store the full
> > specifics in blobstore, regardless of it's layout.
> 
> Because we want to be able to store multiple favicons with different
> resolutions, but want to limit it to 1 per scale type. The other option is a
> repeated field, like before, but that means the client has to scan to find the
> particular scale it wants, and it doesn't enforce the 1 per scale type as
well.
> This just seems more straightforward.

Just a general observation, but by storing the same information three times (ie.
the field name, the height/width fields and the PNG itself) you run the risk of
having them disagree. Maybe it doesn't matter, but are you really saving that
much by not to parsing the PNG?  

I guess it comes down to which scenario you're optimizing for. Either you trust
all writers to do the right thing and store the right favicon in the right
place. Or you don't trust anyone and always validate the data before using it. I
guess I tend to be more pessimistic. :)

> Note that these are max resolutions. There are valid resolutions that don't
fit
> the 16, 32, 64, 128 standards (e.g. 48x48, 72x72, 114x114). If a website has
one
> of those, you just store the largest resolution that fits into the specific
> field. I'll add a comment about that to make it more clear.
> 
> Also, I believe it's possible for favicons to be non-square. Peter, is that
> right, or did I misunderstand you?
> 
> Lastly, isn't there still some benefit in extracting the individual images
> server-side for de-duping? Although I guess theoretically, since this proto is
> keyed off an icon url, all versions should be the same, once you account for
the
> catchup time during favicon changes. But, future versions of this proto might
> support more (higher) resolutions, which would then conflict with those from
old
> version clients, so there might still be some benefit.

If you're planning to often support new sizes, there could be value in splitting
each resolution into its own blob. But is this a likely scenario? The
disadvantage of splitting the favicons is that it requires a server push to work
correctly. If I just store the whole specifics, the layout is irrelevant.

[pkotwicz, 2013-02-14 21:15:05]

https://codereview.chromium.org/12089103/diff/14001/sync/protocol/favicon_ima...
File sync/protocol/favicon_image_specifics.proto (right):

https://codereview.chromium.org/12089103/diff/14001/sync/protocol/favicon_ima...
sync/protocol/favicon_image_specifics.proto:19: optional int64 height = 3;
1) Favicons can have any dimensions. In practice they are overwhelmingly square
but there is nothing stopping a web designer from providing a non square
favicon.

2) PNGs do track their own resolution. The size information is in the IHDR chunk
which is the first chunk in the PNG file. So, we would not need to parse the
entire PNG image. However, we do not extract the width and height yet from the
PNG anywhere else in Chromium.

[Nicolas Zea, 2013-02-15 19:13:44]

On 2013/02/14 20:31:49, albertb wrote:
>
https://codereview.chromium.org/12089103/diff/14001/sync/protocol/favicon_ima...
> File sync/protocol/favicon_image_specifics.proto (right):
> 
>
https://codereview.chromium.org/12089103/diff/14001/sync/protocol/favicon_ima...
> sync/protocol/favicon_image_specifics.proto:30: optional FaviconData
favicon_web
> = 2;
> On 2013/02/14 19:45:51, Nicolas Zea wrote:
> > On 2013/02/14 19:07:57, albertb wrote:
> > > Since the size is already encoded in FaviconData, why have separate fields
> for
> > > each resolution?
> > > 
> > > Note that since you've split the favicons into two separate datatypes, the
> > > server-side favicon extraction no longer applies. I can just store the
full
> > > specifics in blobstore, regardless of it's layout.
> > 
> > Because we want to be able to store multiple favicons with different
> > resolutions, but want to limit it to 1 per scale type. The other option is a
> > repeated field, like before, but that means the client has to scan to find
the
> > particular scale it wants, and it doesn't enforce the 1 per scale type as
> well.
> > This just seems more straightforward.
> 
> Just a general observation, but by storing the same information three times
(ie.
> the field name, the height/width fields and the PNG itself) you run the risk
of
> having them disagree. Maybe it doesn't matter, but are you really saving that
> much by not to parsing the PNG?  
> 
> I guess it comes down to which scenario you're optimizing for. Either you
trust
> all writers to do the right thing and store the right favicon in the right
> place. Or you don't trust anyone and always validate the data before using it.
I
> guess I tend to be more pessimistic. :)

Yeah. If it were a repeated field, I'd be more concerned about confusion in
writing the favicon. But for individual fields like this, I'm not too worried
about it. I also prefer having the separate fields so we can better understand
the contents when debugging (about:sync's node browser won't tell you the size
otherwise).
> 
> > Note that these are max resolutions. There are valid resolutions that don't
> fit
> > the 16, 32, 64, 128 standards (e.g. 48x48, 72x72, 114x114). If a website has
> one
> > of those, you just store the largest resolution that fits into the specific
> > field. I'll add a comment about that to make it more clear.
> > 
> > Also, I believe it's possible for favicons to be non-square. Peter, is that
> > right, or did I misunderstand you?
> > 
> > Lastly, isn't there still some benefit in extracting the individual images
> > server-side for de-duping? Although I guess theoretically, since this proto
is
> > keyed off an icon url, all versions should be the same, once you account for
> the
> > catchup time during favicon changes. But, future versions of this proto
might
> > support more (higher) resolutions, which would then conflict with those from
> old
> > version clients, so there might still be some benefit.
> 
> If you're planning to often support new sizes, there could be value in
splitting
> each resolution into its own blob. But is this a likely scenario? The
> disadvantage of splitting the favicons is that it requires a server push to
work
> correctly. If I just store the whole specifics, the layout is irrelevant.

Fair enough. Either way, I'd still rather have it in separate fields for client
use/clarity in debugging.

[Nicolas Zea, 2013-02-15 19:14:03]

On 2013/02/15 19:13:44, Nicolas Zea wrote:
> On 2013/02/14 20:31:49, albertb wrote:
> >
>
https://codereview.chromium.org/12089103/diff/14001/sync/protocol/favicon_ima...
> > File sync/protocol/favicon_image_specifics.proto (right):
> > 
> >
>
https://codereview.chromium.org/12089103/diff/14001/sync/protocol/favicon_ima...
> > sync/protocol/favicon_image_specifics.proto:30: optional FaviconData
> favicon_web
> > = 2;
> > On 2013/02/14 19:45:51, Nicolas Zea wrote:
> > > On 2013/02/14 19:07:57, albertb wrote:
> > > > Since the size is already encoded in FaviconData, why have separate
fields
> > for
> > > > each resolution?
> > > > 
> > > > Note that since you've split the favicons into two separate datatypes,
the
> > > > server-side favicon extraction no longer applies. I can just store the
> full
> > > > specifics in blobstore, regardless of it's layout.
> > > 
> > > Because we want to be able to store multiple favicons with different
> > > resolutions, but want to limit it to 1 per scale type. The other option is
a
> > > repeated field, like before, but that means the client has to scan to find
> the
> > > particular scale it wants, and it doesn't enforce the 1 per scale type as
> > well.
> > > This just seems more straightforward.
> > 
> > Just a general observation, but by storing the same information three times
> (ie.
> > the field name, the height/width fields and the PNG itself) you run the risk
> of
> > having them disagree. Maybe it doesn't matter, but are you really saving
that
> > much by not to parsing the PNG?  
> > 
> > I guess it comes down to which scenario you're optimizing for. Either you
> trust
> > all writers to do the right thing and store the right favicon in the right
> > place. Or you don't trust anyone and always validate the data before using
it.
> I
> > guess I tend to be more pessimistic. :)
> 
> Yeah. If it were a repeated field, I'd be more concerned about confusion in
> writing the favicon. But for individual fields like this, I'm not too worried
> about it. I also prefer having the separate fields so we can better understand
> the contents when debugging (about:sync's node browser won't tell you the size
> otherwise).
> > 
> > > Note that these are max resolutions. There are valid resolutions that
don't
> > fit
> > > the 16, 32, 64, 128 standards (e.g. 48x48, 72x72, 114x114). If a website
has
> > one
> > > of those, you just store the largest resolution that fits into the
specific
> > > field. I'll add a comment about that to make it more clear.
> > > 
> > > Also, I believe it's possible for favicons to be non-square. Peter, is
that
> > > right, or did I misunderstand you?
> > > 
> > > Lastly, isn't there still some benefit in extracting the individual images
> > > server-side for de-duping? Although I guess theoretically, since this
proto
> is
> > > keyed off an icon url, all versions should be the same, once you account
for
> > the
> > > catchup time during favicon changes. But, future versions of this proto
> might
> > > support more (higher) resolutions, which would then conflict with those
from
> > old
> > > version clients, so there might still be some benefit.
> > 
> > If you're planning to often support new sizes, there could be value in
> splitting
> > each resolution into its own blob. But is this a likely scenario? The
> > disadvantage of splitting the favicons is that it requires a server push to
> work
> > correctly. If I just store the whole specifics, the layout is irrelevant.
> 
> Fair enough. Either way, I'd still rather have it in separate fields for
client
> use/clarity in debugging.

Were there any other issues with the patch, or is this good to land as is?

[pkotwicz, 2013-02-15 19:15:32]

No other issues on my end

[albertb, 2013-02-15 19:15:35]

LGTM

[commit-bot: I haz the power, 2013-02-15 19:17:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/12089103/11002

[commit-bot: I haz the power, 2013-02-15 22:22:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/12089103/11002

[commit-bot: I haz the power, 2013-02-16 00:45:19]

Change committed as 182869