[MD] update EV bubble icon and location bar https icons

[MD] update EV bubble icon and location bar https icons

- Lock icon in EV bubble is not supposed to have a background box
- location bar https icon has different colors for incognito

Colors that are hardcoded for inverted color schemes (as they no longer match their native theme values):
- chip border and background colors are white
- selected keyword chip is blue700
These colors could be made part of NativeTheme, but they seem specific to these chips, so I just set them inline.

BUG=501377
Committed: https://crrev.com/e9b57b3e0569268df0e760c7471858856679444a
Cr-Commit-Position: refs/heads/master@{#360447}

[Evan Stade, 2015-11-14 00:08:24]

Description was changed from

==========
[MD] update EV bubble icon and location bar https icons

- EV bubble icon is not supposed to have a background box
- location bar https icon has different colors for incognito
==========

to

==========
[MD] update EV bubble icon and location bar https icons

- Lock icon in EV bubble is not supposed to have a background box
- location bar https icon has different colors for incognito

Colors that are hardcoded for inverted color schemes (as they no longer match
their native theme values):
- chip border and background colors are white
- selected keyword chip is blue700
==========

[Evan Stade, 2015-11-17 20:31:14]

Description was changed from

==========
[MD] update EV bubble icon and location bar https icons

- Lock icon in EV bubble is not supposed to have a background box
- location bar https icon has different colors for incognito

Colors that are hardcoded for inverted color schemes (as they no longer match
their native theme values):
- chip border and background colors are white
- selected keyword chip is blue700
==========

to

==========
[MD] update EV bubble icon and location bar https icons

- Lock icon in EV bubble is not supposed to have a background box
- location bar https icon has different colors for incognito

Colors that are hardcoded for inverted color schemes (as they no longer match
their native theme values):
- chip border and background colors are white
- selected keyword chip is blue700
These colors could be made part of NativeTheme, but they seem specific to these
chips, so I just set them inline.
==========

[Evan Stade, 2015-11-17 20:31:14]

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

[Evan Stade, 2015-11-17 20:31:58]

Description was changed from

==========
[MD] update EV bubble icon and location bar https icons

- Lock icon in EV bubble is not supposed to have a background box
- location bar https icon has different colors for incognito

Colors that are hardcoded for inverted color schemes (as they no longer match
their native theme values):
- chip border and background colors are white
- selected keyword chip is blue700
These colors could be made part of NativeTheme, but they seem specific to these
chips, so I just set them inline.
==========

to

==========
[MD] update EV bubble icon and location bar https icons

- Lock icon in EV bubble is not supposed to have a background box
- location bar https icon has different colors for incognito

Colors that are hardcoded for inverted color schemes (as they no longer match
their native theme values):
- chip border and background colors are white
- selected keyword chip is blue700
These colors could be made part of NativeTheme, but they seem specific to these
chips, so I just set them inline.

BUG=501377
==========

[Evan Stade, 2015-11-17 20:33:16]

I posted a (1x) screenshot on the bug. The colors and icons are correct on this
screenshot, although I noticed the in_chip icon looks oddly squared off. I'll
follow up with Sebastien about that, but I don't think it needs to block this
patch.

[Peter Kasting, 2015-11-17 21:27:03]

LGTM

https://codereview.chromium.org/1448433004/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/1448433004/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:137:
ui::NativeTheme::kColorId_TextfieldDefaultBackground));
The code to get this background color is repeated in SetLabelBackgroundColor().

I think it might make sense to have a member function that returns that color. 
Then the question is whether we still need IsInvertedColorScheme(), or whether
the two callsites of it can just do

  bool inverted = color_utils::IsDark(GetBackgroundColor());

...which is almost as short and seems just about as clear.  This would also
match e.g. LocationBarView::GetSecureTextColor().

https://codereview.chromium.org/1448433004/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1448433004/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:872: //
|omnibox_view_| may not be ready yet.
If you really need this conditional, this comment might be better as something
like "This can be called before LocationBarView::Init(), in which case we can
just ignore it; OnChanged() will call us shortly and we can set the correct icon
then."

(I don't love this wording, I'm just trying to get the general idea across.)

https://codereview.chromium.org/1448433004/diff/80001/components/omnibox/brow...
File components/omnibox/browser/omnibox_view.cc (right):

https://codereview.chromium.org/1448433004/diff/80001/components/omnibox/brow...
components/omnibox/browser/omnibox_view.cc:99: }
Nit: Shorter:

    gfx::VectorIconId id = controller_->GetToolbarModel()->GetVectorIcon();
    if (invert) {
      if (id == gfx::VectorIconId::LOCATION_BAR_HTTPS_VALID)
        return gfx::VectorIconId::LOCATION_BAR_HTTPS_VALID_INVERT;
      if (id == gfx::VectorIconId::LOCATION_BAR_HTTPS_INVALID)
        return gfx::VectorIconId::LOCATION_BAR_HTTPS_INVALID_INVERT;
    }
    return id;

https://codereview.chromium.org/1448433004/diff/80001/components/omnibox/brow...
File components/omnibox/browser/omnibox_view.h (right):

https://codereview.chromium.org/1448433004/diff/80001/components/omnibox/brow...
components/omnibox/browser/omnibox_view.h:79: // change in a dark theme, hence
|invert|.
Nit: Maybe this second sentence: "If |invert| is true, this returns an icon
suitable for display in an inverted (light-on-dark) color scheme."

https://codereview.chromium.org/1448433004/diff/80001/ui/gfx/vector_icons/loc...
File ui/gfx/vector_icons/location_bar_https_invalid_invert.icon (right):

https://codereview.chromium.org/1448433004/diff/80001/ui/gfx/vector_icons/loc...
ui/gfx/vector_icons/location_bar_https_invalid_invert.icon:6: PATH_COLOR_ARGB,
0xFF, 0xFF, 0xFF, 0xFF,
Might it make sense to compose this icon (and similar) from two pieces?  Then
code wanting either normal or inverted icons could always display both pieces,
and simply give them swapped colors when creating them.

This would end up being the same number of icon files overall, but the files
would be smaller and non-overlapping, and more importantly, the colors to use
would stay in the code rather than being hardcoded into the icon.

[Evan Stade, 2015-11-17 22:57:22]

https://codereview.chromium.org/1448433004/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/1448433004/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:137:
ui::NativeTheme::kColorId_TextfieldDefaultBackground));
On 2015/11/17 21:27:03, Peter Kasting wrote:
> The code to get this background color is repeated in
SetLabelBackgroundColor().
> 
> I think it might make sense to have a member function that returns that color.

> Then the question is whether we still need IsInvertedColorScheme(), or whether
> the two callsites of it can just do
> 
>   bool inverted = color_utils::IsDark(GetBackgroundColor());
> 
> ...which is almost as short and seems just about as clear.  This would also
> match e.g. LocationBarView::GetSecureTextColor().

Done.

https://codereview.chromium.org/1448433004/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1448433004/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:872: //
|omnibox_view_| may not be ready yet.
On 2015/11/17 21:27:03, Peter Kasting wrote:
> If you really need this conditional, this comment might be better as something
> like "This can be called before LocationBarView::Init(), in which case we can
> just ignore it; OnChanged() will call us shortly and we can set the correct
icon
> then."
> 
> (I don't love this wording, I'm just trying to get the general idea across.)

Done.

https://codereview.chromium.org/1448433004/diff/80001/components/omnibox/brow...
File components/omnibox/browser/omnibox_view.h (right):

https://codereview.chromium.org/1448433004/diff/80001/components/omnibox/brow...
components/omnibox/browser/omnibox_view.h:79: // change in a dark theme, hence
|invert|.
On 2015/11/17 21:27:03, Peter Kasting wrote:
> Nit: Maybe this second sentence: "If |invert| is true, this returns an icon
> suitable for display in an inverted (light-on-dark) color scheme."

Done.

https://codereview.chromium.org/1448433004/diff/80001/ui/gfx/vector_icons/loc...
File ui/gfx/vector_icons/location_bar_https_invalid_invert.icon (right):

https://codereview.chromium.org/1448433004/diff/80001/ui/gfx/vector_icons/loc...
ui/gfx/vector_icons/location_bar_https_invalid_invert.icon:6: PATH_COLOR_ARGB,
0xFF, 0xFF, 0xFF, 0xFF,
On 2015/11/17 21:27:03, Peter Kasting wrote:
> Might it make sense to compose this icon (and similar) from two pieces?  Then
> code wanting either normal or inverted icons could always display both pieces,
> and simply give them swapped colors when creating them.
> 
> This would end up being the same number of icon files overall, but the files
> would be smaller and non-overlapping, and more importantly, the colors to use
> would stay in the code rather than being hardcoded into the icon.

This would be tricky because the 1x versions have extra paths with subtly
different colors. I thought about making it possible to pass a vector of colors
to PaintVectorIcon but this added complexity didn't seem worth it until we need
it in more places or it saves us more icon files.

[Evan Stade, 2015-11-17 22:57:41]

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

[commit-bot: I haz the power, 2015-11-17 22:59:57]

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

[commit-bot: I haz the power, 2015-11-17 23:18:09]

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

[commit-bot: I haz the power, 2015-11-17 23:18:10]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Evan Stade, 2015-11-18 00:51:55]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-11-18 00:51:55]

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/1448433004/#ps120001
(title: "fix build.gn")

[commit-bot: I haz the power, 2015-11-18 00:53:38]

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

[commit-bot: I haz the power, 2015-11-18 01:07:44]

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

[commit-bot: I haz the power, 2015-11-18 01:07:44]

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

[Evan Stade, 2015-11-18 23:07:59]

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

[Evan Stade, 2015-11-18 23:08:00]

+sadrul for color_utils

[sadrul, 2015-11-18 23:11:05]

lgtm

[Evan Stade, 2015-11-18 23:13:59]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-11-18 23:16:55]

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

[commit-bot: I haz the power, 2015-11-18 23:57:53]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-18 23:58:48]

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