[Sync] Add favicon datatype proto support

sync/protocol/favicon_image_specifics.proto

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// Sync protocol datatype extension for the favicon image specifics.
+
+syntax = "proto2";
+
+option optimize_for = LITE_RUNTIME;
+option retain_unknown_fields = true;
+
+package sync_pb;
+
+message FaviconData {
+  // The image data for the favicon (PNG encoded).
+  optional bytes favicon = 1;
+  // The favicon dimensions.
+  optional int32 width = 2;
+  optional int64 height = 3;

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

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?

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

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.

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

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.

+}
+
+// Image data for favicons. Multiple resolutions are supported.
+message FaviconImageSpecifics {
+  // The url of the favicon image.
+  optional string favicon_url = 1;
+  // The favicons associated with this source, one per scale type.
+  // Favicons images should be stored into the field that best corresponds to
+  // their scale.
+  // A normal low-resolution web favicon (max resolution 16x16).
+  optional FaviconData favicon_web = 2;

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

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.

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

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.

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

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. :)
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.

+  // A slightly higher-resolution web favicon (max resolution 32x32).
+  optional FaviconData favicon_web_32 = 3;
+  // A high-resolution touch favicon (not precomposed, max resolution 64x64).
+  optional FaviconData favicon_touch_64 = 4;
+  // A high-resolution touch favicon (precomposed, max resolution 64x64).
+  optional FaviconData favicon_touch_precomposed_64 = 5;
+}