Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2813353002:
Ensure that the focus ring in the bookmarks bar does not paint outside the parent view. (Closed)
DescriptionEnsure that the focus ring in the bookmarks bar does not paint outside the parent view.
The proposed fix is to notify the parent views when a child enables layering. The parent in
this case the ScrollView overrides the newly added notification OnChildLayerChanged() and enables
viewport layering. This ensures that the ring gets clipped.
Longer term it seems like the focus ring should really be a property of the view and should not
be instantiated by different controls all over the place.
That for a later patchset.
BUG=665412, 656198
TEST=Covered by test ViewObserverTest.ScrollViewChildAddLayerTest and ViewObserverTest.ChildViewLayerNotificationTest
Review-Url: https://codereview.chromium.org/2813353002
Cr-Commit-Position: refs/heads/master@{#465720}
Committed: https://chromium.googlesource.com/chromium/src/+/5c05d7143480feda606f6425f78f3b18598f9310
Patch Set 1 #
Total comments: 1
Patch Set 2 : Fix compile failures #Patch Set 3 : Rename layer notification to OnChildLayerChanged() #
Total comments: 12
Patch Set 4 : Address review comments and add test #Patch Set 5 : Remove newline and global variable #
Total comments: 22
Patch Set 6 : Address review comments and add new test which validates we don't receive two layer change notifica… #Patch Set 7 : Remove the contents_viewport() accessor #Patch Set 8 : Fix test #
Total comments: 12
Patch Set 9 : Next round of review comments #Patch Set 10 : Move the View::NotifyParentsOfLayerChange() function to the accelerated functions block #Patch Set 11 : Fix compile failures #
Total comments: 4
Patch Set 12 : Address review comments #
Total comments: 2
Patch Set 13 : \ #
Depends on Patchset: Messages
Total messages: 42 (27 generated)
Description was changed from ========== Ensure that the focus ring in the bookmarks bar does not paint outside the parent view. The proposed fix is to notify the parent views when a child enables layering. The parent in this case the ScrollView overrides the newly added notification ChildLayerAdded() and enables viewport layering. This ensures that the ring gets clipped. Longer term it seems like the focus ring should really be a property of the view and should not be instantiated by different controls all over the place. That for a later patchset. BUG=665412 ========== to ========== Ensure that the focus ring in the bookmarks bar does not paint outside the parent view. The proposed fix is to notify the parent views when a child enables layering. The parent in this case the ScrollView overrides the newly added notification OnChildLayerChanged() and enables viewport layering. This ensures that the ring gets clipped. Longer term it seems like the focus ring should really be a property of the view and should not be instantiated by different controls all over the place. That for a later patchset. BUG=665412 ==========
ananta@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/2813353002/diff/1/ui/views/controls/focus_rin... File ui/views/controls/focus_ring.cc (left): https://codereview.chromium.org/2813353002/diff/1/ui/views/controls/focus_rin... ui/views/controls/focus_ring.cc:90: SetPaintToLayer(); Moved this code to FocusRing::Install() as we need the parent to be setup on the view for the ChildLayerAdded() notification to be propagated up the parent chain.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Also, please add test coverage. https://codereview.chromium.org/2813353002/diff/40001/ui/views/controls/focus... File ui/views/controls/focus_ring.cc (right): https://codereview.chromium.org/2813353002/diff/40001/ui/views/controls/focus... ui/views/controls/focus_ring.cc:43: // A layer is necessary to paint beyond the parent's bounds. Why are you moving this? Perhaps it's because otherwise ScrollView doesn't get OnChildLayerChanged? ScrollView should also modify layer state when children are added. https://codereview.chromium.org/2813353002/diff/40001/ui/views/controls/scrol... File ui/views/controls/scroll_view.cc (right): https://codereview.chromium.org/2813353002/diff/40001/ui/views/controls/scrol... ui/views/controls/scroll_view.cc:76: g_scroll_with_layers_enabled && viewport->layer() != nullptr; Make ScrollView have a member for scroll_with_layers_enabled is true and then pass it into this function. In other words, no global for g_scroll_with_layers_enabled. https://codereview.chromium.org/2813353002/diff/40001/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/2813353002/diff/40001/ui/views/view.cc#newcod... ui/views/view.cc:545: View* view = this; View* view = parent(); and then you don't need two conditionals on 546 (and 547 needs to change then). https://codereview.chromium.org/2813353002/diff/40001/ui/views/view.cc#newcod... ui/views/view.cc:552: void View::DestroyLayer() { DestroyLayer needs to call OnChildLayerChanged too. https://codereview.chromium.org/2813353002/diff/40001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/2813353002/diff/40001/ui/views/view.h#newcode... ui/views/view.h:1192: // Notifies parents about a change in a child layer. An example where a Be explicit and say either the layer is being created, destroyed or the type is changing. https://codereview.chromium.org/2813353002/diff/40001/ui/views/view.h#newcode... ui/views/view.h:1195: virtual void OnChildLayerChanged(View* child) {} Don't inline the implementation.
Description was changed from ========== Ensure that the focus ring in the bookmarks bar does not paint outside the parent view. The proposed fix is to notify the parent views when a child enables layering. The parent in this case the ScrollView overrides the newly added notification OnChildLayerChanged() and enables viewport layering. This ensures that the ring gets clipped. Longer term it seems like the focus ring should really be a property of the view and should not be instantiated by different controls all over the place. That for a later patchset. BUG=665412 ========== to ========== Ensure that the focus ring in the bookmarks bar does not paint outside the parent view. The proposed fix is to notify the parent views when a child enables layering. The parent in this case the ScrollView overrides the newly added notification OnChildLayerChanged() and enables viewport layering. This ensures that the ring gets clipped. Longer term it seems like the focus ring should really be a property of the view and should not be instantiated by different controls all over the place. That for a later patchset. BUG=665412 TEST=Covered by test ViewObserverTest, ScrollViewChildAddLayerTest ==========
https://codereview.chromium.org/2813353002/diff/40001/ui/views/controls/scrol... File ui/views/controls/scroll_view.cc (right): https://codereview.chromium.org/2813353002/diff/40001/ui/views/controls/scrol... ui/views/controls/scroll_view.cc:76: g_scroll_with_layers_enabled && viewport->layer() != nullptr; On 2017/04/17 15:24:01, sky wrote: > Make ScrollView have a member for scroll_with_layers_enabled is true and then > pass it into this function. In other words, no global for > g_scroll_with_layers_enabled. Done. https://codereview.chromium.org/2813353002/diff/40001/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/2813353002/diff/40001/ui/views/view.cc#newcod... ui/views/view.cc:545: View* view = this; On 2017/04/17 15:24:01, sky wrote: > View* view = parent(); > and then you don't need two conditionals on 546 (and 547 needs to change then). Done. https://codereview.chromium.org/2813353002/diff/40001/ui/views/view.cc#newcod... ui/views/view.cc:552: void View::DestroyLayer() { On 2017/04/17 15:24:01, sky wrote: > DestroyLayer needs to call OnChildLayerChanged too. Done. https://codereview.chromium.org/2813353002/diff/40001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/2813353002/diff/40001/ui/views/view.h#newcode... ui/views/view.h:1192: // Notifies parents about a change in a child layer. An example where a On 2017/04/17 15:24:01, sky wrote: > Be explicit and say either the layer is being created, destroyed or the type is > changing. Done. https://codereview.chromium.org/2813353002/diff/40001/ui/views/view.h#newcode... ui/views/view.h:1195: virtual void OnChildLayerChanged(View* child) {} On 2017/04/17 15:24:01, sky wrote: > Don't inline the implementation. Done.
https://codereview.chromium.org/2813353002/diff/40001/ui/views/controls/focus... File ui/views/controls/focus_ring.cc (right): https://codereview.chromium.org/2813353002/diff/40001/ui/views/controls/focus... ui/views/controls/focus_ring.cc:43: // A layer is necessary to paint beyond the parent's bounds. On 2017/04/17 15:24:01, sky wrote: > Why are you moving this? Perhaps it's because otherwise ScrollView doesn't get > OnChildLayerChanged? ScrollView should also modify layer state when children are > added. Done.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2813353002/diff/80001/ui/views/controls/scrol... File ui/views/controls/scroll_view.cc (right): https://codereview.chromium.org/2813353002/diff/80001/ui/views/controls/scrol... ui/views/controls/scroll_view.cc:540: if (details.is_add) This should be: details.is_add && Contains(details.new_parent) And you should early out if already have a layer. https://codereview.chromium.org/2813353002/diff/80001/ui/views/controls/scrol... ui/views/controls/scroll_view.cc:791: if (view->layer()) { Early out if viewport_layer_enabled_ or scroll_with_layers_enabled_. https://codereview.chromium.org/2813353002/diff/80001/ui/views/controls/scrol... File ui/views/controls/scroll_view.h (right): https://codereview.chromium.org/2813353002/diff/80001/ui/views/controls/scrol... ui/views/controls/scroll_view.h:59: const View* contents_viewport() const { return contents_viewport_; } I would prefer not to expose this. Can you friend the test? https://codereview.chromium.org/2813353002/diff/80001/ui/views/controls/scrol... ui/views/controls/scroll_view.h:120: // TODO(djacobo): Remove this method when http://crbug.com/656198 is closed. 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). https://codereview.chromium.org/2813353002/diff/80001/ui/views/controls/scrol... ui/views/controls/scroll_view.h:170: // Enables view port layering if layering is enabled for the |child| or any 'layering is enabled' -> |child| has a layer https://codereview.chromium.org/2813353002/diff/80001/ui/views/controls/scrol... ui/views/controls/scroll_view.h:221: bool scroll_with_layers_enabled_ = false; Make const https://codereview.chromium.org/2813353002/diff/80001/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/2813353002/diff/80001/ui/views/view.cc#newcod... ui/views/view.cc:542: DestroyLayer(); When called from here DestroyLayer() should not call OnChildLayerChanged() as otherwise SetPaintToLayer() with an existing layer results in double the calls to OnChildLayerChanged(). It would be good to add test coverage of this. https://codereview.chromium.org/2813353002/diff/80001/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): https://codereview.chromium.org/2813353002/diff/80001/ui/views/view_unittest.... ui/views/view_unittest.cc:5029: ScrollView* scroll_view = new ScrollView(); Declare this on the stack, otherwise you're going to leak it. https://codereview.chromium.org/2813353002/diff/80001/ui/views/view_unittest.... ui/views/view_unittest.cc:5031: EXPECT_TRUE(scroll_view->contents_viewport()->layer() == nullptr); EXPECT_FALSE(scroll_view->contents_viewport()->layer()) https://codereview.chromium.org/2813353002/diff/80001/ui/views/view_unittest.... ui/views/view_unittest.cc:5038: EXPECT_TRUE(scroll_view->contents_viewport()->layer() != nullptr); EXPECT_TRUE(scroll_view->contents_viewport()->layer())
https://codereview.chromium.org/2813353002/diff/80001/ui/views/controls/scrol... File ui/views/controls/scroll_view.cc (right): https://codereview.chromium.org/2813353002/diff/80001/ui/views/controls/scrol... ui/views/controls/scroll_view.cc:540: if (details.is_add) On 2017/04/18 15:35:47, sky wrote: > This should be: > details.is_add && Contains(details.new_parent) > > And you should early out if already have a layer. Done. https://codereview.chromium.org/2813353002/diff/80001/ui/views/controls/scrol... ui/views/controls/scroll_view.cc:791: if (view->layer()) { On 2017/04/18 15:35:47, sky wrote: > Early out if viewport_layer_enabled_ or scroll_with_layers_enabled_. Done. https://codereview.chromium.org/2813353002/diff/80001/ui/views/controls/scrol... File ui/views/controls/scroll_view.h (right): https://codereview.chromium.org/2813353002/diff/80001/ui/views/controls/scrol... ui/views/controls/scroll_view.h:59: const View* contents_viewport() const { return contents_viewport_; } On 2017/04/18 15:35:48, sky wrote: > I would prefer not to expose this. Can you friend the test? Done. https://codereview.chromium.org/2813353002/diff/80001/ui/views/controls/scrol... ui/views/controls/scroll_view.h:120: // TODO(djacobo): Remove this method when http://crbug.com/656198 is closed. On 2017/04/18 15:35:48, sky wrote: > 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). Will do this in a later patch. Will ask QA to test a local build with the change to see if it works https://codereview.chromium.org/2813353002/diff/80001/ui/views/controls/scrol... ui/views/controls/scroll_view.h:170: // Enables view port layering if layering is enabled for the |child| or any On 2017/04/18 15:35:47, sky wrote: > 'layering is enabled' -> |child| has a layer Done. https://codereview.chromium.org/2813353002/diff/80001/ui/views/controls/scrol... ui/views/controls/scroll_view.h:221: bool scroll_with_layers_enabled_ = false; On 2017/04/18 15:35:47, sky wrote: > Make const We cannot do that as this is set based on the experiment. https://codereview.chromium.org/2813353002/diff/80001/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/2813353002/diff/80001/ui/views/view.cc#newcod... ui/views/view.cc:542: DestroyLayer(); On 2017/04/18 15:35:48, sky wrote: > When called from here DestroyLayer() should not call OnChildLayerChanged() as > otherwise SetPaintToLayer() with an existing layer results in double the calls > to OnChildLayerChanged(). It would be good to add test coverage of this. Done. https://codereview.chromium.org/2813353002/diff/80001/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): https://codereview.chromium.org/2813353002/diff/80001/ui/views/view_unittest.... ui/views/view_unittest.cc:5029: ScrollView* scroll_view = new ScrollView(); On 2017/04/18 15:35:48, sky wrote: > Declare this on the stack, otherwise you're going to leak it. Changed to unique_ptr https://codereview.chromium.org/2813353002/diff/80001/ui/views/view_unittest.... ui/views/view_unittest.cc:5031: EXPECT_TRUE(scroll_view->contents_viewport()->layer() == nullptr); On 2017/04/18 15:35:48, sky wrote: > EXPECT_FALSE(scroll_view->contents_viewport()->layer()) Done. https://codereview.chromium.org/2813353002/diff/80001/ui/views/view_unittest.... ui/views/view_unittest.cc:5038: EXPECT_TRUE(scroll_view->contents_viewport()->layer() != nullptr); On 2017/04/18 15:35:48, sky wrote: > EXPECT_TRUE(scroll_view->contents_viewport()->layer()) Done.
Description was changed from ========== Ensure that the focus ring in the bookmarks bar does not paint outside the parent view. The proposed fix is to notify the parent views when a child enables layering. The parent in this case the ScrollView overrides the newly added notification OnChildLayerChanged() and enables viewport layering. This ensures that the ring gets clipped. Longer term it seems like the focus ring should really be a property of the view and should not be instantiated by different controls all over the place. That for a later patchset. BUG=665412 TEST=Covered by test ViewObserverTest, ScrollViewChildAddLayerTest ========== to ========== Ensure that the focus ring in the bookmarks bar does not paint outside the parent view. The proposed fix is to notify the parent views when a child enables layering. The parent in this case the ScrollView overrides the newly added notification OnChildLayerChanged() and enables viewport layering. This ensures that the ring gets clipped. Longer term it seems like the focus ring should really be a property of the view and should not be instantiated by different controls all over the place. That for a later patchset. BUG=665412, 656198 TEST=Covered by test ViewObserverTest, ScrollViewChildAddLayerTest ==========
Description was changed from ========== Ensure that the focus ring in the bookmarks bar does not paint outside the parent view. The proposed fix is to notify the parent views when a child enables layering. The parent in this case the ScrollView overrides the newly added notification OnChildLayerChanged() and enables viewport layering. This ensures that the ring gets clipped. Longer term it seems like the focus ring should really be a property of the view and should not be instantiated by different controls all over the place. That for a later patchset. BUG=665412, 656198 TEST=Covered by test ViewObserverTest, ScrollViewChildAddLayerTest ========== to ========== Ensure that the focus ring in the bookmarks bar does not paint outside the parent view. The proposed fix is to notify the parent views when a child enables layering. The parent in this case the ScrollView overrides the newly added notification OnChildLayerChanged() and enables viewport layering. This ensures that the ring gets clipped. Longer term it seems like the focus ring should really be a property of the view and should not be instantiated by different controls all over the place. That for a later patchset. BUG=665412, 656198 TEST=Covered by test ViewObserverTest.ScrollViewChildAddLayerTest and ViewObserverTest.ChildViewLayerNotificationTest ==========
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2813353002/diff/140001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/2813353002/diff/140001/ui/views/view.h#newcod... ui/views/view.h:343: enum LayerChangeNotifyBehavior { enum class https://codereview.chromium.org/2813353002/diff/140001/ui/views/view.h#newcod... ui/views/view.h:359: void DestroyLayerImpl(LayerChangeNotifyBehavior notify_parents); Move this (and enum) to the private section in the section named "Accelerated painting". And when you move it make sure the declaration/definition positions are the same. https://codereview.chromium.org/2813353002/diff/140001/ui/views/view.h#newcod... ui/views/view.h:1538: void NotifyParentsOfLayerChange(); Move this to the "Accelerated painting" section above. https://codereview.chromium.org/2813353002/diff/140001/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): https://codereview.chromium.org/2813353002/diff/140001/ui/views/view_unittest... ui/views/view_unittest.cc:5027: #if !defined(OS_MACOSX) I don't think you need the ifdef, instead early out if the scrollview already has a layer. https://codereview.chromium.org/2813353002/diff/140001/ui/views/view_unittest... ui/views/view_unittest.cc:5048: : View(), This line isn't necessary. https://codereview.chromium.org/2813353002/diff/140001/ui/views/view_unittest... ui/views/view_unittest.cc:5076: }; DISALLOW... https://codereview.chromium.org/2813353002/diff/140001/ui/views/view_unittest... ui/views/view_unittest.cc:5089: EXPECT_EQ(parent_view->layer_change_count(), 0); Generally we go with expected, actual, e.g. EXPECT_EQ(0, parent_view->layer_change_count())
https://codereview.chromium.org/2813353002/diff/140001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/2813353002/diff/140001/ui/views/view.h#newcod... ui/views/view.h:343: enum LayerChangeNotifyBehavior { On 2017/04/18 23:02:21, sky wrote: > enum class Done. https://codereview.chromium.org/2813353002/diff/140001/ui/views/view.h#newcod... ui/views/view.h:359: void DestroyLayerImpl(LayerChangeNotifyBehavior notify_parents); On 2017/04/18 23:02:21, sky wrote: > Move this (and enum) to the private section in the section named "Accelerated > painting". And when you move it make sure the declaration/definition positions > are the same. Done. https://codereview.chromium.org/2813353002/diff/140001/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): https://codereview.chromium.org/2813353002/diff/140001/ui/views/view_unittest... ui/views/view_unittest.cc:5048: : View(), On 2017/04/18 23:02:21, sky wrote: > This line isn't necessary. Done. https://codereview.chromium.org/2813353002/diff/140001/ui/views/view_unittest... ui/views/view_unittest.cc:5076: }; On 2017/04/18 23:02:21, sky wrote: > DISALLOW... Done. https://codereview.chromium.org/2813353002/diff/140001/ui/views/view_unittest... ui/views/view_unittest.cc:5089: EXPECT_EQ(parent_view->layer_change_count(), 0); On 2017/04/18 23:02:21, sky wrote: > Generally we go with expected, actual, e.g. > EXPECT_EQ(0, parent_view->layer_change_count()) Done.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2813353002/diff/80001/ui/views/controls/scrol... File ui/views/controls/scroll_view.h (right): https://codereview.chromium.org/2813353002/diff/80001/ui/views/controls/scrol... ui/views/controls/scroll_view.h:221: bool scroll_with_layers_enabled_ = false; On 2017/04/18 22:22:39, ananta wrote: > On 2017/04/18 15:35:47, sky wrote: > > Make const > > We cannot do that as this is set based on the experiment. If you move scroll_with_layers_enabled_ = base::FeatureList::IsEnabled(kToolkitViewsScrollWithLayers); to the member initializer can't it be const? https://codereview.chromium.org/2813353002/diff/200001/ui/views/controls/scro... File ui/views/controls/scroll_view.cc (right): https://codereview.chromium.org/2813353002/diff/200001/ui/views/controls/scro... ui/views/controls/scroll_view.cc:540: if (details.is_add && Contains(details.parent) && !viewport_layer_enabled_) Move !viewport_layer_enabled_ before Contains() as Contains() has to walk the hierarchy. https://codereview.chromium.org/2813353002/diff/200001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/2813353002/diff/200001/ui/views/view.h#newcod... ui/views/view.h:1196: enum class LayerChangeNotifyBehavior { Move this and the other functions (except for OnChildLayerChanged) to the private 'Accelerated painting' section.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2813353002/diff/80001/ui/views/controls/scrol... File ui/views/controls/scroll_view.h (right): https://codereview.chromium.org/2813353002/diff/80001/ui/views/controls/scrol... ui/views/controls/scroll_view.h:221: bool scroll_with_layers_enabled_ = false; On 2017/04/19 00:03:04, sky wrote: > On 2017/04/18 22:22:39, ananta wrote: > > On 2017/04/18 15:35:47, sky wrote: > > > Make const > > > > We cannot do that as this is set based on the experiment. > > If you move scroll_with_layers_enabled_ = > base::FeatureList::IsEnabled(kToolkitViewsScrollWithLayers); > > to the member initializer can't it be const? Thanks. done https://codereview.chromium.org/2813353002/diff/200001/ui/views/controls/scro... File ui/views/controls/scroll_view.cc (right): https://codereview.chromium.org/2813353002/diff/200001/ui/views/controls/scro... ui/views/controls/scroll_view.cc:540: if (details.is_add && Contains(details.parent) && !viewport_layer_enabled_) On 2017/04/19 00:03:05, sky wrote: > Move !viewport_layer_enabled_ before Contains() as Contains() has to walk the > hierarchy. Thanks. done https://codereview.chromium.org/2813353002/diff/200001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/2813353002/diff/200001/ui/views/view.h#newcod... ui/views/view.h:1196: enum class LayerChangeNotifyBehavior { On 2017/04/19 00:03:05, sky wrote: > Move this and the other functions (except for OnChildLayerChanged) to the > private 'Accelerated painting' section. Done.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
LGTM https://codereview.chromium.org/2813353002/diff/220001/ui/views/controls/scro... File ui/views/controls/scroll_view.h (right): https://codereview.chromium.org/2813353002/diff/220001/ui/views/controls/scro... ui/views/controls/scroll_view.h:222: const bool scroll_with_layers_enabled_ = false; Remove the = false here as this is now assigned in the member initializer list. In fact I'm surprised this compiles.
https://codereview.chromium.org/2813353002/diff/220001/ui/views/controls/scro... File ui/views/controls/scroll_view.h (right): https://codereview.chromium.org/2813353002/diff/220001/ui/views/controls/scro... ui/views/controls/scroll_view.h:222: const bool scroll_with_layers_enabled_ = false; On 2017/04/19 15:03:56, sky wrote: > Remove the = false here as this is now assigned in the member initializer list. > In fact I'm surprised this compiles. Done.
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2813353002/#ps230001 (title: "\")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 230001, "attempt_start_ts": 1492628669444500,
"parent_rev": "f18929c777dd3d711bebdb7927a32d3133f4e066", "commit_rev":
"5c05d7143480feda606f6425f78f3b18598f9310"}
Message was sent while issue was closed.
Description was changed from ========== Ensure that the focus ring in the bookmarks bar does not paint outside the parent view. The proposed fix is to notify the parent views when a child enables layering. The parent in this case the ScrollView overrides the newly added notification OnChildLayerChanged() and enables viewport layering. This ensures that the ring gets clipped. Longer term it seems like the focus ring should really be a property of the view and should not be instantiated by different controls all over the place. That for a later patchset. BUG=665412, 656198 TEST=Covered by test ViewObserverTest.ScrollViewChildAddLayerTest and ViewObserverTest.ChildViewLayerNotificationTest ========== to ========== Ensure that the focus ring in the bookmarks bar does not paint outside the parent view. The proposed fix is to notify the parent views when a child enables layering. The parent in this case the ScrollView overrides the newly added notification OnChildLayerChanged() and enables viewport layering. This ensures that the ring gets clipped. Longer term it seems like the focus ring should really be a property of the view and should not be instantiated by different controls all over the place. That for a later patchset. BUG=665412, 656198 TEST=Covered by test ViewObserverTest.ScrollViewChildAddLayerTest and ViewObserverTest.ChildViewLayerNotificationTest Review-Url: https://codereview.chromium.org/2813353002 Cr-Commit-Position: refs/heads/master@{#465720} Committed: https://chromium.googlesource.com/chromium/src/+/5c05d7143480feda606f6425f78f... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:230001) as https://chromium.googlesource.com/chromium/src/+/5c05d7143480feda606f6425f78f...
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:230001) has been created in https://codereview.chromium.org/2883273007/ by timbrown@google.com. The reason for reverting is: Causes a UI regression on Linux where the overflow extension icons in the settings menu always render with a white background. See http://crbug.com/722965. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
