Update appearance of omnibox focus for MD.

Update appearance of omnibox focus for MD.

Also remove some dead focus painter code in TextField.
Also remove OmniboxEditController::OnSetFocus(), which was totally dead on Views and not necessary on Cocoa.

BUG=591140
Committed: https://crrev.com/333ac64c2e01fce66aa363da999dbcffa9724343
Cr-Commit-Position: refs/heads/master@{#395213}

[Evan Stade, 2016-05-20 00:56:40]

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

[Evan Stade, 2016-05-20 00:56:41]

https://codereview.chromium.org/2001433002/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2001433002/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:106: const SkColor
kBorderColor = SkColorSetA(SK_ColorBLACK, 0x4D);
I'm going to look at whether we can align this with UnfocusedBorderColor as a
TODO

https://codereview.chromium.org/2001433002/diff/1/ui/native_theme/common_them...
File ui/native_theme/common_theme.cc (left):

https://codereview.chromium.org/2001433002/diff/1/ui/native_theme/common_them...
ui/native_theme/common_theme.cc:104: if
(ui::MaterialDesignController::IsSecondaryUiMaterial()) {
this change means that secondary ui surfaces which use this color will get a
different color even without the --secondary-ui-md flag. The color change is so
slight I don't think this is a problem. Non-MD should remain unaffected.

[sky, 2016-05-20 16:08:00]

LGTM

https://codereview.chromium.org/2001433002/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2001433002/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1265: SchedulePaint();
Is this only necessary if modematerial and show_focus_rect?

[Evan Stade, 2016-05-20 18:01:32]

Description was changed from

==========
Update appearance of omnibox focus for MD.

Also remove some dead focus painter code in TextField.

BUG=591140
==========

to

==========
Update appearance of omnibox focus for MD.

Also remove some dead focus painter code in TextField.
Also remove OmniboxEditController::OnSetFocus(), which was totally dead on Views
and not necessary on Cocoa.

BUG=591140
==========

[Evan Stade, 2016-05-20 18:01:59]

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

[Evan Stade, 2016-05-20 18:02:00]

+pkasting for components/omnibox

https://codereview.chromium.org/2001433002/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2001433002/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1265: SchedulePaint();
On 2016/05/20 16:08:00, sky wrote:
> Is this only necessary if modematerial and show_focus_rect?

hmm, turns out this didn't actually work (although at first it seemed like it
did). See updates --- OmniboxViewViews has to do this in its OnBlur. Observing
the focus manager from LocationBarView is a pain because of the need to remove
the listener when either |this| or the widget is destructing.

Also, I found that OnSetFocus() was dead code on Views and not necessary on
Cocoa, so I removed that.

[Evan Stade, 2016-05-20 18:02:55]

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

[commit-bot: I haz the power, 2016-05-20 18:03:12]

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

[Evan Stade, 2016-05-20 18:11:58]

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

[commit-bot: I haz the power, 2016-05-20 18:12:11]

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

[Evan Stade, 2016-05-20 18:12:19]

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

[Evan Stade, 2016-05-20 18:12:19]

+pkl for ios

[commit-bot: I haz the power, 2016-05-20 18:58:52]

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

[commit-bot: I haz the power, 2016-05-20 18:58:53]

Dry run: This issue passed the CQ dry run.

[pkl (ping after 24h if needed), 2016-05-20 20:30:16]

pkl@chromium.org changed reviewers:
+ eugenebut@chromium.org

[pkl (ping after 24h if needed), 2016-05-20 20:30:17]

+eugenebut

[Peter Kasting, 2016-05-20 20:32:49]

Mostly OK, just one issue

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

https://codereview.chromium.org/2001433002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1274:
paint.setStrokeWidth(1);
This will scale up on higher scale factors, which the code in
BackgroundWith1PxBorder won't do.  It seems like this should match that
behavior, just in a different color.

This suggests to me that maybe instead of adding this code here,
BackgroundWith1PxBorder should just do what it's doing today except use
kColorId_FocusedBorderColor in this case.  I'd think the easiest way to do that
would be for that class to be able to call a method on this one that either
answers "is this view visibly focused" or else answers "what color should the
border be drawn in".

https://codereview.chromium.org/2001433002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1317: HasFocus()) {
Nit: No {} on multiline conditional with single-line body; see e.g. line 717 for
existing file style

[Eugene But (OOO till 7-30), 2016-05-20 20:36:25]

ios lgtm

https://codereview.chromium.org/2001433002/diff/40001/ios/chrome/browser/ui/o...
File ios/chrome/browser/ui/omnibox/web_omnibox_edit_controller.h (right):

https://codereview.chromium.org/2001433002/diff/40001/ios/chrome/browser/ui/o...
ios/chrome/browser/ui/omnibox/web_omnibox_edit_controller.h:22: virtual void
OnSetFocus() = 0;
Per C++ Style Guide, could you please add comments for this method.

[Evan Stade, 2016-05-20 20:54:43]

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

https://codereview.chromium.org/2001433002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1274:
paint.setStrokeWidth(1);
On 2016/05/20 20:32:49, Peter Kasting wrote:
> This will scale up on higher scale factors, which the code in
> BackgroundWith1PxBorder won't do.  It seems like this should match that
> behavior, just in a different color.

It's my understanding that the focus border is intentionally thicker in the 2x
case (the mock is at 2x and shows a 2px border).

[Peter Kasting, 2016-05-20 20:57:13]

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

https://codereview.chromium.org/2001433002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1274:
paint.setStrokeWidth(1);
On 2016/05/20 20:54:43, Evan Stade wrote:
> On 2016/05/20 20:32:49, Peter Kasting wrote:
> > This will scale up on higher scale factors, which the code in
> > BackgroundWith1PxBorder won't do.  It seems like this should match that
> > behavior, just in a different color.
> 
> It's my understanding that the focus border is intentionally thicker in the 2x
> case (the mock is at 2x and shows a 2px border).

Can we double-check that this intentionally differs from the omnibox border
behavior (unless you already asked)?  I'm OK with it if so (though in that case
we should probably have a comment about why this isn't using the normal border),
but the difference seems so unexpected that I'd think maybe the mock was put
together reasonably quickly or Sebastien forgot about the hairline omnibox
border in the normal case or something.

[Evan Stade, 2016-05-20 22:07:32]

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

https://codereview.chromium.org/2001433002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1317: HasFocus()) {
On 2016/05/20 20:32:49, Peter Kasting wrote:
> Nit: No {} on multiline conditional with single-line body; see e.g. line 717
for
> existing file style

Done.

https://codereview.chromium.org/2001433002/diff/40001/ios/chrome/browser/ui/o...
File ios/chrome/browser/ui/omnibox/web_omnibox_edit_controller.h (right):

https://codereview.chromium.org/2001433002/diff/40001/ios/chrome/browser/ui/o...
ios/chrome/browser/ui/omnibox/web_omnibox_edit_controller.h:22: virtual void
OnSetFocus() = 0;
On 2016/05/20 20:36:25, Eugene But wrote:
> Per C++ Style Guide, could you please add comments for this method.

Done.

[Evan Stade, 2016-05-20 22:07:39]

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

[commit-bot: I haz the power, 2016-05-20 22:08:22]

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

[Peter Kasting, 2016-05-20 22:47:43]

Looks like Sebastien said on the bug this is intentional.  I'd at least leave a
comment about this; you may also want to find a way to keep the 2.5 constant for
rounded corners the same between the background class and the code here.

LGTM

[Evan Stade, 2016-05-20 23:00:31]

On 2016/05/20 22:47:43, Peter Kasting wrote:
> Looks like Sebastien said on the bug this is intentional.  I'd at least leave
a
> comment about this; you may also want to find a way to keep the 2.5 constant
for
> rounded corners the same between the background class and the code here.

Done.

[Evan Stade, 2016-05-20 23:00:36]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-05-20 23:00:37]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
eugenebut@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2001433002/#ps80001
(title: "share constant")

[commit-bot: I haz the power, 2016-05-20 23:00:58]

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

[commit-bot: I haz the power, 2016-05-21 00:13:35]

Description was changed from

==========
Update appearance of omnibox focus for MD.

Also remove some dead focus painter code in TextField.
Also remove OmniboxEditController::OnSetFocus(), which was totally dead on Views
and not necessary on Cocoa.

BUG=591140
==========

to

==========
Update appearance of omnibox focus for MD.

Also remove some dead focus painter code in TextField.
Also remove OmniboxEditController::OnSetFocus(), which was totally dead on Views
and not necessary on Cocoa.

BUG=591140
==========

[commit-bot: I haz the power, 2016-05-21 00:13:37]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-21 00:14:40]

Description was changed from

==========
Update appearance of omnibox focus for MD.

Also remove some dead focus painter code in TextField.
Also remove OmniboxEditController::OnSetFocus(), which was totally dead on Views
and not necessary on Cocoa.

BUG=591140
==========

to

==========
Update appearance of omnibox focus for MD.

Also remove some dead focus painter code in TextField.
Also remove OmniboxEditController::OnSetFocus(), which was totally dead on Views
and not necessary on Cocoa.

BUG=591140

Committed: https://crrev.com/333ac64c2e01fce66aa363da999dbcffa9724343
Cr-Commit-Position: refs/heads/master@{#395213}
==========

[commit-bot: I haz the power, 2016-05-21 00:14:41]

Patchset 5 (id:??) landed as
https://crrev.com/333ac64c2e01fce66aa363da999dbcffa9724343
Cr-Commit-Position: refs/heads/master@{#395213}