Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2396133005:
[ash-md] Animates ToggleButton highlight to move it in sync with the thumb (Closed)
Description[ash-md] Animates ToggleButton highlight to move it in sync with the thumb
BUG=654116
Committed: https://crrev.com/52aea21592433931e8e612484284d8d4e65cf17e
Cr-Commit-Position: refs/heads/master@{#425320}
Patch Set 1 #
Total comments: 4
Patch Set 2 : [ash-md] Animates ToggleButton highlight to move it in sync with the thumb (thumb in its own layer) #
Total comments: 2
Patch Set 3 : [ash-md] Animates ToggleButton highlight to move it in sync with the thumb (thumb view always exist… #
Total comments: 6
Patch Set 4 : [ash-md] Animates ToggleButton highlight to move it in sync with the thumb (comments) #
Total comments: 6
Patch Set 5 : [ash-md] Animates ToggleButton highlight to move it in sync with the thumb (simpler ctor and order … #Patch Set 6 : [ash-md] Animates ToggleButton highlight to move it in sync with the thumb (better destruction sequ… #
Total comments: 5
Messages
Total messages: 53 (32 generated)
The CQ bit was checked by varkha@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...
varkha@chromium.org changed reviewers: + bruthig@chromium.org
bruthig@, can you please take a first look? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
One comment, but otherwise looks good at a high level. https://codereview.chromium.org/2396133005/diff/1/ui/views/animation/ink_drop... File ui/views/animation/ink_drop_host.h (right): https://codereview.chromium.org/2396133005/diff/1/ui/views/animation/ink_drop... ui/views/animation/ink_drop_host.h:48: virtual void OffsetInkDropRipple(const gfx::Vector2d& offset) = 0; I think it would be better to move this function into the InkDrop interface instead of here. All of the descendants of InkDropHostView would be able to access the ink_drop_ anyway.
estade@chromium.org changed reviewers: + estade@chromium.org
https://codereview.chromium.org/2396133005/diff/1/ui/views/controls/button/to... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2396133005/diff/1/ui/views/controls/button/to... ui/views/controls/button/toggle_button.cc:104: OffsetInkDropRipple(thumb_bounds.CenterPoint() - original_center_point_); I'm kind of skeptical this will look great/stay in sync at a per-frame level. I think instead we should have the thumb be a separate View and AddInkDropLayer makes the ink drop layer a child of that view's layer.
The CQ bit was checked by varkha@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...
Is this what you had in mind? https://codereview.chromium.org/2396133005/diff/1/ui/views/animation/ink_drop... File ui/views/animation/ink_drop_host.h (right): https://codereview.chromium.org/2396133005/diff/1/ui/views/animation/ink_drop... ui/views/animation/ink_drop_host.h:48: virtual void OffsetInkDropRipple(const gfx::Vector2d& offset) = 0; On 2016/10/08 22:44:46, bruthig wrote: > I think it would be better to move this function into the InkDrop interface > instead of here. All of the descendants of InkDropHostView would be able to > access the ink_drop_ anyway. Acknowledged. https://codereview.chromium.org/2396133005/diff/1/ui/views/controls/button/to... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2396133005/diff/1/ui/views/controls/button/to... ui/views/controls/button/toggle_button.cc:104: OffsetInkDropRipple(thumb_bounds.CenterPoint() - original_center_point_); On 2016/10/10 17:33:15, Evan Stade wrote: > I'm kind of skeptical this will look great/stay in sync at a per-frame level. I > think instead we should have the thumb be a separate View and AddInkDropLayer > makes the ink drop layer a child of that view's layer. Done.
pretty much https://codereview.chromium.org/2396133005/diff/20001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2396133005/diff/20001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:163: new ToggleButton::ThumbView(ink_drop_layer, I was kind of expecting the thumb to just always be a child view.
Patchset #3 (id:40001) has been deleted
Good idea, PTAL. https://codereview.chromium.org/2396133005/diff/20001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2396133005/diff/20001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:163: new ToggleButton::ThumbView(ink_drop_layer, On 2016/10/12 00:46:55, Evan Stade (ooo till 10-20) wrote: > I was kind of expecting the thumb to just always be a child view. Done.
The CQ bit was checked by varkha@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.
Two requests/notes otherwise lgtm https://codereview.chromium.org/2396133005/diff/60001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2396133005/diff/60001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:169: CustomButton::AddInkDropLayer(ink_drop_layer); You shouldn't need to call into the parent here since the layer is actually being hosted by the ThumbView. https://codereview.chromium.org/2396133005/diff/60001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:176: thumb_view_->RemoveInkDropLayer(ink_drop_layer); You should maybe check that |thumb_view_| isn't null here. In case the ThumbView is destroyed before the InkDrop cleans up the InkDropRipple layer.
+sadrul for OWNERS in ui/views. https://codereview.chromium.org/2396133005/diff/60001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2396133005/diff/60001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:169: CustomButton::AddInkDropLayer(ink_drop_layer); On 2016/10/12 13:38:09, bruthig wrote: > You shouldn't need to call into the parent here since the layer is actually > being hosted by the ThumbView. Done. https://codereview.chromium.org/2396133005/diff/60001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:176: thumb_view_->RemoveInkDropLayer(ink_drop_layer); On 2016/10/12 13:38:09, bruthig wrote: > You should maybe check that |thumb_view_| isn't null here. In case the > ThumbView is destroyed before the InkDrop cleans up the InkDropRipple layer. I think this gets handled auto-magically. When a ink drop host view (ToggleButton which owns the ThumbView) becomes hidden the ink drop is destroyed and gets removed from the host. The thumb should not be destroyed before ToggleView destructor gets called (and if it ever is then you are right we should add this check).
varkha@chromium.org changed reviewers: + sadrul@chromium.org
+sadrul@ for OWNERS in ui/views. Thanks!
The CQ bit was checked by varkha@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.
lgtm https://codereview.chromium.org/2396133005/diff/80001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2396133005/diff/80001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:42: ThumbView(views::View* parent) : color_ratio_(0.) { explicit Or may be better: AddChild() from the caller, and let ThumbView not have to worry about it. https://codereview.chromium.org/2396133005/diff/80001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:198: SchedulePaint(); Now that the thumb is a separate View, it would be nice to not have to re-paint at each step of the animation. I see that the animation-step affects the background/thumb colors. Would it be possible to apply some filter on the layers to get the same visual effect instead? That should give some good perf gain. https://codereview.chromium.org/2396133005/diff/80001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.h (right): https://codereview.chromium.org/2396133005/diff/80001/ui/views/controls/butto... ui/views/controls/button/toggle_button.h:47: void UpdateThumb(); non-virtuals should be before virtuals (although I see that GetThumbBounds() doesn't follow that either).
The CQ bit was checked by varkha@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/2396133005/diff/80001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2396133005/diff/80001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:42: ThumbView(views::View* parent) : color_ratio_(0.) { On 2016/10/13 03:30:23, sadrul wrote: > explicit > > Or may be better: AddChild() from the caller, and let ThumbView not have to > worry about it. I like your second choice more. Done. https://codereview.chromium.org/2396133005/diff/80001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:198: SchedulePaint(); On 2016/10/13 03:30:23, sadrul wrote: > Now that the thumb is a separate View, it would be nice to not have to re-paint > at each step of the animation. I see that the animation-step affects the > background/thumb colors. Would it be possible to apply some filter on the layers > to get the same visual effect instead? That should give some good perf gain. Acknowledged. Added a TODO for a possible followup. https://codereview.chromium.org/2396133005/diff/80001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.h (right): https://codereview.chromium.org/2396133005/diff/80001/ui/views/controls/butto... ui/views/controls/button/toggle_button.h:47: void UpdateThumb(); On 2016/10/13 03:30:23, sadrul wrote: > non-virtuals should be before virtuals (although I see that GetThumbBounds() > doesn't follow that either). Done.
I think I forgot to post this... https://codereview.chromium.org/2396133005/diff/60001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2396133005/diff/60001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:176: thumb_view_->RemoveInkDropLayer(ink_drop_layer); On 2016/10/12 14:45:24, varkha wrote: > On 2016/10/12 13:38:09, bruthig wrote: > > You should maybe check that |thumb_view_| isn't null here. In case the > > ThumbView is destroyed before the InkDrop cleans up the InkDropRipple layer. > > I think this gets handled auto-magically. When a ink drop host view > (ToggleButton which owns the ThumbView) becomes hidden the ink drop is destroyed > and gets removed from the host. The thumb should not be destroyed before > ToggleView destructor gets called (and if it ever is then you are right we > should add this check). The |thumb_view_| is being destroyed in ~ToggleButton() on line 111 which is going to occur before the destruction percolates up to the ~InkDropHostView() dtor and thus destroying the |ink_drop_| instance. It should be simple enough to write a test like so: 1. Create ToggleButton 2. Mouse down on thumb to create ripple 3. Destroy the ToggleButton while the ripple is active. I suspect this is going to raise a use-after-free error.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/13 17:09:46, bruthig wrote: > I think I forgot to post this... > > https://codereview.chromium.org/2396133005/diff/60001/ui/views/controls/butto... > File ui/views/controls/button/toggle_button.cc (right): > > https://codereview.chromium.org/2396133005/diff/60001/ui/views/controls/butto... > ui/views/controls/button/toggle_button.cc:176: > thumb_view_->RemoveInkDropLayer(ink_drop_layer); > On 2016/10/12 14:45:24, varkha wrote: > > On 2016/10/12 13:38:09, bruthig wrote: > > > You should maybe check that |thumb_view_| isn't null here. In case the > > > ThumbView is destroyed before the InkDrop cleans up the InkDropRipple layer. > > > > I think this gets handled auto-magically. When a ink drop host view > > (ToggleButton which owns the ThumbView) becomes hidden the ink drop is > destroyed > > and gets removed from the host. The thumb should not be destroyed before > > ToggleView destructor gets called (and if it ever is then you are right we > > should add this check). > > The |thumb_view_| is being destroyed in ~ToggleButton() on line 111 which is > going to occur before the destruction percolates up to the ~InkDropHostView() > dtor and thus destroying the |ink_drop_| instance. > > It should be simple enough to write a test like so: > 1. Create ToggleButton > 2. Mouse down on thumb to create ripple > 3. Destroy the ToggleButton while the ripple is active. > > I suspect this is going to raise a use-after-free error. I've tried this with extra slow animation times and the visibility callbacks were taking care of safely destroying the ripple but I will re-check it and see if a test can exercise this sequence better.
The CQ bit was checked by varkha@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...
Patchset #6 (id:120001) has been deleted
https://codereview.chromium.org/2396133005/diff/60001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2396133005/diff/60001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:176: thumb_view_->RemoveInkDropLayer(ink_drop_layer); On 2016/10/13 17:09:46, bruthig wrote: > On 2016/10/12 14:45:24, varkha wrote: > > On 2016/10/12 13:38:09, bruthig wrote: > > > You should maybe check that |thumb_view_| isn't null here. In case the > > > ThumbView is destroyed before the InkDrop cleans up the InkDropRipple layer. > > > > I think this gets handled auto-magically. When a ink drop host view > > (ToggleButton which owns the ThumbView) becomes hidden the ink drop is > destroyed > > and gets removed from the host. The thumb should not be destroyed before > > ToggleView destructor gets called (and if it ever is then you are right we > > should add this check). > > The |thumb_view_| is being destroyed in ~ToggleButton() on line 111 which is > going to occur before the destruction percolates up to the ~InkDropHostView() > dtor and thus destroying the |ink_drop_| instance. > > It should be simple enough to write a test like so: > 1. Create ToggleButton > 2. Mouse down on thumb to create ripple > 3. Destroy the ToggleButton while the ripple is active. > > I suspect this is going to raise a use-after-free error. Good call. So the test doesn't crash because the vtable is rolled and the base class version of InkDropHostView::RemoveInkDropLayer() is called which is harmless since it checks |destroying_|. Chrome on the other hand doesn't crash because VisibilityChanged() callback proactively resets the ink drop layers when a view gets hidden. We talked about it offline and agreed that this is subtle enough to force the ink drop to get destroyed explicitly, hence a call to SetInkDropMode(InkDropMode::OFF) in ~ToggleButton(). I've also added the test that exercises this sequence.
The CQ bit was checked by varkha@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.
lgtm On another note though, I think this destruction tear down issue is a problem for all InkDropHostView descendants that override RemoveInkDropLayer(). I will have to investigate.
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2396133005/#ps140001 (title: "[ash-md] Animates ToggleButton highlight to move it in sync with the thumb (better destruction sequ…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [ash-md] Animates ToggleButton highlight to move it in sync with the thumb BUG=654116 ========== to ========== [ash-md] Animates ToggleButton highlight to move it in sync with the thumb BUG=654116 Committed: https://crrev.com/52aea21592433931e8e612484284d8d4e65cf17e Cr-Commit-Position: refs/heads/master@{#425320} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/52aea21592433931e8e612484284d8d4e65cf17e Cr-Commit-Position: refs/heads/master@{#425320}
Message was sent while issue was closed.
https://codereview.chromium.org/2396133005/diff/140001/ui/views/controls/butt... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2396133005/diff/140001/ui/views/controls/butt... ui/views/controls/button/toggle_button.cc:45: void AddInkDropLayer(ui::Layer* ink_drop_layer) { if ThumbView were an InkDropHostView could we not remove this? https://codereview.chromium.org/2396133005/diff/140001/ui/views/controls/butt... ui/views/controls/button/toggle_button.cc:218: // repainting in every animation step to update colors could be avoided. I don't understand this comment. How can we avoid repainting when we want to change the color? Needing to repaint at every animation frame is why it didn't previously make sense (i.e. there was no advantage) for this to be its own view that just moved horizontally.
Message was sent while issue was closed.
https://codereview.chromium.org/2396133005/diff/140001/ui/views/controls/butt... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2396133005/diff/140001/ui/views/controls/butt... ui/views/controls/button/toggle_button.cc:45: void AddInkDropLayer(ui::Layer* ink_drop_layer) { On 2016/10/17 15:37:58, Evan Stade (ooo till 10-20) wrote: > if ThumbView were an InkDropHostView could we not remove this? Yes, but this would mean we would have to fix the targeting in some way. Today clicking on a toggle outside the thumb activates the ripple which I find a good behavior. We could of course make the thumb as big as the toggle button but then we would need to complicate by animating position through painting the thumb in a right spot rather than simply changing the thumb bounds. https://codereview.chromium.org/2396133005/diff/140001/ui/views/controls/butt... ui/views/controls/button/toggle_button.cc:218: // repainting in every animation step to update colors could be avoided. On 2016/10/17 15:37:58, Evan Stade (ooo till 10-20) wrote: > I don't understand this comment. How can we avoid repainting when we want to > change the color? Needing to repaint at every animation frame is why it didn't > previously make sense (i.e. there was no advantage) for this to be its own view > that just moved horizontally. I think sadrul@ was asking if some layer animation via opacity or color change could make redrawing on every frame unnecessary.
Message was sent while issue was closed.
https://codereview.chromium.org/2396133005/diff/140001/ui/views/controls/butt... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2396133005/diff/140001/ui/views/controls/butt... ui/views/controls/button/toggle_button.cc:45: void AddInkDropLayer(ui::Layer* ink_drop_layer) { On 2016/10/17 16:12:53, varkha wrote: > On 2016/10/17 15:37:58, Evan Stade (ooo till 10-20) wrote: > > if ThumbView were an InkDropHostView could we not remove this? > > Yes, but this would mean we would have to fix the targeting in some way. Today > clicking on a toggle outside the thumb activates the ripple which I find a good > behavior. We could of course make the thumb as big as the toggle button but then > we would need to complicate by animating position through painting the thumb in > a right spot rather than simply changing the thumb bounds. I didn't mean to replace the ToggleButton as the InkDropHostView, I meant making this an InkDropHostView as well so that we didn't have to reimplement these functions. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
