Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1280953003:
Enhance the material design ripple API so the ripple's state can be controlled by it's owning View. (Closed)
DescriptionEnhance the material design ripple API so the ripple's state can be controlled by it's owning View.
This CL makes it possible for the different InkDropAnimationController
consumers to control the animation state as needed.
Previously the logic to control the material design ripple was separated
from the Views that it will be applied to. After trying to apply the
ripple to the ToolbarButton's it became clear that the mapping from each
View's state to the ripple's animation state is quite different for each
different View. e.g. The "Back" and "Forward" toolbar buttons show the
menu on a long press whereas the "Refresh"/"Stop" and "Home" buttons do not.
Follow up review: https://codereview.chromium.org/1286693004/
TEST=Tested clicking, touching, and long pressing all the affected
ToolbarButton's with and without material design enabled. Verified
reasonable visuals and no crashes.
BUG=517903
Committed: https://crrev.com/57421779a58c260999ee2a9008bdfc7bfb442ddc
Cr-Commit-Position: refs/heads/master@{#344488}
Patch Set 1 #Patch Set 2 : Fixed typo. #
Total comments: 36
Patch Set 3 : Addressed some comments from patch set 1. #
Total comments: 26
Patch Set 4 : Merge with trunk. (Similarity=10) #Patch Set 5 : Similarity=20 #Patch Set 6 : Similiarty=default (aka 50) #Patch Set 7 : Addressed tdanderson@'s comments from patch set 3. #Patch Set 8 : Minor changes after self review. #Patch Set 9 : Added CHECK to ET_GESTURE_TAP handling and a TODO comment. #
Total comments: 21
Patch Set 10 : Addressed concerns from patch set 9. #
Total comments: 17
Patch Set 11 : Addressed pkasting@'s comments from patch set 10. #Patch Set 12 : Addressed pkasting@'s comments from patch set 11 #
Total comments: 18
Patch Set 13 : Address sadrul@'s concerns from patch set 12. #Patch Set 14 : Moved the LAYER_NOT_DRAWN layer initilization to the ToolbarButton ctor. #Patch Set 15 : Replaced raw ui::Layer* with ui::LayerOwners. #
Total comments: 13
Patch Set 16 : Addressed comments from patch set 15. #
Total comments: 6
Patch Set 17 : Addressed comments from patch set 16. #Patch Set 18 : Fixed minor typo. #Messages
Total messages: 35 (4 generated)
bruthig@chromium.org changed reviewers: + jonross@chromium.org, tdanderson@chromium.org
tdanderson@, jonross@: Can you two take a first pass look before I send it off to OWNERS?
https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/back_button.cc (right): https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/back_button.cc:37: gfx::Rect(margin_leading_, 0, width() - margin_leading_, height())); Is this something that could be gained by applying the view's insets to the ink drop? Otherwise we'll be having similar code littered throughout. https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:39: SetPaintToLayer(true); Will this now always be the responsibility of the View? https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:129: ShowDropDownMenu(ui::GetMenuSourceTypeForEvent(event)); State change? https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:182: case ui::ET_GESTURE_TAP_CANCEL: We won't receive these if TAP_DOWN is not marked as handled, and the finger leaves the View's region. https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:337: ink_drop_animation_controller_->AnimateToState(views::InkDropState::HIDDEN); Can the transition to Hidden be triggered twice for the same gesture? https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.h (right): https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:77: views::InkDropAnimationController* ink_drop_animation_controller() { I think this should be owned in InkDropHost. I commented there for debate. https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... File ui/views/animation/ink_drop_animation_controller.cc (right): https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_animation_controller.cc:222: switch (state) { Do we need to restrict state transitions? I think we should track the current state, and not transition to it if we are already in it. https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... File ui/views/animation/ink_drop_animation_controller.h (right): https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_animation_controller.h:38: SLOW_ACTION, Do we plan for SLOW_ACTION to be used for right-clicks as well? Some Views have both primary and secondary actions. Where the secondary can be triggered by events of different speeds. https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... File ui/views/animation/ink_drop_delegate.h (right): https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_delegate.h:20: class InkDropDelegate : public ui::LayerDelegate { Can you change the similarity setting on your review to try to pick this up as a split https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_delegate.h:41: protected: Curious as to why we have these protected accessors. https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host.h (right): https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_host.h:19: class VIEWS_EXPORT InkDropHost { Currently this does not own the InkDropAnimationController, however I think that it should. I'd only be concerned about a class inheriting this from multiple ancestors. Though we should be able to address that.
https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/back_button.cc (right): https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/back_button.cc:37: gfx::Rect(margin_leading_, 0, width() - margin_leading_, height())); On 2015/08/07 17:14:31, jonross wrote: > Is this something that could be gained by applying the view's insets to the ink > drop? > > Otherwise we'll be having similar code littered throughout. Discussed offline, the inset's track the space between the border and the contents and not the view's edge and the border. https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:39: SetPaintToLayer(true); On 2015/08/07 17:14:31, jonross wrote: > Will this now always be the responsibility of the View? Yes, the reason being that the InkDropHost could potentially add the ink drop layer wherever it wants now and it might not always be the case that the View has to do this. https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:129: ShowDropDownMenu(ui::GetMenuSourceTypeForEvent(event)); On 2015/08/07 17:14:31, jonross wrote: > State change? I've omitted setting the ACTIVATED state for the time being because there is no animation currently to show this and thus I can't be sure it is implemented correctly. Once the ripple properly handles that state it will definitely need to be added in. Most likely it will be in ShowDropDownMenu though. For now the ACTION_PENDING circle will be visible until the menu closes. https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:182: case ui::ET_GESTURE_TAP_CANCEL: On 2015/08/07 17:14:31, jonross wrote: > We won't receive these if TAP_DOWN is not marked as handled, and the finger > leaves the View's region. Done. https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:337: ink_drop_animation_controller_->AnimateToState(views::InkDropState::HIDDEN); On 2015/08/07 17:14:31, jonross wrote: > Can the transition to Hidden be triggered twice for the same gesture? My thinking is that the InkDropAnimationController will be able to transition from any state, or mid-state, to HIDDEN, and it shouldn't matter if it is called more than once. And yes this currently works. These are the details that will get ironed out in the subsequent work that actually implements the revamped ripple and its states. https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... File ui/views/animation/ink_drop_animation_controller.cc (right): https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_animation_controller.cc:222: switch (state) { On 2015/08/07 17:14:31, jonross wrote: > Do we need to restrict state transitions? > > I think we should track the current state, and not transition to it if we are > already in it. All the work for state transitions and the visuals for each state will come in a subsequent review. But yes you are right, we will most likely need to only allow certain state transitions. And when we do I plan to provide a mechanism for clients to determine what the current state is and potentially events for when the animation reaches the different states. My current plan, in a subsequent review of course, is to add an InkDropAnimation class that will manages the lifetime of the many ripple layers, animating them, the state, etc. i.e. A lot of the work that the InkDropAnimationController is currently doing. I envision the InkDropAnimationController becoming very thin and delegating many of the current methods off to this new InkDropAnimationController. The InkDropAnimationController's main responsibility will then become acquiring and releasing the InkDropAnimation from an InkDropAnimationPool object and attaching/removing it from the InkDropHost. https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... File ui/views/animation/ink_drop_animation_controller.h (right): https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_animation_controller.h:38: SLOW_ACTION, On 2015/08/07 17:14:31, jonross wrote: > Do we plan for SLOW_ACTION to be used for right-clicks as well? > > Some Views have both primary and secondary actions. Where the secondary can be > triggered by events of different speeds. These states are placeholders and will need to be flushed out in a subsequent review. As well as being renamed most likely. To answer the question though, it may or may not be used for right-clicks. The beauty of providing the API like so is that it allows each View to decide what user actions map to which animation actions. https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... File ui/views/animation/ink_drop_delegate.h (right): https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_delegate.h:20: class InkDropDelegate : public ui::LayerDelegate { On 2015/08/07 17:14:31, jonross wrote: > Can you change the similarity setting on your review to try to pick this up as a > split I will have to try playing around with this... https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_delegate.h:41: protected: On 2015/08/07 17:14:31, jonross wrote: > Curious as to why we have these protected accessors. Whoops, this was left over from experimental work. Fixed. https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host.h (right): https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_host.h:19: class VIEWS_EXPORT InkDropHost { On 2015/08/07 17:14:31, jonross wrote: > Currently this does not own the InkDropAnimationController, however I think that > it should. > > I'd only be concerned about a class inheriting this from multiple ancestors. > Though we should be able to address that. Let's discuss offline.
https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:39: SetPaintToLayer(true); On 2015/08/07 22:00:45, bruthig wrote: > On 2015/08/07 17:14:31, jonross wrote: > > Will this now always be the responsibility of the View? > > Yes, the reason being that the InkDropHost could potentially add the ink drop > layer wherever it wants now and it might not always be the case that the View > has to do this. It may be a good idea to include this in a comment somewhere. Needing to call SetPaintToLayer(true) could be easy to miss for another UI surface that wants the ripple. https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:182: case ui::ET_GESTURE_TAP_CANCEL: On 2015/08/07 22:00:45, bruthig wrote: > On 2015/08/07 17:14:31, jonross wrote: > > We won't receive these if TAP_DOWN is not marked as handled, and the finger > > leaves the View's region. > > Done. Did you verify this? It seems to me that the implementation of CustomButton::OnGestureEvent() should take care of everything for us already. And as a general guideline I feel like events should not be modified as a result of animation state changes (only as a result of already-existing functional state changes). https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host.h (right): https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_host.h:19: class VIEWS_EXPORT InkDropHost { On 2015/08/07 22:00:45, bruthig wrote: > On 2015/08/07 17:14:31, jonross wrote: > > Currently this does not own the InkDropAnimationController, however I think > that > > it should. > > > > I'd only be concerned about a class inheriting this from multiple ancestors. > > Though we should be able to address that. > > Let's discuss offline. If IDC owned IDAC, it would seem to become overly messy for the actual view (ToolbarButton, BackButton, etc) to call out to IDAC when needed. +1 for offline chat. https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/back_button.cc (right): https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/back_button.cc:36: ink_drop_animation_controller()->SetBounds( possible nit: perhaps SetBounds() on IDAC should be SetInkDropBounds() (similarly for bounds())? Would we also need to update the bounds on IDAC whenever the bounds of the hosting view changes? Or is it sufficient to only do this when a layout happens? https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:113: if (ui::MaterialDesignController::IsModeMaterial()) nit: use {} since body is more than one line. https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:145: if (ui::MaterialDesignController::IsModeMaterial()) What do you think of containing all of the IsModeMaterial() checks inside of IDAC (by way of early returns / no-ops) instead of requiring each host to include the check in multiple places? https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:170: ink_drop_animation_controller_->AnimateToState( [I think this is a repeat of one of Jon's questions...] Are there ever any times where the call site of AnimateToState() would actually need to know about the current/previous state its controller is/was in, or can that always be derived from the members of the host? One example that comes to mind is how to handle the animation for pressing down on the back button but then moving your finger off in the cases where the history menu is shown vs. not shown. https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:250: layer()->Remove(ink_drop_layer); How will the implementations of AddInkDropLayer() and RemoveInkDropLayer() differ between InkDropHosts? In other words, do these really need to be pure virtual? https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.h (right): https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:81: // Performs a layout of the ink drop. i.e. Set's the ink drop's size and/or nit: Set's -> Sets https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:83: virtual void LayoutInkDrop(); Would it be more appropriate for this to be pure virtual in InkDropHost? Also, it may be good to include more details about how this should be implemented and to include a note to say that any class which owns an InkDropAnimationController should call this method from within Layout() as you have done in this CL. https://codereview.chromium.org/1280953003/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_animation_controller.cc (right): https://codereview.chromium.org/1280953003/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_animation_controller.cc:221: void InkDropAnimationController::AnimateToState(InkDropState state) { It seems we're placing all of our trust in the InkDropHost to kick off the right animations at the right times. Apart from the animations looking strange, is IDAC always going to be well-behaved no matter what sequence of AnimateToState() calls are made by the IDH? https://codereview.chromium.org/1280953003/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_delegate.h (right): https://codereview.chromium.org/1280953003/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_delegate.h:20: class InkDropDelegate : public ui::LayerDelegate { Would it ever be necessary to introduce subclasses of InkDropDelegate? https://codereview.chromium.org/1280953003/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_delegate.h:44: ui::Layer* layer() { return layer_; } What's the point of having the private accessors color() and layer()? https://codereview.chromium.org/1280953003/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host.h (right): https://codereview.chromium.org/1280953003/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_host.h:18: // from a hosts layer tree. nit: hosts -> host's
https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:129: ShowDropDownMenu(ui::GetMenuSourceTypeForEvent(event)); On 2015/08/07 22:00:45, bruthig wrote: > On 2015/08/07 17:14:31, jonross wrote: > > State change? > > I've omitted setting the ACTIVATED state for the time being because there is no > animation currently to show this and thus I can't be sure it is implemented > correctly. Once the ripple properly handles that state it will definitely need > to be added in. Most likely it will be in ShowDropDownMenu though. > > For now the ACTION_PENDING circle will be visible until the menu closes. Acknowledged. https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:182: case ui::ET_GESTURE_TAP_CANCEL: On 2015/08/10 16:31:01, tdanderson wrote: > On 2015/08/07 22:00:45, bruthig wrote: > > On 2015/08/07 17:14:31, jonross wrote: > > > We won't receive these if TAP_DOWN is not marked as handled, and the finger > > > leaves the View's region. > > > > Done. > > Did you verify this? It seems to me that the implementation of > CustomButton::OnGestureEvent() should take care of everything for us already. > And as a general guideline I feel like events should not be modified as a result > of animation state changes (only as a result of already-existing functional > state changes). Well I would be concerned about each usage of the animation requires that ancestral classes mark gestures appropriately so that we receive the events. https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:337: ink_drop_animation_controller_->AnimateToState(views::InkDropState::HIDDEN); On 2015/08/07 22:00:45, bruthig wrote: > On 2015/08/07 17:14:31, jonross wrote: > > Can the transition to Hidden be triggered twice for the same gesture? > > My thinking is that the InkDropAnimationController will be able to transition > from any state, or mid-state, to HIDDEN, and it shouldn't matter if it is called > more than once. And yes this currently works. > > These are the details that will get ironed out in the subsequent work that > actually implements the revamped ripple and its states. Acknowledged. https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... File ui/views/animation/ink_drop_animation_controller.cc (right): https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_animation_controller.cc:222: switch (state) { On 2015/08/07 22:00:45, bruthig wrote: > On 2015/08/07 17:14:31, jonross wrote: > > Do we need to restrict state transitions? > > > > I think we should track the current state, and not transition to it if we are > > already in it. > > All the work for state transitions and the visuals for each state will come in a > subsequent review. But yes you are right, we will most likely need to only > allow certain state transitions. And when we do I plan to provide a mechanism > for clients to determine what the current state is and potentially events for > when the animation reaches the different states. > > My current plan, in a subsequent review of course, is to add an InkDropAnimation > class that will manages the lifetime of the many ripple layers, animating them, > the state, etc. i.e. A lot of the work that the InkDropAnimationController is > currently doing. I envision the InkDropAnimationController becoming very thin > and delegating many of the current methods off to this new > InkDropAnimationController. The InkDropAnimationController's main > responsibility will then become acquiring and releasing the InkDropAnimation > from an InkDropAnimationPool object and attaching/removing it from the > InkDropHost. A good plan. Do we have this noted in the bug? Or should we add a todo? https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... File ui/views/animation/ink_drop_animation_controller.h (right): https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_animation_controller.h:38: SLOW_ACTION, On 2015/08/07 22:00:45, bruthig wrote: > On 2015/08/07 17:14:31, jonross wrote: > > Do we plan for SLOW_ACTION to be used for right-clicks as well? > > > > Some Views have both primary and secondary actions. Where the secondary can be > > triggered by events of different speeds. > > These states are placeholders and will need to be flushed out in a subsequent > review. As well as being renamed most likely. > > To answer the question though, it may or may not be used for right-clicks. The > beauty of providing the API like so is that it allows each View to decide what > user actions map to which animation actions. Acknowledged.
Can you guys PTAL? https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:39: SetPaintToLayer(true); On 2015/08/10 16:31:01, tdanderson wrote: > On 2015/08/07 22:00:45, bruthig wrote: > > On 2015/08/07 17:14:31, jonross wrote: > > > Will this now always be the responsibility of the View? > > > > Yes, the reason being that the InkDropHost could potentially add the ink drop > > layer wherever it wants now and it might not always be the case that the View > > has to do this. > > It may be a good idea to include this in a comment somewhere. Needing to call > SetPaintToLayer(true) could be easy to miss for another UI surface that wants > the ripple. Done. See documentation for the InkDropHost. https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:182: case ui::ET_GESTURE_TAP_CANCEL: On 2015/08/10 20:32:02, jonross wrote: > On 2015/08/10 16:31:01, tdanderson wrote: > > On 2015/08/07 22:00:45, bruthig wrote: > > > On 2015/08/07 17:14:31, jonross wrote: > > > > We won't receive these if TAP_DOWN is not marked as handled, and the > finger > > > > leaves the View's region. > > > > > > Done. > > > > Did you verify this? It seems to me that the implementation of > > CustomButton::OnGestureEvent() should take care of everything for us already. > > And as a general guideline I feel like events should not be modified as a > result > > of animation state changes (only as a result of already-existing functional > > state changes). > > Well I would be concerned about each usage of the animation requires that > ancestral classes mark gestures appropriately so that we receive the events. I could call LabelButton::OnGestureEvent(...) before the switch statement and add CHECK(event->handled()) in the ET_GESTURE_TAP_DOWN case. WDYT? https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... File ui/views/animation/ink_drop_animation_controller.cc (right): https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_animation_controller.cc:222: switch (state) { On 2015/08/10 20:32:02, jonross wrote: > On 2015/08/07 22:00:45, bruthig wrote: > > On 2015/08/07 17:14:31, jonross wrote: > > > Do we need to restrict state transitions? > > > > > > I think we should track the current state, and not transition to it if we > are > > > already in it. > > > > All the work for state transitions and the visuals for each state will come in > a > > subsequent review. But yes you are right, we will most likely need to only > > allow certain state transitions. And when we do I plan to provide a mechanism > > for clients to determine what the current state is and potentially events for > > when the animation reaches the different states. > > > > My current plan, in a subsequent review of course, is to add an > InkDropAnimation > > class that will manages the lifetime of the many ripple layers, animating > them, > > the state, etc. i.e. A lot of the work that the InkDropAnimationController is > > currently doing. I envision the InkDropAnimationController becoming very thin > > and delegating many of the current methods off to this new > > InkDropAnimationController. The InkDropAnimationController's main > > responsibility will then become acquiring and releasing the InkDropAnimation > > from an InkDropAnimationPool object and attaching/removing it from the > > InkDropHost. > > A good plan. Do we have this noted in the bug? Or should we add a todo? Hmmm I'm not sure either of those two places are a good spot for speculative design. https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/back_button.cc (right): https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/back_button.cc:36: ink_drop_animation_controller()->SetBounds( On 2015/08/10 16:31:01, tdanderson wrote: > possible nit: perhaps SetBounds() on IDAC should be SetInkDropBounds() > (similarly for bounds())? > > Would we also need to update the bounds on IDAC whenever the bounds of the > hosting view changes? Or is it sufficient to only do this when a layout happens? Renaming done. I believe updating the ink drop bounds on a layout only will be sufficient. It appears that View::BoundsChanged(...) will already trigger a Layout if the size has changed. https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:113: if (ui::MaterialDesignController::IsModeMaterial()) On 2015/08/10 16:31:01, tdanderson wrote: > nit: use {} since body is more than one line. Done. https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:145: if (ui::MaterialDesignController::IsModeMaterial()) On 2015/08/10 16:31:01, tdanderson wrote: > What do you think of containing all of the IsModeMaterial() checks inside of > IDAC (by way of early returns / no-ops) instead of requiring each host to > include the check in multiple places? I've implemented this with a factory, take a look see. I'm thinking a InkDropAnimationController test double might be useful here. https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:170: ink_drop_animation_controller_->AnimateToState( On 2015/08/10 16:31:01, tdanderson wrote: > [I think this is a repeat of one of Jon's questions...] > > Are there ever any times where the call site of AnimateToState() would actually > need to know about the current/previous state its controller is/was in, or can > that always be derived from the members of the host? One example that comes to > mind is how to handle the animation for pressing down on the back button but > then moving your finger off in the cases where the history menu is shown vs. not > shown. Yes I can imagine that it would be very useful for an IDAC owner to know the IDAC's animation state. At this point I wasn't sure if the IDAC would be the best place to track this state but I was planning to add something in the later patch set that better defines all the ink drops states. I'm thinking it would best be tracked in the unborn 'InkDropLayer' class... https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:250: layer()->Remove(ink_drop_layer); On 2015/08/10 16:31:01, tdanderson wrote: > How will the implementations of AddInkDropLayer() and RemoveInkDropLayer() > differ between InkDropHosts? In other words, do these really need to be pure > virtual? The most important thing that I can see changing between different implementations is the z-layer ordering but I can imagine some host's doing some more advanced things like adding the layer to a child/parent view's layer or something like that. We could definitely entertain the idea of having a default implementation in some views::View higher up in the hierarchy though. https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.h (right): https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:81: // Performs a layout of the ink drop. i.e. Set's the ink drop's size and/or On 2015/08/10 16:31:01, tdanderson wrote: > nit: Set's -> Sets Done. https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:83: virtual void LayoutInkDrop(); On 2015/08/10 16:31:01, tdanderson wrote: > Would it be more appropriate for this to be pure virtual in InkDropHost? Also, > it may be good to include more details about how this should be implemented and > to include a note to say that any class which owns an InkDropAnimationController > should call this method from within Layout() as you have done in this CL. What is your reasoning to move it to InkDropHost? I'm hesitant to move LayoutInkDrop to InkDropHost for a couple of: 1) The InkDropHost does not necessarily have to be the same class that owns the IDAC. e.g. A container view might be more capable and responsible for defining the InkDrop's bounds and it's child view would be the host. e.g. A container view could own multiple IDAC's because it has to sync some animations between them for some reason, and its child Views would actually be the InkDropHosts. 2) A class that inherits InkDropHost can easily host more than one ink drop. It would be odd if we had to implement InkDropHost::SetInkDropBounds if it was hosting multiple ink drops. It will be much more flexible if these two concepts are not coupled together. I agree more details and documentation are needed and I was planning to add them to the InkDropAnimationController once all the details have been implemented/added. https://codereview.chromium.org/1280953003/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_animation_controller.cc (right): https://codereview.chromium.org/1280953003/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_animation_controller.cc:221: void InkDropAnimationController::AnimateToState(InkDropState state) { On 2015/08/10 16:31:01, tdanderson wrote: > It seems we're placing all of our trust in the InkDropHost to kick off the right > animations at the right times. Apart from the animations looking strange, is > IDAC always going to be well-behaved no matter what sequence of AnimateToState() > calls are made by the IDH? I agree, the IDH has a lot of responsibility but it's the one that knows the View's state. What else could trigger the animations at the right times? Once the states are better defined with the new specs I'm thinking that some state transitions will become restricted and not allowed by the IDAC. Having said that I am optimistic that we can make it so any valid state transitions will not look strange if one is preempted before it's normal duration. https://codereview.chromium.org/1280953003/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_delegate.h (right): https://codereview.chromium.org/1280953003/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_delegate.h:20: class InkDropDelegate : public ui::LayerDelegate { On 2015/08/10 16:31:01, tdanderson wrote: > Would it ever be necessary to introduce subclasses of InkDropDelegate? Perhaps why do you ask? If the states are the same for the toolbar ripple and the bookmarks bar ripple then that might be a prime candidate for such a case. Keep in mind that this is going to get a lot of re-work when implementing the new ink drop specs. https://codereview.chromium.org/1280953003/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_delegate.h:44: ui::Layer* layer() { return layer_; } On 2015/08/10 16:31:01, tdanderson wrote: > What's the point of having the private accessors color() and layer()? Whoops, this was left over from experimental work where I actually had a subclass of InkDropDelegate. Removed. https://codereview.chromium.org/1280953003/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host.h (right): https://codereview.chromium.org/1280953003/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_host.h:18: // from a hosts layer tree. On 2015/08/10 16:31:01, tdanderson wrote: > nit: hosts -> host's Done.
https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:182: case ui::ET_GESTURE_TAP_CANCEL: On 2015/08/10 21:57:15, bruthig wrote: > On 2015/08/10 20:32:02, jonross wrote: > > On 2015/08/10 16:31:01, tdanderson wrote: > > > On 2015/08/07 22:00:45, bruthig wrote: > > > > On 2015/08/07 17:14:31, jonross wrote: > > > > > We won't receive these if TAP_DOWN is not marked as handled, and the > > finger > > > > > leaves the View's region. > > > > > > > > Done. > > > > > > Did you verify this? It seems to me that the implementation of > > > CustomButton::OnGestureEvent() should take care of everything for us > already. > > > And as a general guideline I feel like events should not be modified as a > > result > > > of animation state changes (only as a result of already-existing functional > > > state changes). > > > > Well I would be concerned about each usage of the animation requires that > > ancestral classes mark gestures appropriately so that we receive the events. > > I could call LabelButton::OnGestureEvent(...) before the switch statement and > add CHECK(event->handled()) in the ET_GESTURE_TAP_DOWN case. WDYT? I would prefer that, but I don't think it's necessary. IIRC if a class isn't already marking TAP_DOWN as handled then it is not guaranteed to receive the remainder of the gesture sequence and will have functional problems independent of the ripple animation. https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... File ui/views/animation/ink_drop_animation_controller.cc (right): https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_animation_controller.cc:222: switch (state) { On 2015/08/10 21:57:15, bruthig wrote: > On 2015/08/10 20:32:02, jonross wrote: > > On 2015/08/07 22:00:45, bruthig wrote: > > > On 2015/08/07 17:14:31, jonross wrote: > > > > Do we need to restrict state transitions? > > > > > > > > I think we should track the current state, and not transition to it if we > > are > > > > already in it. > > > > > > All the work for state transitions and the visuals for each state will come > in > > a > > > subsequent review. But yes you are right, we will most likely need to only > > > allow certain state transitions. And when we do I plan to provide a > mechanism > > > for clients to determine what the current state is and potentially events > for > > > when the animation reaches the different states. > > > > > > My current plan, in a subsequent review of course, is to add an > > InkDropAnimation > > > class that will manages the lifetime of the many ripple layers, animating > > them, > > > the state, etc. i.e. A lot of the work that the InkDropAnimationController > is > > > currently doing. I envision the InkDropAnimationController becoming very > thin > > > and delegating many of the current methods off to this new > > > InkDropAnimationController. The InkDropAnimationController's main > > > responsibility will then become acquiring and releasing the InkDropAnimation > > > from an InkDropAnimationPool object and attaching/removing it from the > > > InkDropHost. > > > > A good plan. Do we have this noted in the bug? Or should we add a todo? > > Hmmm I'm not sure either of those two places are a good spot for speculative > design. If you're fairly sure of what you're going to be doing in a follow-up CL, I think a brief TODO might be appropriate - no need to go into too much detail though. https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/back_button.cc (right): https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/back_button.cc:36: ink_drop_animation_controller()->SetBounds( On 2015/08/10 21:57:15, bruthig wrote: > On 2015/08/10 16:31:01, tdanderson wrote: > > possible nit: perhaps SetBounds() on IDAC should be SetInkDropBounds() > > (similarly for bounds())? > > > > Would we also need to update the bounds on IDAC whenever the bounds of the > > hosting view changes? Or is it sufficient to only do this when a layout > happens? > > Renaming done. I believe updating the ink drop bounds on a layout only will be > sufficient. It appears that View::BoundsChanged(...) will already trigger a > Layout if the size has changed. Acknowledged. https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:250: layer()->Remove(ink_drop_layer); On 2015/08/10 21:57:15, bruthig wrote: > On 2015/08/10 16:31:01, tdanderson wrote: > > How will the implementations of AddInkDropLayer() and RemoveInkDropLayer() > > differ between InkDropHosts? In other words, do these really need to be pure > > virtual? > > The most important thing that I can see changing between different > implementations is the z-layer ordering but I can imagine some host's doing some > more advanced things like adding the layer to a child/parent view's layer or > something like that. > We could definitely entertain the idea of having a default implementation in > some views::View higher up in the hierarchy though. OK, sounds good to leave as is and have it evolve as needed. https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.h (right): https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:83: virtual void LayoutInkDrop(); On 2015/08/10 21:57:15, bruthig wrote: > On 2015/08/10 16:31:01, tdanderson wrote: > > Would it be more appropriate for this to be pure virtual in InkDropHost? Also, > > it may be good to include more details about how this should be implemented > and > > to include a note to say that any class which owns an > InkDropAnimationController > > should call this method from within Layout() as you have done in this CL. > > What is your reasoning to move it to InkDropHost? My understanding was that every class which implements IDH will have to specify logic to lay out its ink drop (and will also have to make a call to LayoutInkDrop() from its Layout()). So for that reason it felt reasonable to move that into the interface. > I'm hesitant to move LayoutInkDrop to InkDropHost for a couple of: > > 1) The InkDropHost does not necessarily have to be the same class that owns the > IDAC. e.g. A container view might be more capable and responsible for defining > the InkDrop's bounds and it's child view would be the host. e.g. A container > view could own multiple IDAC's because it has to sync some animations between > them for some reason, and its child Views would actually be the InkDropHosts. I see. My assumption was that you were intending for a class to own an IDAC if and only if it implemented IDH. > > 2) A class that inherits InkDropHost can easily host more than one ink drop. It > would be odd if we had to implement InkDropHost::SetInkDropBounds if it was > hosting multiple ink drops. It will be much more flexible if these two concepts > are not coupled together. OK, fair enough. > > I agree more details and documentation are needed and I was planning to add them > to the InkDropAnimationController once all the details have been > implemented/added. https://codereview.chromium.org/1280953003/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_delegate.h (right): https://codereview.chromium.org/1280953003/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_delegate.h:20: class InkDropDelegate : public ui::LayerDelegate { On 2015/08/10 21:57:15, bruthig wrote: > On 2015/08/10 16:31:01, tdanderson wrote: > > Would it ever be necessary to introduce subclasses of InkDropDelegate? > > Perhaps why do you ask? If the states are the same for the toolbar ripple and > the bookmarks bar ripple then that might be a prime candidate for such a case. > Keep in mind that this is going to get a lot of re-work when implementing the > new ink drop specs. I don't see any problems, I was just curious. Looks good as is.
Small update. https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:182: case ui::ET_GESTURE_TAP_CANCEL: On 2015/08/11 12:57:36, tdanderson wrote: > On 2015/08/10 21:57:15, bruthig wrote: > > On 2015/08/10 20:32:02, jonross wrote: > > > On 2015/08/10 16:31:01, tdanderson wrote: > > > > On 2015/08/07 22:00:45, bruthig wrote: > > > > > On 2015/08/07 17:14:31, jonross wrote: > > > > > > We won't receive these if TAP_DOWN is not marked as handled, and the > > > finger > > > > > > leaves the View's region. > > > > > > > > > > Done. > > > > > > > > Did you verify this? It seems to me that the implementation of > > > > CustomButton::OnGestureEvent() should take care of everything for us > > already. > > > > And as a general guideline I feel like events should not be modified as a > > > result > > > > of animation state changes (only as a result of already-existing > functional > > > > state changes). > > > > > > Well I would be concerned about each usage of the animation requires that > > > ancestral classes mark gestures appropriately so that we receive the events. > > > > I could call LabelButton::OnGestureEvent(...) before the switch statement and > > add CHECK(event->handled()) in the ET_GESTURE_TAP_DOWN case. WDYT? > > I would prefer that, but I don't think it's necessary. IIRC if a class isn't > already marking TAP_DOWN as handled then it is not guaranteed to receive the > remainder of the gesture sequence and will have functional problems independent > of the ripple animation. Done. https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... File ui/views/animation/ink_drop_animation_controller.cc (right): https://codereview.chromium.org/1280953003/diff/20001/ui/views/animation/ink_... ui/views/animation/ink_drop_animation_controller.cc:222: switch (state) { On 2015/08/11 12:57:36, tdanderson wrote: > On 2015/08/10 21:57:15, bruthig wrote: > > On 2015/08/10 20:32:02, jonross wrote: > > > On 2015/08/07 22:00:45, bruthig wrote: > > > > On 2015/08/07 17:14:31, jonross wrote: > > > > > Do we need to restrict state transitions? > > > > > > > > > > I think we should track the current state, and not transition to it if > we > > > are > > > > > already in it. > > > > > > > > All the work for state transitions and the visuals for each state will > come > > in > > > a > > > > subsequent review. But yes you are right, we will most likely need to > only > > > > allow certain state transitions. And when we do I plan to provide a > > mechanism > > > > for clients to determine what the current state is and potentially events > > for > > > > when the animation reaches the different states. > > > > > > > > My current plan, in a subsequent review of course, is to add an > > > InkDropAnimation > > > > class that will manages the lifetime of the many ripple layers, animating > > > them, > > > > the state, etc. i.e. A lot of the work that the > InkDropAnimationController > > is > > > > currently doing. I envision the InkDropAnimationController becoming very > > thin > > > > and delegating many of the current methods off to this new > > > > InkDropAnimationController. The InkDropAnimationController's main > > > > responsibility will then become acquiring and releasing the > InkDropAnimation > > > > from an InkDropAnimationPool object and attaching/removing it from the > > > > InkDropHost. > > > > > > A good plan. Do we have this noted in the bug? Or should we add a todo? > > > > Hmmm I'm not sure either of those two places are a good spot for speculative > > design. > > If you're fairly sure of what you're going to be doing in a follow-up CL, I > think a brief TODO might be appropriate - no need to go into too much detail > though. Done.
Hey Ben, there are a few comments below, otherwise lgtm https://codereview.chromium.org/1280953003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:48: image()->SetFillsBoundsOpaquely(false); Do you still want to call lines 46-48 if the MD flag is turned off? https://codereview.chromium.org/1280953003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:171: CHECK(event->handled()) << "event should be marked handled so that " I'm still on the fence about whether this is needed / a good idea. I'm leaning toward how you had it before (animation before call to superclass) but without an explicit call to mark the event as handled. See what happens during ONWNERs review. https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... File ui/views/animation/ink_drop_animation_controller_factory.cc (right): https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_factory.cc:35: gfx::Rect ink_drop_bounds_; Do you actually need to have this member in the stub? https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... File ui/views/animation/ink_drop_animation_controller_factory.h (right): https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_factory.h:22: CreateInkDropAnimationController(InkDropHost* ink_drop_host); nit: I think this should be indented https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... File ui/views/animation/ink_drop_animation_controller_impl.cc (right): https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_impl.cc:217: // be called when the Ink Drop becomes hidden. Don't include this TODO in the review, but do look into whether this will cause issues / ask the OWNER for their thoughts. https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... File ui/views/animation/ink_drop_host.h (right): https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... ui/views/animation/ink_drop_host.h:20: // Note: Some View's do not own a Layer but, you can use nit: View's -> views and move comma after Layer
https://codereview.chromium.org/1280953003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:48: image()->SetFillsBoundsOpaquely(false); On 2015/08/11 19:07:40, tdanderson wrote: > Do you still want to call lines 46-48 if the MD flag is turned off? If the factory is abstracting MD, should 46-48 possibly be triggered by the controller? https://codereview.chromium.org/1280953003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:93: LayoutInkDrop(); Thoughts on having this changed to: virtual void InkDropHost::LayoutInkDrop() {} Then: InkDropAnimationController calling it? https://codereview.chromium.org/1280953003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:171: CHECK(event->handled()) << "event should be marked handled so that " On 2015/08/11 19:07:41, tdanderson wrote: > I'm still on the fence about whether this is needed / a good idea. I'm leaning > toward how you had it before (animation before call to superclass) but without > an explicit call to mark the event as handled. See what happens during ONWNERs > review. This is just ensuring that it is handled. A valid check IMO. Otherwise there is no guarantee that we'll see subsequent events. That could lead to partially completed animations on controls. https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... File ui/views/animation/ink_drop_animation_controller_factory.cc (right): https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_factory.cc:21: : public InkDropAnimationController { Why not just have InkDropAnimationController contain this stub's implementation? Instead of being purely virtual. https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... File ui/views/animation/ink_drop_animation_controller_factory.h (right): https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_factory.h:22: CreateInkDropAnimationController(InkDropHost* ink_drop_host); On 2015/08/11 19:07:41, tdanderson wrote: > nit: I think this should be indented I'd just recommend running: git cl format https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... File ui/views/animation/ink_drop_animation_controller_impl.cc (right): https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_impl.cc:295: new AppearAnimationObserver(long_press_layer_, false)); Long press no longer hides upon completion?
bruthig@chromium.org changed reviewers: + pkasting@chromium.org, sadrul@chromium.org
pkasting@, Please review changes in: - chrome/browser/* sadrul@, Please review changes in: - ui/views/* https://codereview.chromium.org/1280953003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:48: image()->SetFillsBoundsOpaquely(false); On 2015/08/11 19:07:40, tdanderson wrote: > Do you still want to call lines 46-48 if the MD flag is turned off? They don't need to be called but they didn't seem to have any visual effect so I left them in originally. I've wrapped them in an 'if' now. https://codereview.chromium.org/1280953003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:48: image()->SetFillsBoundsOpaquely(false); On 2015/08/11 20:34:46, jonross wrote: > On 2015/08/11 19:07:40, tdanderson wrote: > > Do you still want to call lines 46-48 if the MD flag is turned off? > > If the factory is abstracting MD, should 46-48 possibly be triggered by the > controller? I don't think the factory can in all general cases set up the InkDropHost correctly. First it wouldn't make any sense for the InkDropHost/InkDropAnimationControllerFactory to know anything about the image() in the ToolbarButton. This is not something that applies to all Views/InkDropHosts. Similarly the SetFillsBoundsOpaquely() might not apply to all InkDropHosts either. https://codereview.chromium.org/1280953003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:93: LayoutInkDrop(); On 2015/08/11 20:34:46, jonross wrote: > Thoughts on having this changed to: > > virtual void InkDropHost::LayoutInkDrop() {} > > Then: > > InkDropAnimationController calling it? Please see thread about this here: https://codereview.chromium.org/1280953003/diff/40001/chrome/browser/ui/views... Additionally, I would like to point out that the InkDropAnimationController wouldn't be able to call LayoutInkDrop() at the appropriate times because it is not observing layout or bounds changes anyway. https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... File ui/views/animation/ink_drop_animation_controller_factory.cc (right): https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_factory.cc:21: : public InkDropAnimationController { On 2015/08/11 20:34:46, jonross wrote: > Why not just have InkDropAnimationController contain this stub's implementation? > Instead of being purely virtual. Since InkDropAnimationController is purely virtual it is clear to a developer trying to use it that there is a choice to be made, instead of them blindly using the InkDropAnimationController and wondering why it's not working. Secondly it provides a test seam to inject a TestInkDropAnimationController that can be used for test with the simplification that it can be controlled synchronously from the tests and wouldn't depend on the actual animation framework for timing. https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_factory.cc:35: gfx::Rect ink_drop_bounds_; On 2015/08/11 19:07:41, tdanderson wrote: > Do you actually need to have this member in the stub? I put this in, in case any of the InkDropHosts or IDAC owners rely on the fact that the following code must be true: idac->SetInkDropBounds(bounds); CHECK_EQ(idac->GetInkDropBounds(), bounds); Essentially this is a contract declared by the pure virtual InkDropAnimationController class, not just it's InkDropAnimationControllerImpl derivative. https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... File ui/views/animation/ink_drop_animation_controller_factory.h (right): https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_factory.h:22: CreateInkDropAnimationController(InkDropHost* ink_drop_host); On 2015/08/11 20:34:46, jonross wrote: > On 2015/08/11 19:07:41, tdanderson wrote: > > nit: I think this should be indented > > I'd just recommend running: git cl format This is how 'git cl format' formatted it. https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... File ui/views/animation/ink_drop_animation_controller_impl.cc (right): https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_impl.cc:295: new AppearAnimationObserver(long_press_layer_, false)); On 2015/08/11 20:34:46, jonross wrote: > Long press no longer hides upon completion? That is correct, at least it won't be hidden implicitly. This change was required so the long press would still be shown when the menus are shown for the "Back"/"Forward" buttons. https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... File ui/views/animation/ink_drop_host.h (right): https://codereview.chromium.org/1280953003/diff/160001/ui/views/animation/ink... ui/views/animation/ink_drop_host.h:20: // Note: Some View's do not own a Layer but, you can use On 2015/08/11 19:07:41, tdanderson wrote: > nit: View's -> views and move comma after Layer Done. https://codereview.chromium.org/1280953003/diff/180001/ui/views/animation/ink... File ui/views/animation/ink_drop_animation_controller_impl.cc (right): https://codereview.chromium.org/1280953003/diff/180001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_impl.cc:215: // TODO(bruthig): Currently this virtual function is going to be called during sadrul@, what are your thoughts on this? PS I plan to remove this TODO doc before submitting.
https://codereview.chromium.org/1280953003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:48: image()->SetFillsBoundsOpaquely(false); On 2015/08/12 15:26:40, bruthig wrote: > On 2015/08/11 20:34:46, jonross wrote: > > On 2015/08/11 19:07:40, tdanderson wrote: > > > Do you still want to call lines 46-48 if the MD flag is turned off? > > > > If the factory is abstracting MD, should 46-48 possibly be triggered by the > > controller? > > I don't think the factory can in all general cases set up the InkDropHost > correctly. First it wouldn't make any sense for the > InkDropHost/InkDropAnimationControllerFactory to know anything about the image() > in the ToolbarButton. This is not something that applies to all > Views/InkDropHosts. Similarly the SetFillsBoundsOpaquely() might not apply to > all InkDropHosts either. True, but could the InkDropHost implement a method that the controller calls? Could help clearly define what is ink drop related, vs needed for general MD work.
c/b/ui LGMT https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:172: views::InkDropState::ACTION_PENDING); Nit: Since all these arms call AnimateToState(), this would be simpler if you can remove the CHECK (see next comment): ... views::InkDropState state = HIDDEN; switch (event->type()) { case ui::ET_GESTURE_TAP_DOWN: state = views::InkDropState::ACTION_PENDING; break; case ui::ET_GESTURE_LONG_PRESS: state = views::InkDropState::SLOW_ACTION; break; case ui::ET_GESTURE_TAP: state = views::InkDropState::QUICK_ACTION; break; case ui::ET_GESTURE_END: case ui::ET_GESTURE_TAP_CANCEL: break; // Use HIDDEN. default: return; } ink_drop_animation_controller_->AnimateToState(state); } https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:174: "subsequent gestures are received."; Are you trying to catch a particular bug with this CHECK? Normally we avoid CHECKs in the codebase unless there's a security issue or it's a short-term thing to track down some kind of bug. https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:335: ink_drop_animation_controller_->SetInkDropSize(gfx::Size(width(), height())); Nit: Just pass size() https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.h (right): https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.h:79: } Nit: Put this accessor below the next function so all the virtuals are together. https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.h:82: // bounds. Nit: The second part of this comment just restates the first (the meaning of "layout" is clear), and the first part just restates the function name, so I'd remove this comment entirely.
Er, LGTM even
pkasting@, I've addressed your comments, can you take a quick look at the event->SetHandled() change in toolbar_button.cc? sadrul@, can you PTAL at ui/views/*? https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:172: views::InkDropState::ACTION_PENDING); On 2015/08/12 20:04:31, Peter Kasting wrote: > Nit: Since all these arms call AnimateToState(), this would be simpler if you > can remove the CHECK (see next comment): > > ... > views::InkDropState state = HIDDEN; > switch (event->type()) { > case ui::ET_GESTURE_TAP_DOWN: > state = views::InkDropState::ACTION_PENDING; > break; > case ui::ET_GESTURE_LONG_PRESS: > state = views::InkDropState::SLOW_ACTION; > break; > case ui::ET_GESTURE_TAP: > state = views::InkDropState::QUICK_ACTION; > break; > case ui::ET_GESTURE_END: > case ui::ET_GESTURE_TAP_CANCEL: > break; // Use HIDDEN. > default: > return; > } > ink_drop_animation_controller_->AnimateToState(state); > } I don't think your suggestion will work here anyhow, i.e. the ink drop will be hidden on an ET_GESTURE_SCROLL_BEGIN which is not what we want in this case. Keep in mind this code is going to be updated as the actual animation states are flushed out in a subsequent review. https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:174: "subsequent gestures are received."; On 2015/08/12 20:04:32, Peter Kasting wrote: > Are you trying to catch a particular bug with this CHECK? Normally we avoid > CHECKs in the codebase unless there's a security issue or it's a short-term > thing to track down some kind of bug. Originally this was suppposed to detect breaking changes to parent classes (and probably should have been a DCHECK as opposed to a CHECK), however, tdanderson@, jonross@, and I discussed offline and decided to explicitly set the event as handled. This will ensure that the subsequent events in the pipeline are properly handled and not be dependent on the ancestor classes. https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:335: ink_drop_animation_controller_->SetInkDropSize(gfx::Size(width(), height())); On 2015/08/12 20:04:32, Peter Kasting wrote: > Nit: Just pass size() Done. https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.h (right): https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.h:79: } On 2015/08/12 20:04:32, Peter Kasting wrote: > Nit: Put this accessor below the next function so all the virtuals are together. Done, I moved it up to be with the other non-virtual method. https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.h:82: // bounds. On 2015/08/12 20:04:32, Peter Kasting wrote: > Nit: The second part of this comment just restates the first (the meaning of > "layout" is clear), and the first part just restates the function name, so I'd > remove this comment entirely. Done.
https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:172: views::InkDropState::ACTION_PENDING); On 2015/08/13 15:15:45, bruthig wrote: > On 2015/08/12 20:04:31, Peter Kasting wrote: > > Nit: Since all these arms call AnimateToState(), this would be simpler if you > > can remove the CHECK (see next comment): > > > > ... > > views::InkDropState state = HIDDEN; > > switch (event->type()) { > > case ui::ET_GESTURE_TAP_DOWN: > > state = views::InkDropState::ACTION_PENDING; > > break; > > case ui::ET_GESTURE_LONG_PRESS: > > state = views::InkDropState::SLOW_ACTION; > > break; > > case ui::ET_GESTURE_TAP: > > state = views::InkDropState::QUICK_ACTION; > > break; > > case ui::ET_GESTURE_END: > > case ui::ET_GESTURE_TAP_CANCEL: > > break; // Use HIDDEN. > > default: > > return; > > } > > ink_drop_animation_controller_->AnimateToState(state); > > } > > I don't think your suggestion will work here anyhow, i.e. the ink drop will be > hidden on an ET_GESTURE_SCROLL_BEGIN which is not what we want in this case. No it won't; see how the "default" case returns rather than allowing fallthrough to the animate call. > Keep in mind this code is going to be updated as the actual animation states are > flushed out in a subsequent review. Are you saying you'll be adding more functionality beyond the function call? Will it be happening after the function call? If those aren't both "yes" this suggestion will still work in the future. (But see comment below) https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:174: "subsequent gestures are received."; On 2015/08/13 15:15:45, bruthig wrote: > On 2015/08/12 20:04:32, Peter Kasting wrote: > > Are you trying to catch a particular bug with this CHECK? Normally we avoid > > CHECKs in the codebase unless there's a security issue or it's a short-term > > thing to track down some kind of bug. > > Originally this was suppposed to detect breaking changes to parent classes (and > probably should have been a DCHECK as opposed to a CHECK), however, tdanderson@, > jonross@, and I discussed offline and decided to explicitly set the event as > handled. This will ensure that the subsequent events in the pipeline are > properly handled and not be dependent on the ancestor classes. Why not SetHandled() on the other events too? In any case this deserves a comment about what it's doing, and especially one to explain why this case differs from the others if necessary. https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.h (right): https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.h:79: } On 2015/08/13 15:15:45, bruthig wrote: > On 2015/08/12 20:04:32, Peter Kasting wrote: > > Nit: Put this accessor below the next function so all the virtuals are > together. > > Done, I moved it up to be with the other non-virtual method. That method is actually virtual as well (see the "override" declaration on it). You should probably move to the bottom of the list here instead.
pkasting@, I've addressed your comments, review if you wish. sadrul@, can you PTAL at ui/views/*? https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:172: views::InkDropState::ACTION_PENDING); On 2015/08/13 18:34:08, Peter Kasting wrote: > On 2015/08/13 15:15:45, bruthig wrote: > > On 2015/08/12 20:04:31, Peter Kasting wrote: > > > Nit: Since all these arms call AnimateToState(), this would be simpler if > you > > > can remove the CHECK (see next comment): > > > > > > ... > > > views::InkDropState state = HIDDEN; > > > switch (event->type()) { > > > case ui::ET_GESTURE_TAP_DOWN: > > > state = views::InkDropState::ACTION_PENDING; > > > break; > > > case ui::ET_GESTURE_LONG_PRESS: > > > state = views::InkDropState::SLOW_ACTION; > > > break; > > > case ui::ET_GESTURE_TAP: > > > state = views::InkDropState::QUICK_ACTION; > > > break; > > > case ui::ET_GESTURE_END: > > > case ui::ET_GESTURE_TAP_CANCEL: > > > break; // Use HIDDEN. > > > default: > > > return; > > > } > > > ink_drop_animation_controller_->AnimateToState(state); > > > } > > > > I don't think your suggestion will work here anyhow, i.e. the ink drop will be > > hidden on an ET_GESTURE_SCROLL_BEGIN which is not what we want in this case. > > No it won't; see how the "default" case returns rather than allowing fallthrough > to the animate call. > > > Keep in mind this code is going to be updated as the actual animation states > are > > flushed out in a subsequent review. > > Are you saying you'll be adding more functionality beyond the function call? > Will it be happening after the function call? If those aren't both "yes" this > suggestion will still work in the future. (But see comment below) Whoops, I missed that. Fixed. https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:174: "subsequent gestures are received."; On 2015/08/13 18:34:08, Peter Kasting wrote: > On 2015/08/13 15:15:45, bruthig wrote: > > On 2015/08/12 20:04:32, Peter Kasting wrote: > > > Are you trying to catch a particular bug with this CHECK? Normally we avoid > > > CHECKs in the codebase unless there's a security issue or it's a short-term > > > thing to track down some kind of bug. > > > > Originally this was suppposed to detect breaking changes to parent classes > (and > > probably should have been a DCHECK as opposed to a CHECK), however, > tdanderson@, > > jonross@, and I discussed offline and decided to explicitly set the event as > > handled. This will ensure that the subsequent events in the pipeline are > > properly handled and not be dependent on the ancestor classes. > > Why not SetHandled() on the other events too? In any case this deserves a > comment about what it's doing, and especially one to explain why this case > differs from the others if necessary. Done. https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.h (right): https://codereview.chromium.org/1280953003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.h:79: } On 2015/08/13 18:34:08, Peter Kasting wrote: > On 2015/08/13 15:15:45, bruthig wrote: > > On 2015/08/12 20:04:32, Peter Kasting wrote: > > > Nit: Put this accessor below the next function so all the virtuals are > > together. > > > > Done, I moved it up to be with the other non-virtual method. > > That method is actually virtual as well (see the "override" declaration on it). > You should probably move to the bottom of the list here instead. Done.
LGTM
https://codereview.chromium.org/1280953003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:37: SetPaintToLayer(true); Is this only so you can add the ink-drop layers? Can you do that without forcing this View to have its own layer? https://codereview.chromium.org/1280953003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:55: } Maybe explicitly destroy the ink-drop controller here, since its dtor calls back on ToolbarButton. https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... File ui/views/animation/ink_drop_animation_controller.h (right): https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller.h:49: explicit InkDropAnimationController(InkDropHost* ink_drop_host) {} You don't need this constructor, do you? https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller.h:55: // Set the size of the ink drop. *Sets If the size changes, does the origin remain fixed (i.e. left/top aligned), or can that change (e.g. to keep things center-aligned)? https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... File ui/views/animation/ink_drop_animation_controller_impl.cc (right): https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_impl.cc:209: layer_tree_.root()->Add(long_press_layer_); This does not pass ownership of |ink_drop_layer_|, |long_press_layer_| layers, right? Are these leaking? https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_impl.cc:217: // be called when the Ink Drop becomes hidden. Update the comment (e.g. 'Change this to be called when Ink Drop becomes hidden.') https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_impl.cc:247: size.width(), size.height())); gfx::Rect(ink_drop_bounds_.origin(), size()) https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... File ui/views/animation/ink_drop_delegate.h (right): https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... ui/views/animation/ink_drop_delegate.h:33: bool should_render_circle() { return should_render_circle_; } const https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... File ui/views/animation/ink_drop_host.h (right): https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... ui/views/animation/ink_drop_host.h:21: // View::SetPaintToLayer(true) to force the View to have a Layer. Using View::SetPaintToLayer(true) should not be the [only] advice here. Adding the layer to an ancestor View's layer may also work.
https://codereview.chromium.org/1280953003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:37: SetPaintToLayer(true); On 2015/08/17 14:33:06, sadrul wrote: > Is this only so you can add the ink-drop layers? Can you do that without forcing > this View to have its own layer? Yes. I'm not sure how else to do it without creating its own layer. Any suggestions? I have, however, updated it to use a LAYER_NOT_DRAWN type instead of the default LAYER_TEXTURED type. https://codereview.chromium.org/1280953003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:55: } On 2015/08/17 14:33:06, sadrul wrote: > Maybe explicitly destroy the ink-drop controller here, since its dtor calls back > on ToolbarButton. Done. https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... File ui/views/animation/ink_drop_animation_controller.h (right): https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller.h:49: explicit InkDropAnimationController(InkDropHost* ink_drop_host) {} On 2015/08/17 14:33:06, sadrul wrote: > You don't need this constructor, do you? Removed. https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller.h:55: // Set the size of the ink drop. On 2015/08/17 14:33:06, sadrul wrote: > *Sets > > If the size changes, does the origin remain fixed (i.e. left/top aligned), or > can that change (e.g. to keep things center-aligned)? Yes, the origin will be fixed if the size changes. If the ink drop needs to be re-aligned then an IDAC client can use SetInkDropBounds(). For example see BackButton::LayoutInkDrop(). FTR InkDropAnimationController::SetInkDropBounds() is replaced with SetInkDropOrigin() in a follow up review found here: https://codereview.chromium.org/1298513003/ https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... File ui/views/animation/ink_drop_animation_controller_impl.cc (right): https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_impl.cc:209: layer_tree_.root()->Add(long_press_layer_); On 2015/08/17 14:33:06, sadrul wrote: > This does not pass ownership of |ink_drop_layer_|, |long_press_layer_| layers, > right? Are these leaking? The LayerTreeOwner will delete all the roots descendants as well so this shouldn't be leaking. But, this did make me realize that we might have an issue if an ink drop ripple is active when the screen rotates because the screen rotation animation is creating a copy of the full layer tree. I will have to think more about this... https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_impl.cc:217: // be called when the Ink Drop becomes hidden. On 2015/08/17 14:33:06, sadrul wrote: > Update the comment (e.g. 'Change this to be called when Ink Drop becomes > hidden.') Done. https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_impl.cc:247: size.width(), size.height())); On 2015/08/17 14:33:06, sadrul wrote: > gfx::Rect(ink_drop_bounds_.origin(), size()) Done. https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... File ui/views/animation/ink_drop_delegate.h (right): https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... ui/views/animation/ink_drop_delegate.h:33: bool should_render_circle() { return should_render_circle_; } On 2015/08/17 14:33:06, sadrul wrote: > const Done. https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... File ui/views/animation/ink_drop_host.h (right): https://codereview.chromium.org/1280953003/diff/220001/ui/views/animation/ink... ui/views/animation/ink_drop_host.h:21: // View::SetPaintToLayer(true) to force the View to have a Layer. On 2015/08/17 14:33:06, sadrul wrote: > Using View::SetPaintToLayer(true) should not be the [only] advice here. Adding > the layer to an ancestor View's layer may also work. Done.
I had to do some minor re-work. pkasting@, can you please have another look at: - chrome/browser/ui/views/toolbar/toolbar_button.cc sadrul@, can you take another look at: - ui/views/*
https://codereview.chromium.org/1280953003/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:57: // RemoveInkDropLayer(). Is that actually a problem? It seems like that call ought to work fine even when made during the normal destruction sequence. I would rather not do this explicitly unless we actually need to do so for correctness' sake.
https://codereview.chromium.org/1280953003/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:37: // TODO(bruthig): Only set the layer when the ink drop is active. Make sure there's a blocking crbug for this. https://codereview.chromium.org/1280953003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:38: SetLayer(new ui::Layer(ui::LAYER_NOT_DRAWN)); You should use SetPaintToLayer() instead. https://codereview.chromium.org/1280953003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:231: layer()->Add(ink_drop_layer); Maybe SetPaintToLayer etc. should be moved in here (and again in RemoveInkDropLayer() below)? Would that address the TODO? https://codereview.chromium.org/1280953003/diff/280001/ui/views/animation/ink... File ui/views/animation/ink_drop_animation_controller_impl.h (right): https://codereview.chromium.org/1280953003/diff/280001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_impl.h:82: ui::LayerOwner long_press_layer_owner_; Is there a reason to not use scoped_ptr<Layer> for these?
https://codereview.chromium.org/1280953003/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:37: // TODO(bruthig): Only set the layer when the ink drop is active. On 2015/08/18 17:51:49, sadrul wrote: > Make sure there's a blocking crbug for this. Created www.crbug.com/522175 https://codereview.chromium.org/1280953003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:38: SetLayer(new ui::Layer(ui::LAYER_NOT_DRAWN)); On 2015/08/18 17:51:49, sadrul wrote: > You should use SetPaintToLayer() instead. Done. https://codereview.chromium.org/1280953003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:57: // RemoveInkDropLayer(). On 2015/08/17 21:18:15, Peter Kasting wrote: > Is that actually a problem? It seems like that call ought to work fine even > when made during the normal destruction sequence. > > I would rather not do this explicitly unless we actually need to do so for > correctness' sake. Originally I added this at sadrul@'s request but I have removed it. sadrul@, WDYT? https://codereview.chromium.org/1280953003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:231: layer()->Add(ink_drop_layer); On 2015/08/18 17:51:49, sadrul wrote: > Maybe SetPaintToLayer etc. should be moved in here (and again in > RemoveInkDropLayer() below)? Would that address the TODO? This will address the TODO at this level anyway. The IDAC will still need to be changed to add and remove the ink drop layer only when they are active. This is tracked by www.crbug.com/522175 . https://codereview.chromium.org/1280953003/diff/280001/ui/views/animation/ink... File ui/views/animation/ink_drop_animation_controller_impl.h (right): https://codereview.chromium.org/1280953003/diff/280001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_impl.h:82: ui::LayerOwner long_press_layer_owner_; On 2015/08/18 17:51:49, sadrul wrote: > Is there a reason to not use scoped_ptr<Layer> for these? Yes, ScreenRotationAnimator creates a copy of all the layers and if these were scoped_ptr<Layer>'s then they would become stale/invalid every time the screen rotated.
LGTM
https://codereview.chromium.org/1280953003/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:57: // RemoveInkDropLayer(). On 2015/08/18 19:04:38, bruthig wrote: > On 2015/08/17 21:18:15, Peter Kasting wrote: > > Is that actually a problem? It seems like that call ought to work fine even > > when made during the normal destruction sequence. > > > > I would rather not do this explicitly unless we actually need to do so for > > correctness' sake. > > Originally I added this at sadrul@'s request but I have removed it. > sadrul@, WDYT? Reaching into virtual functions during destruction make me nervous. But I don't feel strongly enough about it. https://codereview.chromium.org/1280953003/diff/280001/ui/views/animation/ink... File ui/views/animation/ink_drop_animation_controller_impl.h (right): https://codereview.chromium.org/1280953003/diff/280001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_impl.h:82: ui::LayerOwner long_press_layer_owner_; On 2015/08/18 19:04:38, bruthig wrote: > On 2015/08/18 17:51:49, sadrul wrote: > > Is there a reason to not use scoped_ptr<Layer> for these? > > Yes, ScreenRotationAnimator creates a copy of all the layers and if these were > scoped_ptr<Layer>'s then they would become stale/invalid every time the screen > rotated. That sounds odd (and a buggy behaviour; ScreenRotationAnimator should not invalidate layers owned by someone else (or if it does, it needs a mechanism to notify the owners)). Looking at window_util.cc:CloneChildren(), it looks like that should not be the case though (i.e. since it seems to skip over the Layers that don't have LayerOwners?) https://codereview.chromium.org/1280953003/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:38: image()->SetFillsBoundsOpaquely(false); You should do these down in AddInkDropLayer too https://codereview.chromium.org/1280953003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:46: // list. Update the comment. https://codereview.chromium.org/1280953003/diff/300001/ui/views/animation/ink... File ui/views/animation/ink_drop_animation_controller_impl.cc (right): https://codereview.chromium.org/1280953003/diff/300001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_impl.cc:211: ink_drop_host_->AddInkDropLayer(root_layer_owner_.layer()); Add a TODO and refer to 522175
sadrul@, if you are good with this review can you take a look at the follow up too? It should be a simple one. https://codereview.chromium.org/1286693004/ https://codereview.chromium.org/1280953003/diff/280001/ui/views/animation/ink... File ui/views/animation/ink_drop_animation_controller_impl.h (right): https://codereview.chromium.org/1280953003/diff/280001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_impl.h:82: ui::LayerOwner long_press_layer_owner_; On 2015/08/18 19:52:38, sadrul wrote: > On 2015/08/18 19:04:38, bruthig wrote: > > On 2015/08/18 17:51:49, sadrul wrote: > > > Is there a reason to not use scoped_ptr<Layer> for these? > > > > Yes, ScreenRotationAnimator creates a copy of all the layers and if these were > > scoped_ptr<Layer>'s then they would become stale/invalid every time the screen > > rotated. > > That sounds odd (and a buggy behaviour; ScreenRotationAnimator should not > invalidate layers owned by someone else (or if it does, it needs a mechanism to > notify the owners)). Looking at window_util.cc:CloneChildren(), it looks like > that should not be the case though (i.e. since it seems to skip over the Layers > that don't have LayerOwners?) Hmm, you are right, I was mistaken. I've changed them to scoped_ptr<Layer>'s. https://codereview.chromium.org/1280953003/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1280953003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:38: image()->SetFillsBoundsOpaquely(false); On 2015/08/18 19:52:39, sadrul wrote: > You should do these down in AddInkDropLayer too Done. https://codereview.chromium.org/1280953003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.cc:46: // list. On 2015/08/18 19:52:39, sadrul wrote: > Update the comment. Done. https://codereview.chromium.org/1280953003/diff/300001/ui/views/animation/ink... File ui/views/animation/ink_drop_animation_controller_impl.cc (right): https://codereview.chromium.org/1280953003/diff/300001/ui/views/animation/ink... ui/views/animation/ink_drop_animation_controller_impl.cc:211: ink_drop_host_->AddInkDropLayer(root_layer_owner_.layer()); On 2015/08/18 19:52:39, sadrul wrote: > Add a TODO and refer to 522175 Done.
lgtm
The CQ bit was checked by bruthig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1280953003/#ps340001 (title: "Fixed minor typo.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1280953003/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1280953003/340001
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/57421779a58c260999ee2a9008bdfc7bfb442ddc Cr-Commit-Position: refs/heads/master@{#344488} |
