Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1411833006:
Refactoring to make adding ink drop animations easier (Closed)
DescriptionRefactor ink drop animations to allow adding them to any CustomButton descendants.
Looking for a way to allow organizing event handling necessary for ink drop animations. See goals description here:
https://docs.google.com/document/d/1U4AdR4wZVsZ8g0y95EblY5YiYVbVXcBTJ59XJBF147M/edit#heading=h.ai99f1d1eumr
BUG=537202
Committed: https://crrev.com/bcd9c28cd2858c07ef2b0b5107e1c5523cf84645
Cr-Commit-Position: refs/heads/master@{#362013}
Patch Set 1 : Attempt to refactor ink drop animations #
Total comments: 9
Patch Set 2 : Attempt to refactor ink drop animations (rebase) #Patch Set 3 : Attempt to refactor ink drop animations (moving more stuff into InkDropDelegate) #
Total comments: 12
Patch Set 4 : Attempt to refactor ink drop animations (wired up chained EventHandler) #
Total comments: 8
Patch Set 5 : Attempt to refactor ink drop animations (comments) #Patch Set 6 : Refactor ink drop animations (simplify MenuButton) #
Total comments: 12
Patch Set 7 : Refactor ink drop animations (moved event handling into ScopedTargetHandler) #Patch Set 8 : Refactor ink drop animations (moved ACTION_PENDING handling to toolbar_action_view for now) #
Total comments: 13
Patch Set 9 : Refactor ink drop animations (rebased to master) #Patch Set 10 : Refactor ink drop animations (Moving more generic functionality into ScopedTargetHandler) #
Total comments: 4
Patch Set 11 : Refactor ink drop animations (rebased) #
Total comments: 20
Patch Set 12 : Refactor ink drop animations (comments in c/b/ui/views/) #
Total comments: 4
Patch Set 13 : Refactor ink drop animations (nits in c/b/ui/views/) #Patch Set 14 : Refactor ink drop animations (comments in ui/views/) #
Total comments: 26
Patch Set 15 : Refactor ink drop animations (comments in ui/views/) #
Total comments: 2
Patch Set 16 : Refactor ink drop animations (more comments in ui/views/, test) #
Total comments: 10
Patch Set 17 : Refactor ink drop animations (nits in ui/views/ #Messages
Total messages: 90 (39 generated)
Description was changed from ========== Refactor ink drop animations to allow handling events through a View*. This is not for review yet. Includes https://codereview.chromium.org/1409183003/ Looking for a way to allow organizing event handling necessary for ink drop animations. See goals description here: https://docs.google.com/document/d/1U4AdR4wZVsZ8g0y95EblY5YiYVbVXcBTJ59XJBF14... BUG=537202 ========== to ========== Refactor ink drop animations to allow handling events through a View*. This is not for review yet. Includes https://codereview.chromium.org/1409183003/ Looking for a way to allow organizing event handling necessary for ink drop animations. See goals description here: https://docs.google.com/document/d/1U4AdR4wZVsZ8g0y95EblY5YiYVbVXcBTJ59XJBF14... BUG=537202 ==========
Patchset #1 (id:1) has been deleted
bruthig@chromium.org changed reviewers: + bruthig@chromium.org
I've left some comments, mostly concerns. Normally I would spend some time thinking of alternative considerations to propose but I have to leave early. Let's maybe get together next week, perhaps with Roger and see where we stand on the goals/requirements doc and proceed from there. https://codereview.chromium.org/1411833006/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1411833006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:151: bool ToolbarActionView::OnMousePressed(const ui::MouseEvent& event) { Is this an example of how the View can override an InkDropDelegate for more View specific behaviour or just something that's being used for dev purposes? https://codereview.chromium.org/1411833006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:163: if (menu_showing_) { Is this View state logic currently captured in the ToolbarInkDropDelegate? It doesn't appear to be. So as it is iimplemented now if the ToolbarActionView gets a ET_GESTURE_TAP_DOWN the View won't actually trigger an event however the ripple will be shown. Major takeaway here is that it appears there to be a necessary coupling between a concrete InkDropDelegate and the concrete View. This is a risk that we may not be able to re-use some of the general InkDropDelegates as much as we'd like to. https://codereview.chromium.org/1411833006/diff/20001/ui/views/animation/tool... File ui/views/animation/toolbar_ink_drop_delegate.cc (right): https://codereview.chromium.org/1411833006/diff/20001/ui/views/animation/tool... ui/views/animation/toolbar_ink_drop_delegate.cc:20: void ToolbarInkDropDelegate::OnGestureEvent(ui::GestureEvent* event) { Just re-iterating my quick parting comments from when we brainstormed solutions for this. Most of this event handling log must already exist somewhere. i.e. Within the ToolbarActionView -> ... -> View hierarchy. To be candid, I really think it's a bad decision if we duplicated it between there and an InkDropDelegate hierarchy. This distributed logic is hard to understand, get right, and maintain to be equivalent between the two hierarchies. We've already got a yo-yo like event handling mechanism with the Pre/Post target handlers and then the actual Views handling. We would now be adding yet another up/down handling of the event. https://codereview.chromium.org/1411833006/diff/20001/ui/views/animation/tool... File ui/views/animation/toolbar_ink_drop_delegate.h (right): https://codereview.chromium.org/1411833006/diff/20001/ui/views/animation/tool... ui/views/animation/toolbar_ink_drop_delegate.h:21: class VIEWS_EXPORT ToolbarInkDropDelegate : public InkDropDelegate { Am I correct to assume that you've only implemented a very concrete ToolbarInkDropDelegate here for example purposes and the real proposal would be to have some more general delegate implementation somewhere between the ToolbarInkDropDelegate and the InkDropDelegate that would be re-usable?
Thanks! All valid concerns, let's chat next week. https://codereview.chromium.org/1411833006/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1411833006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:151: bool ToolbarActionView::OnMousePressed(const ui::MouseEvent& event) { On 2015/11/06 20:29:10, bruthig wrote: > Is this an example of how the View can override an InkDropDelegate for more View > specific behaviour or just something that's being used for dev purposes? No, this is same as in "parent" CL (I know, I should have rebased it). I imagine that OnMousePressed would need to be added to the delegate interface and then the ink-related code could be moved to the concrete delegate impl. Not unlike with OnGestureEvent example. https://codereview.chromium.org/1411833006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:163: if (menu_showing_) { On 2015/11/06 20:29:10, bruthig wrote: > Is this View state logic currently captured in the ToolbarInkDropDelegate? It > doesn't appear to be. So as it is iimplemented now if the ToolbarActionView gets > a ET_GESTURE_TAP_DOWN the View won't actually trigger an event however the > ripple will be shown. > > Major takeaway here is that it appears there to be a necessary coupling between > a concrete InkDropDelegate and the concrete View. This is a risk that we may > not be able to re-use some of the general InkDropDelegates as much as we'd like > to. Not sure I follow you on this one. In this code path I am not sending OnGestureEvent up the View* hierarchy so the event is not triggered and the ripple is not shown, no? Major takeaway notwithstanding - I agree with that concern. https://codereview.chromium.org/1411833006/diff/20001/ui/views/animation/tool... File ui/views/animation/toolbar_ink_drop_delegate.cc (right): https://codereview.chromium.org/1411833006/diff/20001/ui/views/animation/tool... ui/views/animation/toolbar_ink_drop_delegate.cc:20: void ToolbarInkDropDelegate::OnGestureEvent(ui::GestureEvent* event) { On 2015/11/06 20:29:11, bruthig wrote: > Just re-iterating my quick parting comments from when we brainstormed solutions > for this. > Most of this event handling log must already exist somewhere. i.e. Within the > ToolbarActionView -> ... -> View hierarchy. To be candid, I really think it's a > bad decision if we duplicated it between there and an InkDropDelegate hierarchy. > This distributed logic is hard to understand, get right, and maintain to be > equivalent between the two hierarchies. We've already got a yo-yo like event > handling mechanism with the Pre/Post target handlers and then the actual Views > handling. We would now be adding yet another up/down handling of the event. Maybe. This however allows to refactor what we have now with less code duplication and then possibly address the cases where the two hierarchies are too coupled. https://codereview.chromium.org/1411833006/diff/20001/ui/views/animation/tool... File ui/views/animation/toolbar_ink_drop_delegate.h (right): https://codereview.chromium.org/1411833006/diff/20001/ui/views/animation/tool... ui/views/animation/toolbar_ink_drop_delegate.h:21: class VIEWS_EXPORT ToolbarInkDropDelegate : public InkDropDelegate { On 2015/11/06 20:29:11, bruthig wrote: > Am I correct to assume that you've only implemented a very concrete > ToolbarInkDropDelegate here for example purposes and the real proposal would be > to have some more general delegate implementation somewhere between the > ToolbarInkDropDelegate and the InkDropDelegate that would be re-usable? Of course. I have only implemented a concrete part of a concrete delegate just for illustration of wiring.
https://codereview.chromium.org/1411833006/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1411833006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:163: if (menu_showing_) { On 2015/11/06 20:51:50, varkha wrote: > On 2015/11/06 20:29:10, bruthig wrote: > > Is this View state logic currently captured in the ToolbarInkDropDelegate? > It > > doesn't appear to be. So as it is iimplemented now if the ToolbarActionView > gets > > a ET_GESTURE_TAP_DOWN the View won't actually trigger an event however the > > ripple will be shown. > > > > Major takeaway here is that it appears there to be a necessary coupling > between > > a concrete InkDropDelegate and the concrete View. This is a risk that we may > > not be able to re-use some of the general InkDropDelegates as much as we'd > like > > to. > > Not sure I follow you on this one. In this code path I am not sending > OnGestureEvent up the View* hierarchy so the event is not triggered and the > ripple is not shown, no? > Major takeaway notwithstanding - I agree with that concern. My mistake, I was thinking of OnGestureEvent() as if it were a View specific event like OnMouseExited().
Uploaded a rebase to https://codereview.chromium.org/1409183003/.
Description was changed from ========== Refactor ink drop animations to allow handling events through a View*. This is not for review yet. Includes https://codereview.chromium.org/1409183003/ Looking for a way to allow organizing event handling necessary for ink drop animations. See goals description here: https://docs.google.com/document/d/1U4AdR4wZVsZ8g0y95EblY5YiYVbVXcBTJ59XJBF14... BUG=537202 ========== to ========== Refactor ink drop animations to allow handling events through a View*. This is not for review yet. This CL is upstreamed to https://codereview.chromium.org/1409183003/ Looking for a way to allow organizing event handling necessary for ink drop animations. See goals description here: https://docs.google.com/document/d/1U4AdR4wZVsZ8g0y95EblY5YiYVbVXcBTJ59XJBF14... BUG=537202 ==========
Patchset #3 (id:60001) has been deleted
bruthig@, can you please take a look if you think this is going in the right direction for the first step in refactoring that we discussed? I haven't yet touched ink surfaces other than the extension toolbar buttons but this allows the changes to add the ripples to the extension buttons to be limited to just a few lines.
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411833006/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411833006/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Some moderate concerns but I think we're on the right track here. I think we really need to rename the InkDropDelegate as something like InkDropController, and rename the existing InkDropAnimationController as InkDrop. https://codereview.chromium.org/1411833006/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1411833006/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:150: if (event->type() == ui::ET_GESTURE_TAP_DOWN) { This is set to handled for ink drop purposes only correct? I would think it should be moved to the ToolbarInkDropDelegate. https://codereview.chromium.org/1411833006/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:351: ink_drop_delegate->OnActivated(); Is there any benefit to having the Views invoke state changes on the InkDropDelegate and having it proxy the call off to the InkDropAnimationController instead of just calling InkDropAnimationController::AnimateToState() directly? The disadvantage I see with the current implementation is Views might eventually want access to some InkDropController specific data, i.e. InkDropState, and I don't think having to reach through the InkDropDelegate, or proxying the calls is the best choice here. https://codereview.chromium.org/1411833006/diff/120001/ui/views/animation/ink... File ui/views/animation/ink_drop_delegate.h (right): https://codereview.chromium.org/1411833006/diff/120001/ui/views/animation/ink... ui/views/animation/ink_drop_delegate.h:33: virtual void Layout() = 0; Unless there is a compelling reason/benefit to move non-state controlling logic here (i.e. Layout, ink drop size) I think we should make the InkDropDelegate (pending a better name) only be responsible for controlling the InkDropState given input events, ink drop animation events, and View specific state information. https://codereview.chromium.org/1411833006/diff/120001/ui/views/animation/ink... ui/views/animation/ink_drop_delegate.h:36: virtual void OnGestureEvent(const ui::GestureEvent& event) = 0; |event| probably shouldn't be const. I imagine some InkDropDelegates will have to set the |event| to handled to ensure it gets future events in the sequence. https://codereview.chromium.org/1411833006/diff/120001/ui/views/animation/ink... ui/views/animation/ink_drop_delegate.h:42: virtual void OnActionPending(const ui::Event& event) = 0; Is |event| used anywhere, or are there plans to use it? https://codereview.chromium.org/1411833006/diff/120001/ui/views/animation/ink... File ui/views/animation/ink_drop_host.h (right): https://codereview.chromium.org/1411833006/diff/120001/ui/views/animation/ink... ui/views/animation/ink_drop_host.h:37: virtual gfx::Point CalculateInkDropCenter() const = 0; Adding this here ties a host 1-to-1 with ripples. My thinking was the host should only know how to add/remove layers and as such it would be possible to host more than 1 ink drop. I suggest moving this in to the InkDropConsumer class that I've defined in this CL here: https://codereview.chromium.org/1390113006/
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411833006/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411833006/140001
PTAL. I will do renaming as a next step. https://codereview.chromium.org/1411833006/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1411833006/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:150: if (event->type() == ui::ET_GESTURE_TAP_DOWN) { On 2015/11/12 16:41:15, bruthig wrote: > This is set to handled for ink drop purposes only correct? I would think it > should be moved to the ToolbarInkDropDelegate. Done. https://codereview.chromium.org/1411833006/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:351: ink_drop_delegate->OnActivated(); On 2015/11/12 16:41:15, bruthig wrote: > Is there any benefit to having the Views invoke state changes on the > InkDropDelegate and having it proxy the call off to the > InkDropAnimationController instead of just calling > InkDropAnimationController::AnimateToState() directly? > > The disadvantage I see with the current implementation is Views might eventually > want access to some InkDropController specific data, i.e. InkDropState, and I > don't think having to reach through the InkDropDelegate, or proxying the calls > is the best choice here. I wanted to hide more of the InkDropAnimationcontroller from the clients in case it needs to do more. I was hoping that what is now called InkDropDelegate would be able to query and use view's state if necessary. https://codereview.chromium.org/1411833006/diff/120001/ui/views/animation/ink... File ui/views/animation/ink_drop_delegate.h (right): https://codereview.chromium.org/1411833006/diff/120001/ui/views/animation/ink... ui/views/animation/ink_drop_delegate.h:33: virtual void Layout() = 0; On 2015/11/12 16:41:15, bruthig wrote: > Unless there is a compelling reason/benefit to move non-state controlling logic > here (i.e. Layout, ink drop size) I think we should make the InkDropDelegate > (pending a better name) only be responsible for controlling the InkDropState > given input events, ink drop animation events, and View specific state > information. As we talked offline. I've renamed it to OnLayout() (as it is not really doing any layout) but I think it is worthwhile to consolidate ink ripple client here including re-positioning. If the ink painter is more complicated then I guess this implementation could be more complicated but the interface would stay same. https://codereview.chromium.org/1411833006/diff/120001/ui/views/animation/ink... ui/views/animation/ink_drop_delegate.h:36: virtual void OnGestureEvent(const ui::GestureEvent& event) = 0; On 2015/11/12 16:41:15, bruthig wrote: > |event| probably shouldn't be const. I imagine some InkDropDelegates will have > to set the |event| to handled to ensure it gets future events in the sequence. Done. https://codereview.chromium.org/1411833006/diff/120001/ui/views/animation/ink... ui/views/animation/ink_drop_delegate.h:42: virtual void OnActionPending(const ui::Event& event) = 0; On 2015/11/12 16:41:15, bruthig wrote: > Is |event| used anywhere, or are there plans to use it? Done. https://codereview.chromium.org/1411833006/diff/120001/ui/views/animation/ink... File ui/views/animation/ink_drop_host.h (right): https://codereview.chromium.org/1411833006/diff/120001/ui/views/animation/ink... ui/views/animation/ink_drop_host.h:37: virtual gfx::Point CalculateInkDropCenter() const = 0; On 2015/11/12 16:41:15, bruthig wrote: > Adding this here ties a host 1-to-1 with ripples. My thinking was the host > should only know how to add/remove layers and as such it would be possible to > host more than 1 ink drop. > > I suggest moving this in to the InkDropConsumer class that I've defined in this > CL here: https://codereview.chromium.org/1390113006/ Added a TODO. I can change that once your CL lands.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
A few discussion points. https://codereview.chromium.org/1411833006/diff/140001/ui/views/animation/ink... File ui/views/animation/ink_drop_delegate.h (right): https://codereview.chromium.org/1411833006/diff/140001/ui/views/animation/ink... ui/views/animation/ink_drop_delegate.h:36: virtual void OnActionComplete() = 0; What do you think about having a single function OnAction(InkDropState)? That way we wouldn't have to update this class if we edit the InkDropState enum and I think it's fair to say it should be one-to-one. https://codereview.chromium.org/1411833006/diff/140001/ui/views/animation/too... File ui/views/animation/toolbar_ink_drop_delegate.cc (right): https://codereview.chromium.org/1411833006/diff/140001/ui/views/animation/too... ui/views/animation/toolbar_ink_drop_delegate.cc:27: view_->set_target_handler(original_event_handler_); What do you think about adding a D/CHECK here to verify this is still the current target_handler() here? As an aside, it might be worthwhile extracting this target_handler wrapper/decorator functionality in to a stand alone class that can be used by this. https://codereview.chromium.org/1411833006/diff/140001/ui/views/animation/too... ui/views/animation/toolbar_ink_drop_delegate.cc:94: LOG(ERROR) << __FUNCTION__ << " event:" << event->type(); Don't forget to remove this log line. https://codereview.chromium.org/1411833006/diff/140001/ui/views/animation/too... File ui/views/animation/toolbar_ink_drop_delegate.h (right): https://codereview.chromium.org/1411833006/diff/140001/ui/views/animation/too... ui/views/animation/toolbar_ink_drop_delegate.h:51: View* view_; Do we actually need to interface with View specific stuff or would the EventHandler type be more appropriate here? Or since this is the Toolbar specific delegate, then perhaps this should be typed as ToolbarActionView?
Patchset #5 (id:160001) has been deleted
Patchset #5 (id:180001) has been deleted
Thanks, all good suggestions. PTAL. https://chromiumcodereview.appspot.com/1411833006/diff/140001/ui/views/animat... File ui/views/animation/ink_drop_delegate.h (right): https://chromiumcodereview.appspot.com/1411833006/diff/140001/ui/views/animat... ui/views/animation/ink_drop_delegate.h:36: virtual void OnActionComplete() = 0; On 2015/11/16 22:37:11, bruthig wrote: > What do you think about having a single function OnAction(InkDropState)? That > way we wouldn't have to update this class if we edit the InkDropState enum and I > think it's fair to say it should be one-to-one. Done. https://chromiumcodereview.appspot.com/1411833006/diff/140001/ui/views/animat... File ui/views/animation/toolbar_ink_drop_delegate.cc (right): https://chromiumcodereview.appspot.com/1411833006/diff/140001/ui/views/animat... ui/views/animation/toolbar_ink_drop_delegate.cc:27: view_->set_target_handler(original_event_handler_); On 2015/11/16 22:37:11, bruthig wrote: > What do you think about adding a D/CHECK here to verify this is still the > current target_handler() here? > > As an aside, it might be worthwhile extracting this target_handler > wrapper/decorator functionality in to a stand alone class that can be used by > this. Done (inside ScopedTargetHandler). https://chromiumcodereview.appspot.com/1411833006/diff/140001/ui/views/animat... ui/views/animation/toolbar_ink_drop_delegate.cc:94: LOG(ERROR) << __FUNCTION__ << " event:" << event->type(); On 2015/11/16 22:37:11, bruthig wrote: > Don't forget to remove this log line. Done. https://chromiumcodereview.appspot.com/1411833006/diff/140001/ui/views/animat... File ui/views/animation/toolbar_ink_drop_delegate.h (right): https://chromiumcodereview.appspot.com/1411833006/diff/140001/ui/views/animat... ui/views/animation/toolbar_ink_drop_delegate.h:51: View* view_; On 2015/11/16 22:37:11, bruthig wrote: > Do we actually need to interface with View specific stuff or would the > EventHandler type be more appropriate here? > > Or since this is the Toolbar specific delegate, then perhaps this should be > typed as ToolbarActionView? Done.
Description was changed from ========== Refactor ink drop animations to allow handling events through a View*. This is not for review yet. This CL is upstreamed to https://codereview.chromium.org/1409183003/ Looking for a way to allow organizing event handling necessary for ink drop animations. See goals description here: https://docs.google.com/document/d/1U4AdR4wZVsZ8g0y95EblY5YiYVbVXcBTJ59XJBF14... BUG=537202 ========== to ========== Refactor ink drop animations to allow adding them to any View and not just Button descendants. This CL is upstreamed to https://codereview.chromium.org/1409183003/ Looking for a way to allow organizing event handling necessary for ink drop animations. See goals description here: https://docs.google.com/document/d/1U4AdR4wZVsZ8g0y95EblY5YiYVbVXcBTJ59XJBF14... BUG=537202 ==========
varkha@chromium.org changed reviewers: + sadrul@chromium.org
sadrul@, can you please also take a look if this matches what we discussed? Thanks!
https://codereview.chromium.org/1411833006/diff/220001/ui/events/scoped_targe... File ui/events/scoped_target_handler.h (right): https://codereview.chromium.org/1411833006/diff/220001/ui/events/scoped_targe... ui/events/scoped_target_handler.h:15: Some class level documentation would be useful here. https://codereview.chromium.org/1411833006/diff/220001/ui/events/scoped_targe... ui/events/scoped_target_handler.h:16: class EVENTS_EXPORT ScopedTargetHandler { I envisioned that ScopedTargetHandler (or general subclasses of it) would extend EventHandler and would set itself as the 'new' target handler, i.e. set_target_handler(this). This would allow us to push the ToolbarButtonDelegate::On*Event() implementations up in to this more general class. +1 for code re-use and less duplication once more InkDropDelegates are born. https://codereview.chromium.org/1411833006/diff/220001/ui/views/animation/too... File ui/views/animation/toolbar_ink_drop_delegate.cc (right): https://codereview.chromium.org/1411833006/diff/220001/ui/views/animation/too... ui/views/animation/toolbar_ink_drop_delegate.cc:19: : event_handler_(event_target), |event_handler_| should not be needed. See comments in scoped_target_handler.h. https://codereview.chromium.org/1411833006/diff/220001/ui/views/controls/butt... File ui/views/controls/button/menu_button.cc (right): https://codereview.chromium.org/1411833006/diff/220001/ui/views/controls/butt... ui/views/controls/button/menu_button.cc:130: InkDropDelegate* ink_drop_delegate = GetInkDropDelegate(); Would it make things easier if GetInkDropDelegate() returned a dummy/stub InkDropDelegate instead of null with no-op implementations for all methods? We wouldn't have to have 'if (ink_drop_delegate)' everywhere. This was the intention of the current InkDropAnimationControllerStub class. (Which may not be needed anymore if we use a stub delegate instead.) https://codereview.chromium.org/1411833006/diff/220001/ui/views/controls/butt... ui/views/controls/button/menu_button.cc:183: ink_drop_delegate->OnAction(InkDropState::ACTION_PENDING); I don't think a MenuButton can know for certain that all mouse-pressed events should show an ACTION_PENDING ripple. e.g. The ChevronMenuButton::OnMenuButtonClicked() may close a menu on a mouse-press. I'm not sure how the ACTION_PENDING ripple would look in this case. The underlying issue here is that I don't think a MenuButton has enough information to know how to react to a mouse pressed event. Also, it's odd that the ripple controlling logic is not the same as the logic that calls Activate(). From what I can tell if a mouse press occurs in quicker than |kMinimumMsBetweenButtonClicks| from when the menu was last shown, then the ACTION_PENDING ripple is going to get shown even though no action will be triggered. https://codereview.chromium.org/1411833006/diff/220001/ui/views/controls/butt... ui/views/controls/button/menu_button.cc:205: ink_drop_delegate->OnAction(InkDropState::HIDDEN); Somewhat similar concerns here to the ones in OnMousePressed. How can we be sure that all mouse release events should hide the ripple? Looking forward, once jonross@'s work to make menu's asynchronous, I'm assuming we will be getting the mouse release events when the menu is shown. In which case the ripple should be in the ACTIVATED state, but this logic would cause it to be hidden.
Patchset #8 (id:260001) has been deleted
PTAL. Let's chat tomorrow about the PENDING / HIDDEN events. https://codereview.chromium.org/1411833006/diff/220001/ui/events/scoped_targe... File ui/events/scoped_target_handler.h (right): https://codereview.chromium.org/1411833006/diff/220001/ui/events/scoped_targe... ui/events/scoped_target_handler.h:15: On 2015/11/19 16:49:53, bruthig wrote: > Some class level documentation would be useful here. Done. https://codereview.chromium.org/1411833006/diff/220001/ui/events/scoped_targe... ui/events/scoped_target_handler.h:16: class EVENTS_EXPORT ScopedTargetHandler { On 2015/11/19 16:49:53, bruthig wrote: > I envisioned that ScopedTargetHandler (or general subclasses of it) would extend > EventHandler and would set itself as the 'new' target handler, i.e. > set_target_handler(this). This would allow us to push the > ToolbarButtonDelegate::On*Event() implementations up in to this more general > class. +1 for code re-use and less duplication once more InkDropDelegates are > born. Done. https://codereview.chromium.org/1411833006/diff/220001/ui/views/animation/too... File ui/views/animation/toolbar_ink_drop_delegate.cc (right): https://codereview.chromium.org/1411833006/diff/220001/ui/views/animation/too... ui/views/animation/toolbar_ink_drop_delegate.cc:19: : event_handler_(event_target), On 2015/11/19 16:49:53, bruthig wrote: > |event_handler_| should not be needed. See comments in scoped_target_handler.h. Done. https://codereview.chromium.org/1411833006/diff/220001/ui/views/controls/butt... File ui/views/controls/button/menu_button.cc (right): https://codereview.chromium.org/1411833006/diff/220001/ui/views/controls/butt... ui/views/controls/button/menu_button.cc:130: InkDropDelegate* ink_drop_delegate = GetInkDropDelegate(); On 2015/11/19 16:49:53, bruthig wrote: > Would it make things easier if GetInkDropDelegate() returned a dummy/stub > InkDropDelegate instead of null with no-op implementations for all methods? We > wouldn't have to have 'if (ink_drop_delegate)' everywhere. > > This was the intention of the current InkDropAnimationControllerStub class. > (Which may not be needed anymore if we use a stub delegate instead.) Done. May remove InkDropAnimationControllerStub later. https://codereview.chromium.org/1411833006/diff/220001/ui/views/controls/butt... ui/views/controls/button/menu_button.cc:183: ink_drop_delegate->OnAction(InkDropState::ACTION_PENDING); On 2015/11/19 16:49:53, bruthig wrote: > I don't think a MenuButton can know for certain that all mouse-pressed events > should show an ACTION_PENDING ripple. e.g. The > ChevronMenuButton::OnMenuButtonClicked() may close a menu on a mouse-press. I'm > not sure how the ACTION_PENDING ripple would look in this case. > > The underlying issue here is that I don't think a MenuButton has enough > information to know how to react to a mouse pressed event. > > Also, it's odd that the ripple controlling logic is not the same as the logic > that calls Activate(). From what I can tell if a mouse press occurs in quicker > than |kMinimumMsBetweenButtonClicks| from when the menu was last shown, then the > ACTION_PENDING ripple is going to get shown even though no action will be > triggered. Done. https://codereview.chromium.org/1411833006/diff/220001/ui/views/controls/butt... ui/views/controls/button/menu_button.cc:205: ink_drop_delegate->OnAction(InkDropState::HIDDEN); On 2015/11/19 16:49:53, bruthig wrote: > Somewhat similar concerns here to the ones in OnMousePressed. How can we be > sure that all mouse release events should hide the ripple? > Looking forward, once jonross@'s work to make menu's asynchronous, I'm assuming > we will be getting the mouse release events when the menu is shown. In which > case the ripple should be in the ACTIVATED state, but this logic would cause it > to be hidden. Will need to rethink what happens in asynchronous case.
Patchset #8 (id:280001) has been deleted
https://codereview.chromium.org/1411833006/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1411833006/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:145: if (event.IsOnlyLeftMouseButton()) Should this be calling 'MenuButton::ShouldEnterPushedState(event)' instead of 'event.IsOnlyLeftMouseButton()'? Or perhaps we can initiate the ACTION_PENDING animation in a 'StateChanged()' override. I just don't want this logic to drift from MenuButton::ShouldEnterPushedState(). https://codereview.chromium.org/1411833006/diff/300001/ui/events/scoped_targe... File ui/events/scoped_target_handler.cc (right): https://codereview.chromium.org/1411833006/diff/300001/ui/events/scoped_targe... ui/events/scoped_target_handler.cc:24: EventHandler::OnEvent(event); Don't we want the ToolbarActionView to handle the event before the ToolbarInkDropDelegate? https://codereview.chromium.org/1411833006/diff/300001/ui/events/scoped_targe... ui/events/scoped_target_handler.cc:30: static_cast<EventHandler*>(target_)->OnKeyEvent(event); Are these casts necessary? EventTarget extends EventHandler. https://codereview.chromium.org/1411833006/diff/300001/ui/events/scoped_targe... File ui/events/scoped_target_handler.h (right): https://codereview.chromium.org/1411833006/diff/300001/ui/events/scoped_targe... ui/events/scoped_target_handler.h:20: ScopedTargetHandler(EventTarget* target); I think the ScopedTargetHandler should still accept an EventHandler. That way it can be used by clients with a "has a" relationship instead of an "is a" one. (And it would still work for "is a" relationships too) Might be easiest to discuss offline.
https://codereview.chromium.org/1411833006/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1411833006/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:145: if (event.IsOnlyLeftMouseButton()) On 2015/11/20 16:25:33, bruthig wrote: > Should this be calling 'MenuButton::ShouldEnterPushedState(event)' instead of > 'event.IsOnlyLeftMouseButton()'? Or perhaps we can initiate the ACTION_PENDING > animation in a 'StateChanged()' override. > > I just don't want this logic to drift from MenuButton::ShouldEnterPushedState(). Probably yes to the first question and yes, this will get hopefully better when we tackle the state model. https://codereview.chromium.org/1411833006/diff/300001/ui/events/scoped_targe... File ui/events/scoped_target_handler.cc (right): https://codereview.chromium.org/1411833006/diff/300001/ui/events/scoped_targe... ui/events/scoped_target_handler.cc:24: EventHandler::OnEvent(event); On 2015/11/20 16:25:33, bruthig wrote: > Don't we want the ToolbarActionView to handle the event before the > ToolbarInkDropDelegate? This is what happens with this code (inside EventHandler::OnEvent). The stack when ToolbarActionView gets the event is this: View::OnGestureEvent() CustomButton::OnGestureEvent() MenuButton::OnGestureEvent() ToolbarActionView::OnGestureEvent() <- is set as a |target_| ScopedTargetHandler::OnGestureEvent ** ToolbarInkDropDelegate::OnGestureEvent() EventHandler::OnEvent() ScopedTargetHandler::OnEvent() <- in an instance of ToolbarInkDropDelegate After ScopedTargetHandler::OnGestureEvent() returns in ** above ToolbarInkDropDelegate gets to handle the ink drop animations. https://codereview.chromium.org/1411833006/diff/300001/ui/events/scoped_targe... ui/events/scoped_target_handler.cc:30: static_cast<EventHandler*>(target_)->OnKeyEvent(event); On 2015/11/20 16:25:33, bruthig wrote: > Are these casts necessary? EventTarget extends EventHandler. Yes they are even though yes it does. This is because the calls are on |target_| (which is the View) and not on |this| (which is an instance of InkDropDelegate). https://codereview.chromium.org/1411833006/diff/300001/ui/events/scoped_targe... File ui/events/scoped_target_handler.h (right): https://codereview.chromium.org/1411833006/diff/300001/ui/events/scoped_targe... ui/events/scoped_target_handler.h:20: ScopedTargetHandler(EventTarget* target); On 2015/11/20 16:25:33, bruthig wrote: > I think the ScopedTargetHandler should still accept an EventHandler. That way > it can be used by clients with a "has a" relationship instead of an "is a" one. > (And it would still work for "is a" relationships too) > Might be easiest to discuss offline. Acknowledged.
https://codereview.chromium.org/1411833006/diff/300001/ui/events/scoped_targe... File ui/events/scoped_target_handler.cc (right): https://codereview.chromium.org/1411833006/diff/300001/ui/events/scoped_targe... ui/events/scoped_target_handler.cc:24: EventHandler::OnEvent(event); On 2015/11/20 17:48:26, varkha wrote: > On 2015/11/20 16:25:33, bruthig wrote: > > Don't we want the ToolbarActionView to handle the event before the > > ToolbarInkDropDelegate? > > This is what happens with this code (inside EventHandler::OnEvent). > The stack when ToolbarActionView gets the event is this: > View::OnGestureEvent() > CustomButton::OnGestureEvent() > MenuButton::OnGestureEvent() > ToolbarActionView::OnGestureEvent() <- is set as a |target_| > ScopedTargetHandler::OnGestureEvent > ** ToolbarInkDropDelegate::OnGestureEvent() > EventHandler::OnEvent() > ScopedTargetHandler::OnEvent() <- in an instance of ToolbarInkDropDelegate > > After ScopedTargetHandler::OnGestureEvent() returns in ** above > ToolbarInkDropDelegate gets to handle the ink drop animations. Ah yes you're right, man that is hard to follow. If we made the ScopedTargetHandler take in the EventHandler as well then it would enforce all InkDropDelegates to call the original event handlers first. Maybe a good thing? We would just have to make sure that plays nicely with the destruction detection. https://codereview.chromium.org/1411833006/diff/300001/ui/events/scoped_targe... ui/events/scoped_target_handler.cc:30: static_cast<EventHandler*>(target_)->OnKeyEvent(event); On 2015/11/20 17:48:26, varkha wrote: > On 2015/11/20 16:25:33, bruthig wrote: > > Are these casts necessary? EventTarget extends EventHandler. > > Yes they are even though yes it does. This is because the calls are on |target_| > (which is the View) and not on |this| (which is an instance of InkDropDelegate). I guess this begs the question, should the On*Event() methods really be protected in EventTarget? They are public in EventHandler AND in View.
bruthig@, PTAL. With what you suggested I could indeed hide more boilerplate code in the ScopedTargetHandler. https://codereview.chromium.org/1411833006/diff/300001/ui/events/scoped_targe... File ui/events/scoped_target_handler.cc (right): https://codereview.chromium.org/1411833006/diff/300001/ui/events/scoped_targe... ui/events/scoped_target_handler.cc:24: EventHandler::OnEvent(event); On 2015/11/20 18:56:27, bruthig wrote: > On 2015/11/20 17:48:26, varkha wrote: > > On 2015/11/20 16:25:33, bruthig wrote: > > > Don't we want the ToolbarActionView to handle the event before the > > > ToolbarInkDropDelegate? > > > > This is what happens with this code (inside EventHandler::OnEvent). > > The stack when ToolbarActionView gets the event is this: > > View::OnGestureEvent() > > CustomButton::OnGestureEvent() > > MenuButton::OnGestureEvent() > > ToolbarActionView::OnGestureEvent() <- is set as a |target_| > > ScopedTargetHandler::OnGestureEvent > > ** ToolbarInkDropDelegate::OnGestureEvent() > > EventHandler::OnEvent() > > ScopedTargetHandler::OnEvent() <- in an instance of ToolbarInkDropDelegate > > > > After ScopedTargetHandler::OnGestureEvent() returns in ** above > > ToolbarInkDropDelegate gets to handle the ink drop animations. > > Ah yes you're right, man that is hard to follow. > If we made the ScopedTargetHandler take in the EventHandler as well then it > would enforce all InkDropDelegates to call the original event handlers first. > Maybe a good thing? We would just have to make sure that plays nicely with the > destruction detection. See if this way (using "has" instead of "is") makes this chaining more understandable. https://codereview.chromium.org/1411833006/diff/300001/ui/events/scoped_targe... ui/events/scoped_target_handler.cc:30: static_cast<EventHandler*>(target_)->OnKeyEvent(event); On 2015/11/20 18:56:27, bruthig wrote: > On 2015/11/20 17:48:26, varkha wrote: > > On 2015/11/20 16:25:33, bruthig wrote: > > > Are these casts necessary? EventTarget extends EventHandler. > > > > Yes they are even though yes it does. This is because the calls are on > |target_| > > (which is the View) and not on |this| (which is an instance of > InkDropDelegate). > > I guess this begs the question, should the On*Event() methods really be > protected in EventTarget? They are public in EventHandler AND in View. I think this prevents _random_ code from doing something like aura::Window* window = GetSomeWindow(); window->OnMouseEvent(); In our case this is not a random code but generic event dispatching code so I hope this is OK. https://codereview.chromium.org/1411833006/diff/300001/ui/events/scoped_targe... File ui/events/scoped_target_handler.h (right): https://codereview.chromium.org/1411833006/diff/300001/ui/events/scoped_targe... ui/events/scoped_target_handler.h:20: ScopedTargetHandler(EventTarget* target); On 2015/11/20 16:25:33, bruthig wrote: > I think the ScopedTargetHandler should still accept an EventHandler. That way > it can be used by clients with a "has a" relationship instead of an "is a" one. > (And it would still work for "is a" relationships too) > Might be easiest to discuss offline. Done.
1 nit, otherwise LGTM https://codereview.chromium.org/1411833006/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1411833006/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:114: SetFillsBoundsOpaquely(false); nit: I think there should either be a mirror call to SetFillsBoundsOpaquely(true); in RemoveInkDropLayer() or this should be done in the constructor.
varkha@chromium.org changed reviewers: + tdanderson@chromium.org
+tdanderson for the question about matching calls to SetFillsBoundsOpaquely. https://codereview.chromium.org/1411833006/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1411833006/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:114: SetFillsBoundsOpaquely(false); On 2015/11/23 15:04:02, bruthig wrote: > nit: I think there should either be a mirror call to > SetFillsBoundsOpaquely(true); in RemoveInkDropLayer() or this should be done in > the constructor. Is same true for changes in https://codereview.chromium.org/1460733002?
https://codereview.chromium.org/1411833006/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1411833006/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:114: SetFillsBoundsOpaquely(false); On 2015/11/23 15:14:13, varkha wrote: > On 2015/11/23 15:04:02, bruthig wrote: > > nit: I think there should either be a mirror call to > > SetFillsBoundsOpaquely(true); in RemoveInkDropLayer() or this should be done > in > > the constructor. > > Is same true for changes in https://codereview.chromium.org/1460733002 AFICT the call is not needed since the separate layer disappears in the next call anyway.
https://codereview.chromium.org/1411833006/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1411833006/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:114: SetFillsBoundsOpaquely(false); On 2015/11/23 15:38:05, tdanderson wrote: > On 2015/11/23 15:14:13, varkha wrote: > > On 2015/11/23 15:04:02, bruthig wrote: > > > nit: I think there should either be a mirror call to > > > SetFillsBoundsOpaquely(true); in RemoveInkDropLayer() or this should be done > > in > > > the constructor. > > > > Is same true for changes in https://codereview.chromium.org/1460733002 > > AFICT the call is not needed since the separate layer disappears in the next > call anyway. Acknowledged.
varkha@chromium.org changed reviewers: + pkasting@chromium.org
+sadrul@ for design insights (and OWNERS in ui/) +pkasting for insights (and OWNERS in c/b/ui/views/) Thanks!
I don't know that I'm capable of responding to a general request for "insights"; if you have specific questions or concerns about something I can do my best to answer. https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:217: // center point (adjusted for RTL layouts) of preferred size which doesn't Nit: preferred size which -> the preferred size, which https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:156: MenuButton::OnGestureEvent(event); Nit: Shorter: // While the dropdown menu is showing, the button should not handle gestures. if (menu_) event->StopPropagation(); else MenuButton::OnGestureEvent(event); https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:361: menu_ = nullptr; I believe if we're in this arm, |this| may be null and thus this is unsafe (as well as unnecessary). https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.h (right): https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.h:87: // views::MenuButton: WHile here: Group these MenuButton overrides with the other one above (and move function ordering in the cc similarly) https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.h:92: // ToolbarActionViewDelegate: (public because called by others). While here: Change this base class name to ToolbarActionViewDelegateViews to match actual parent class name, reorder relative to above override groups to match the order in which this class declares its parents, and remove the unnecessary description of why these methods are public. https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.h:105: using ContextMenuCallback = base::Callback<void(ToolbarActionView*)>; While here: Move this typedef up by the member class declaration to match the style guide's prescribed order for declarations. https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.h:113: // views::InkDropHost: Place this above with the other override groups. https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.h:117: protected: Are there subclasses that actually call these? If not, make private instead. https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.h:121: // views::View: You don't directly subclass views::View. This should be listed as an override of the appropriate direct parent (I assume views::MenuButton) and then the groups of overrides for each parent ordered as noted above. https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.h (right): https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.h:76: // views::InkDropHost: Place up above, with other override. Also, as noted before, make private unless a subclass actually calls this.
Patchset #12 (id:380001) has been deleted
Patchset #12 (id:400001) has been deleted
Thanks and PTAL. The general concerns I had were whether we wanted this to be purely refactoring, introduction of the InkDropDelegate (if it's OK to actually include as an example adding ripples to extension buttons). As for the immediate plans once this lands I can make the rest of the ink ripple sites use those classes simplifying the code and then think of basing the animation transitions on button / view states rather than input events. https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:217: // center point (adjusted for RTL layouts) of preferred size which doesn't On 2015/11/24 20:42:00, Peter Kasting wrote: > Nit: preferred size which -> the preferred size, which Done. https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:156: MenuButton::OnGestureEvent(event); On 2015/11/24 20:42:00, Peter Kasting wrote: > Nit: Shorter: > > // While the dropdown menu is showing, the button should not handle gestures. > if (menu_) > event->StopPropagation(); > else > MenuButton::OnGestureEvent(event); Done. https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:361: menu_ = nullptr; On 2015/11/24 20:42:01, Peter Kasting wrote: > I believe if we're in this arm, |this| may be null and thus this is unsafe (as > well as unnecessary). Yes, I see now why it wasn't done when |menu_| was introduced. Thanks! https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.h (right): https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.h:87: // views::MenuButton: On 2015/11/24 20:42:01, Peter Kasting wrote: > WHile here: Group these MenuButton overrides with the other one above (and move > function ordering in the cc similarly) Done. https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.h:92: // ToolbarActionViewDelegate: (public because called by others). On 2015/11/24 20:42:01, Peter Kasting wrote: > While here: Change this base class name to ToolbarActionViewDelegateViews to > match actual parent class name, reorder relative to above override groups to > match the order in which this class declares its parents, and remove the > unnecessary description of why these methods are public. Done. https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.h:105: using ContextMenuCallback = base::Callback<void(ToolbarActionView*)>; On 2015/11/24 20:42:01, Peter Kasting wrote: > While here: Move this typedef up by the member class declaration to match the > style guide's prescribed order for declarations. Done. https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.h:113: // views::InkDropHost: On 2015/11/24 20:42:01, Peter Kasting wrote: > Place this above with the other override groups. Done. https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.h:117: protected: On 2015/11/24 20:42:01, Peter Kasting wrote: > Are there subclasses that actually call these? If not, make private instead. Done. https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.h:121: // views::View: On 2015/11/24 20:42:01, Peter Kasting wrote: > You don't directly subclass views::View. This should be listed as an override > of the appropriate direct parent (I assume views::MenuButton) and then the > groups of overrides for each parent ordered as noted above. Done. https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_button.h (right): https://codereview.chromium.org/1411833006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_button.h:76: // views::InkDropHost: On 2015/11/24 20:42:01, Peter Kasting wrote: > Place up above, with other override. > > Also, as noted before, make private unless a subclass actually calls this. Done.
I don't have an opinion on how to break what you want to do into a series of different patches (e.g. whether to just do refactoring in this patch); I trust your judgment. LGTM https://codereview.chromium.org/1411833006/diff/420001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1411833006/diff/420001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:232: // While dropdown menu is showing the button should not handle gestures. Nit: Add "the" before "dropdown" and comma after "showing" https://codereview.chromium.org/1411833006/diff/420001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.h (right): https://codereview.chromium.org/1411833006/diff/420001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.h:93: // ToolbarActionViewDelegateViews: Nit: This should be between the MenuButton and MenuButtonListener sections to match the order in which the class lists its parents.
https://codereview.chromium.org/1411833006/diff/420001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1411833006/diff/420001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:232: // While dropdown menu is showing the button should not handle gestures. On 2015/11/24 22:27:16, Peter Kasting wrote: > Nit: Add "the" before "dropdown" and comma after "showing" Done. https://codereview.chromium.org/1411833006/diff/420001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.h (right): https://codereview.chromium.org/1411833006/diff/420001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.h:93: // ToolbarActionViewDelegateViews: On 2015/11/24 22:27:16, Peter Kasting wrote: > Nit: This should be between the MenuButton and MenuButtonListener sections to > match the order in which the class lists its parents. Done.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411833006/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411833006/440001
After offline review with sadrul@ we have decided to move the invocations / ownership of InkDropDelegate to CustomButton for now while all affected concrete classes are CustomButtons. I will make those changes.
sadrul@, PTAL. I've kept the stub instance around in case you want to reconsider since it makes the ripple a bit easier to integrate.
https://codereview.chromium.org/1411833006/diff/460001/ui/aura/window.cc File ui/aura/window.cc (right): https://codereview.chromium.org/1411833006/diff/460001/ui/aura/window.cc#newc... ui/aura/window.cc:96: ignore_result(SetTargetHandler(delegate_)); We shouldn't need to use ignore_result(), since SetTargetHandler() doesn't WARN_UNUSED_RESULT? https://codereview.chromium.org/1411833006/diff/460001/ui/aura/window.cc#newc... ui/aura/window.cc:112: ignore_result(SetTargetHandler(nullptr)); ditto https://codereview.chromium.org/1411833006/diff/460001/ui/events/event_target.h File ui/events/event_target.h (right): https://codereview.chromium.org/1411833006/diff/460001/ui/events/event_target... ui/events/event_target.h:80: // Sets |target_handler| as |target_handler_| and returns its original value. '... and returns the old handler.' https://codereview.chromium.org/1411833006/diff/460001/ui/events/scoped_targe... File ui/events/scoped_target_handler.cc (right): https://codereview.chromium.org/1411833006/diff/460001/ui/events/scoped_targe... ui/events/scoped_target_handler.cc:40: } I think this should just be: if (original_handler_) original_handler_->OnEvent(event); if (!destroyed) new_handler_->OnEvent(event); And the various On*Event() overrides below should be NOTREACHED() https://codereview.chromium.org/1411833006/diff/460001/ui/events/scoped_targe... File ui/events/scoped_target_handler.h (right): https://codereview.chromium.org/1411833006/diff/460001/ui/events/scoped_targe... ui/events/scoped_target_handler.h:18: // EventHandler. Document the ordering (i.e. whether the new handler receives the event or the old one). Also document whether SetHandled()/StopPropagation()'ed on the event affects whether the event reaches both handlers or not. https://codereview.chromium.org/1411833006/diff/460001/ui/views/animation/ink... File ui/views/animation/ink_drop_delegate.h (right): https://codereview.chromium.org/1411833006/diff/460001/ui/views/animation/ink... ui/views/animation/ink_drop_delegate.h:30: int small_corner_radius) = 0; The layers are square-sized, and large_size and small_size refers to the length of one side? Can you clarify that in the comment? https://codereview.chromium.org/1411833006/diff/460001/ui/views/animation/too... File ui/views/animation/toolbar_ink_drop_delegate.h (right): https://codereview.chromium.org/1411833006/diff/460001/ui/views/animation/too... ui/views/animation/toolbar_ink_drop_delegate.h:25: class VIEWS_EXPORT ToolbarInkDropDelegate : public InkDropDelegate, I would call this ButtonInkDropDelegate https://codereview.chromium.org/1411833006/diff/460001/ui/views/controls/butt... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/1411833006/diff/460001/ui/views/controls/butt... ui/views/controls/button/custom_button.cc:49: }; Still not a fan of this stub. I think we should still null-check (I don't think there will be many places that does the nullcheck, so should be ok) https://codereview.chromium.org/1411833006/diff/460001/ui/views/controls/butt... ui/views/controls/button/custom_button.cc:312: Button::OnDragDone(); Is this still needed? https://codereview.chromium.org/1411833006/diff/460001/ui/views/controls/butt... File ui/views/controls/button/custom_button.h (right): https://codereview.chromium.org/1411833006/diff/460001/ui/views/controls/butt... ui/views/controls/button/custom_button.h:136: } non-overrides before overrides. So this should move up https://codereview.chromium.org/1411833006/diff/460001/ui/views/controls/butt... ui/views/controls/button/custom_button.h:145: scoped_ptr<InkDropDelegate> ink_drop_delegate_; Should be a private member. https://codereview.chromium.org/1411833006/diff/460001/ui/views/controls/butt... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/1411833006/diff/460001/ui/views/controls/butt... ui/views/controls/button/label_button.cc:338: CustomButton::Layout(); Is this still needed?
Patchset #15 (id:480001) has been deleted
PTAL. https://codereview.chromium.org/1411833006/diff/460001/ui/aura/window.cc File ui/aura/window.cc (right): https://codereview.chromium.org/1411833006/diff/460001/ui/aura/window.cc#newc... ui/aura/window.cc:96: ignore_result(SetTargetHandler(delegate_)); On 2015/11/25 22:38:58, sadrul wrote: > We shouldn't need to use ignore_result(), since SetTargetHandler() doesn't > WARN_UNUSED_RESULT? Done. https://codereview.chromium.org/1411833006/diff/460001/ui/aura/window.cc#newc... ui/aura/window.cc:112: ignore_result(SetTargetHandler(nullptr)); On 2015/11/25 22:38:58, sadrul wrote: > ditto Done. https://codereview.chromium.org/1411833006/diff/460001/ui/events/event_target.h File ui/events/event_target.h (right): https://codereview.chromium.org/1411833006/diff/460001/ui/events/event_target... ui/events/event_target.h:80: // Sets |target_handler| as |target_handler_| and returns its original value. On 2015/11/25 22:38:58, sadrul wrote: > '... and returns the old handler.' Done. https://codereview.chromium.org/1411833006/diff/460001/ui/events/scoped_targe... File ui/events/scoped_target_handler.cc (right): https://codereview.chromium.org/1411833006/diff/460001/ui/events/scoped_targe... ui/events/scoped_target_handler.cc:40: } On 2015/11/25 22:38:58, sadrul wrote: > I think this should just be: > > if (original_handler_) > original_handler_->OnEvent(event); > if (!destroyed) > new_handler_->OnEvent(event); > > And the various On*Event() overrides below should be NOTREACHED() I don't think so. |original_handler_| here is a delegate that may be set on the |target_| (which is our View) with SetTargetHandler(). First we need to call that delegate _or_ call the concrete View event handlers (which the EventHandler::OnEvent does for us). Only after that we need to call the InkDropDelegate which here is represented by |new_handler|. Calling only new_handler_->OnEvent() will result in a call into say, ButtonInkDropDelegate::OnGestureEvent() but will not call the original View's OnGestureEvent(). https://codereview.chromium.org/1411833006/diff/460001/ui/events/scoped_targe... File ui/events/scoped_target_handler.h (right): https://codereview.chromium.org/1411833006/diff/460001/ui/events/scoped_targe... ui/events/scoped_target_handler.h:18: // EventHandler. On 2015/11/25 22:38:58, sadrul wrote: > Document the ordering (i.e. whether the new handler receives the event or the > old one). Also document whether SetHandled()/StopPropagation()'ed on the event > affects whether the event reaches both handlers or not. Done. https://codereview.chromium.org/1411833006/diff/460001/ui/views/animation/ink... File ui/views/animation/ink_drop_delegate.h (right): https://codereview.chromium.org/1411833006/diff/460001/ui/views/animation/ink... ui/views/animation/ink_drop_delegate.h:30: int small_corner_radius) = 0; On 2015/11/25 22:38:58, sadrul wrote: > The layers are square-sized, and large_size and small_size refers to the length > of one side? Can you clarify that in the comment? Done. https://codereview.chromium.org/1411833006/diff/460001/ui/views/animation/too... File ui/views/animation/toolbar_ink_drop_delegate.h (right): https://codereview.chromium.org/1411833006/diff/460001/ui/views/animation/too... ui/views/animation/toolbar_ink_drop_delegate.h:25: class VIEWS_EXPORT ToolbarInkDropDelegate : public InkDropDelegate, On 2015/11/25 22:38:58, sadrul wrote: > I would call this ButtonInkDropDelegate Done. https://codereview.chromium.org/1411833006/diff/460001/ui/views/controls/butt... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/1411833006/diff/460001/ui/views/controls/butt... ui/views/controls/button/custom_button.cc:49: }; On 2015/11/25 22:38:58, sadrul wrote: > Still not a fan of this stub. I think we should still null-check (I don't think > there will be many places that does the nullcheck, so should be ok) Done. https://codereview.chromium.org/1411833006/diff/460001/ui/views/controls/butt... ui/views/controls/button/custom_button.cc:312: Button::OnDragDone(); On 2015/11/25 22:38:58, sadrul wrote: > Is this still needed? Done. https://codereview.chromium.org/1411833006/diff/460001/ui/views/controls/butt... File ui/views/controls/button/custom_button.h (right): https://codereview.chromium.org/1411833006/diff/460001/ui/views/controls/butt... ui/views/controls/button/custom_button.h:136: } On 2015/11/25 22:38:58, sadrul wrote: > non-overrides before overrides. So this should move up Done. https://codereview.chromium.org/1411833006/diff/460001/ui/views/controls/butt... ui/views/controls/button/custom_button.h:145: scoped_ptr<InkDropDelegate> ink_drop_delegate_; On 2015/11/25 22:38:58, sadrul wrote: > Should be a private member. Done. https://codereview.chromium.org/1411833006/diff/460001/ui/views/controls/butt... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/1411833006/diff/460001/ui/views/controls/butt... ui/views/controls/button/label_button.cc:338: CustomButton::Layout(); On 2015/11/25 22:38:58, sadrul wrote: > Is this still needed? No. Done.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411833006/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411833006/500001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) ios_rel_device_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/1411833006/diff/460001/ui/events/scoped_targe... File ui/events/scoped_target_handler.cc (right): https://codereview.chromium.org/1411833006/diff/460001/ui/events/scoped_targe... ui/events/scoped_target_handler.cc:40: } On 2015/11/26 00:05:46, varkha wrote: > On 2015/11/25 22:38:58, sadrul wrote: > > I think this should just be: > > > > if (original_handler_) > > original_handler_->OnEvent(event); > > if (!destroyed) > > new_handler_->OnEvent(event); > > > > And the various On*Event() overrides below should be NOTREACHED() > > I don't think so. |original_handler_| here is a delegate that may be set on the > |target_| (which is our View) with SetTargetHandler(). First we need to call > that delegate _or_ call the concrete View event handlers (which the > EventHandler::OnEvent does for us). Only after that we need to call the > InkDropDelegate which here is represented by |new_handler|. > Calling only new_handler_->OnEvent() will result in a call into say, > ButtonInkDropDelegate::OnGestureEvent() but will not call the original View's > OnGestureEvent(). Ah, I see what you mean. But sending event directly to the EventTarget from the On*Event() callbacks can send the event back to this ScopedTargetHandler, because it is the |target_handler_| for the target (since this is a generic and deals with EventTarget, we can't assume that it overrides all the On*Event() methods like View), right? And we should avoid that. Can this be more like this: if (original_handler_) original_handler_->OnEvent(event); else target_->OnEvent(event); new_handler_->OnEvent(event); and the various On*Event() overrides in ScopedTargetHandler() are no-ops, where they don't do anything (i.e. don't need to override them at all). Can there be a test for this ScopedTargetHandler?
https://codereview.chromium.org/1411833006/diff/500001/ui/views/controls/butt... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/1411833006/diff/500001/ui/views/controls/butt... ui/views/controls/button/custom_button.cc:385: if (ink_drop_delegate_) Correct me if I'm wrong but the intent here was to set the ink drop centre in the OnBoundsChange() event, at the request of sadrul@ right? And the CustomButton::Layout() method should be removed then? I tried removing CustomButton::Layout() and noticed that the ripple is incorrectly centered on the BackButton. The problem is that the BackButton->SetLeadingMargin() does not trigger an OnBoundsChange() event. And it just so happens that BackButton::SetBounds() is called before BackButton::SetLeadingMargin() in ToolbarView and thus the ripple will actually be in the wrong place every time.
sadrul@, PTAL. I will add a test for the ScopedTargetHandler, if possible in a separate CL since it is holding up Ben's rebase. https://codereview.chromium.org/1411833006/diff/460001/ui/events/scoped_targe... File ui/events/scoped_target_handler.cc (right): https://codereview.chromium.org/1411833006/diff/460001/ui/events/scoped_targe... ui/events/scoped_target_handler.cc:40: } On 2015/11/26 17:48:19, sadrul wrote: > On 2015/11/26 00:05:46, varkha wrote: > > On 2015/11/25 22:38:58, sadrul wrote: > > > I think this should just be: > > > > > > if (original_handler_) > > > original_handler_->OnEvent(event); > > > if (!destroyed) > > > new_handler_->OnEvent(event); > > > > > > And the various On*Event() overrides below should be NOTREACHED() > > > > I don't think so. |original_handler_| here is a delegate that may be set on > the > > |target_| (which is our View) with SetTargetHandler(). First we need to call > > that delegate _or_ call the concrete View event handlers (which the > > EventHandler::OnEvent does for us). Only after that we need to call the > > InkDropDelegate which here is represented by |new_handler|. > > Calling only new_handler_->OnEvent() will result in a call into say, > > ButtonInkDropDelegate::OnGestureEvent() but will not call the original View's > > OnGestureEvent(). > > Ah, I see what you mean. But sending event directly to the EventTarget from the > On*Event() callbacks can send the event back to this ScopedTargetHandler, > because it is the |target_handler_| for the target (since this is a generic and > deals with EventTarget, we can't assume that it overrides all the On*Event() > methods like View), right? And we should avoid that. Can this be more like this: > > if (original_handler_) > original_handler_->OnEvent(event); > else > target_->OnEvent(event); > new_handler_->OnEvent(event); > > and the various On*Event() overrides in ScopedTargetHandler() are no-ops, where > they don't do anything (i.e. don't need to override them at all). > > Can there be a test for this ScopedTargetHandler? As we talked offline - passing in a View instead of TargetHandler should avoid the infinite loop in case we are sending in an EventTarget that is not a View. Using target_->OnEvent(event) would not work because it will trigger that infinite recursion right there calling into ScopedTargetHandler::OnEvent again and so on. https://codereview.chromium.org/1411833006/diff/500001/ui/views/controls/butt... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/1411833006/diff/500001/ui/views/controls/butt... ui/views/controls/button/custom_button.cc:385: if (ink_drop_delegate_) On 2015/11/26 21:27:33, bruthig wrote: > Correct me if I'm wrong but the intent here was to set the ink drop centre in > the OnBoundsChange() event, at the request of sadrul@ right? And the > CustomButton::Layout() method should be removed then? > > I tried removing CustomButton::Layout() and noticed that the ripple is > incorrectly centered on the BackButton. The problem is that the > BackButton->SetLeadingMargin() does not trigger an OnBoundsChange() event. And > it just so happens that BackButton::SetBounds() is called before > BackButton::SetLeadingMargin() in ToolbarView and thus the ripple will actually > be in the wrong place every time. I have restored override of Layout(). Can you see if this works?
Patchset #16 (id:520001) has been deleted
Patchset #16 (id:540001) has been deleted
> Can there be a test for this ScopedTargetHandler? Done.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411833006/560001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411833006/560001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #16 (id:560001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411833006/580001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411833006/580001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Mostly nits. lgtm with these addressed. https://codereview.chromium.org/1411833006/diff/580001/ui/views/animation/but... File ui/views/animation/button_ink_drop_delegate.cc (right): https://codereview.chromium.org/1411833006/diff/580001/ui/views/animation/but... ui/views/animation/button_ink_drop_delegate.cc:47: // ui::ScopedTargetHandler: ui::EventHandler, right? https://codereview.chromium.org/1411833006/diff/580001/ui/views/controls/butt... File ui/views/controls/button/custom_button.h (right): https://codereview.chromium.org/1411833006/diff/580001/ui/views/controls/butt... ui/views/controls/button/custom_button.h:129: void set_ink_drop_delegate(InkDropDelegate* ink_drop_delegate); SetInkDropDelegate(scoped_ptr<...>) to clarify the ownership transfer with the call. https://codereview.chromium.org/1411833006/diff/580001/ui/views/scoped_target... File ui/views/scoped_target_handler.cc (right): https://codereview.chromium.org/1411833006/diff/580001/ui/views/scoped_target... ui/views/scoped_target_handler.cc:34: EventHandler::OnEvent(event); I think this can now be like I suggested before: if (original_handler_) original_handler_->OnEvent(event); else view_->OnEvent(event); if (!destroyed) new_handler_->OnEvent(event); and the On*Event() overrides can be NOTREACHED(). https://codereview.chromium.org/1411833006/diff/580001/ui/views/scoped_target... ui/views/scoped_target_handler.cc:44: static_cast<EventHandler*>(view_)->OnKeyEvent(event); We shouldn't need to static_cast<> https://codereview.chromium.org/1411833006/diff/580001/ui/views/scoped_target... File ui/views/scoped_target_handler.h (right): https://codereview.chromium.org/1411833006/diff/580001/ui/views/scoped_target... ui/views/scoped_target_handler.h:6: #define UI_EVENTS_SCOPED_TARGET_HANDLER_H_ update after the move
Will upload a patch shortly with nits addressed. Responses below. https://codereview.chromium.org/1411833006/diff/580001/ui/views/animation/but... File ui/views/animation/button_ink_drop_delegate.cc (right): https://codereview.chromium.org/1411833006/diff/580001/ui/views/animation/but... ui/views/animation/button_ink_drop_delegate.cc:47: // ui::ScopedTargetHandler: On 2015/11/27 05:59:35, sadrul wrote: > ui::EventHandler, right? Done. https://codereview.chromium.org/1411833006/diff/580001/ui/views/controls/butt... File ui/views/controls/button/custom_button.h (right): https://codereview.chromium.org/1411833006/diff/580001/ui/views/controls/butt... ui/views/controls/button/custom_button.h:129: void set_ink_drop_delegate(InkDropDelegate* ink_drop_delegate); On 2015/11/27 05:59:35, sadrul wrote: > SetInkDropDelegate(scoped_ptr<...>) > > to clarify the ownership transfer with the call. Done. https://codereview.chromium.org/1411833006/diff/580001/ui/views/scoped_target... File ui/views/scoped_target_handler.cc (right): https://codereview.chromium.org/1411833006/diff/580001/ui/views/scoped_target... ui/views/scoped_target_handler.cc:34: EventHandler::OnEvent(event); On 2015/11/27 05:59:36, sadrul wrote: > I think this can now be like I suggested before: > > if (original_handler_) > original_handler_->OnEvent(event); > else > view_->OnEvent(event); > if (!destroyed) > new_handler_->OnEvent(event); > > and the On*Event() overrides can be NOTREACHED(). No, this doesn't work since view_->OnEvent will call into EventTarget::OnEvent which returns control to ScopedTargetHandler::OnEvent and loops indefinitely. I have also confirmed that this is caught by the unit test which crashes with call stack overrun. https://codereview.chromium.org/1411833006/diff/580001/ui/views/scoped_target... ui/views/scoped_target_handler.cc:44: static_cast<EventHandler*>(view_)->OnKeyEvent(event); On 2015/11/27 05:59:36, sadrul wrote: > We shouldn't need to static_cast<> OnXxxxEvent are protected in View and |view_| is not |this| so compiler correctly insists on cast even though ScopedTargetHandler subclasses EventHandler. I thought exposing those methods as public would be worse. https://codereview.chromium.org/1411833006/diff/580001/ui/views/scoped_target... File ui/views/scoped_target_handler.h (right): https://codereview.chromium.org/1411833006/diff/580001/ui/views/scoped_target... ui/views/scoped_target_handler.h:6: #define UI_EVENTS_SCOPED_TARGET_HANDLER_H_ On 2015/11/27 05:59:36, sadrul wrote: > update after the move Done.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411833006/600001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411833006/600001
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 varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org, pkasting@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1411833006/#ps600001 (title: "Refactor ink drop animations (nits in ui/views/")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411833006/600001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411833006/600001
Message was sent while issue was closed.
Description was changed from ========== Refactor ink drop animations to allow adding them to any View and not just Button descendants. This CL is upstreamed to https://codereview.chromium.org/1409183003/ Looking for a way to allow organizing event handling necessary for ink drop animations. See goals description here: https://docs.google.com/document/d/1U4AdR4wZVsZ8g0y95EblY5YiYVbVXcBTJ59XJBF14... BUG=537202 ========== to ========== Refactor ink drop animations to allow adding them to any View and not just Button descendants. This CL is upstreamed to https://codereview.chromium.org/1409183003/ Looking for a way to allow organizing event handling necessary for ink drop animations. See goals description here: https://docs.google.com/document/d/1U4AdR4wZVsZ8g0y95EblY5YiYVbVXcBTJ59XJBF14... BUG=537202 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:600001)
Message was sent while issue was closed.
Description was changed from ========== Refactor ink drop animations to allow adding them to any View and not just Button descendants. This CL is upstreamed to https://codereview.chromium.org/1409183003/ Looking for a way to allow organizing event handling necessary for ink drop animations. See goals description here: https://docs.google.com/document/d/1U4AdR4wZVsZ8g0y95EblY5YiYVbVXcBTJ59XJBF14... BUG=537202 ========== to ========== Refactor ink drop animations to allow adding them to any View and not just Button descendants. This CL is upstreamed to https://codereview.chromium.org/1409183003/ Looking for a way to allow organizing event handling necessary for ink drop animations. See goals description here: https://docs.google.com/document/d/1U4AdR4wZVsZ8g0y95EblY5YiYVbVXcBTJ59XJBF14... BUG=537202 Committed: https://crrev.com/bcd9c28cd2858c07ef2b0b5107e1c5523cf84645 Cr-Commit-Position: refs/heads/master@{#362013} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/bcd9c28cd2858c07ef2b0b5107e1c5523cf84645 Cr-Commit-Position: refs/heads/master@{#362013}
Message was sent while issue was closed.
Description was changed from ========== Refactor ink drop animations to allow adding them to any View and not just Button descendants. This CL is upstreamed to https://codereview.chromium.org/1409183003/ Looking for a way to allow organizing event handling necessary for ink drop animations. See goals description here: https://docs.google.com/document/d/1U4AdR4wZVsZ8g0y95EblY5YiYVbVXcBTJ59XJBF14... BUG=537202 Committed: https://crrev.com/bcd9c28cd2858c07ef2b0b5107e1c5523cf84645 Cr-Commit-Position: refs/heads/master@{#362013} ========== to ========== Refactor ink drop animations to allow adding them to any CustomButton descendants. Looking for a way to allow organizing event handling necessary for ink drop animations. See goals description here: https://docs.google.com/document/d/1U4AdR4wZVsZ8g0y95EblY5YiYVbVXcBTJ59XJBF14... BUG=537202 Committed: https://crrev.com/bcd9c28cd2858c07ef2b0b5107e1c5523cf84645 Cr-Commit-Position: refs/heads/master@{#362013} ========== |
