Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2250783002:
Allow InkDropRipple to co-exist with highlight (Closed)
|
Created:
4 years, 4 months ago by Evan Stade Modified:
4 years, 3 months ago CC:
chromium-reviews, tfarina, bruthig+ink_drop_chromium.org, dcheng Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow InkDropRipple to co-exist with highlight.
Allow InkDropRipple to co-exist with highlight by adding
InkDropRipple::OverridesHighlight. I initially made this part of
InkDropHost but then realized that it was more a property of the ripple
itself, and it would be too easy to overlook when adding an ink drop to
a new surface.
The immediate effect of this change is that flood fill ripples will paint
on top of the hover highlight effect (e.g. on bookmark buttons).
BUG=617961
Committed: https://crrev.com/d17dd4ab4c7802be7190cbc80e9c56a4ddb793c9
Cr-Commit-Position: refs/heads/master@{#419848}
Patch Set 1 #Patch Set 2 : improved docs #Patch Set 3 : git cl try #
Total comments: 30
Patch Set 4 : bruthig feedback #Patch Set 5 : ++tests #
Total comments: 1
Patch Set 6 : changes #Patch Set 7 : with dbg code #Patch Set 8 : w/o dbg code #
Total comments: 4
Patch Set 9 : . #
Total comments: 6
Patch Set 10 : remove EmptyInkDropRipple #Messages
Total messages: 54 (24 generated)
Description was changed from ========== Allow InkDropRipple to co-exist with highlight or not exist at all. 1. Allow InkDropRipple to co-exist with highlight by adding InkDropRipple::OverridesHighlight. I initially made this part of InkDropHost but then realized that it was more a property of the ripple itself, and it would be too easy to overlook when adding an ink drop to a new surface. 2. Allow InkDropRipple to not exist but still retaining an InkDropHighlight. This is desired for a couple menu-like UI surfaces in secondary UI. This is effected with an EmptyInkDropRipple class that just no-ops while allowing the hover effect to remain. BUG=617961 ========== to ========== Allow InkDropRipple to co-exist with highlight or not exist at all. 1. Allow InkDropRipple to co-exist with highlight by adding InkDropRipple::OverridesHighlight. I initially made this part of InkDropHost but then realized that it was more a property of the ripple itself, and it would be too easy to overlook when adding an ink drop to a new surface. 2. Allow InkDropRipple to not exist but still retaining an InkDropHighlight. This is desired for a couple menu-like UI surfaces in secondary UI. This is effected with an EmptyInkDropRipple class that just no-ops while allowing the hover effect to remain. The immediate effect of this change is that flood fill ripples will paint on top of the hover highlight effect (e.g. on bookmark buttons). BUG=617961 ==========
estade@chromium.org changed reviewers: + bruthig@chromium.org
Description was changed from ========== Allow InkDropRipple to co-exist with highlight or not exist at all. 1. Allow InkDropRipple to co-exist with highlight by adding InkDropRipple::OverridesHighlight. I initially made this part of InkDropHost but then realized that it was more a property of the ripple itself, and it would be too easy to overlook when adding an ink drop to a new surface. 2. Allow InkDropRipple to not exist but still retaining an InkDropHighlight. This is desired for a couple menu-like UI surfaces in secondary UI. This is effected with an EmptyInkDropRipple class that just no-ops while allowing the hover effect to remain. The immediate effect of this change is that flood fill ripples will paint on top of the hover highlight effect (e.g. on bookmark buttons). BUG=617961 ========== to ========== Allow InkDropRipple to co-exist with highlight or not exist at all. 1. Allow InkDropRipple to co-exist with highlight by adding InkDropRipple::OverridesHighlight. I initially made this part of InkDropHost but then realized that it was more a property of the ripple itself, and it would be too easy to overlook when adding an ink drop to a new surface. 2. Allow InkDropRipple to not exist but still retain an InkDropHighlight. This is desired for a couple menu-like UI surfaces in secondary UI. This is effected with an EmptyInkDropRipple class that just no-ops while allowing the hover effect to remain. The immediate effect of this change is that flood fill ripples will paint on top of the hover highlight effect (e.g. on bookmark buttons). BUG=617961 ==========
On 2016/08/16 19:06:57, Evan Stade wrote: I took a quick look and in general looks good. I'm not sure if it's handled or not, but issue 617961 also requires the hover highlight to snap in when a floodfill ripple becomes visible. This doesn't need to be in this change I just didn't want to close the issue pre-maturely. And yes, can you please add some tests :) Thanks for doing this!!
I did a more thorough review and left some comments inline. At a high level, I weakly challenge your perspective that the "OverridesHighlight" property should be on the ripple. I agree it probably shouldn't be a property of InkDropHost but I do believe it should be a property of InkDropImpl. But since the InkDropHostView is responsible for creating the InkDropImpl, the IDHV would have to have some knowledge of this property, even if it was simply a parameter to the SetInkDropMode() function. Basically I imagine a surface using a FloodFillInkDropRipple that does want the highlight to fade out or even a SquareInkDropRipple that doesn't want a new highlight flavor to fade out. WDYT? https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/empt... File ui/views/animation/empty_ink_drop_ripple.h (right): https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/empt... ui/views/animation/empty_ink_drop_ripple.h:12: class VIEWS_EXPORT EmptyInkDropRipple : public InkDropRipple { Some docs would be nice or perhaps a more descriptive name like InvisibleInkDropRipple or NonVisibleInkDropRipple would be sufficient. https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_impl.cc (right): https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_impl.cc:179: ink_drop_host_->AddInkDropLayer(root_layer_.get()); Perhaps this should only add the |root_layer_| to the |ink_drop_host_| if |root_layer_| actually has child layers? https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_impl.cc:184: if (root_layer_added_to_host_ && !highlight_ && !ink_drop_ripple_) { This should check if the ink drop layer exists instead of whether |ink_drop_ripple_| is non-null. https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_impl.cc:208: if (ink_drop_ripple_->OverridesHighlight()) { I'm wondering if this should check |ink_drop_highlight_|->IsFadingInOrVisible() instead. That would be more future proof. https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_impl.cc:246: if (highlight_ && !(ink_drop_ripple_ && ink_drop_ripple_->IsVisible())) This clause is currently preventing the highlight from fading in if the ripple is visible. This needs to be updated to adhere to the new OverridesHighlight() semantics. https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_ripple.h (right): https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_ripple.h:60: virtual void AnimateToState(InkDropState ink_drop_state); This was intended to be a non-virtual function that would use the template pattern to allow descendants to add behaviour. The intention was that this base class would manage all the InkDropState transitions, notifying observers, etc and the descendants would add the visuals. So there may be some visual based logic baked into this class that doesn't make sense for the non-visual EmptyInkDropRipple. We should probably extract that visual logic from this class instead so that you don't need to make this function virtual. This would also apply to the SnapToActivate() function which already appears to be virtual but it shouldn't be. It's only working correctly since all descending classes call it when the overridden SnapToActivated() is called. WDYT? https://codereview.chromium.org/2250783002/diff/40001/ui/views/controls/butto... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/2250783002/diff/40001/ui/views/controls/butto... ui/views/controls/button/label_button.cc:448: : base::WrapUnique(new views::FloodFillInkDropRipple( I believe the convention is to use MakeUnique(). See discussion here: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/iQgMedVA8-k
> At a high level, I weakly challenge your perspective that the > "OverridesHighlight" property should be on the ripple. I agree it probably > shouldn't be a property of InkDropHost but I do believe it should be a property > of InkDropImpl. But since the InkDropHostView is responsible for creating the > InkDropImpl, the IDHV would have to have some knowledge of this property, even > if it was simply a parameter to the SetInkDropMode() function. Basically I > imagine a surface using a FloodFillInkDropRipple that does want the highlight to > fade out or even a SquareInkDropRipple that doesn't want a new highlight flavor > to fade out. > > WDYT? I specifically don't think we'll ever want the flood fill ripples to hide highlight, and this CL is intended to make that harder to do (i.e. harder to screw up). I want to give developers the minimum number of choices because more choices is more chances for inconsistency. It's still not impossible though, all you'd need is to add a setter on FloodFillInkDropRipple. https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/empt... File ui/views/animation/empty_ink_drop_ripple.h (right): https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/empt... ui/views/animation/empty_ink_drop_ripple.h:12: class VIEWS_EXPORT EmptyInkDropRipple : public InkDropRipple { On 2016/08/25 17:45:29, bruthig wrote: > Some docs would be nice or perhaps a more descriptive name like > InvisibleInkDropRipple or NonVisibleInkDropRipple would be sufficient. Done. https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_impl.cc (right): https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_impl.cc:179: ink_drop_host_->AddInkDropLayer(root_layer_.get()); On 2016/08/25 17:45:29, bruthig wrote: > Perhaps this should only add the |root_layer_| to the |ink_drop_host_| if > |root_layer_| actually has child layers? But we'll need to do that for highlight anyway. https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_impl.cc:184: if (root_layer_added_to_host_ && !highlight_ && !ink_drop_ripple_) { On 2016/08/25 17:45:29, bruthig wrote: > This should check if the ink drop layer exists instead of whether > |ink_drop_ripple_| is non-null. That would break the case where we destroy the highlight but the ripple is still around. https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_impl.cc:208: if (ink_drop_ripple_->OverridesHighlight()) { On 2016/08/25 17:45:29, bruthig wrote: > I'm wondering if this should check |ink_drop_highlight_|->IsFadingInOrVisible() > instead. That would be more future proof. Done. https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_impl.cc:246: if (highlight_ && !(ink_drop_ripple_ && ink_drop_ripple_->IsVisible())) On 2016/08/25 17:45:29, bruthig wrote: > This clause is currently preventing the highlight from fading in if the ripple > is visible. This needs to be updated to adhere to the new OverridesHighlight() > semantics. Done. https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_ripple.h (right): https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_ripple.h:60: virtual void AnimateToState(InkDropState ink_drop_state); On 2016/08/25 17:45:29, bruthig wrote: > This was intended to be a non-virtual function that would use the template > pattern to allow descendants to add behaviour. The intention was that this base > class would manage all the InkDropState transitions, notifying observers, etc > and the descendants would add the visuals. So there may be some visual based > logic baked into this class that doesn't make sense for the non-visual > EmptyInkDropRipple. We should probably extract that visual logic from this > class instead so that you don't need to make this function virtual. > > This would also apply to the SnapToActivate() function which already appears to > be virtual but it shouldn't be. It's only working correctly since all > descending classes call it when the overridden SnapToActivated() is called. > > WDYT? Generally speaking, extracting logic like this reduces encapsulation, adds to complexity, reduces readability (because what you have to read is spread across more files). This solution seems more elegant to me. I'm not sure what practical harm it's causing? https://codereview.chromium.org/2250783002/diff/40001/ui/views/controls/butto... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/2250783002/diff/40001/ui/views/controls/butto... ui/views/controls/button/label_button.cc:448: : base::WrapUnique(new views::FloodFillInkDropRipple( On 2016/08/25 17:45:29, bruthig wrote: > I believe the convention is to use MakeUnique(). See discussion here: > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/iQgMedVA8-k Done.
also added a unit test
The CQ bit was checked by estade@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.
On 2016/08/26 23:36:15, Evan Stade wrote: > > At a high level, I weakly challenge your perspective that the > > "OverridesHighlight" property should be on the ripple. I agree it probably > > shouldn't be a property of InkDropHost but I do believe it should be a > property > > of InkDropImpl. But since the InkDropHostView is responsible for creating the > > InkDropImpl, the IDHV would have to have some knowledge of this property, even > > if it was simply a parameter to the SetInkDropMode() function. Basically I > > imagine a surface using a FloodFillInkDropRipple that does want the highlight > to > > fade out or even a SquareInkDropRipple that doesn't want a new highlight > flavor > > to fade out. > > > > WDYT? > > I specifically don't think we'll ever want the flood fill ripples to hide > highlight, and this CL is intended to make that harder to do (i.e. harder to > screw up). I want to give developers the minimum number of choices because more > choices is more chances for inconsistency. It's still not impossible though, all > you'd need is to add a setter on FloodFillInkDropRipple. Ack https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_impl.cc (right): https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_impl.cc:179: ink_drop_host_->AddInkDropLayer(root_layer_.get()); On 2016/08/26 23:36:15, Evan Stade wrote: > On 2016/08/25 17:45:29, bruthig wrote: > > Perhaps this should only add the |root_layer_| to the |ink_drop_host_| if > > |root_layer_| actually has child layers? > > But we'll need to do that for highlight anyway. I don't think this method should make assumptions on when it is called and whether the specific highlight type will require this. This type of assumption makes extension much more difficult. Why not just implement it for the general case now? https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_impl.cc:184: if (root_layer_added_to_host_ && !highlight_ && !ink_drop_ripple_) { On 2016/08/26 23:36:15, Evan Stade wrote: > On 2016/08/25 17:45:29, bruthig wrote: > > This should check if the ink drop layer exists instead of whether > > |ink_drop_ripple_| is non-null. > > That would break the case where we destroy the highlight but the ripple is still > around. Sorry I wasn't clear on that. The following re-write would allow |highlight_| and |ink_drop_ripple_| instances to exist that don't actually have Layers. if (root_layer_added_to_host_ && (!highlight_ || !highlight->GetRootLayer()| && (!ink_drop_ripple_ || !ink_drop_ripple_->GetRootLayer())) { ... } https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_ripple.h (right): https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_ripple.h:60: virtual void AnimateToState(InkDropState ink_drop_state); On 2016/08/26 23:36:15, Evan Stade wrote: > On 2016/08/25 17:45:29, bruthig wrote: > > This was intended to be a non-virtual function that would use the template > > pattern to allow descendants to add behaviour. The intention was that this > base > > class would manage all the InkDropState transitions, notifying observers, etc > > and the descendants would add the visuals. So there may be some visual based > > logic baked into this class that doesn't make sense for the non-visual > > EmptyInkDropRipple. We should probably extract that visual logic from this > > class instead so that you don't need to make this function virtual. > > > > This would also apply to the SnapToActivate() function which already appears > to > > be virtual but it shouldn't be. It's only working correctly since all > > descending classes call it when the overridden SnapToActivated() is called. > > > > WDYT? > > Generally speaking, extracting logic like this reduces encapsulation, adds to > complexity, reduces readability (because what you have to read is spread across > more files). This solution seems more elegant to me. I'm not sure what practical > harm it's causing? The InkDropRipple class defines a contract that guarantees that the InkDropRippleObserver is notified for all state changes. IMO your change makes it much more difficult to understand and reason about the InkDropImpl class because sometimes it will be notified and sometimes it won't. Also, by making it virtual you are offering future developers more choice as to which function to override, i.e. AnimateToState() or AnimateStateChange(). Contrary to your point in another comment where you are in favor of limiting choice. I've seen this same pattern in the EventHandler/View/Button hierarchy where the general OnEvent() handler as well as the more specific event handlers like OnGestureEvent() are virtual. The root EventHandler::OnEvent() dispatches to these more specific event handlers and this makes it very difficult to reason about when a more specific event handler will be called in a concrete View because you have to look up and down the hierarchy to make sure all OnEvent()'s call the parent's OnEvent(). I don't understand what you mean when you say this reduces encapsulation. Can you elaborate? The InkDropRipple class encapsulates state transitions and notifying observers. Descendant classes encapsulate the visual (or non-visual) animations. I feel like these two levels of abstraction are a quite good and by extracting some common visual logic doesn't expose it to clients of the InkDropRipple in anyway. The practical harm is that developers will need to understand all of the InkDropRipple flavors when trying to understand the InkDropImpl class. By keeping the uncertainty/complexity closer together it makes the larger system easier to reason about. https://codereview.chromium.org/2250783002/diff/80001/ui/views/animation/ink_... File ui/views/animation/ink_drop_impl_unittest.cc (right): https://codereview.chromium.org/2250783002/diff/80001/ui/views/animation/ink_... ui/views/animation/ink_drop_impl_unittest.cc:317: ink_drop_.SetHovered(true); Can you add a test that makes the ripple visible before the highlight?
https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_impl.cc (right): https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_impl.cc:179: ink_drop_host_->AddInkDropLayer(root_layer_.get()); On 2016/08/31 22:22:29, bruthig wrote: > On 2016/08/26 23:36:15, Evan Stade wrote: > > On 2016/08/25 17:45:29, bruthig wrote: > > > Perhaps this should only add the |root_layer_| to the |ink_drop_host_| if > > > |root_layer_| actually has child layers? > > > > But we'll need to do that for highlight anyway. > > I don't think this method should make assumptions on when it is called and > whether the specific highlight type will require this. This type of assumption > makes extension much more difficult. Why not just implement it for the general > case now? If both highlight and ripple are layerless, then there's no point in having an InkDropImpl, is there? I don't want to support a use case that doesn't exist. That's why. I've made some changes though, do you like them? https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_impl.cc:184: if (root_layer_added_to_host_ && !highlight_ && !ink_drop_ripple_) { On 2016/08/31 22:22:29, bruthig wrote: > On 2016/08/26 23:36:15, Evan Stade wrote: > > On 2016/08/25 17:45:29, bruthig wrote: > > > This should check if the ink drop layer exists instead of whether > > > |ink_drop_ripple_| is non-null. > > > > That would break the case where we destroy the highlight but the ripple is > still > > around. > > Sorry I wasn't clear on that. The following re-write would allow |highlight_| > and |ink_drop_ripple_| instances to exist that don't actually have Layers. > > if (root_layer_added_to_host_ && (!highlight_ || !highlight->GetRootLayer()| && > (!ink_drop_ripple_ || !ink_drop_ripple_->GetRootLayer())) { ... } I don't want to support an empty ripple and highlight until there's a good reason to. But if the ripple has no layer then root_layer_added_to_host will be false. https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_ripple.h (right): https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_ripple.h:60: virtual void AnimateToState(InkDropState ink_drop_state); On 2016/08/31 22:22:29, bruthig wrote: > On 2016/08/26 23:36:15, Evan Stade wrote: > > On 2016/08/25 17:45:29, bruthig wrote: > > > This was intended to be a non-virtual function that would use the template > > > pattern to allow descendants to add behaviour. The intention was that this > > base > > > class would manage all the InkDropState transitions, notifying observers, > etc > > > and the descendants would add the visuals. So there may be some visual > based > > > logic baked into this class that doesn't make sense for the non-visual > > > EmptyInkDropRipple. We should probably extract that visual logic from this > > > class instead so that you don't need to make this function virtual. > > > > > > This would also apply to the SnapToActivate() function which already appears > > to > > > be virtual but it shouldn't be. It's only working correctly since all > > > descending classes call it when the overridden SnapToActivated() is called. > > > > > > WDYT? > > > > Generally speaking, extracting logic like this reduces encapsulation, adds to > > complexity, reduces readability (because what you have to read is spread > across > > more files). This solution seems more elegant to me. I'm not sure what > practical > > harm it's causing? > > The InkDropRipple class defines a contract that guarantees that the > InkDropRippleObserver is notified for all state changes. IMO your change makes > it much more difficult to understand and reason about the InkDropImpl class > because sometimes it will be notified and sometimes it won't. Also, by making > it virtual you are offering future developers more choice as to which function > to override, i.e. AnimateToState() or AnimateStateChange(). Contrary to your > point in another comment where you are in favor of limiting choice. It's not really a choice, because AnimateStateChange is pure virtual so you must override it. > > I've seen this same pattern in the EventHandler/View/Button hierarchy where the > general OnEvent() handler as well as the more specific event handlers like > OnGestureEvent() are virtual. The root EventHandler::OnEvent() dispatches to > these more specific event handlers and this makes it very difficult to reason > about when a more specific event handler will be called in a concrete View > because you have to look up and down the hierarchy to make sure all OnEvent()'s > call the parent's OnEvent(). > > I don't understand what you mean when you say this reduces encapsulation. Can > you elaborate? > > The InkDropRipple class encapsulates state transitions and notifying observers. > Descendant classes encapsulate the visual (or non-visual) animations. I feel > like these two levels of abstraction are a quite good and by extracting some > common visual logic doesn't expose it to clients of the InkDropRipple in anyway. I'm invoking encapsulation as the principle of all related logic being close together as good. > > The practical harm is that developers will need to understand all of the > InkDropRipple flavors when trying to understand the InkDropImpl class. By > keeping the uncertainty/complexity closer together it makes the larger system > easier to reason about. > I mean, EmptyInkDropRipple is pretty easy to understand. It barely has any lines. Would you feel better about adding a new virtual function called bool HasAnimations? AnimateToState would early return if !HasAnimations. That would accomplish the goal here without giving future developers undue amounts of flexibility.
https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_impl.cc (right): https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_impl.cc:179: ink_drop_host_->AddInkDropLayer(root_layer_.get()); On 2016/09/01 01:19:15, Evan Stade wrote: > On 2016/08/31 22:22:29, bruthig wrote: > > On 2016/08/26 23:36:15, Evan Stade wrote: > > > On 2016/08/25 17:45:29, bruthig wrote: > > > > Perhaps this should only add the |root_layer_| to the |ink_drop_host_| if > > > > |root_layer_| actually has child layers? > > > > > > But we'll need to do that for highlight anyway. > > > > I don't think this method should make assumptions on when it is called and > > whether the specific highlight type will require this. This type of > assumption > > makes extension much more difficult. Why not just implement it for the > general > > case now? > > If both highlight and ripple are layerless, then there's no point in having an > InkDropImpl, is there? I don't want to support a use case that doesn't exist. > That's why. > > I've made some changes though, do you like them? Yeah https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_impl.cc:184: if (root_layer_added_to_host_ && !highlight_ && !ink_drop_ripple_) { On 2016/09/01 01:19:15, Evan Stade wrote: > On 2016/08/31 22:22:29, bruthig wrote: > > On 2016/08/26 23:36:15, Evan Stade wrote: > > > On 2016/08/25 17:45:29, bruthig wrote: > > > > This should check if the ink drop layer exists instead of whether > > > > |ink_drop_ripple_| is non-null. > > > > > > That would break the case where we destroy the highlight but the ripple is > > still > > > around. > > > > Sorry I wasn't clear on that. The following re-write would allow |highlight_| > > and |ink_drop_ripple_| instances to exist that don't actually have Layers. > > > > if (root_layer_added_to_host_ && (!highlight_ || !highlight->GetRootLayer()| > && > > (!ink_drop_ripple_ || !ink_drop_ripple_->GetRootLayer())) { ... } > > I don't want to support an empty ripple and highlight until there's a good > reason to. But if the ripple has no layer then root_layer_added_to_host will be > false. Consider the following case: 1. SetHover(true) - Root layer gets added 2. CreateInkDropRipple() 3. SetHover(false) -> DestroyInkDropHighlight() -> RemoveRootLayerFromHostIfNeeded() - Root layer is NOT removed because |ink_drop_ripple_| is not null - And as it currently stands DestroyInkDropRipple() will not be called because EmptyInkDropRipple doesn't notify the AnimationEnded observer. Thus the InkDropHostView will have a 'zombie' ink drop layer. This might be worthy of a test... https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_ripple.h (right): https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_ripple.h:60: virtual void AnimateToState(InkDropState ink_drop_state); On 2016/09/01 01:19:15, Evan Stade wrote: > On 2016/08/31 22:22:29, bruthig wrote: > > On 2016/08/26 23:36:15, Evan Stade wrote: > > > On 2016/08/25 17:45:29, bruthig wrote: > > > > This was intended to be a non-virtual function that would use the template > > > > pattern to allow descendants to add behaviour. The intention was that > this > > > base > > > > class would manage all the InkDropState transitions, notifying observers, > > etc > > > > and the descendants would add the visuals. So there may be some visual > > based > > > > logic baked into this class that doesn't make sense for the non-visual > > > > EmptyInkDropRipple. We should probably extract that visual logic from > this > > > > class instead so that you don't need to make this function virtual. > > > > > > > > This would also apply to the SnapToActivate() function which already > appears > > > to > > > > be virtual but it shouldn't be. It's only working correctly since all > > > > descending classes call it when the overridden SnapToActivated() is > called. > > > > > > > > WDYT? > > > > > > Generally speaking, extracting logic like this reduces encapsulation, adds > to > > > complexity, reduces readability (because what you have to read is spread > > across > > > more files). This solution seems more elegant to me. I'm not sure what > > practical > > > harm it's causing? > > > > The InkDropRipple class defines a contract that guarantees that the > > InkDropRippleObserver is notified for all state changes. IMO your change > makes > > it much more difficult to understand and reason about the InkDropImpl class > > because sometimes it will be notified and sometimes it won't. Also, by making > > it virtual you are offering future developers more choice as to which function > > to override, i.e. AnimateToState() or AnimateStateChange(). Contrary to your > > point in another comment where you are in favor of limiting choice. > > It's not really a choice, because AnimateStateChange is pure virtual so you must > override it. > > > > > I've seen this same pattern in the EventHandler/View/Button hierarchy where > the > > general OnEvent() handler as well as the more specific event handlers like > > OnGestureEvent() are virtual. The root EventHandler::OnEvent() dispatches to > > these more specific event handlers and this makes it very difficult to reason > > about when a more specific event handler will be called in a concrete View > > because you have to look up and down the hierarchy to make sure all > OnEvent()'s > > call the parent's OnEvent(). > > > > I don't understand what you mean when you say this reduces encapsulation. Can > > you elaborate? > > > > The InkDropRipple class encapsulates state transitions and notifying > observers. > > Descendant classes encapsulate the visual (or non-visual) animations. I feel > > like these two levels of abstraction are a quite good and by extracting some > > common visual logic doesn't expose it to clients of the InkDropRipple in > anyway. > > I'm invoking encapsulation as the principle of all related logic being close > together as good. > > > > > The practical harm is that developers will need to understand all of the > > InkDropRipple flavors when trying to understand the InkDropImpl class. By > > keeping the uncertainty/complexity closer together it makes the larger system > > easier to reason about. > > > > I mean, EmptyInkDropRipple is pretty easy to understand. It barely has any > lines. > > Would you feel better about adding a new virtual function called bool > HasAnimations? AnimateToState would early return if !HasAnimations. That would > accomplish the goal here without giving future developers undue amounts of > flexibility. I would have to see this more concretely. The major issue here is that AnimationEnded is not going to be called and thus the |ink_drop_ripple_| is not going to be destroyed.
The CQ bit was checked by estade@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 checked by estade@chromium.org to run a CQ dry run
https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_impl.cc (right): https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_impl.cc:184: if (root_layer_added_to_host_ && !highlight_ && !ink_drop_ripple_) { On 2016/09/01 15:53:57, bruthig wrote: > On 2016/09/01 01:19:15, Evan Stade wrote: > > On 2016/08/31 22:22:29, bruthig wrote: > > > On 2016/08/26 23:36:15, Evan Stade wrote: > > > > On 2016/08/25 17:45:29, bruthig wrote: > > > > > This should check if the ink drop layer exists instead of whether > > > > > |ink_drop_ripple_| is non-null. > > > > > > > > That would break the case where we destroy the highlight but the ripple is > > > still > > > > around. > > > > > > Sorry I wasn't clear on that. The following re-write would allow > |highlight_| > > > and |ink_drop_ripple_| instances to exist that don't actually have Layers. > > > > > > if (root_layer_added_to_host_ && (!highlight_ || !highlight->GetRootLayer()| > > && > > > (!ink_drop_ripple_ || !ink_drop_ripple_->GetRootLayer())) { ... } > > > > I don't want to support an empty ripple and highlight until there's a good > > reason to. But if the ripple has no layer then root_layer_added_to_host will > be > > false. > > Consider the following case: > > 1. SetHover(true) > - Root layer gets added > > 2. CreateInkDropRipple() > > 3. SetHover(false) -> DestroyInkDropHighlight() -> > RemoveRootLayerFromHostIfNeeded() > - Root layer is NOT removed because |ink_drop_ripple_| is not null > - And as it currently stands DestroyInkDropRipple() will not be called because > EmptyInkDropRipple doesn't notify the AnimationEnded observer. Thus the > InkDropHostView will have a 'zombie' ink drop layer. > > This might be worthy of a test... Done. https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_ripple.h (right): https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_ripple.h:60: virtual void AnimateToState(InkDropState ink_drop_state); On 2016/09/01 15:53:57, bruthig wrote: > On 2016/09/01 01:19:15, Evan Stade wrote: > > On 2016/08/31 22:22:29, bruthig wrote: > > > On 2016/08/26 23:36:15, Evan Stade wrote: > > > > On 2016/08/25 17:45:29, bruthig wrote: > > > > > This was intended to be a non-virtual function that would use the > template > > > > > pattern to allow descendants to add behaviour. The intention was that > > this > > > > base > > > > > class would manage all the InkDropState transitions, notifying > observers, > > > etc > > > > > and the descendants would add the visuals. So there may be some visual > > > based > > > > > logic baked into this class that doesn't make sense for the non-visual > > > > > EmptyInkDropRipple. We should probably extract that visual logic from > > this > > > > > class instead so that you don't need to make this function virtual. > > > > > > > > > > This would also apply to the SnapToActivate() function which already > > appears > > > > to > > > > > be virtual but it shouldn't be. It's only working correctly since all > > > > > descending classes call it when the overridden SnapToActivated() is > > called. > > > > > > > > > > WDYT? > > > > > > > > Generally speaking, extracting logic like this reduces encapsulation, adds > > to > > > > complexity, reduces readability (because what you have to read is spread > > > across > > > > more files). This solution seems more elegant to me. I'm not sure what > > > practical > > > > harm it's causing? > > > > > > The InkDropRipple class defines a contract that guarantees that the > > > InkDropRippleObserver is notified for all state changes. IMO your change > > makes > > > it much more difficult to understand and reason about the InkDropImpl class > > > because sometimes it will be notified and sometimes it won't. Also, by > making > > > it virtual you are offering future developers more choice as to which > function > > > to override, i.e. AnimateToState() or AnimateStateChange(). Contrary to > your > > > point in another comment where you are in favor of limiting choice. > > > > It's not really a choice, because AnimateStateChange is pure virtual so you > must > > override it. > > > > > > > > I've seen this same pattern in the EventHandler/View/Button hierarchy where > > the > > > general OnEvent() handler as well as the more specific event handlers like > > > OnGestureEvent() are virtual. The root EventHandler::OnEvent() dispatches > to > > > these more specific event handlers and this makes it very difficult to > reason > > > about when a more specific event handler will be called in a concrete View > > > because you have to look up and down the hierarchy to make sure all > > OnEvent()'s > > > call the parent's OnEvent(). > > > > > > I don't understand what you mean when you say this reduces encapsulation. > Can > > > you elaborate? > > > > > > The InkDropRipple class encapsulates state transitions and notifying > > observers. > > > Descendant classes encapsulate the visual (or non-visual) animations. I > feel > > > like these two levels of abstraction are a quite good and by extracting some > > > common visual logic doesn't expose it to clients of the InkDropRipple in > > anyway. > > > > I'm invoking encapsulation as the principle of all related logic being close > > together as good. > > > > > > > > The practical harm is that developers will need to understand all of the > > > InkDropRipple flavors when trying to understand the InkDropImpl class. By > > > keeping the uncertainty/complexity closer together it makes the larger > system > > > easier to reason about. > > > > > > > I mean, EmptyInkDropRipple is pretty easy to understand. It barely has any > > lines. > > > > Would you feel better about adding a new virtual function called bool > > HasAnimations? AnimateToState would early return if !HasAnimations. That would > > accomplish the goal here without giving future developers undue amounts of > > flexibility. > > I would have to see this more concretely. The major issue here is that > AnimationEnded is not going to be called and thus the |ink_drop_ripple_| is not > going to be destroyed. updated. I don't think it's really a problem if an empty ink drop ripple is never destroyed, but it actually will be destroyed in DestroyHiddenTargetedAnimations because the target state is always HIDDEN.
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Can you: - update InkDropTest to run the tests for the new ripple flavor? - update the InkDropRippleTests to run tests for the new ripple flavor and update existing ones accordingly? Obviously we won't want to run the tests that deal with visual stuff, perhaps you could extend InkDropRippleTest with VisibleInkDropRippleTest and move the related tests under there. https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_ripple.h (right): https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_ripple.h:60: virtual void AnimateToState(InkDropState ink_drop_state); On 2016/09/07 23:46:19, Evan Stade wrote: > On 2016/09/01 15:53:57, bruthig wrote: > > On 2016/09/01 01:19:15, Evan Stade wrote: > > > On 2016/08/31 22:22:29, bruthig wrote: > > > > On 2016/08/26 23:36:15, Evan Stade wrote: > > > > > On 2016/08/25 17:45:29, bruthig wrote: > > > > > > This was intended to be a non-virtual function that would use the > > template > > > > > > pattern to allow descendants to add behaviour. The intention was that > > > this > > > > > base > > > > > > class would manage all the InkDropState transitions, notifying > > observers, > > > > etc > > > > > > and the descendants would add the visuals. So there may be some > visual > > > > based > > > > > > logic baked into this class that doesn't make sense for the non-visual > > > > > > EmptyInkDropRipple. We should probably extract that visual logic from > > > this > > > > > > class instead so that you don't need to make this function virtual. > > > > > > > > > > > > This would also apply to the SnapToActivate() function which already > > > appears > > > > > to > > > > > > be virtual but it shouldn't be. It's only working correctly since all > > > > > > descending classes call it when the overridden SnapToActivated() is > > > called. > > > > > > > > > > > > WDYT? > > > > > > > > > > Generally speaking, extracting logic like this reduces encapsulation, > adds > > > to > > > > > complexity, reduces readability (because what you have to read is spread > > > > across > > > > > more files). This solution seems more elegant to me. I'm not sure what > > > > practical > > > > > harm it's causing? > > > > > > > > The InkDropRipple class defines a contract that guarantees that the > > > > InkDropRippleObserver is notified for all state changes. IMO your change > > > makes > > > > it much more difficult to understand and reason about the InkDropImpl > class > > > > because sometimes it will be notified and sometimes it won't. Also, by > > making > > > > it virtual you are offering future developers more choice as to which > > function > > > > to override, i.e. AnimateToState() or AnimateStateChange(). Contrary to > > your > > > > point in another comment where you are in favor of limiting choice. > > > > > > It's not really a choice, because AnimateStateChange is pure virtual so you > > must > > > override it. > > > > > > > > > > > I've seen this same pattern in the EventHandler/View/Button hierarchy > where > > > the > > > > general OnEvent() handler as well as the more specific event handlers like > > > > OnGestureEvent() are virtual. The root EventHandler::OnEvent() dispatches > > to > > > > these more specific event handlers and this makes it very difficult to > > reason > > > > about when a more specific event handler will be called in a concrete View > > > > because you have to look up and down the hierarchy to make sure all > > > OnEvent()'s > > > > call the parent's OnEvent(). > > > > > > > > I don't understand what you mean when you say this reduces encapsulation. > > Can > > > > you elaborate? > > > > > > > > The InkDropRipple class encapsulates state transitions and notifying > > > observers. > > > > Descendant classes encapsulate the visual (or non-visual) animations. I > > feel > > > > like these two levels of abstraction are a quite good and by extracting > some > > > > common visual logic doesn't expose it to clients of the InkDropRipple in > > > anyway. > > > > > > I'm invoking encapsulation as the principle of all related logic being close > > > together as good. > > > > > > > > > > > The practical harm is that developers will need to understand all of the > > > > InkDropRipple flavors when trying to understand the InkDropImpl class. By > > > > keeping the uncertainty/complexity closer together it makes the larger > > system > > > > easier to reason about. > > > > > > > > > > I mean, EmptyInkDropRipple is pretty easy to understand. It barely has any > > > lines. > > > > > > Would you feel better about adding a new virtual function called bool > > > HasAnimations? AnimateToState would early return if !HasAnimations. That > would > > > accomplish the goal here without giving future developers undue amounts of > > > flexibility. > > > > I would have to see this more concretely. The major issue here is that > > AnimationEnded is not going to be called and thus the |ink_drop_ripple_| is > not > > going to be destroyed. > > updated. > > I don't think it's really a problem if an empty ink drop ripple is never > destroyed, but it actually will be destroyed in DestroyHiddenTargetedAnimations > because the target state is always HIDDEN. Right, the layer won't leak memory but it will exist until the InkDropImpl::AnimateToState() or InkDropImpl::SnapToAcitvated() is called the next time. In which case the existing EmptyInkDrop will be destroyed but a new one will be created right away. Thus any InkDrop enabled surface, that has had an animation will have an instance of the EmptyInkDrop. At this point it would be a simple change. Instead of returning early in InkDropRipple::AnimateToState() and SnapToActivated() just allow the creation of the |animation_observer| and the SetActive() called on it and prevent all the other code from being called. https://codereview.chromium.org/2250783002/diff/140001/ui/views/animation/ink... File ui/views/animation/ink_drop_impl.cc (right): https://codereview.chromium.org/2250783002/diff/140001/ui/views/animation/ink... ui/views/animation/ink_drop_impl.cc:249: ink_drop_ripple_->OverridesHighlight())) { Can this have a test? https://codereview.chromium.org/2250783002/diff/140001/ui/views/animation/ink... File ui/views/animation/ink_drop_impl_unittest.cc (right): https://codereview.chromium.org/2250783002/diff/140001/ui/views/animation/ink... ui/views/animation/ink_drop_impl_unittest.cc:311: TEST_F(InkDropImplTest, HighlightCanCoexistWithRipple) { Can you add a similar test for SnapToActivated()?
On 2016/09/08 15:56:04, bruthig wrote: > Can you: > - update InkDropTest to run the tests for the new ripple flavor? I don't understand this request because InkDropTest doesn't test different ripple flavors. > - update the InkDropRippleTests to run tests for the new ripple flavor and > update existing ones accordingly? Obviously we won't want to run the tests that > deal with visual stuff, perhaps you could extend InkDropRippleTest with > VisibleInkDropRippleTest and move the related tests under there. I started to do this, but literally all of them except the trivial one deal with visuals
https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_ripple.h (right): https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_ripple.h:60: virtual void AnimateToState(InkDropState ink_drop_state); On 2016/09/08 15:56:04, bruthig wrote: > On 2016/09/07 23:46:19, Evan Stade wrote: > > On 2016/09/01 15:53:57, bruthig wrote: > > > On 2016/09/01 01:19:15, Evan Stade wrote: > > > > On 2016/08/31 22:22:29, bruthig wrote: > > > > > On 2016/08/26 23:36:15, Evan Stade wrote: > > > > > > On 2016/08/25 17:45:29, bruthig wrote: > > > > > > > This was intended to be a non-virtual function that would use the > > > template > > > > > > > pattern to allow descendants to add behaviour. The intention was > that > > > > this > > > > > > base > > > > > > > class would manage all the InkDropState transitions, notifying > > > observers, > > > > > etc > > > > > > > and the descendants would add the visuals. So there may be some > > visual > > > > > based > > > > > > > logic baked into this class that doesn't make sense for the > non-visual > > > > > > > EmptyInkDropRipple. We should probably extract that visual logic > from > > > > this > > > > > > > class instead so that you don't need to make this function virtual. > > > > > > > > > > > > > > This would also apply to the SnapToActivate() function which already > > > > appears > > > > > > to > > > > > > > be virtual but it shouldn't be. It's only working correctly since > all > > > > > > > descending classes call it when the overridden SnapToActivated() is > > > > called. > > > > > > > > > > > > > > WDYT? > > > > > > > > > > > > Generally speaking, extracting logic like this reduces encapsulation, > > adds > > > > to > > > > > > complexity, reduces readability (because what you have to read is > spread > > > > > across > > > > > > more files). This solution seems more elegant to me. I'm not sure what > > > > > practical > > > > > > harm it's causing? > > > > > > > > > > The InkDropRipple class defines a contract that guarantees that the > > > > > InkDropRippleObserver is notified for all state changes. IMO your > change > > > > makes > > > > > it much more difficult to understand and reason about the InkDropImpl > > class > > > > > because sometimes it will be notified and sometimes it won't. Also, by > > > making > > > > > it virtual you are offering future developers more choice as to which > > > function > > > > > to override, i.e. AnimateToState() or AnimateStateChange(). Contrary to > > > your > > > > > point in another comment where you are in favor of limiting choice. > > > > > > > > It's not really a choice, because AnimateStateChange is pure virtual so > you > > > must > > > > override it. > > > > > > > > > > > > > > I've seen this same pattern in the EventHandler/View/Button hierarchy > > where > > > > the > > > > > general OnEvent() handler as well as the more specific event handlers > like > > > > > OnGestureEvent() are virtual. The root EventHandler::OnEvent() > dispatches > > > to > > > > > these more specific event handlers and this makes it very difficult to > > > reason > > > > > about when a more specific event handler will be called in a concrete > View > > > > > because you have to look up and down the hierarchy to make sure all > > > > OnEvent()'s > > > > > call the parent's OnEvent(). > > > > > > > > > > I don't understand what you mean when you say this reduces > encapsulation. > > > Can > > > > > you elaborate? > > > > > > > > > > The InkDropRipple class encapsulates state transitions and notifying > > > > observers. > > > > > Descendant classes encapsulate the visual (or non-visual) animations. I > > > feel > > > > > like these two levels of abstraction are a quite good and by extracting > > some > > > > > common visual logic doesn't expose it to clients of the InkDropRipple in > > > > anyway. > > > > > > > > I'm invoking encapsulation as the principle of all related logic being > close > > > > together as good. > > > > > > > > > > > > > > The practical harm is that developers will need to understand all of the > > > > > InkDropRipple flavors when trying to understand the InkDropImpl class. > By > > > > > keeping the uncertainty/complexity closer together it makes the larger > > > system > > > > > easier to reason about. > > > > > > > > > > > > > I mean, EmptyInkDropRipple is pretty easy to understand. It barely has any > > > > lines. > > > > > > > > Would you feel better about adding a new virtual function called bool > > > > HasAnimations? AnimateToState would early return if !HasAnimations. That > > would > > > > accomplish the goal here without giving future developers undue amounts of > > > > flexibility. > > > > > > I would have to see this more concretely. The major issue here is that > > > AnimationEnded is not going to be called and thus the |ink_drop_ripple_| is > > not > > > going to be destroyed. > > > > updated. > > > > I don't think it's really a problem if an empty ink drop ripple is never > > destroyed, but it actually will be destroyed in > DestroyHiddenTargetedAnimations > > because the target state is always HIDDEN. > > Right, the layer won't leak memory but it will exist until the > InkDropImpl::AnimateToState() or InkDropImpl::SnapToAcitvated() is called the > next time. In which case the existing EmptyInkDrop will be destroyed but a new > one will be created right away. Thus any InkDrop enabled surface, that has had > an animation will have an instance of the EmptyInkDrop. At this point it would > be a simple change. Instead of returning early in > InkDropRipple::AnimateToState() and SnapToActivated() just allow the creation of > the |animation_observer| and the SetActive() called on it and prevent all the > other code from being called. I don't understand the motivation to do that. We're releasing some memory a little earlier by allocating more memory? https://codereview.chromium.org/2250783002/diff/140001/ui/views/animation/ink... File ui/views/animation/ink_drop_impl.cc (right): https://codereview.chromium.org/2250783002/diff/140001/ui/views/animation/ink... ui/views/animation/ink_drop_impl.cc:249: ink_drop_ripple_->OverridesHighlight())) { On 2016/09/08 15:56:04, bruthig wrote: > Can this have a test? added to HighlightCanCoexistWithRipple https://codereview.chromium.org/2250783002/diff/140001/ui/views/animation/ink... File ui/views/animation/ink_drop_impl_unittest.cc (right): https://codereview.chromium.org/2250783002/diff/140001/ui/views/animation/ink... ui/views/animation/ink_drop_impl_unittest.cc:311: TEST_F(InkDropImplTest, HighlightCanCoexistWithRipple) { On 2016/09/08 15:56:04, bruthig wrote: > Can you add a similar test for SnapToActivated()? Done.
On 2016/09/08 19:16:18, Evan Stade wrote: > On 2016/09/08 15:56:04, bruthig wrote: > > Can you: > > - update InkDropTest to run the tests for the new ripple flavor? > > I don't understand this request because InkDropTest doesn't test different > ripple flavors. Hmm, I was thinking testing an InkDropImpl with the EmptyInkDropRipple, but I looked at the tests there again and they don't look like they'd add any value. Forget I said anything. > > > - update the InkDropRippleTests to run tests for the new ripple flavor and > > update existing ones accordingly? Obviously we won't want to run the tests > that > > deal with visual stuff, perhaps you could extend InkDropRippleTest with > > VisibleInkDropRippleTest and move the related tests under there. > > I started to do this, but literally all of them except the trivial one deal with > visuals I was thinking the following tests would apply but they hinge on the debate about notifying observers and when the EmptyInkDropRipple is destroyed. InkDropRippleTest::InitialStateAfterConstruction InkDropRippleTest::VerifyObserversAreNotified InkDropRippleTest::VerifyObserversAreNotifiedOfSuccessfulAnimations InkDropRippleTest::VerifyObserversAreNotifiedOfPreemptedAnimations InkDropRippleTest::InkDropStatesPersistWhenCallingAnimateToState https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_ripple.h (right): https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_ripple.h:60: virtual void AnimateToState(InkDropState ink_drop_state); On 2016/09/08 19:17:31, Evan Stade wrote: > On 2016/09/08 15:56:04, bruthig wrote: > > On 2016/09/07 23:46:19, Evan Stade wrote: > > > On 2016/09/01 15:53:57, bruthig wrote: > > > > On 2016/09/01 01:19:15, Evan Stade wrote: > > > > > On 2016/08/31 22:22:29, bruthig wrote: > > > > > > On 2016/08/26 23:36:15, Evan Stade wrote: > > > > > > > On 2016/08/25 17:45:29, bruthig wrote: > > > > > > > > This was intended to be a non-virtual function that would use the > > > > template > > > > > > > > pattern to allow descendants to add behaviour. The intention was > > that > > > > > this > > > > > > > base > > > > > > > > class would manage all the InkDropState transitions, notifying > > > > observers, > > > > > > etc > > > > > > > > and the descendants would add the visuals. So there may be some > > > visual > > > > > > based > > > > > > > > logic baked into this class that doesn't make sense for the > > non-visual > > > > > > > > EmptyInkDropRipple. We should probably extract that visual logic > > from > > > > > this > > > > > > > > class instead so that you don't need to make this function > virtual. > > > > > > > > > > > > > > > > This would also apply to the SnapToActivate() function which > already > > > > > appears > > > > > > > to > > > > > > > > be virtual but it shouldn't be. It's only working correctly since > > all > > > > > > > > descending classes call it when the overridden SnapToActivated() > is > > > > > called. > > > > > > > > > > > > > > > > WDYT? > > > > > > > > > > > > > > Generally speaking, extracting logic like this reduces > encapsulation, > > > adds > > > > > to > > > > > > > complexity, reduces readability (because what you have to read is > > spread > > > > > > across > > > > > > > more files). This solution seems more elegant to me. I'm not sure > what > > > > > > practical > > > > > > > harm it's causing? > > > > > > > > > > > > The InkDropRipple class defines a contract that guarantees that the > > > > > > InkDropRippleObserver is notified for all state changes. IMO your > > change > > > > > makes > > > > > > it much more difficult to understand and reason about the InkDropImpl > > > class > > > > > > because sometimes it will be notified and sometimes it won't. Also, > by > > > > making > > > > > > it virtual you are offering future developers more choice as to which > > > > function > > > > > > to override, i.e. AnimateToState() or AnimateStateChange(). Contrary > to > > > > your > > > > > > point in another comment where you are in favor of limiting choice. > > > > > > > > > > It's not really a choice, because AnimateStateChange is pure virtual so > > you > > > > must > > > > > override it. > > > > > > > > > > > > > > > > > I've seen this same pattern in the EventHandler/View/Button hierarchy > > > where > > > > > the > > > > > > general OnEvent() handler as well as the more specific event handlers > > like > > > > > > OnGestureEvent() are virtual. The root EventHandler::OnEvent() > > dispatches > > > > to > > > > > > these more specific event handlers and this makes it very difficult to > > > > reason > > > > > > about when a more specific event handler will be called in a concrete > > View > > > > > > because you have to look up and down the hierarchy to make sure all > > > > > OnEvent()'s > > > > > > call the parent's OnEvent(). > > > > > > > > > > > > I don't understand what you mean when you say this reduces > > encapsulation. > > > > Can > > > > > > you elaborate? > > > > > > > > > > > > The InkDropRipple class encapsulates state transitions and notifying > > > > > observers. > > > > > > Descendant classes encapsulate the visual (or non-visual) animations. > I > > > > feel > > > > > > like these two levels of abstraction are a quite good and by > extracting > > > some > > > > > > common visual logic doesn't expose it to clients of the InkDropRipple > in > > > > > anyway. > > > > > > > > > > I'm invoking encapsulation as the principle of all related logic being > > close > > > > > together as good. > > > > > > > > > > > > > > > > > The practical harm is that developers will need to understand all of > the > > > > > > InkDropRipple flavors when trying to understand the InkDropImpl class. > > > By > > > > > > keeping the uncertainty/complexity closer together it makes the larger > > > > system > > > > > > easier to reason about. > > > > > > > > > > > > > > > > I mean, EmptyInkDropRipple is pretty easy to understand. It barely has > any > > > > > lines. > > > > > > > > > > Would you feel better about adding a new virtual function called bool > > > > > HasAnimations? AnimateToState would early return if !HasAnimations. That > > > would > > > > > accomplish the goal here without giving future developers undue amounts > of > > > > > flexibility. > > > > > > > > I would have to see this more concretely. The major issue here is that > > > > AnimationEnded is not going to be called and thus the |ink_drop_ripple_| > is > > > not > > > > going to be destroyed. > > > > > > updated. > > > > > > I don't think it's really a problem if an empty ink drop ripple is never > > > destroyed, but it actually will be destroyed in > > DestroyHiddenTargetedAnimations > > > because the target state is always HIDDEN. > > > > Right, the layer won't leak memory but it will exist until the > > InkDropImpl::AnimateToState() or InkDropImpl::SnapToAcitvated() is called the > > next time. In which case the existing EmptyInkDrop will be destroyed but a > new > > one will be created right away. Thus any InkDrop enabled surface, that has > had > > an animation will have an instance of the EmptyInkDrop. At this point it > would > > be a simple change. Instead of returning early in > > InkDropRipple::AnimateToState() and SnapToActivated() just allow the creation > of > > the |animation_observer| and the SetActive() called on it and prevent all the > > other code from being called. > > I don't understand the motivation to do that. We're releasing some memory a > little earlier by allocating more memory? That is the current scenario. I am requesting that the memory is released much sooner, i.e. when the button is no longer being interacted with. https://codereview.chromium.org/2250783002/diff/160001/ui/views/animation/ink... File ui/views/animation/ink_drop_impl_unittest.cc (right): https://codereview.chromium.org/2250783002/diff/160001/ui/views/animation/ink... ui/views/animation/ink_drop_impl_unittest.cc:341: EXPECT_EQ(1, ink_drop_host_.num_ink_drop_layers()); Shouldn't this be 0? https://codereview.chromium.org/2250783002/diff/160001/ui/views/animation/ink... ui/views/animation/ink_drop_impl_unittest.cc:367: EXPECT_EQ(1, ink_drop_host_.num_ink_drop_layers()); Shouldn't this be 0?
https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_ripple.h (right): https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_ripple.h:60: virtual void AnimateToState(InkDropState ink_drop_state); On 2016/09/09 16:05:47, bruthig wrote: > On 2016/09/08 19:17:31, Evan Stade wrote: > > On 2016/09/08 15:56:04, bruthig wrote: > > > On 2016/09/07 23:46:19, Evan Stade wrote: > > > > On 2016/09/01 15:53:57, bruthig wrote: > > > > > On 2016/09/01 01:19:15, Evan Stade wrote: > > > > > > On 2016/08/31 22:22:29, bruthig wrote: > > > > > > > On 2016/08/26 23:36:15, Evan Stade wrote: > > > > > > > > On 2016/08/25 17:45:29, bruthig wrote: > > > > > > > > > This was intended to be a non-virtual function that would use > the > > > > > template > > > > > > > > > pattern to allow descendants to add behaviour. The intention > was > > > that > > > > > > this > > > > > > > > base > > > > > > > > > class would manage all the InkDropState transitions, notifying > > > > > observers, > > > > > > > etc > > > > > > > > > and the descendants would add the visuals. So there may be some > > > > visual > > > > > > > based > > > > > > > > > logic baked into this class that doesn't make sense for the > > > non-visual > > > > > > > > > EmptyInkDropRipple. We should probably extract that visual > logic > > > from > > > > > > this > > > > > > > > > class instead so that you don't need to make this function > > virtual. > > > > > > > > > > > > > > > > > > This would also apply to the SnapToActivate() function which > > already > > > > > > appears > > > > > > > > to > > > > > > > > > be virtual but it shouldn't be. It's only working correctly > since > > > all > > > > > > > > > descending classes call it when the overridden SnapToActivated() > > is > > > > > > called. > > > > > > > > > > > > > > > > > > WDYT? > > > > > > > > > > > > > > > > Generally speaking, extracting logic like this reduces > > encapsulation, > > > > adds > > > > > > to > > > > > > > > complexity, reduces readability (because what you have to read is > > > spread > > > > > > > across > > > > > > > > more files). This solution seems more elegant to me. I'm not sure > > what > > > > > > > practical > > > > > > > > harm it's causing? > > > > > > > > > > > > > > The InkDropRipple class defines a contract that guarantees that the > > > > > > > InkDropRippleObserver is notified for all state changes. IMO your > > > change > > > > > > makes > > > > > > > it much more difficult to understand and reason about the > InkDropImpl > > > > class > > > > > > > because sometimes it will be notified and sometimes it won't. Also, > > by > > > > > making > > > > > > > it virtual you are offering future developers more choice as to > which > > > > > function > > > > > > > to override, i.e. AnimateToState() or AnimateStateChange(). > Contrary > > to > > > > > your > > > > > > > point in another comment where you are in favor of limiting choice. > > > > > > > > > > > > It's not really a choice, because AnimateStateChange is pure virtual > so > > > you > > > > > must > > > > > > override it. > > > > > > > > > > > > > > > > > > > > I've seen this same pattern in the EventHandler/View/Button > hierarchy > > > > where > > > > > > the > > > > > > > general OnEvent() handler as well as the more specific event > handlers > > > like > > > > > > > OnGestureEvent() are virtual. The root EventHandler::OnEvent() > > > dispatches > > > > > to > > > > > > > these more specific event handlers and this makes it very difficult > to > > > > > reason > > > > > > > about when a more specific event handler will be called in a > concrete > > > View > > > > > > > because you have to look up and down the hierarchy to make sure all > > > > > > OnEvent()'s > > > > > > > call the parent's OnEvent(). > > > > > > > > > > > > > > I don't understand what you mean when you say this reduces > > > encapsulation. > > > > > Can > > > > > > > you elaborate? > > > > > > > > > > > > > > The InkDropRipple class encapsulates state transitions and notifying > > > > > > observers. > > > > > > > Descendant classes encapsulate the visual (or non-visual) > animations. > > I > > > > > feel > > > > > > > like these two levels of abstraction are a quite good and by > > extracting > > > > some > > > > > > > common visual logic doesn't expose it to clients of the > InkDropRipple > > in > > > > > > anyway. > > > > > > > > > > > > I'm invoking encapsulation as the principle of all related logic being > > > close > > > > > > together as good. > > > > > > > > > > > > > > > > > > > > The practical harm is that developers will need to understand all of > > the > > > > > > > InkDropRipple flavors when trying to understand the InkDropImpl > class. > > > > > By > > > > > > > keeping the uncertainty/complexity closer together it makes the > larger > > > > > system > > > > > > > easier to reason about. > > > > > > > > > > > > > > > > > > > I mean, EmptyInkDropRipple is pretty easy to understand. It barely has > > any > > > > > > lines. > > > > > > > > > > > > Would you feel better about adding a new virtual function called bool > > > > > > HasAnimations? AnimateToState would early return if !HasAnimations. > That > > > > would > > > > > > accomplish the goal here without giving future developers undue > amounts > > of > > > > > > flexibility. > > > > > > > > > > I would have to see this more concretely. The major issue here is that > > > > > AnimationEnded is not going to be called and thus the |ink_drop_ripple_| > > is > > > > not > > > > > going to be destroyed. > > > > > > > > updated. > > > > > > > > I don't think it's really a problem if an empty ink drop ripple is never > > > > destroyed, but it actually will be destroyed in > > > DestroyHiddenTargetedAnimations > > > > because the target state is always HIDDEN. > > > > > > Right, the layer won't leak memory but it will exist until the > > > InkDropImpl::AnimateToState() or InkDropImpl::SnapToAcitvated() is called > the > > > next time. In which case the existing EmptyInkDrop will be destroyed but a > > new > > > one will be created right away. Thus any InkDrop enabled surface, that has > > had > > > an animation will have an instance of the EmptyInkDrop. At this point it > > would > > > be a simple change. Instead of returning early in > > > InkDropRipple::AnimateToState() and SnapToActivated() just allow the > creation > > of > > > the |animation_observer| and the SetActive() called on it and prevent all > the > > > other code from being called. > > > > I don't understand the motivation to do that. We're releasing some memory a > > little earlier by allocating more memory? > > That is the current scenario. I am requesting that the memory is released much > sooner, i.e. when the button is no longer being interacted with. The memory footprint is negligible and it's not a leak in the sense that it's limited to one per button. When I said "by allocating more memory" I was referring to the observer you'd need to create. https://codereview.chromium.org/2250783002/diff/160001/ui/views/animation/ink... File ui/views/animation/ink_drop_impl_unittest.cc (right): https://codereview.chromium.org/2250783002/diff/160001/ui/views/animation/ink... ui/views/animation/ink_drop_impl_unittest.cc:341: EXPECT_EQ(1, ink_drop_host_.num_ink_drop_layers()); On 2016/09/09 16:05:47, bruthig wrote: > Shouldn't this be 0? why? we're animating the ripple in https://codereview.chromium.org/2250783002/diff/160001/ui/views/animation/ink... ui/views/animation/ink_drop_impl_unittest.cc:367: EXPECT_EQ(1, ink_drop_host_.num_ink_drop_layers()); On 2016/09/09 16:05:47, bruthig wrote: > Shouldn't this be 0? after snapping to activated?
https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_ripple.h (right): https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_ripple.h:60: virtual void AnimateToState(InkDropState ink_drop_state); On 2016/09/09 18:55:48, Evan Stade wrote: > On 2016/09/09 16:05:47, bruthig wrote: > > On 2016/09/08 19:17:31, Evan Stade wrote: > > > On 2016/09/08 15:56:04, bruthig wrote: > > > > On 2016/09/07 23:46:19, Evan Stade wrote: > > > > > On 2016/09/01 15:53:57, bruthig wrote: > > > > > > On 2016/09/01 01:19:15, Evan Stade wrote: > > > > > > > On 2016/08/31 22:22:29, bruthig wrote: > > > > > > > > On 2016/08/26 23:36:15, Evan Stade wrote: > > > > > > > > > On 2016/08/25 17:45:29, bruthig wrote: > > > > > > > > > > This was intended to be a non-virtual function that would use > > the > > > > > > template > > > > > > > > > > pattern to allow descendants to add behaviour. The intention > > was > > > > that > > > > > > > this > > > > > > > > > base > > > > > > > > > > class would manage all the InkDropState transitions, notifying > > > > > > observers, > > > > > > > > etc > > > > > > > > > > and the descendants would add the visuals. So there may be > some > > > > > visual > > > > > > > > based > > > > > > > > > > logic baked into this class that doesn't make sense for the > > > > non-visual > > > > > > > > > > EmptyInkDropRipple. We should probably extract that visual > > logic > > > > from > > > > > > > this > > > > > > > > > > class instead so that you don't need to make this function > > > virtual. > > > > > > > > > > > > > > > > > > > > This would also apply to the SnapToActivate() function which > > > already > > > > > > > appears > > > > > > > > > to > > > > > > > > > > be virtual but it shouldn't be. It's only working correctly > > since > > > > all > > > > > > > > > > descending classes call it when the overridden > SnapToActivated() > > > is > > > > > > > called. > > > > > > > > > > > > > > > > > > > > WDYT? > > > > > > > > > > > > > > > > > > Generally speaking, extracting logic like this reduces > > > encapsulation, > > > > > adds > > > > > > > to > > > > > > > > > complexity, reduces readability (because what you have to read > is > > > > spread > > > > > > > > across > > > > > > > > > more files). This solution seems more elegant to me. I'm not > sure > > > what > > > > > > > > practical > > > > > > > > > harm it's causing? > > > > > > > > > > > > > > > > The InkDropRipple class defines a contract that guarantees that > the > > > > > > > > InkDropRippleObserver is notified for all state changes. IMO your > > > > change > > > > > > > makes > > > > > > > > it much more difficult to understand and reason about the > > InkDropImpl > > > > > class > > > > > > > > because sometimes it will be notified and sometimes it won't. > Also, > > > by > > > > > > making > > > > > > > > it virtual you are offering future developers more choice as to > > which > > > > > > function > > > > > > > > to override, i.e. AnimateToState() or AnimateStateChange(). > > Contrary > > > to > > > > > > your > > > > > > > > point in another comment where you are in favor of limiting > choice. > > > > > > > > > > > > > > It's not really a choice, because AnimateStateChange is pure virtual > > so > > > > you > > > > > > must > > > > > > > override it. > > > > > > > > > > > > > > > > > > > > > > > I've seen this same pattern in the EventHandler/View/Button > > hierarchy > > > > > where > > > > > > > the > > > > > > > > general OnEvent() handler as well as the more specific event > > handlers > > > > like > > > > > > > > OnGestureEvent() are virtual. The root EventHandler::OnEvent() > > > > dispatches > > > > > > to > > > > > > > > these more specific event handlers and this makes it very > difficult > > to > > > > > > reason > > > > > > > > about when a more specific event handler will be called in a > > concrete > > > > View > > > > > > > > because you have to look up and down the hierarchy to make sure > all > > > > > > > OnEvent()'s > > > > > > > > call the parent's OnEvent(). > > > > > > > > > > > > > > > > I don't understand what you mean when you say this reduces > > > > encapsulation. > > > > > > Can > > > > > > > > you elaborate? > > > > > > > > > > > > > > > > The InkDropRipple class encapsulates state transitions and > notifying > > > > > > > observers. > > > > > > > > Descendant classes encapsulate the visual (or non-visual) > > animations. > > > I > > > > > > feel > > > > > > > > like these two levels of abstraction are a quite good and by > > > extracting > > > > > some > > > > > > > > common visual logic doesn't expose it to clients of the > > InkDropRipple > > > in > > > > > > > anyway. > > > > > > > > > > > > > > I'm invoking encapsulation as the principle of all related logic > being > > > > close > > > > > > > together as good. > > > > > > > > > > > > > > > > > > > > > > > The practical harm is that developers will need to understand all > of > > > the > > > > > > > > InkDropRipple flavors when trying to understand the InkDropImpl > > class. > > > > > > > By > > > > > > > > keeping the uncertainty/complexity closer together it makes the > > larger > > > > > > system > > > > > > > > easier to reason about. > > > > > > > > > > > > > > > > > > > > > > I mean, EmptyInkDropRipple is pretty easy to understand. It barely > has > > > any > > > > > > > lines. > > > > > > > > > > > > > > Would you feel better about adding a new virtual function called > bool > > > > > > > HasAnimations? AnimateToState would early return if !HasAnimations. > > That > > > > > would > > > > > > > accomplish the goal here without giving future developers undue > > amounts > > > of > > > > > > > flexibility. > > > > > > > > > > > > I would have to see this more concretely. The major issue here is > that > > > > > > AnimationEnded is not going to be called and thus the > |ink_drop_ripple_| > > > is > > > > > not > > > > > > going to be destroyed. > > > > > > > > > > updated. > > > > > > > > > > I don't think it's really a problem if an empty ink drop ripple is never > > > > > destroyed, but it actually will be destroyed in > > > > DestroyHiddenTargetedAnimations > > > > > because the target state is always HIDDEN. > > > > > > > > Right, the layer won't leak memory but it will exist until the > > > > InkDropImpl::AnimateToState() or InkDropImpl::SnapToAcitvated() is called > > the > > > > next time. In which case the existing EmptyInkDrop will be destroyed but > a > > > new > > > > one will be created right away. Thus any InkDrop enabled surface, that > has > > > had > > > > an animation will have an instance of the EmptyInkDrop. At this point it > > > would > > > > be a simple change. Instead of returning early in > > > > InkDropRipple::AnimateToState() and SnapToActivated() just allow the > > creation > > > of > > > > the |animation_observer| and the SetActive() called on it and prevent all > > the > > > > other code from being called. > > > > > > I don't understand the motivation to do that. We're releasing some memory a > > > little earlier by allocating more memory? > > > > That is the current scenario. I am requesting that the memory is released > much > > sooner, i.e. when the button is no longer being interacted with. > > The memory footprint is negligible and it's not a leak in the sense that it's > limited to one per button. When I said "by allocating more memory" I was > referring to the observer you'd need to create. How about this, in the quick return you could invoke AnimationStartedCallback() and AnimationEndedCallback() directly? https://codereview.chromium.org/2250783002/diff/160001/ui/views/animation/ink... File ui/views/animation/ink_drop_impl_unittest.cc (right): https://codereview.chromium.org/2250783002/diff/160001/ui/views/animation/ink... ui/views/animation/ink_drop_impl_unittest.cc:341: EXPECT_EQ(1, ink_drop_host_.num_ink_drop_layers()); On 2016/09/09 18:55:48, Evan Stade wrote: > On 2016/09/09 16:05:47, bruthig wrote: > > Shouldn't this be 0? > > why? we're animating the ripple in I guess I was thinking of the IsVisible() clause but that's probably overkill, withdrawn. https://codereview.chromium.org/2250783002/diff/160001/ui/views/animation/ink... ui/views/animation/ink_drop_impl_unittest.cc:367: EXPECT_EQ(1, ink_drop_host_.num_ink_drop_layers()); On 2016/09/09 18:55:48, Evan Stade wrote: > On 2016/09/09 16:05:47, bruthig wrote: > > Shouldn't this be 0? > > after snapping to activated? Nevermind, I thought it was similar to above.
On 2016/09/09 19:30:05, bruthig wrote: > https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... > File ui/views/animation/ink_drop_ripple.h (right): > > https://codereview.chromium.org/2250783002/diff/40001/ui/views/animation/ink_... > ui/views/animation/ink_drop_ripple.h:60: virtual void > AnimateToState(InkDropState ink_drop_state); > On 2016/09/09 18:55:48, Evan Stade wrote: > > On 2016/09/09 16:05:47, bruthig wrote: > > > On 2016/09/08 19:17:31, Evan Stade wrote: > > > > On 2016/09/08 15:56:04, bruthig wrote: > > > > > On 2016/09/07 23:46:19, Evan Stade wrote: > > > > > > On 2016/09/01 15:53:57, bruthig wrote: > > > > > > > On 2016/09/01 01:19:15, Evan Stade wrote: > > > > > > > > On 2016/08/31 22:22:29, bruthig wrote: > > > > > > > > > On 2016/08/26 23:36:15, Evan Stade wrote: > > > > > > > > > > On 2016/08/25 17:45:29, bruthig wrote: > > > > > > > > > > > This was intended to be a non-virtual function that would > use > > > the > > > > > > > template > > > > > > > > > > > pattern to allow descendants to add behaviour. The > intention > > > was > > > > > that > > > > > > > > this > > > > > > > > > > base > > > > > > > > > > > class would manage all the InkDropState transitions, > notifying > > > > > > > observers, > > > > > > > > > etc > > > > > > > > > > > and the descendants would add the visuals. So there may be > > some > > > > > > visual > > > > > > > > > based > > > > > > > > > > > logic baked into this class that doesn't make sense for the > > > > > non-visual > > > > > > > > > > > EmptyInkDropRipple. We should probably extract that visual > > > logic > > > > > from > > > > > > > > this > > > > > > > > > > > class instead so that you don't need to make this function > > > > virtual. > > > > > > > > > > > > > > > > > > > > > > This would also apply to the SnapToActivate() function which > > > > already > > > > > > > > appears > > > > > > > > > > to > > > > > > > > > > > be virtual but it shouldn't be. It's only working correctly > > > since > > > > > all > > > > > > > > > > > descending classes call it when the overridden > > SnapToActivated() > > > > is > > > > > > > > called. > > > > > > > > > > > > > > > > > > > > > > WDYT? > > > > > > > > > > > > > > > > > > > > Generally speaking, extracting logic like this reduces > > > > encapsulation, > > > > > > adds > > > > > > > > to > > > > > > > > > > complexity, reduces readability (because what you have to read > > is > > > > > spread > > > > > > > > > across > > > > > > > > > > more files). This solution seems more elegant to me. I'm not > > sure > > > > what > > > > > > > > > practical > > > > > > > > > > harm it's causing? > > > > > > > > > > > > > > > > > > The InkDropRipple class defines a contract that guarantees that > > the > > > > > > > > > InkDropRippleObserver is notified for all state changes. IMO > your > > > > > change > > > > > > > > makes > > > > > > > > > it much more difficult to understand and reason about the > > > InkDropImpl > > > > > > class > > > > > > > > > because sometimes it will be notified and sometimes it won't. > > Also, > > > > by > > > > > > > making > > > > > > > > > it virtual you are offering future developers more choice as to > > > which > > > > > > > function > > > > > > > > > to override, i.e. AnimateToState() or AnimateStateChange(). > > > Contrary > > > > to > > > > > > > your > > > > > > > > > point in another comment where you are in favor of limiting > > choice. > > > > > > > > > > > > > > > > It's not really a choice, because AnimateStateChange is pure > virtual > > > so > > > > > you > > > > > > > must > > > > > > > > override it. > > > > > > > > > > > > > > > > > > > > > > > > > > I've seen this same pattern in the EventHandler/View/Button > > > hierarchy > > > > > > where > > > > > > > > the > > > > > > > > > general OnEvent() handler as well as the more specific event > > > handlers > > > > > like > > > > > > > > > OnGestureEvent() are virtual. The root EventHandler::OnEvent() > > > > > dispatches > > > > > > > to > > > > > > > > > these more specific event handlers and this makes it very > > difficult > > > to > > > > > > > reason > > > > > > > > > about when a more specific event handler will be called in a > > > concrete > > > > > View > > > > > > > > > because you have to look up and down the hierarchy to make sure > > all > > > > > > > > OnEvent()'s > > > > > > > > > call the parent's OnEvent(). > > > > > > > > > > > > > > > > > > I don't understand what you mean when you say this reduces > > > > > encapsulation. > > > > > > > Can > > > > > > > > > you elaborate? > > > > > > > > > > > > > > > > > > The InkDropRipple class encapsulates state transitions and > > notifying > > > > > > > > observers. > > > > > > > > > Descendant classes encapsulate the visual (or non-visual) > > > animations. > > > > I > > > > > > > feel > > > > > > > > > like these two levels of abstraction are a quite good and by > > > > extracting > > > > > > some > > > > > > > > > common visual logic doesn't expose it to clients of the > > > InkDropRipple > > > > in > > > > > > > > anyway. > > > > > > > > > > > > > > > > I'm invoking encapsulation as the principle of all related logic > > being > > > > > close > > > > > > > > together as good. > > > > > > > > > > > > > > > > > > > > > > > > > > The practical harm is that developers will need to understand > all > > of > > > > the > > > > > > > > > InkDropRipple flavors when trying to understand the InkDropImpl > > > class. > > > > > > > > > By > > > > > > > > > keeping the uncertainty/complexity closer together it makes the > > > larger > > > > > > > system > > > > > > > > > easier to reason about. > > > > > > > > > > > > > > > > > > > > > > > > > I mean, EmptyInkDropRipple is pretty easy to understand. It barely > > has > > > > any > > > > > > > > lines. > > > > > > > > > > > > > > > > Would you feel better about adding a new virtual function called > > bool > > > > > > > > HasAnimations? AnimateToState would early return if > !HasAnimations. > > > That > > > > > > would > > > > > > > > accomplish the goal here without giving future developers undue > > > amounts > > > > of > > > > > > > > flexibility. > > > > > > > > > > > > > > I would have to see this more concretely. The major issue here is > > that > > > > > > > AnimationEnded is not going to be called and thus the > > |ink_drop_ripple_| > > > > is > > > > > > not > > > > > > > going to be destroyed. > > > > > > > > > > > > updated. > > > > > > > > > > > > I don't think it's really a problem if an empty ink drop ripple is > never > > > > > > destroyed, but it actually will be destroyed in > > > > > DestroyHiddenTargetedAnimations > > > > > > because the target state is always HIDDEN. > > > > > > > > > > Right, the layer won't leak memory but it will exist until the > > > > > InkDropImpl::AnimateToState() or InkDropImpl::SnapToAcitvated() is > called > > > the > > > > > next time. In which case the existing EmptyInkDrop will be destroyed > but > > a > > > > new > > > > > one will be created right away. Thus any InkDrop enabled surface, that > > has > > > > had > > > > > an animation will have an instance of the EmptyInkDrop. At this point > it > > > > would > > > > > be a simple change. Instead of returning early in > > > > > InkDropRipple::AnimateToState() and SnapToActivated() just allow the > > > creation > > > > of > > > > > the |animation_observer| and the SetActive() called on it and prevent > all > > > the > > > > > other code from being called. > > > > > > > > I don't understand the motivation to do that. We're releasing some memory > a > > > > little earlier by allocating more memory? > > > > > > That is the current scenario. I am requesting that the memory is released > > much > > > sooner, i.e. when the button is no longer being interacted with. > > > > The memory footprint is negligible and it's not a leak in the sense that it's > > limited to one per button. When I said "by allocating more memory" I was > > referring to the observer you'd need to create. > > How about this, in the quick return you could invoke AnimationStartedCallback() > and AnimationEndedCallback() directly? I just investigated what this would practically mean. AnimationStarted() is a no op. (According to my worldview it should probably not exist since it's only used for tests.) AnimationEndedCallback() will essentially no-op but it will indeed destroy the ripple. This means that for each click you get one (or several ink drops created). Wouldn't repeated allocation/dealloacation be at least as much of a performance drawback as a single allocation (of approximately 8 bytes)? I mean, would you object if I added 8 bytes to the memory footprint of views::LabelButton by adding two new member vars? Plus if we did this, then ink_drop_ripple_->AnimateToState() would in some cases synchronously delete ink_drop_ripple_. That seems dangerous.
> > > > How about this, in the quick return you could invoke > AnimationStartedCallback() > > and AnimationEndedCallback() directly? > > I just investigated what this would practically mean. AnimationStarted() is a no > op. (According to my worldview it should probably not exist since it's only used > for tests.) AnimationEndedCallback() will essentially no-op but it will indeed I disagree with this premise, and feel like AnimationStartedCallback() is adding enough value in tests. But let's leave that debate for another day. > destroy the ripple. This means that for each click you get one (or several ink > drops created). Wouldn't repeated allocation/dealloacation be at least as much > of a performance drawback as a single allocation (of approximately 8 bytes)? I > mean, would you object if I added 8 bytes to the memory footprint of > views::LabelButton by adding two new member vars? Most likely I wouldn't, unless there was an easy and obvious way to not add the 8 bytes that doesn't sacrifice maintainability. Having said that, the main motivation for my request is not memory concerns. I think we both agree that the memory costs between the two options here are negligible. My main motivation is about maintainability. I perceive the current patch as adding a "special case" scenario between the InkDropImpl class and the InkDropRipple. If I'm trying to understand the InkDropImpl class, I feel like it would be a lot easier to reason about the synchronous/asynchronous management of the |ink_drop_ripple_| if all flavors of the InkDropRipple behaved the same where reasonably possible. Whereas with the current patch set I now need to understand that some ripple flavours track the InkDropState properly, or some notify the AnimationEndedCallback() and some don't and thus some get deleted and some don't, etc. IMO adding these types of special cases makes code harder to maintain. For example: Now when I'm reasoning about InkDropImpl::DestroyHiddenTargetedAnimations() I need to know that |ink_drop_ripple_|->target_ink_drop_state() might equal InkDropState::HIDDEN even though the last animation request was not to HIDDEN. And now that I've thought about this I actually think with the current patch set it will be creating several EmptyInkDropRipples for each click because of this subtle point. > > Plus if we did this, then ink_drop_ripple_->AnimateToState() would in some cases > synchronously delete ink_drop_ripple_. That seems dangerous. The ripple could already be deleted synchronously when CallbackLayerAnimationObserver::SetActive() is called in InkDropRipple::AnimateToState(). And I believe I would have added tests to make sure this is safe. It would probably be a good idea to add a comment after "ink_drop_ripple_->AnimateToState(ink_drop_state);" call on line 98 that |ink_drop_ripple_| may be deleted. I really think the best/easiest, path forward here is to not short circuit return from InkDropRipple::AnimateToState() and allow the base InkDropRipple to track it's state and notify observers. It would make sense to skip over the SetVisible() and AnimateToState() calls based on the existence of a Layer though.
On 2016/09/14 22:03:11, bruthig wrote: > > > > > > How about this, in the quick return you could invoke > > AnimationStartedCallback() > > > and AnimationEndedCallback() directly? > > > > I just investigated what this would practically mean. AnimationStarted() is a > no > > op. (According to my worldview it should probably not exist since it's only > used > > for tests.) AnimationEndedCallback() will essentially no-op but it will indeed > > I disagree with this premise, and feel like AnimationStartedCallback() is adding > enough value in tests. But let's leave that debate for another day. > > > destroy the ripple. This means that for each click you get one (or several ink > > drops created). Wouldn't repeated allocation/dealloacation be at least as much > > of a performance drawback as a single allocation (of approximately 8 bytes)? I > > mean, would you object if I added 8 bytes to the memory footprint of > > views::LabelButton by adding two new member vars? > > Most likely I wouldn't, unless there was an easy and obvious way to not add the > 8 bytes that doesn't sacrifice maintainability. Having said that, the main > motivation for my request is not memory concerns. I think we both agree that > the memory costs between the two options here are negligible. My main > motivation is about maintainability. I perceive the current patch as adding a > "special case" scenario between the InkDropImpl class and the InkDropRipple. If > I'm trying to understand the InkDropImpl class, I feel like it would be a lot > easier to reason about the synchronous/asynchronous management of the > |ink_drop_ripple_| if all flavors of the InkDropRipple behaved the same where > reasonably possible. Whereas with the current patch set I now need to > understand that some ripple flavours track the InkDropState properly, or some > notify the AnimationEndedCallback() and some don't and thus some get deleted and > some don't, etc. IMO adding these types of special cases makes code harder to > maintain. For example: Now when I'm reasoning about > InkDropImpl::DestroyHiddenTargetedAnimations() I need to know that > |ink_drop_ripple_|->target_ink_drop_state() might equal InkDropState::HIDDEN > even though the last animation request was not to HIDDEN. And now that I've > thought about this I actually think with the current patch set it will be > creating several EmptyInkDropRipples for each click because of this subtle > point. > > > > > Plus if we did this, then ink_drop_ripple_->AnimateToState() would in some > cases > > synchronously delete ink_drop_ripple_. That seems dangerous. > > The ripple could already be deleted synchronously when > CallbackLayerAnimationObserver::SetActive() is called in > InkDropRipple::AnimateToState(). And I believe I would have added tests to make > sure this is safe. It would probably be a good idea to add a comment after > "ink_drop_ripple_->AnimateToState(ink_drop_state);" call on line 98 that > |ink_drop_ripple_| may be deleted. > > I really think the best/easiest, path forward here is to not short circuit > return from InkDropRipple::AnimateToState() and allow the base InkDropRipple to > track it's state and notify observers. It would make sense to skip over the > SetVisible() and AnimateToState() calls based on the existence of a Layer > though. Isn't skipping AnimateToState based on the existence of a layer pretty much exactly what this patch is doing?
On 2016/09/14 22:19:12, Evan Stade wrote: > On 2016/09/14 22:03:11, bruthig wrote: > > > > > > > > How about this, in the quick return you could invoke > > > AnimationStartedCallback() > > > > and AnimationEndedCallback() directly? > > > > > > I just investigated what this would practically mean. AnimationStarted() is > a > > no > > > op. (According to my worldview it should probably not exist since it's only > > used > > > for tests.) AnimationEndedCallback() will essentially no-op but it will > indeed > > > > I disagree with this premise, and feel like AnimationStartedCallback() is > adding > > enough value in tests. But let's leave that debate for another day. > > > > > destroy the ripple. This means that for each click you get one (or several > ink > > > drops created). Wouldn't repeated allocation/dealloacation be at least as > much > > > of a performance drawback as a single allocation (of approximately 8 bytes)? > I > > > mean, would you object if I added 8 bytes to the memory footprint of > > > views::LabelButton by adding two new member vars? > > > > Most likely I wouldn't, unless there was an easy and obvious way to not add > the > > 8 bytes that doesn't sacrifice maintainability. Having said that, the main > > motivation for my request is not memory concerns. I think we both agree that > > the memory costs between the two options here are negligible. My main > > motivation is about maintainability. I perceive the current patch as adding a > > "special case" scenario between the InkDropImpl class and the InkDropRipple. > If > > I'm trying to understand the InkDropImpl class, I feel like it would be a lot > > easier to reason about the synchronous/asynchronous management of the > > |ink_drop_ripple_| if all flavors of the InkDropRipple behaved the same where > > reasonably possible. Whereas with the current patch set I now need to > > understand that some ripple flavours track the InkDropState properly, or some > > notify the AnimationEndedCallback() and some don't and thus some get deleted > and > > some don't, etc. IMO adding these types of special cases makes code harder to > > maintain. For example: Now when I'm reasoning about > > InkDropImpl::DestroyHiddenTargetedAnimations() I need to know that > > |ink_drop_ripple_|->target_ink_drop_state() might equal InkDropState::HIDDEN > > even though the last animation request was not to HIDDEN. And now that I've > > thought about this I actually think with the current patch set it will be > > creating several EmptyInkDropRipples for each click because of this subtle > > point. > > > > > > > > Plus if we did this, then ink_drop_ripple_->AnimateToState() would in some > > cases > > > synchronously delete ink_drop_ripple_. That seems dangerous. > > > > The ripple could already be deleted synchronously when > > CallbackLayerAnimationObserver::SetActive() is called in > > InkDropRipple::AnimateToState(). And I believe I would have added tests to > make > > sure this is safe. It would probably be a good idea to add a comment after > > "ink_drop_ripple_->AnimateToState(ink_drop_state);" call on line 98 that > > |ink_drop_ripple_| may be deleted. > > > > I really think the best/easiest, path forward here is to not short circuit > > return from InkDropRipple::AnimateToState() and allow the base InkDropRipple > to > > track it's state and notify observers. It would make sense to skip over the > > SetVisible() and AnimateToState() calls based on the existence of a Layer > > though. > > Isn't skipping AnimateToState based on the existence of a layer pretty much > exactly what this patch is doing? Close, but not exactly. As I pointed out this patch is also skipping over |target_ink_drop_state_| tracking and observer notifications as well.
On 2016/09/14 22:44:45, bruthig wrote: > On 2016/09/14 22:19:12, Evan Stade wrote: > > On 2016/09/14 22:03:11, bruthig wrote: > > > > > > > > > > How about this, in the quick return you could invoke > > > > AnimationStartedCallback() > > > > > and AnimationEndedCallback() directly? > > > > > > > > I just investigated what this would practically mean. AnimationStarted() > is > > a > > > no > > > > op. (According to my worldview it should probably not exist since it's > only > > > used > > > > for tests.) AnimationEndedCallback() will essentially no-op but it will > > indeed > > > > > > I disagree with this premise, and feel like AnimationStartedCallback() is > > adding > > > enough value in tests. But let's leave that debate for another day. > > > > > > > destroy the ripple. This means that for each click you get one (or several > > ink > > > > drops created). Wouldn't repeated allocation/dealloacation be at least as > > much > > > > of a performance drawback as a single allocation (of approximately 8 > bytes)? > > I > > > > mean, would you object if I added 8 bytes to the memory footprint of > > > > views::LabelButton by adding two new member vars? > > > > > > Most likely I wouldn't, unless there was an easy and obvious way to not add > > the > > > 8 bytes that doesn't sacrifice maintainability. Having said that, the main > > > motivation for my request is not memory concerns. I think we both agree > that > > > the memory costs between the two options here are negligible. My main > > > motivation is about maintainability. I perceive the current patch as adding > a > > > "special case" scenario between the InkDropImpl class and the InkDropRipple. > > > If > > > I'm trying to understand the InkDropImpl class, I feel like it would be a > lot > > > easier to reason about the synchronous/asynchronous management of the > > > |ink_drop_ripple_| if all flavors of the InkDropRipple behaved the same > where > > > reasonably possible. Whereas with the current patch set I now need to > > > understand that some ripple flavours track the InkDropState properly, or > some > > > notify the AnimationEndedCallback() and some don't and thus some get deleted > > and > > > some don't, etc. IMO adding these types of special cases makes code harder > to > > > maintain. For example: Now when I'm reasoning about > > > InkDropImpl::DestroyHiddenTargetedAnimations() I need to know that > > > |ink_drop_ripple_|->target_ink_drop_state() might equal InkDropState::HIDDEN > > > even though the last animation request was not to HIDDEN. And now that I've > > > thought about this I actually think with the current patch set it will be > > > creating several EmptyInkDropRipples for each click because of this subtle > > > point. ... while also guaranteeing that the ripple is getting destroyed. Isn't that what you want? > > > > > > > > > > > Plus if we did this, then ink_drop_ripple_->AnimateToState() would in some > > > cases > > > > synchronously delete ink_drop_ripple_. That seems dangerous. > > > > > > The ripple could already be deleted synchronously when > > > CallbackLayerAnimationObserver::SetActive() is called in > > > InkDropRipple::AnimateToState(). And I believe I would have added tests to > > make > > > sure this is safe. It would probably be a good idea to add a comment after > > > "ink_drop_ripple_->AnimateToState(ink_drop_state);" call on line 98 that > > > |ink_drop_ripple_| may be deleted. And the next time someone adds AnimateToState somewhere they're going to realize this, write a test, and add a comment? I don't think anyone reliably reads comments and relying on comments to avoid crashes in my mind qualifies as perilous. > > > > > > I really think the best/easiest, path forward here is to not short circuit > > > return from InkDropRipple::AnimateToState() and allow the base InkDropRipple > > to > > > track it's state and notify observers. It would make sense to skip over the > > > SetVisible() and AnimateToState() calls based on the existence of a Layer > > > though. > > > > Isn't skipping AnimateToState based on the existence of a layer pretty much > > exactly what this patch is doing? > > Close, but not exactly. As I pointed out this patch is also skipping over > |target_ink_drop_state_| tracking and observer notifications as well. Then I guess I don't understand what you mean by "skip over AnimateToState()". I thought you meant to avoid calling it. Wouldn't that similarly mean the target ink drop state isn't updated? I'm sort of at a loss about how to proceed with this CL. It doesn't look like we're coming to an agreement any time soon, but this is functionality that we need to have. Do you have any ideas about how to make progress? Do you have time to create the patch that you envision?
On 2016/09/14 23:36:36, Evan Stade wrote: > On 2016/09/14 22:44:45, bruthig wrote: > > On 2016/09/14 22:19:12, Evan Stade wrote: > > > On 2016/09/14 22:03:11, bruthig wrote: > > > > > > > > > > > > How about this, in the quick return you could invoke > > > > > AnimationStartedCallback() > > > > > > and AnimationEndedCallback() directly? > > > > > > > > > > I just investigated what this would practically mean. AnimationStarted() > > is > > > a > > > > no > > > > > op. (According to my worldview it should probably not exist since it's > > only > > > > used > > > > > for tests.) AnimationEndedCallback() will essentially no-op but it will > > > indeed > > > > > > > > I disagree with this premise, and feel like AnimationStartedCallback() is > > > adding > > > > enough value in tests. But let's leave that debate for another day. > > > > > > > > > destroy the ripple. This means that for each click you get one (or > several > > > ink > > > > > drops created). Wouldn't repeated allocation/dealloacation be at least > as > > > much > > > > > of a performance drawback as a single allocation (of approximately 8 > > bytes)? > > > I > > > > > mean, would you object if I added 8 bytes to the memory footprint of > > > > > views::LabelButton by adding two new member vars? > > > > > > > > Most likely I wouldn't, unless there was an easy and obvious way to not > add > > > the > > > > 8 bytes that doesn't sacrifice maintainability. Having said that, the > main > > > > motivation for my request is not memory concerns. I think we both agree > > that > > > > the memory costs between the two options here are negligible. My main > > > > motivation is about maintainability. I perceive the current patch as > adding > > a > > > > "special case" scenario between the InkDropImpl class and the > InkDropRipple. > > > > > If > > > > I'm trying to understand the InkDropImpl class, I feel like it would be a > > lot > > > > easier to reason about the synchronous/asynchronous management of the > > > > |ink_drop_ripple_| if all flavors of the InkDropRipple behaved the same > > where > > > > reasonably possible. Whereas with the current patch set I now need to > > > > understand that some ripple flavours track the InkDropState properly, or > > some > > > > notify the AnimationEndedCallback() and some don't and thus some get > deleted > > > and > > > > some don't, etc. IMO adding these types of special cases makes code > harder > > to > > > > maintain. For example: Now when I'm reasoning about > > > > InkDropImpl::DestroyHiddenTargetedAnimations() I need to know that > > > > |ink_drop_ripple_|->target_ink_drop_state() might equal > InkDropState::HIDDEN > > > > even though the last animation request was not to HIDDEN. And now that > I've > > > > thought about this I actually think with the current patch set it will be > > > > creating several EmptyInkDropRipples for each click because of this subtle > > > > point. > > ... while also guaranteeing that the ripple is getting destroyed. Isn't that > what you want? Well, I wanted it to be destroyed when the user was finished interacting with the View. i.e. when the ripple is animated to the HIDDEN state in the normal flow. > > > > > > > > > > > > > > > Plus if we did this, then ink_drop_ripple_->AnimateToState() would in > some > > > > cases > > > > > synchronously delete ink_drop_ripple_. That seems dangerous. > > > > > > > > The ripple could already be deleted synchronously when > > > > CallbackLayerAnimationObserver::SetActive() is called in > > > > InkDropRipple::AnimateToState(). And I believe I would have added tests > to > > > make > > > > sure this is safe. It would probably be a good idea to add a comment > after > > > > "ink_drop_ripple_->AnimateToState(ink_drop_state);" call on line 98 that > > > > |ink_drop_ripple_| may be deleted. > > And the next time someone adds AnimateToState somewhere they're going to realize > this, write a test, and add a comment? I don't think anyone reliably reads > comments and relying on comments to avoid crashes in my mind qualifies as > perilous. > > > > > > > > > I really think the best/easiest, path forward here is to not short circuit > > > > return from InkDropRipple::AnimateToState() and allow the base > InkDropRipple > > > to > > > > track it's state and notify observers. It would make sense to skip over > the > > > > SetVisible() and AnimateToState() calls based on the existence of a Layer > > > > though. > > > > > > Isn't skipping AnimateToState based on the existence of a layer pretty much > > > exactly what this patch is doing? > > > > Close, but not exactly. As I pointed out this patch is also skipping over > > |target_ink_drop_state_| tracking and observer notifications as well. > > Then I guess I don't understand what you mean by "skip over AnimateToState()". I > thought you meant to avoid calling it. Wouldn't that similarly mean the target > ink drop state isn't updated? Sorry my bad :( I meant "AnimateStateChange()" not "AnimateToState()". > > I'm sort of at a loss about how to proceed with this CL. It doesn't look like > we're coming to an agreement any time soon, but this is functionality that we > need to have. Do you have any ideas about how to make progress? Do you have time > to create the patch that you envision? I've posted the minor changes here. Patch set 1 is your original, patch set 2 is the tweaks. https://codereview.chromium.org/2340993002/ WDYT?
Description was changed from ========== Allow InkDropRipple to co-exist with highlight or not exist at all. 1. Allow InkDropRipple to co-exist with highlight by adding InkDropRipple::OverridesHighlight. I initially made this part of InkDropHost but then realized that it was more a property of the ripple itself, and it would be too easy to overlook when adding an ink drop to a new surface. 2. Allow InkDropRipple to not exist but still retain an InkDropHighlight. This is desired for a couple menu-like UI surfaces in secondary UI. This is effected with an EmptyInkDropRipple class that just no-ops while allowing the hover effect to remain. The immediate effect of this change is that flood fill ripples will paint on top of the hover highlight effect (e.g. on bookmark buttons). BUG=617961 ========== to ========== Allow InkDropRipple to co-exist with highlight. Allow InkDropRipple to co-exist with highlight by adding InkDropRipple::OverridesHighlight. I initially made this part of InkDropHost but then realized that it was more a property of the ripple itself, and it would be too easy to overlook when adding an ink drop to a new surface. The immediate effect of this change is that flood fill ripples will paint on top of the hover highlight effect (e.g. on bookmark buttons). BUG=617961 ==========
I've excised the controversial part of this change (EmptyInkDropRipple) and now all that remains is the ability for ripples and highlights to both show simultaneously. It's not clear to me any more whether we actually need support for invisible ink drop ripples.
The CQ bit was checked by estade@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...
nit: Please update the CL description, i.e. remove "or not exist at all" Otherwise, lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by estade@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
estade@chromium.org changed reviewers: + sky@chromium.org
+sky for ownership
LabelButton LGTM
The CQ bit was checked by estade@chromium.org
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.
Description was changed from ========== Allow InkDropRipple to co-exist with highlight. Allow InkDropRipple to co-exist with highlight by adding InkDropRipple::OverridesHighlight. I initially made this part of InkDropHost but then realized that it was more a property of the ripple itself, and it would be too easy to overlook when adding an ink drop to a new surface. The immediate effect of this change is that flood fill ripples will paint on top of the hover highlight effect (e.g. on bookmark buttons). BUG=617961 ========== to ========== Allow InkDropRipple to co-exist with highlight. Allow InkDropRipple to co-exist with highlight by adding InkDropRipple::OverridesHighlight. I initially made this part of InkDropHost but then realized that it was more a property of the ripple itself, and it would be too easy to overlook when adding an ink drop to a new surface. The immediate effect of this change is that flood fill ripples will paint on top of the hover highlight effect (e.g. on bookmark buttons). BUG=617961 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Allow InkDropRipple to co-exist with highlight. Allow InkDropRipple to co-exist with highlight by adding InkDropRipple::OverridesHighlight. I initially made this part of InkDropHost but then realized that it was more a property of the ripple itself, and it would be too easy to overlook when adding an ink drop to a new surface. The immediate effect of this change is that flood fill ripples will paint on top of the hover highlight effect (e.g. on bookmark buttons). BUG=617961 ========== to ========== Allow InkDropRipple to co-exist with highlight. Allow InkDropRipple to co-exist with highlight by adding InkDropRipple::OverridesHighlight. I initially made this part of InkDropHost but then realized that it was more a property of the ripple itself, and it would be too easy to overlook when adding an ink drop to a new surface. The immediate effect of this change is that flood fill ripples will paint on top of the hover highlight effect (e.g. on bookmark buttons). BUG=617961 Committed: https://crrev.com/d17dd4ab4c7802be7190cbc80e9c56a4ddb793c9 Cr-Commit-Position: refs/heads/master@{#419848} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/d17dd4ab4c7802be7190cbc80e9c56a4ddb793c9 Cr-Commit-Position: refs/heads/master@{#419848} |
