Vectorize omnibox dropdown icons

Vectorize omnibox dropdown icons

Only some of them require special-casing for 1x.

BUG=542443
TBR=sky@chromium.org

Committed: https://crrev.com/07775d2d83230ad94fde14e65d46513ad9fd6cd3
Cr-Commit-Position: refs/heads/master@{#355153}

[Evan Stade, 2015-10-20 00:49:11]

Description was changed from

==========
Vectorize (most) omnibox dropdown icons

Only some of them require special-casing for 1x. History doesn't require any
tweaks at all from the go/icons base (hence no OMNIBOX_ prefix).

BUG=542443
==========

to

==========
Vectorize omnibox dropdown icons

Only some of them require special-casing for 1x. History doesn't require any
tweaks at all from the go/icons base (hence no OMNIBOX_ prefix).

BUG=542443
==========

[Evan Stade, 2015-10-20 00:49:12]

estade@chromium.org changed reviewers:
+ pkasting@chromium.org

[Evan Stade, 2015-10-20 00:49:13]

https://codereview.chromium.org/1418543002/diff/20001/components/omnibox/brow...
File components/omnibox/browser/autocomplete_match.h (right):

https://codereview.chromium.org/1418543002/diff/20001/components/omnibox/brow...
components/omnibox/browser/autocomplete_match.h:110: static gfx::VectorIconId
TypeToVectorIcon(Type type);
we may want this to return a gfx::Image, but if that becomes clear later it
should be easy to change.

[Peter Kasting, 2015-10-20 01:03:00]

LGTM

https://codereview.chromium.org/1418543002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/1418543002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:574: return
GetMaterialIcon();
Nit: I dunno if separating this into its own helper buys much, especially since
we need to explain what the function is in the .h file.  I'd probably just
inline the helper code here.

https://codereview.chromium.org/1418543002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:607: return
gfx::CreateVectorIcon(icon_id, 16, gfx::kChromeIconGrey);
Using grey here will be problematic for dropdowns with a non-white background. 
(This is an existing bug with the images used today, that we can now fix.)  We
should probably use color_utils::BlendTowardOppositeLuminance() with the
dropdown background color, and choose an alpha that results in the same color as
this grey when used with white backgrounds.

https://codereview.chromium.org/1418543002/diff/20001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/1418543002/diff/20001/ui/gfx/BUILD.gn#newcode458
ui/gfx/BUILD.gn:458: "vector_icons/history.icon",
Will we be using this in non-omnibox contexts later?  If not we should probably
name it omnibox_history anyway to reflect its usage in this codebase; reading
the code as-is, it implies the omnibox is stealing and reusing some random
history icon from elsewhere in Chrome or something.

[Evan Stade, 2015-10-20 01:12:10]

https://codereview.chromium.org/1418543002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/1418543002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:574: return
GetMaterialIcon();
On 2015/10/20 01:02:59, Peter Kasting wrote:
> Nit: I dunno if separating this into its own helper buys much, especially
since
> we need to explain what the function is in the .h file.  I'd probably just
> inline the helper code here.

Done.

https://codereview.chromium.org/1418543002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:607: return
gfx::CreateVectorIcon(icon_id, 16, gfx::kChromeIconGrey);
On 2015/10/20 01:02:59, Peter Kasting wrote:
> Using grey here will be problematic for dropdowns with a non-white background.

> (This is an existing bug with the images used today, that we can now fix.)  We
> should probably use color_utils::BlendTowardOppositeLuminance() with the
> dropdown background color, and choose an alpha that results in the same color
as
> this grey when used with white backgrounds.

I didn't even realize the dropdown could change. Is that done for inverted color
schemes on Windows or something?

https://codereview.chromium.org/1418543002/diff/20001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/1418543002/diff/20001/ui/gfx/BUILD.gn#newcode458
ui/gfx/BUILD.gn:458: "vector_icons/history.icon",
On 2015/10/20 01:02:59, Peter Kasting wrote:
> Will we be using this in non-omnibox contexts later?  If not we should
probably
> name it omnibox_history anyway to reflect its usage in this codebase; reading
> the code as-is, it implies the omnibox is stealing and reusing some random
> history icon from elsewhere in Chrome or something.

yes, BackForwardMenuModel should use it. And on osx,
browser/ui/cocoa/history_menu_bridge.mm. And if we could figure out a way to use
vector icons for favicons, chrome://history would use it too.

[Evan Stade, 2015-10-20 01:12:24]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-10-20 01:12:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1418543002/#ps40001
(title: "pkasting review")

[commit-bot: I haz the power, 2015-10-20 01:13:24]

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

[commit-bot: I haz the power, 2015-10-20 01:23:54]

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

[commit-bot: I haz the power, 2015-10-20 01:23:54]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Evan Stade, 2015-10-20 01:25:20]

Description was changed from

==========
Vectorize omnibox dropdown icons

Only some of them require special-casing for 1x. History doesn't require any
tweaks at all from the go/icons base (hence no OMNIBOX_ prefix).

BUG=542443
==========

to

==========
Vectorize omnibox dropdown icons

Only some of them require special-casing for 1x.

BUG=542443
==========

[Evan Stade, 2015-10-20 01:25:48]

estade@chromium.org changed reviewers:
+ sky@chromium.org

[Evan Stade, 2015-10-20 01:25:49]

+sky for ui/gfx/BUILD.gn

https://codereview.chromium.org/1418543002/diff/40001/components/omnibox/brow...
File components/omnibox/browser/autocomplete_match.cc (right):

https://codereview.chromium.org/1418543002/diff/40001/components/omnibox/brow...
components/omnibox/browser/autocomplete_match.cc:228:
gfx::VectorIconId::HISTORY,                // HISTORY_URL
oops, I copied this block from the ios one above. We don't actually need history
or star. Fixed.

[Evan Stade, 2015-10-20 01:25:58]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-20 01:26:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1418543002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1418543002/60001

[commit-bot: I haz the power, 2015-10-20 01:35:00]

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

[commit-bot: I haz the power, 2015-10-20 01:35:00]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Peter Kasting, 2015-10-20 01:39:18]

https://codereview.chromium.org/1418543002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/1418543002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:607: return
gfx::CreateVectorIcon(icon_id, 16, gfx::kChromeIconGrey);
On 2015/10/20 01:12:09, Evan Stade wrote:
> On 2015/10/20 01:02:59, Peter Kasting wrote:
> > Using grey here will be problematic for dropdowns with a non-white
background.
> 
> > (This is an existing bug with the images used today, that we can now fix.) 
We
> > should probably use color_utils::BlendTowardOppositeLuminance() with the
> > dropdown background color, and choose an alpha that results in the same
color
> as
> > this grey when used with white backgrounds.
> 
> I didn't even realize the dropdown could change. Is that done for inverted
color
> schemes on Windows or something?

Yes.  It's supposed to use the system window color.  I think there might have
been a bug introduced in a rewrite where it only currently does that on GTK and
not Win, though, based on code inspection.  In any case, the color is attainable
from GetColor(state, BACKGROUND);.  This depends on the current state because we
might need a different color in a normal row versus a hovered one.

[Evan Stade, 2015-10-20 16:41:50]

Description was changed from

==========
Vectorize omnibox dropdown icons

Only some of them require special-casing for 1x.

BUG=542443
==========

to

==========
Vectorize omnibox dropdown icons

Only some of them require special-casing for 1x.

BUG=542443
TBR=sky@chromium.org
==========

[Evan Stade, 2015-10-20 16:42:06]

tbr'ing sky for ui/gfx/BUILD.gn

[Evan Stade, 2015-10-20 16:42:12]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-10-20 16:42:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1418543002/#ps100001
(title: "compile")

[commit-bot: I haz the power, 2015-10-20 16:42:27]

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

[commit-bot: I haz the power, 2015-10-20 17:40:44]

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

[commit-bot: I haz the power, 2015-10-20 17:40:45]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Evan Stade, 2015-10-20 19:51:47]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-10-20 19:51:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1418543002/#ps120001
(title: "add gyp dependency")

[commit-bot: I haz the power, 2015-10-20 19:52:23]

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

[commit-bot: I haz the power, 2015-10-20 21:17:30]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-20 21:18:18]

Patchset 7 (id:??) landed as
https://crrev.com/07775d2d83230ad94fde14e65d46513ad9fd6cd3
Cr-Commit-Position: refs/heads/master@{#355153}