Ensure there are always nice icons for bookmark apps.

Ensure there are always nice icons for bookmark apps.

This change does a few things. Firstly, it stops bookmark apps from
using downloaded favicons / manifest icons. Secondly, it syncs the
color it uses to generate icons. Thirdly, machines that bookmark apps
are synced onto will now generate icons using the synced color.

This means bookmark apps will always have nice icons (although not the
ones downloaded) and they will always be consistent across machines.

Once blob sync is available the downloaded icons will be kept and
synced.

BUG=439347
Committed: https://crrev.com/d4b64a7bf6f7d450e41f5133890cdaebb30623da
Cr-Commit-Position: refs/heads/master@{#307889}

[benwells, 2014-12-08 09:36:21]

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

[benwells, 2014-12-08 09:36:22]

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

[benwells, 2014-12-08 09:36:22]

calamity, please do an initial review.

[benwells, 2014-12-08 09:36:23]

calamity, please do an initial review.

[calamity, 2014-12-09 03:46:12]

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

https://codereview.chromium.org/782693002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/bookmark_app_helper.cc:358: allowed_sizes));
Hmm. Technically, this is now unnecessary work. You can probably get away with
just using the downloaded_icons vector. Maybe you'd want to sort it by size or
by color_util::CalculateBoringScore(). You can use lambdas to do this sort now!

https://codereview.chromium.org/782693002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/bookmark_app_helper.cc:361: // Determine the color
that will be used for the icon's background/
s/d\//./

https://codereview.chromium.org/782693002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/bookmark_app_helper.cc:409: WebApplicationInfo&
web_app_info) {
non-const ref? Can this be changed to a pointer?

https://codereview.chromium.org/782693002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/manifest_handlers/app_icon_color_info.cc (right):

https://codereview.chromium.org/782693002/diff/20001/chrome/common/extensions...
chrome/common/extensions/manifest_handlers/app_icon_color_info.cc:67: //
TODO(benwells): ditto.
Eh. Just copy-paste the comment above. I don't want a stray TODO: ditto sitting
around someday.

https://codereview.chromium.org/782693002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/manifest_handlers/app_icon_color_info.h (right):

https://codereview.chromium.org/782693002/diff/20001/chrome/common/extensions...
chrome/common/extensions/manifest_handlers/app_icon_color_info.h:20: // Define
out of line constructor/destructor to please Clang.
I don't think this comment is necessary.

https://codereview.chromium.org/782693002/diff/20001/extensions/common/image_...
File extensions/common/image_util.cc (right):

https://codereview.chromium.org/782693002/diff/20001/extensions/common/image_...
extensions/common/image_util.cc:49: return std::string(buffer);
Use base::StringPrintf instead. Fixed size buffers make me nervous.

[benwells, 2014-12-09 08:28:35]

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

https://codereview.chromium.org/782693002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/bookmark_app_helper.cc:358: allowed_sizes));
On 2014/12/09 03:46:11, calamity wrote:
> Hmm. Technically, this is now unnecessary work. You can probably get away with
> just using the downloaded_icons vector. Maybe you'd want to sort it by size or
> by color_util::CalculateBoringScore(). You can use lambdas to do this sort
now!

OK. I'm just going to use the first one we find, I don't think we need to sort
it as the order should be deterministic (although fairly meaningless).

https://codereview.chromium.org/782693002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/bookmark_app_helper.cc:361: // Determine the color
that will be used for the icon's background/
On 2014/12/09 03:46:11, calamity wrote:
> s/d\//./

Done.

https://codereview.chromium.org/782693002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/bookmark_app_helper.cc:409: WebApplicationInfo&
web_app_info) {
On 2014/12/09 03:46:11, calamity wrote:
> non-const ref? Can this be changed to a pointer?

Nice spot, done.

https://codereview.chromium.org/782693002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/manifest_handlers/app_icon_color_info.cc (right):

https://codereview.chromium.org/782693002/diff/20001/chrome/common/extensions...
chrome/common/extensions/manifest_handlers/app_icon_color_info.cc:67: //
TODO(benwells): ditto.
On 2014/12/09 03:46:11, calamity wrote:
> Eh. Just copy-paste the comment above. I don't want a stray TODO: ditto
sitting
> around someday.

Oops they are both gone now.

https://codereview.chromium.org/782693002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/manifest_handlers/app_icon_color_info.h (right):

https://codereview.chromium.org/782693002/diff/20001/chrome/common/extensions...
chrome/common/extensions/manifest_handlers/app_icon_color_info.h:20: // Define
out of line constructor/destructor to please Clang.
On 2014/12/09 03:46:12, calamity wrote:
> I don't think this comment is necessary.

Done.

https://codereview.chromium.org/782693002/diff/20001/extensions/common/image_...
File extensions/common/image_util.cc (right):

https://codereview.chromium.org/782693002/diff/20001/extensions/common/image_...
extensions/common/image_util.cc:49: return std::string(buffer);
On 2014/12/09 03:46:12, calamity wrote:
> Use base::StringPrintf instead. Fixed size buffers make me nervous.

Done.

[calamity, 2014-12-09 08:37:25]

lgtm, there's a typo in the CL description (syced).

[benwells, 2014-12-09 11:44:48]

benwells@chromium.org changed reviewers:
+ atwilson@chromium.org, thestig@chromium.org

[benwells, 2014-12-09 11:44:49]

+atwilson for sync/
+thestig for chrome/common, chrome/browser/ui

[Lei Zhang, 2014-12-09 20:28:15]

lgtm

https://codereview.chromium.org/782693002/diff/60001/chrome/common/extensions...
File chrome/common/extensions/manifest_handlers/app_icon_color_info.cc (right):

https://codereview.chromium.org/782693002/diff/60001/chrome/common/extensions...
chrome/common/extensions/manifest_handlers/app_icon_color_info.cc:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
nit: no (C), and 2014.

[Andrew T Wilson (Slow), 2014-12-09 21:07:05]

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

[Andrew T Wilson (Slow), 2014-12-09 21:07:06]

+zea FYI
Don't see anything under sync/ for me to review? I'm pretty jetlagged today so
maybe I'm just being stupid :(

[benwells, 2014-12-09 21:18:33]

On 2014/12/09 21:07:06, Andrew T Wilson wrote:
> +zea FYI
> Don't see anything under sync/ for me to review? I'm pretty jetlagged today so
> maybe I'm just being stupid :(

There is one change in sync/protocol/app_specifics.proto.

[Andrew T Wilson (Slow), 2014-12-09 22:09:02]

lgtm with one nit

https://codereview.chromium.org/782693002/diff/60001/sync/protocol/app_specif...
File sync/protocol/app_specifics.proto (right):

https://codereview.chromium.org/782693002/diff/60001/sync/protocol/app_specif...
sync/protocol/app_specifics.proto:79: // This is the color to use when
generating bookmark app icons.
What is the format here - #rgb or something else? Does it make sense to describe
this here?

[benwells, 2014-12-10 04:30:04]

Patchset #5 (id:80001) has been deleted

[benwells, 2014-12-10 04:32:43]

https://codereview.chromium.org/782693002/diff/60001/chrome/common/extensions...
File chrome/common/extensions/manifest_handlers/app_icon_color_info.cc (right):

https://codereview.chromium.org/782693002/diff/60001/chrome/common/extensions...
chrome/common/extensions/manifest_handlers/app_icon_color_info.cc:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2014/12/09 20:28:15, Lei Zhang wrote:
> nit: no (C), and 2014.

Done.

https://codereview.chromium.org/782693002/diff/60001/sync/protocol/app_specif...
File sync/protocol/app_specifics.proto (right):

https://codereview.chromium.org/782693002/diff/60001/sync/protocol/app_specif...
sync/protocol/app_specifics.proto:79: // This is the color to use when
generating bookmark app icons.
On 2014/12/09 22:09:01, Andrew T Wilson wrote:
> What is the format here - #rgb or something else? Does it make sense to
describe
> this here?

Done.

[benwells, 2014-12-10 04:34:56]

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

[benwells, 2014-12-10 04:34:57]

+bsalomon as I've added a dependency onto skia from extensions/common and that
needs a skia owner to lg.

zea - should i wait for your lg?

[bsalomon, 2014-12-10 19:37:55]

On 2014/12/10 04:34:57, benwells wrote:
> +bsalomon as I've added a dependency onto skia from extensions/common and that
> needs a skia owner to lg.

lgtm

> 
> zea - should i wait for your lg?

[benwells, 2014-12-10 23:12:10]

The CQ bit was checked by benwells@chromium.org

[benwells, 2014-12-10 23:12:40]

On 2014/12/10 19:37:55, bsalomon wrote:
> On 2014/12/10 04:34:57, benwells wrote:
> > +bsalomon as I've added a dependency onto skia from extensions/common and
that
> > needs a skia owner to lg.
> 
> lgtm
> 
> > 
> > zea - should i wait for your lg?

(I checked offline with zea who didn't need to review)

[commit-bot: I haz the power, 2014-12-10 23:13:42]

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

[commit-bot: I haz the power, 2014-12-11 00:22:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-11 00:22:57]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[benwells, 2014-12-11 03:33:22]

On 2014/12/11 00:22:57, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   win_chromium_compile_dbg on tryserver.chromium.win
>
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
>   win_chromium_rel_ng on tryserver.chromium.win
>
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

Note to atwilson: I changed
chrome/browser/test/sync/integration/two_client_apps_sync_test.cc to make it
compile. 

I think it is a TBR'able change (as it is basically fallout from an API change
elsewhere) so am going to put it in the CQ now. But if you want to look, take a
look :)

[benwells, 2014-12-11 03:33:28]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2014-12-11 03:34:08]

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

[commit-bot: I haz the power, 2014-12-11 04:01:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-11 04:01:05]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/92760)

[benwells, 2014-12-11 11:07:46]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2014-12-11 11:08:14]

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

[commit-bot: I haz the power, 2014-12-11 12:38:35]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2014-12-11 12:39:19]

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