Ensure that the focus ring in the bookmarks bar does not paint outside the parent view.

ui/views/controls/scroll_view.h

 // 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.
 
 #ifndef UI_VIEWS_CONTROLS_SCROLL_VIEW_H_
 #define UI_VIEWS_CONTROLS_SCROLL_VIEW_H_
 
 #include <string>
 
 #include "base/compiler_specific.h"
@@ skipping 38 matching lines @@
   // Returns the ScrollView for which |contents| is its contents, or null if
   // |contents| is not in a ScrollView.
   static ScrollView* GetScrollViewForContents(View* contents);
 
   // Set the contents. Any previous contents will be deleted. The contents
   // is the view that needs to scroll.
   void SetContents(View* a_view);
   const View* contents() const { return contents_; }
   View* contents() { return contents_; }
 
+  const View* contents_viewport() const { return contents_viewport_; }

[sky, 2017/04/18 15:35:48]

I would prefer not to expose this. Can you friend the test?

[ananta, 2017/04/18 22:22:39]

Done.

+
   // Sets the header, deleting the previous header.
   void SetHeader(View* header);
 
   // Sets the background color. The default is white when scrolling with layers,
   // otherwise transparent. An opaque color when scrolling with layers ensures
   // fonts can be drawn with subpixel antialiasing.
   void SetBackgroundColor(SkColor color);
 
   // Returns the visible region of the content View.
   gfx::Rect GetVisibleRect() const;
@@ skipping 29 matching lines @@
   // View overrides:
   gfx::Size GetPreferredSize() const override;
   int GetHeightForWidth(int width) const override;
   void Layout() override;
   bool OnKeyPressed(const ui::KeyEvent& event) override;
   bool OnMouseWheel(const ui::MouseWheelEvent& e) override;
   void OnScrollEvent(ui::ScrollEvent* event) override;
   void OnGestureEvent(ui::GestureEvent* event) override;
   const char* GetClassName() const override;
   void OnNativeThemeChanged(const ui::NativeTheme* theme) override;
+  void ViewHierarchyChanged(
+      const ViewHierarchyChangedDetails& details) override;
+  void OnChildLayerChanged(View* child) override;
 
   // ScrollBarController overrides:
   void ScrollToPosition(ScrollBar* source, int position) override;
   int GetScrollIncrement(ScrollBar* source,
                          bool is_page,
                          bool is_positive) override;
 
   // TODO(djacobo): Remove this method when http://crbug.com/656198  is closed.

[sky, 2017/04/18 15:35:48]

This is the bug you are fixing, and you should be able to make
EnableViewportLayer private now (the calls to it out side of this class should
no longer be necessary).

[ananta, 2017/04/18 22:22:39]

Will do this in a later patch. Will ask QA to test a local build with the change
to see if it works

   // Force |contents_viewport_| to enable a Layer().
   void EnableViewPortLayer();
 
  private:
   friend class test::ScrollViewTestApi;
 
   class Viewport;
 
   // Used internally by SetHeader() and SetContents() to reset the view.  Sets
   // |member| to |new_view|. If |new_view| is non-null it is added to |parent|.
@@ skipping 29 matching lines @@
 
   // Callback entrypoint when hosted Layers are scrolled by the Compositor.
   void OnLayerScrolled(const gfx::ScrollOffset& offset);
 
   // Horizontally scrolls the header (if any) to match the contents.
   void ScrollHeader();
 
   void AddBorder();
   void UpdateBorder();
 
+  // Enables view port layering if layering is enabled for the |child| or any

[sky, 2017/04/18 15:35:47]

'layering is enabled' -> |child| has a layer

[ananta, 2017/04/18 22:22:39]

Done.

+  // of its descendants. Returns true if yes. We short circuit the recursion
+  // if we enabled layering.
+  bool EnableLayeringRecursivelyForChild(View* child);
+
   // The current contents and its viewport. |contents_| is contained in
   // |contents_viewport_|.
   View* contents_;
   View* contents_viewport_;
 
   // The current header and its viewport. |header_| is contained in
   // |header_viewport_|.
   View* header_;
   View* header_viewport_;
 
@@ skipping 22 matching lines @@
   // In Harmony, the indicator is a focus ring. Pre-Harmony, the indicator is a
   // different border painter.
   bool draw_focus_indicator_ = false;
 
   // Only needed for pre-Harmony. Remove when Harmony is default.
   bool draw_border_ = false;
 
   // Focus ring, if one is installed.
   View* focus_ring_ = nullptr;
 
+  // Set to true if we enabled layering for the viewport.
+  bool viewport_layer_enabled_ = false;
+
+  // Set to true if the scroll with layers feature is enabled.
+  bool scroll_with_layers_enabled_ = false;

[sky, 2017/04/18 15:35:47]

Make const

[ananta, 2017/04/18 22:22:39]

We cannot do that as this is set based on the experiment.

[sky, 2017/04/19 00:03:04]

If you move scroll_with_layers_enabled_ =
       base::FeatureList::IsEnabled(kToolkitViewsScrollWithLayers);

to the member initializer can't it be const?

[ananta, 2017/04/19 00:53:17]

Thanks. done

+
   DISALLOW_COPY_AND_ASSIGN(ScrollView);
 };
 
 // VariableRowHeightScrollHelper is intended for views that contain rows of
 // varying height. To use a VariableRowHeightScrollHelper create one supplying
 // a Controller and delegate GetPageScrollIncrement and GetLineScrollIncrement
 // to the helper. VariableRowHeightScrollHelper calls back to the
 // Controller to determine row boundaries.
 class VariableRowHeightScrollHelper {
  public:
@@ skipping 56 matching lines @@
  private:
   int top_margin_;
   int row_height_;
 
   DISALLOW_COPY_AND_ASSIGN(FixedRowHeightScrollHelper);
 };
 
 }  // namespace views
 
 #endif  // UI_VIEWS_CONTROLS_SCROLL_VIEW_H_