Sync bookmark app icon urls and sizes, and download icons for new apps.

Sync bookmark app icon urls and sizes, and download icons for new apps.

This means bookmark apps can use proper icons from the web page or the
pages manifest, and have these reflectd on all devices the bookmark
app is synced to.

BUG=480049
Committed: https://crrev.com/ed75d1b590e7ed38fa1a8f2f688925b390019aa6
Cr-Commit-Position: refs/heads/master@{#327424}

[benwells, 2015-04-23 06:39:00]

benwells@chromium.org changed reviewers:
+ calamity@chromium.org

[benwells, 2015-04-23 06:39:00]

https://codereview.chromium.org/1066623008/diff/20001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/linked_app_icons.cc (right):

https://codereview.chromium.org/1066623008/diff/20001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/linked_app_icons.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
Self nit: 2015

[calamity, 2015-04-24 05:05:41]

Icons urls will only get used once, at downsync creation time, right? We don't
want any inconsistencies causing sync to do endlessly trigger updates.

https://codereview.chromium.org/1066623008/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/bookmark_app_helper.cc (right):

https://codereview.chromium.org/1066623008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:204: void
UpdateWebAppInfoIcons(
This function is mostly about generating icons if needed right?

GenerateWebAppInfoIconsIfNeeded() perhaps?

https://codereview.chromium.org/1066623008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:253:
urls_to_download_.push_back(icon.url);
Are these URLs deduped anywhere? Since you can get multiple icons from the same
URL coming in from sync, you might redownload the same image multiple times.

https://codereview.chromium.org/1066623008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:351:
BookmarkAppHelper::ConstrainBitmapsToSizes(
This reverts a deletion of this code right? I think there were tests that got
deleted too. Should add those back.

https://codereview.chromium.org/1066623008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:506: bitmap_and_source.bitmap =
icon;
bitmap_and_source.source_url = it->url?

or is there a reason for this not to be done.

https://codereview.chromium.org/1066623008/diff/20001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/linked_app_icons.h (right):

https://codereview.chromium.org/1066623008/diff/20001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/linked_app_icons.h:19: // A structure
to hold the parsed app icon color data.
parsed linked app icon data.

https://codereview.chromium.org/1066623008/diff/20001/sync/protocol/app_speci...
File sync/protocol/app_specifics.proto (right):

https://codereview.chromium.org/1066623008/diff/20001/sync/protocol/app_speci...
sync/protocol/app_specifics.proto:46: // The size of the app icon.
// The size of the app icon in DIPs.

[benwells, 2015-04-27 09:54:54]

https://codereview.chromium.org/1066623008/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/bookmark_app_helper.cc (right):

https://codereview.chromium.org/1066623008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:204: void
UpdateWebAppInfoIcons(
On 2015/04/24 05:05:40, calamity wrote:
> This function is mostly about generating icons if needed right?
> 
> GenerateWebAppInfoIconsIfNeeded() perhaps?

It does a bit more than that, it resizes as well. I've renamed it and moved it
to be a public static function (for testing).

https://codereview.chromium.org/1066623008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:253:
urls_to_download_.push_back(icon.url);
On 2015/04/24 05:05:40, calamity wrote:
> Are these URLs deduped anywhere? Since you can get multiple icons from the
same
> URL coming in from sync, you might redownload the same image multiple times.

Yeah, I thought about this. The bitmapfetcher only ever returns one image per
file. I don't think the extra downloads are really a worry but instead of having
all the different images extracted the same one will be used each time.

I'm thinking either leave it like it is for now and later update the bitmap
fetcher thing to support icos properly, or maybe just ignore .icos totally.

https://codereview.chromium.org/1066623008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:351:
BookmarkAppHelper::ConstrainBitmapsToSizes(
On 2015/04/24 05:05:40, calamity wrote:
> This reverts a deletion of this code right? I think there were tests that got
> deleted too. Should add those back.

Yep, done. Here is the original CL if you're interested...
https://codereview.chromium.org/782693002

https://codereview.chromium.org/1066623008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:506: bitmap_and_source.bitmap =
icon;
On 2015/04/24 05:05:40, calamity wrote:
> bitmap_and_source.source_url = it->url?
> 
> or is there a reason for this not to be done.

I didn't think they would have a URL in this case... but it doesn't hurt to copy
it over in case they do. Done, and I added a convenience constructor to make
this patter simpler.

https://codereview.chromium.org/1066623008/diff/20001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/linked_app_icons.cc (right):

https://codereview.chromium.org/1066623008/diff/20001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/linked_app_icons.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2015/04/23 06:39:00, benwells wrote:
> Self nit: 2015

Done.

https://codereview.chromium.org/1066623008/diff/20001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/linked_app_icons.h (right):

https://codereview.chromium.org/1066623008/diff/20001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/linked_app_icons.h:19: // A structure
to hold the parsed app icon color data.
On 2015/04/24 05:05:40, calamity wrote:
> parsed linked app icon data.

Done.

https://codereview.chromium.org/1066623008/diff/20001/sync/protocol/app_speci...
File sync/protocol/app_specifics.proto (right):

https://codereview.chromium.org/1066623008/diff/20001/sync/protocol/app_speci...
sync/protocol/app_specifics.proto:46: // The size of the app icon.
On 2015/04/24 05:05:40, calamity wrote:
> // The size of the app icon in DIPs.

Done.

[calamity, 2015-04-28 06:56:36]

https://codereview.chromium.org/1066623008/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/bookmark_app_helper.cc (right):

https://codereview.chromium.org/1066623008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:253:
urls_to_download_.push_back(icon.url);
On 2015/04/27 09:54:54, benwells wrote:
> On 2015/04/24 05:05:40, calamity wrote:
> > Are these URLs deduped anywhere? Since you can get multiple icons from the
> same
> > URL coming in from sync, you might redownload the same image multiple times.
> 
> Yeah, I thought about this. The bitmapfetcher only ever returns one image per
> file. I don't think the extra downloads are really a worry but instead of
having
> all the different images extracted the same one will be used each time.
> 
> I'm thinking either leave it like it is for now and later update the bitmap
> fetcher thing to support icos properly, or maybe just ignore .icos totally.

Acknowledged. I'm happy to just leave it as is for now. Ignoring icos sounds
like a good option.

https://codereview.chromium.org/1066623008/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/bookmark_app_helper_unittest.cc (right):

https://codereview.chromium.org/1066623008/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper_unittest.cc:108: namespace
extensions {
Where did this go?

https://codereview.chromium.org/1066623008/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/bookmark_app_helper.cc (right):

https://codereview.chromium.org/1066623008/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:413: for (const auto& pair :
bitmap_map) {
Use bitmap_map.find(icon.width)?

https://codereview.chromium.org/1066623008/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:422: // Now add in any icons
from the updated list that don't have URls.
nit: URLs

https://codereview.chromium.org/1066623008/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/bookmark_app_helper.h (right):

https://codereview.chromium.org/1066623008/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.h:91: // icon from
Nit: sentence fragment.

https://codereview.chromium.org/1066623008/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/bookmark_app_helper_unittest.cc (right):

https://codereview.chromium.org/1066623008/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper_unittest.cc:381:
ValidateAllIconsWithURLsArePresent(new_web_app_info, web_app_info);
You could just use a counter in ValidateAllIconsWithURLsArePresent() to count
the number of matching icons and check that found == info_to_check.icons.size()
and info_to_check.icons.size() == other_info.icons.size()

[benwells, 2015-04-28 08:10:57]

https://codereview.chromium.org/1066623008/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/bookmark_app_helper.cc (right):

https://codereview.chromium.org/1066623008/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:413: for (const auto& pair :
bitmap_map) {
On 2015/04/28 06:56:35, calamity wrote:
> Use bitmap_map.find(icon.width)?

Done.

https://codereview.chromium.org/1066623008/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.cc:422: // Now add in any icons
from the updated list that don't have URls.
On 2015/04/28 06:56:35, calamity wrote:
> nit: URLs

Done.

https://codereview.chromium.org/1066623008/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/bookmark_app_helper.h (right):

https://codereview.chromium.org/1066623008/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper.h:91: // icon from
On 2015/04/28 06:56:35, calamity wrote:
> Nit: sentence fragment.

Done.

https://codereview.chromium.org/1066623008/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/bookmark_app_helper_unittest.cc (right):

https://codereview.chromium.org/1066623008/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/bookmark_app_helper_unittest.cc:381:
ValidateAllIconsWithURLsArePresent(new_web_app_info, web_app_info);
On 2015/04/28 06:56:35, calamity wrote:
> You could just use a counter in ValidateAllIconsWithURLsArePresent() to count
> the number of matching icons and check that found ==
info_to_check.icons.size()
> and info_to_check.icons.size() == other_info.icons.size()

As discussed there could be icons without URLs.

[calamity, 2015-04-28 08:18:23]

lgtm

[benwells, 2015-04-28 08:27:39]

benwells@chromium.org changed reviewers:
+ zea@chromium.org

[benwells, 2015-04-28 08:27:40]

+zea for owners review of sync

[Nicolas Zea, 2015-04-28 16:51:50]

Could you update proto_value_conversions as well?

[benwells, 2015-04-28 23:36:39]

On 2015/04/28 16:51:50, Nicolas Zea wrote:
> Could you update proto_value_conversions as well?

Done.

[Nicolas Zea, 2015-04-28 23:37:42]

sync LGTM

[benwells, 2015-04-29 00:01:51]

The CQ bit was checked by benwells@chromium.org

[benwells, 2015-04-29 00:01:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
calamity@chromium.org
Link to the patchset: https://codereview.chromium.org/1066623008/#ps80001
(title: "Updated sync value conversions")

[commit-bot: I haz the power, 2015-04-29 00:03:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1066623008/80001

[commit-bot: I haz the power, 2015-04-29 02:39:41]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-04-29 02:40:39]

Patchset 5 (id:??) landed as
https://crrev.com/ed75d1b590e7ed38fa1a8f2f688925b390019aa6
Cr-Commit-Position: refs/heads/master@{#327424}