[Mac][Material Design] Restore bookmark icons to default favicon.

[Mac][Material Design] Restore bookmark icons to default favicon.

The Material Design bookmark buttons incorrectly used the
location_bar_http vector icon to display a document icon for the
default favicon. This cl adds default favicon vector icons based on the
original version of the location_bar_http vector, and changes the
bookmark buttons to use them.

R=avi@chromium.org, estade@chromium.org
BUG=614539
Committed: https://crrev.com/a0776f8a55b798c81fc142ea531c9d8a762c2c85
Cr-Commit-Position: refs/heads/master@{#396020}

[shrike, 2016-05-25 01:13:36]

PTAL

estade@
  Additions to ui/gfx/vector_icons

avi@
  chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm

[Avi (use Gerrit), 2016-05-25 03:34:59]

lgtm

[Evan Stade, 2016-05-25 17:23:49]

https://codereview.chromium.org/2009643003/diff/1/ui/gfx/vector_icons/default...
File ui/gfx/vector_icons/default_favicon.1x.icon (left):

https://codereview.chromium.org/2009643003/diff/1/ui/gfx/vector_icons/default...
ui/gfx/vector_icons/default_favicon.1x.icon:7: LINE_TO, 4, 14,
this is almost identical to omnibox_http.icon --- can we use that?

https://codereview.chromium.org/2009643003/diff/1/ui/gfx/vector_icons/default...
File ui/gfx/vector_icons/default_favicon.icon (right):

https://codereview.chromium.org/2009643003/diff/1/ui/gfx/vector_icons/default...
ui/gfx/vector_icons/default_favicon.icon:5: CANVAS_DIMENSIONS, 32,
where did this one come from? omnibox_http.icon apparently looks good enough at
both scale factors that it doesn't need separate icon definitions (according to
sgabriel). If we no longer believe that's true, shouldn't we add different
versions for omnibox_http.icon as well?

[shrike, 2016-05-25 17:36:42]

On 2016/05/25 17:23:49, Evan Stade wrote:
>
https://codereview.chromium.org/2009643003/diff/1/ui/gfx/vector_icons/default...
> File ui/gfx/vector_icons/default_favicon.1x.icon (left):
> 
>
https://codereview.chromium.org/2009643003/diff/1/ui/gfx/vector_icons/default...
> ui/gfx/vector_icons/default_favicon.1x.icon:7: LINE_TO, 4, 14,
> this is almost identical to omnibox_http.icon --- can we use that?
> 
>
https://codereview.chromium.org/2009643003/diff/1/ui/gfx/vector_icons/default...
> File ui/gfx/vector_icons/default_favicon.icon (right):
> 
>
https://codereview.chromium.org/2009643003/diff/1/ui/gfx/vector_icons/default...
> ui/gfx/vector_icons/default_favicon.icon:5: CANVAS_DIMENSIONS, 32,
> where did this one come from? omnibox_http.icon apparently looks good enough
at
> both scale factors that it doesn't need separate icon definitions (according
to
> sgabriel). If we no longer believe that's true, shouldn't we add different
> versions for omnibox_http.icon as well?

Hello estade@,

I didn't realize omnibox_http exists. I can use that, although it seems like
we're setting ourselves up for a redux of this problem that I'm fixing (i.e. if
omnibox_http ever changes to something else it will also change bookmarks).

As far as 1x and 2x sizes, I just recovered the the contents of
location_bar_https_valid.icon and location_bar_https_valid.1x.icon and placed
them into new files. I assumed the existence of both sizes meant they were both
needed.

[Evan Stade, 2016-05-25 18:01:21]

On 2016/05/25 17:36:42, shrike wrote:
> On 2016/05/25 17:23:49, Evan Stade wrote:
> >
>
https://codereview.chromium.org/2009643003/diff/1/ui/gfx/vector_icons/default...
> > File ui/gfx/vector_icons/default_favicon.1x.icon (left):
> > 
> >
>
https://codereview.chromium.org/2009643003/diff/1/ui/gfx/vector_icons/default...
> > ui/gfx/vector_icons/default_favicon.1x.icon:7: LINE_TO, 4, 14,
> > this is almost identical to omnibox_http.icon --- can we use that?
> > 
> >
>
https://codereview.chromium.org/2009643003/diff/1/ui/gfx/vector_icons/default...
> > File ui/gfx/vector_icons/default_favicon.icon (right):
> > 
> >
>
https://codereview.chromium.org/2009643003/diff/1/ui/gfx/vector_icons/default...
> > ui/gfx/vector_icons/default_favicon.icon:5: CANVAS_DIMENSIONS, 32,
> > where did this one come from? omnibox_http.icon apparently looks good enough
> at
> > both scale factors that it doesn't need separate icon definitions (according
> to
> > sgabriel). If we no longer believe that's true, shouldn't we add different
> > versions for omnibox_http.icon as well?
> 
> Hello estade@,
> 
> I didn't realize omnibox_http exists. I can use that, although it seems like
> we're setting ourselves up for a redux of this problem that I'm fixing (i.e.
if
> omnibox_http ever changes to something else it will also change bookmarks).

Yes, people should be careful about searching for references to the images
they're updating. This applies to any image that's used in more than one place.
I can't think of any great ways to enforce this, but I'm open to suggestions. At
least a regression like this is very visible and likely to be found quickly.

> 
> As far as 1x and 2x sizes, I just recovered the the contents of
> location_bar_https_valid.icon and location_bar_https_valid.1x.icon and placed
> them into new files. I assumed the existence of both sizes meant they were
both
> needed.

Ah, I think that those may have been tweaked especially for display in the
location bar. Perhaps we can go with omnibox_http for now and then ask a
designer to verify they look good on canary (or show them screenshots).

[shrike, 2016-05-25 18:23:16]

On 2016/05/25 18:01:21, Evan Stade wrote:
> Yes, people should be careful about searching for references to the images
> they're updating. This applies to any image that's used in more than one
place.
> I can't think of any great ways to enforce this, but I'm open to suggestions.
At
> least a regression like this is very visible and likely to be found quickly.

I will go ahead and make the switch to this existing icon.

One way to make sure things don't break with an icon change is to name these
vector icons by content rather than usage. So changing omnibox_http.icon to
document.icon, for example. That makes the code explicit (right now it's strange
for bookmarks to display an icon labeled "omnibox," and it's not obvious to
someone reading the code what that icon is). If one of the spots that uses the
icon wants to switch to something else, they pick or add a different icon
instead of changing the contents of document.icon

[shrike, 2016-05-25 21:07:09]

The CQ bit was checked by shrike@chromium.org

[shrike, 2016-05-25 21:07:09]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2009643003/#ps20001
(title: "Switch to OMNIBOX_HTTP.")

[commit-bot: I haz the power, 2016-05-25 21:08:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2009643003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2009643003/20001

[commit-bot: I haz the power, 2016-05-25 22:47:52]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-25 22:49:45]

Description was changed from

==========
[Mac][Material Design] Restore bookmark icons to default favicon.

The Material Design bookmark buttons incorrectly used the
location_bar_http vector icon to display a document icon for the
default favicon. This cl adds default favicon vector icons based on the
original version of the location_bar_http vector, and changes the
bookmark buttons to use them.

R=avi@chromium.org, estade@chromium.org
BUG=614539
==========

to

==========
[Mac][Material Design] Restore bookmark icons to default favicon.

The Material Design bookmark buttons incorrectly used the
location_bar_http vector icon to display a document icon for the
default favicon. This cl adds default favicon vector icons based on the
original version of the location_bar_http vector, and changes the
bookmark buttons to use them.

R=avi@chromium.org, estade@chromium.org
BUG=614539

Committed: https://crrev.com/a0776f8a55b798c81fc142ea531c9d8a762c2c85
Cr-Commit-Position: refs/heads/master@{#396020}
==========

[commit-bot: I haz the power, 2016-05-25 22:49:46]

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