Update appearance of omnibox focus for MD.

chrome/browser/ui/views/location_bar/location_bar_view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 
 #include <algorithm>
 #include <map>
 
 #include "base/i18n/rtl.h"
@@ skipping 1240 matching lines @@
   return kViewClassName;
 }
 
 void LocationBarView::OnBoundsChanged(const gfx::Rect& previous_bounds) {
   OmniboxPopupView* popup = omnibox_view_->model()->popup_model()->view();
   if (popup->IsOpen())
     popup->UpdatePopupAppearance();
 }
 
 void LocationBarView::OnFocus() {
-  // Explicitly focus the omnibox so a focus ring will be displayed around it on
-  // Windows.
   omnibox_view_->SetFocus();
 }
 
 void LocationBarView::OnPaint(gfx::Canvas* canvas) {
   View::OnPaint(canvas);
 
-  if (ui::MaterialDesignController::IsModeMaterial() && !is_popup_mode_)
-    return;  // The background and border are painted by our Background.
+  if (ui::MaterialDesignController::IsModeMaterial()) {
+    if (show_focus_rect_ && omnibox_view_->HasFocus()) {
+      SkPaint paint;
+      paint.setAntiAlias(true);
+      paint.setColor(GetNativeTheme()->GetSystemColor(
+          ui::NativeTheme::NativeTheme::kColorId_FocusedBorderColor));
+      paint.setStyle(SkPaint::kStroke_Style);
+      paint.setStrokeWidth(1);

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

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

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

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]

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.

+      gfx::RectF focus_rect(GetLocalBounds());
+      focus_rect.Inset(gfx::InsetsF(0.5f));
+      canvas->DrawRoundRect(focus_rect, 2.5f, paint);
+    }
+    if (!is_popup_mode_)
+      return;  // The background and border are painted by our Background.
+  }
 
   // Fill the location bar background color behind the border.  Parts of the
   // border images are meant to rest atop the toolbar background and parts atop
   // the omnibox background, so we can't just blindly fill our entire bounds.
   gfx::Rect bounds(GetContentsBounds());
   bounds.Inset(GetHorizontalEdgeThickness(),
                is_popup_mode_ ? 0 : GetVerticalEdgeThickness());
   SkColor background_color(GetColor(BACKGROUND));
   if (!is_popup_mode_) {
     SkPaint paint;
@@ skipping 14 matching lines @@
 
 void LocationBarView::PaintChildren(const ui::PaintContext& context) {
   View::PaintChildren(context);
 
   ui::PaintRecorder recorder(context, size());
 
   // For non-InstantExtendedAPI cases, if necessary, show focus rect. As we need
   // the focus rect to appear on top of children we paint here rather than
   // OnPaint().
   // Note: |Canvas::DrawFocusRect| paints a dashed rect with gray color.
-  if (show_focus_rect_ && HasFocus())
+  if (!ui::MaterialDesignController::IsModeMaterial() && show_focus_rect_ &&
+      HasFocus()) {

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

Nit: No {} on multiline conditional with single-line body; see e.g. line 717 for
existing file style

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

Done.

     recorder.canvas()->DrawFocusRect(omnibox_view_->bounds());
+  }
 
   if (!ui::MaterialDesignController::IsModeMaterial() && !is_popup_mode_) {
     views::Painter::PaintPainterAt(recorder.canvas(), border_painter_.get(),
                                    GetContentsBounds());
   }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // LocationBarView, private views::DragController implementation:
 
@@ skipping 41 matching lines @@
 ////////////////////////////////////////////////////////////////////////////////
 // LocationBarView, private OmniboxEditController implementation:
 
 void LocationBarView::OnChanged() {
   RefreshLocationIcon();
   location_icon_view_->set_show_tooltip(!GetOmniboxView()->IsEditingOrEmpty());
   Layout();
   SchedulePaint();
 }
 
-void LocationBarView::OnSetFocus() {
-  GetFocusManager()->SetFocusedView(this);
-}
-
 const ToolbarModel* LocationBarView::GetToolbarModel() const {
   return delegate_->GetToolbarModel();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // LocationBarView, private DropdownBarHostDelegate implementation:
 
 void LocationBarView::SetFocusAndSelection(bool select_all) {
   FocusLocation(select_all);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // LocationBarView, private TemplateURLServiceObserver implementation:
 
 void LocationBarView::OnTemplateURLServiceChanged() {
   template_url_service_->RemoveObserver(this);
   template_url_service_ = nullptr;
   // If the browser is no longer active, let's not show the info bubble, as this
   // would make the browser the active window again.
   if (omnibox_view_ && omnibox_view_->GetWidget()->IsActive())
     ShowFirstRunBubble();
 }