Remove non-md code in location bar (Views).

Remove non-md code in location bar (Views).

Some of the assets and APIs are left behind because Mac still uses them for now. OmniboxView::GetIcon and ToolbarModel::GetIcon are two examples.

BUG=648281
TBR=oshima@chromium.org

Committed: https://crrev.com/8bb3fa56dda5f1eb17f796e0a2fca965e14aa28d
Cr-Commit-Position: refs/heads/master@{#420142}

[Evan Stade, 2016-09-16 23:29:24]

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

[commit-bot: I haz the power, 2016-09-16 23:29:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 23:57:41]

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

[commit-bot: I haz the power, 2016-09-16 23:57:42]

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

[Evan Stade, 2016-09-19 20:21:44]

Description was changed from

==========
Remove non-md code in location bar.

BUG=none
==========

to

==========
Remove non-md code in location bar (Views).

Some of the assets and APIs are left behind because Mac still uses them for now.
OmniboxView::GetIcon and ToolbarModel::GetIcon are two examples.

BUG=648281
==========

[Evan Stade, 2016-09-19 20:21:44]

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

[Evan Stade, 2016-09-19 20:28:23]

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

[commit-bot: I haz the power, 2016-09-19 20:28:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 20:39:04]

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

[commit-bot: I haz the power, 2016-09-19 20:39:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Kasting, 2016-09-19 23:46:46]

I commented on a couple #includes that looked unnecessary, but check for others

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right):

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/bubble_icon_view.cc:188: const int
icon_size = 16;
Nit: Make constexpr, name kConstantStyle... or just inline it

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

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:20: #include
"ui/views/painter.h"
Nit: Maybe don't need this #include

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:77: return
label_->visible() && !label_->text().empty();
Is this logic synthesized from the cases when we called SetBackground() in the
location bar code?  There are a few different layers in the old code and I
didn't check all of them to ensure this is never a behavior change.

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:113: // same, so
it's not necessary to handle the two types differently.
Nit: This comment needs updating

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:115: int
bubble_trailing_padding = GetOuterPadding();
Nit: I'd just do "bubble_trailing_padding = image_x;" to avoid the call

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:226: return
GetLayoutConstant(LOCATION_BAR_HORIZONTAL_PADDING);
Nit: Seems like we could just make this call directly in the callers, the helper
no longer buys much

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/keyword_hint_view.cc (right):

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/keyword_hint_view.cc:31: #include
"ui/views/controls/image_view.h"
Nit: Maybe don't need this #include

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

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:201: gfx::FontList
chip_font_list = font_list;
Nit: Just use |font_list| below directly

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:335: }
Nit: If we pull out the SECURITY_ERROR case we can save a line:

  const bool is_security_error =
      (security_level == security_state::SecurityStateModel::SECURITY_ERROR);
  if (is_security_error ||
      (security_level == security_state::SecurityStateModel::EV_SECURE) ||
      (security_level == security_state::SecurityStateModel::SECURE)) {
    if (color_utils::IsDark(GetColor(BACKGROUND)))
      return text_color;
    text_color = is_security_error ? gfx::kGoogleRed700 : gfx::kGoogleGreen700;
  }

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:397: return
kVectorIconSize;
Nit: Since this is no longer computed at runtime, could just convert to a public
constexpr in the header (no accessor function needed).

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:446: gfx::Rect
location_bar_bounds(bounds());
Nit: Just use x() and bounds.right() directly below

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:482:
min_size.set_height(GetLayoutConstant(LOCATION_BAR_HEIGHT));
Nit: Maybe:

  gfx::Size min_size(0, GetLayoutConstant(LOCATION_BAR_HEIGHT));

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:799: return
GetLayoutConstant(LOCATION_BAR_BORDER_THICKNESS);
Nit: At this point I'd inline this into the callers; this might allow us to
rename GetVerticalEdgeThicknessWithPadding() to just GetVerticalEdgeThickness()
(though I'm not 100% sure that's the right thing to do)

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.h:126: // Returns the
color for the location bar border in Windows, given the
Nit: Should have been "windows" rather than "Windows" (it's not referring to the
OS), but I'd just remove "in Windows" entirely and avoid the ambiguity.

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_icon_view.cc:102: NOTIMPLEMENTED()
<< " HITTING THIS";
Um.

Should we ever reach here?  Maybe the right thing to do is to remove this
method?

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_icon_view.cc:146:
OnNativeThemeChanged(GetNativeTheme());
Is this a bugfix?

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/selected_keyword_view.cc (right):

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/selected_keyword_view.cc:98:
SetLabel(full_name);
Is this a bugfix?

[Evan Stade, 2016-09-20 17:37:03]

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

[Evan Stade, 2016-09-20 17:37:04]

+oshima tbr for asset removal

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right):

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/bubble_icon_view.cc:188: const int
icon_size = 16;
On 2016/09/19 23:46:45, Peter Kasting wrote:
> Nit: Make constexpr, name kConstantStyle... or just inline it

Done.

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

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:20: #include
"ui/views/painter.h"
On 2016/09/19 23:46:45, Peter Kasting wrote:
> Nit: Maybe don't need this #include

Done.

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:77: return
label_->visible() && !label_->text().empty();
On 2016/09/19 23:46:45, Peter Kasting wrote:
> Is this logic synthesized from the cases when we called SetBackground() in the
> location bar code?  There are a few different layers in the old code and I
> didn't check all of them to ensure this is never a behavior change.

In LocationBarView, it's only called on LocationIconView, and you can see that
SetLabel and SetBackground are called together. In other cases I believe it also
retains the same behavior.

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:113: // same, so
it's not necessary to handle the two types differently.
On 2016/09/19 23:46:45, Peter Kasting wrote:
> Nit: This comment needs updating

Done.

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:115: int
bubble_trailing_padding = GetOuterPadding();
On 2016/09/19 23:46:45, Peter Kasting wrote:
> Nit: I'd just do "bubble_trailing_padding = image_x;" to avoid the call

Done.

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:226: return
GetLayoutConstant(LOCATION_BAR_HORIZONTAL_PADDING);
On 2016/09/19 23:46:45, Peter Kasting wrote:
> Nit: Seems like we could just make this call directly in the callers, the
helper
> no longer buys much

Done.

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/keyword_hint_view.cc (right):

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/keyword_hint_view.cc:31: #include
"ui/views/controls/image_view.h"
On 2016/09/19 23:46:45, Peter Kasting wrote:
> Nit: Maybe don't need this #include

Done.

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

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:201: gfx::FontList
chip_font_list = font_list;
On 2016/09/19 23:46:46, Peter Kasting wrote:
> Nit: Just use |font_list| below directly

Done.

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:335: }
On 2016/09/19 23:46:45, Peter Kasting wrote:
> Nit: If we pull out the SECURITY_ERROR case we can save a line:
> 
>   const bool is_security_error =
>       (security_level == security_state::SecurityStateModel::SECURITY_ERROR);
>   if (is_security_error ||
>       (security_level == security_state::SecurityStateModel::EV_SECURE) ||
>       (security_level == security_state::SecurityStateModel::SECURE)) {
>     if (color_utils::IsDark(GetColor(BACKGROUND)))
>       return text_color;
>     text_color = is_security_error ? gfx::kGoogleRed700 :
gfx::kGoogleGreen700;
>   }

refactored slightly so we don't have to make that comparison twice (or store it
in a local variable)

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:397: return
kVectorIconSize;
On 2016/09/19 23:46:46, Peter Kasting wrote:
> Nit: Since this is no longer computed at runtime, could just convert to a
public
> constexpr in the header (no accessor function needed).

Done.

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:446: gfx::Rect
location_bar_bounds(bounds());
On 2016/09/19 23:46:45, Peter Kasting wrote:
> Nit: Just use x() and bounds.right() directly below

Done.

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:482:
min_size.set_height(GetLayoutConstant(LOCATION_BAR_HEIGHT));
On 2016/09/19 23:46:45, Peter Kasting wrote:
> Nit: Maybe:
> 
>   gfx::Size min_size(0, GetLayoutConstant(LOCATION_BAR_HEIGHT));

Done.

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:799: return
GetLayoutConstant(LOCATION_BAR_BORDER_THICKNESS);
On 2016/09/19 23:46:46, Peter Kasting wrote:
> Nit: At this point I'd inline this into the callers; this might allow us to
> rename GetVerticalEdgeThicknessWithPadding() to just
GetVerticalEdgeThickness()
> (though I'm not 100% sure that's the right thing to do)

done, renamed the ...WithPadding fn to GetTotalVerticalPadding.

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.h:126: // Returns the
color for the location bar border in Windows, given the
On 2016/09/19 23:46:46, Peter Kasting wrote:
> Nit: Should have been "windows" rather than "Windows" (it's not referring to
the
> OS), but I'd just remove "in Windows" entirely and avoid the ambiguity.

Done.

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_icon_view.cc:102: NOTIMPLEMENTED()
<< " HITTING THIS";
On 2016/09/19 23:46:46, Peter Kasting wrote:
> Um.
> 
> Should we ever reach here?  Maybe the right thing to do is to remove this
> method?

oops

yes we need this function still. We don't need GetBorderColor though (there are
no borders any more and the separator is based on default textfield color).

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_icon_view.cc:146:
OnNativeThemeChanged(GetNativeTheme());
On 2016/09/19 23:46:46, Peter Kasting wrote:
> Is this a bugfix?

no. It worked before because this line was in SetBackgroundImageGrid.

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/selected_keyword_view.cc (right):

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/selected_keyword_view.cc:98:
SetLabel(full_name);
On 2016/09/19 23:46:46, Peter Kasting wrote:
> Is this a bugfix?

not really, it replaces the SetBackgroundImageGrid call from before. It's the
simplest way to get ShouldShowLabel to return true. The label may be updated in
Layout().

[Evan Stade, 2016-09-20 17:37:12]

Description was changed from

==========
Remove non-md code in location bar (Views).

Some of the assets and APIs are left behind because Mac still uses them for now.
OmniboxView::GetIcon and ToolbarModel::GetIcon are two examples.

BUG=648281
==========

to

==========
Remove non-md code in location bar (Views).

Some of the assets and APIs are left behind because Mac still uses them for now.
OmniboxView::GetIcon and ToolbarModel::GetIcon are two examples.

BUG=648281
TBR=oshima@chromium.org
==========

[Evan Stade, 2016-09-20 17:37:12]

estade@chromium.org changed reviewers:
- oshima@chromium.org

[Evan Stade, 2016-09-20 17:37:40]

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

[commit-bot: I haz the power, 2016-09-20 17:38:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2016-09-20 17:45:37]

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

[commit-bot: I haz the power, 2016-09-20 17:46:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 17:59:50]

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

[commit-bot: I haz the power, 2016-09-20 17:59:51]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Evan Stade, 2016-09-20 20:02:40]

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

[commit-bot: I haz the power, 2016-09-20 20:03:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2016-09-20 20:03:47]

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

[Peter Kasting, 2016-09-20 20:18:05]

LGTM

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

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:335: }
On 2016/09/20 17:37:04, Evan Stade wrote:
> On 2016/09/19 23:46:45, Peter Kasting wrote:
> > Nit: If we pull out the SECURITY_ERROR case we can save a line:
> > 
> >   const bool is_security_error =
> >       (security_level ==
security_state::SecurityStateModel::SECURITY_ERROR);
> >   if (is_security_error ||
> >       (security_level == security_state::SecurityStateModel::EV_SECURE) ||
> >       (security_level == security_state::SecurityStateModel::SECURE)) {
> >     if (color_utils::IsDark(GetColor(BACKGROUND)))
> >       return text_color;
> >     text_color = is_security_error ? gfx::kGoogleRed700 :
> gfx::kGoogleGreen700;
> >   }
> 
> refactored slightly so we don't have to make that comparison twice (or store
it
> in a local variable)

Technically this is a behavior change since we now run through
GetReadableColor() for the IsDark() case, but that's probably a good change,
since if it could have any effect, we'd _want_ the effect.

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/selected_keyword_view.cc (right):

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/selected_keyword_view.cc:98:
SetLabel(full_name);
On 2016/09/20 17:37:04, Evan Stade wrote:
> On 2016/09/19 23:46:46, Peter Kasting wrote:
> > Is this a bugfix?
> 
> not really, it replaces the SetBackgroundImageGrid call from before. It's the
> simplest way to get ShouldShowLabel to return true. The label may be updated
in
> Layout().

Should we just call Layout() here?

Maybe this deserves a comment on why it's here/why we need ShouldShowLabel() to
be true from this point and can't wait for layout?

https://codereview.chromium.org/2348853004/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2348853004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:544:
GetLayoutConstant(LOCATION_BAR_VERTICAL_PADDING);
Nit: Use GetTotalVerticalPadding()

https://codereview.chromium.org/2348853004/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/selected_keyword_view.cc (right):

https://codereview.chromium.org/2348853004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/selected_keyword_view.cc:35:
gfx::CreateVectorIcon(gfx::VectorIconId::OMNIBOX_SEARCH, GetTextColor()));
Explicit size not needed?

[commit-bot: I haz the power, 2016-09-20 20:39:16]

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

[commit-bot: I haz the power, 2016-09-20 20:39:17]

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

[Evan Stade, 2016-09-21 17:05:00]

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

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:335: }
On 2016/09/20 20:18:05, Peter Kasting wrote:
> On 2016/09/20 17:37:04, Evan Stade wrote:
> > On 2016/09/19 23:46:45, Peter Kasting wrote:
> > > Nit: If we pull out the SECURITY_ERROR case we can save a line:
> > > 
> > >   const bool is_security_error =
> > >       (security_level ==
> security_state::SecurityStateModel::SECURITY_ERROR);
> > >   if (is_security_error ||
> > >       (security_level == security_state::SecurityStateModel::EV_SECURE) ||
> > >       (security_level == security_state::SecurityStateModel::SECURE)) {
> > >     if (color_utils::IsDark(GetColor(BACKGROUND)))
> > >       return text_color;
> > >     text_color = is_security_error ? gfx::kGoogleRed700 :
> > gfx::kGoogleGreen700;
> > >   }
> > 
> > refactored slightly so we don't have to make that comparison twice (or store
> it
> > in a local variable)
> 
> Technically this is a behavior change since we now run through
> GetReadableColor() for the IsDark() case, but that's probably a good change,
> since if it could have any effect, we'd _want_ the effect.

yea. I actually think we shouldn't have to call GetReadableColor here ---
concretely, when could it change the color? But it seems safer to leave it if
someone decided it was necessary at some point.

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/selected_keyword_view.cc (right):

https://codereview.chromium.org/2348853004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/selected_keyword_view.cc:98:
SetLabel(full_name);
On 2016/09/20 20:18:05, Peter Kasting wrote:
> On 2016/09/20 17:37:04, Evan Stade wrote:
> > On 2016/09/19 23:46:46, Peter Kasting wrote:
> > > Is this a bugfix?
> > 
> > not really, it replaces the SetBackgroundImageGrid call from before. It's
the
> > simplest way to get ShouldShowLabel to return true. The label may be updated
> in
> > Layout().
> 
> Should we just call Layout() here?
> 
> Maybe this deserves a comment on why it's here/why we need ShouldShowLabel()
to
> be true from this point and can't wait for layout?

added a comment

https://codereview.chromium.org/2348853004/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2348853004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:544:
GetLayoutConstant(LOCATION_BAR_VERTICAL_PADDING);
On 2016/09/20 20:18:05, Peter Kasting wrote:
> Nit: Use GetTotalVerticalPadding()

oops, done.

https://codereview.chromium.org/2348853004/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/selected_keyword_view.cc (right):

https://codereview.chromium.org/2348853004/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/selected_keyword_view.cc:35:
gfx::CreateVectorIcon(gfx::VectorIconId::OMNIBOX_SEARCH, GetTextColor()));
On 2016/09/20 20:18:05, Peter Kasting wrote:
> Explicit size not needed?

correct, it's part of this icon's definition

[Evan Stade, 2016-09-21 17:05:08]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-09-21 17:05:08]

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/2348853004/#ps120001
(title: "few more review updates")

[commit-bot: I haz the power, 2016-09-21 17:06:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 17:19:28]

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

[commit-bot: I haz the power, 2016-09-21 17:19:30]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Evan Stade, 2016-09-21 19:24:50]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-09-21 19:24:51]

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/2348853004/#ps140001
(title: "resolve change collision")

[commit-bot: I haz the power, 2016-09-21 19:25:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 20:10:55]

Description was changed from

==========
Remove non-md code in location bar (Views).

Some of the assets and APIs are left behind because Mac still uses them for now.
OmniboxView::GetIcon and ToolbarModel::GetIcon are two examples.

BUG=648281
TBR=oshima@chromium.org
==========

to

==========
Remove non-md code in location bar (Views).

Some of the assets and APIs are left behind because Mac still uses them for now.
OmniboxView::GetIcon and ToolbarModel::GetIcon are two examples.

BUG=648281
TBR=oshima@chromium.org
==========

[commit-bot: I haz the power, 2016-09-21 20:10:57]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-09-21 20:13:06]

Description was changed from

==========
Remove non-md code in location bar (Views).

Some of the assets and APIs are left behind because Mac still uses them for now.
OmniboxView::GetIcon and ToolbarModel::GetIcon are two examples.

BUG=648281
TBR=oshima@chromium.org
==========

to

==========
Remove non-md code in location bar (Views).

Some of the assets and APIs are left behind because Mac still uses them for now.
OmniboxView::GetIcon and ToolbarModel::GetIcon are two examples.

BUG=648281
TBR=oshima@chromium.org

Committed: https://crrev.com/8bb3fa56dda5f1eb17f796e0a2fca965e14aa28d
Cr-Commit-Position: refs/heads/master@{#420142}
==========

[commit-bot: I haz the power, 2016-09-21 20:13:07]

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