Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1396903003:
Added the ripple effect animation to find bar buttons. (Closed)
DescriptionAdded the ripple effect animation to find bar buttons.
BUG=520682
Committed: https://crrev.com/19649dc5691070dde6cde203bd36e4b2a6bbb243
Cr-Commit-Position: refs/heads/master@{#353829}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed comments from patch set 1. #
Total comments: 9
Patch Set 3 : Addressed the feedback from patch set 2. #
Total comments: 2
Patch Set 4 : Addressed feedback from patch set 3. #Messages
Total messages: 17 (4 generated)
moshayedi@chromium.org changed reviewers: + bruthig@chromium.org
https://codereview.chromium.org/1396903003/diff/1/chrome/browser/ui/views/fin... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/1396903003/diff/1/chrome/browser/ui/views/fin... chrome/browser/ui/views/find_bar_view.cc:147: // views::Button actions are only triggered by left and middle mouse clicks. Can you double check that the middle mouse click activates the button? If not we may need to update this comment, and fix the original comment in toolbar_button.cc if it's not accurate. https://codereview.chromium.org/1396903003/diff/1/chrome/browser/ui/views/fin... chrome/browser/ui/views/find_bar_view.cc:188: ink_drop_animation_controller_->AnimateToState(views::InkDropState::HIDDEN); Can you update this to check the event location similar to what I've done here: https://codereview.chromium.org/1373983002/diff/120001/chrome/browser/ui/view...
https://codereview.chromium.org/1396903003/diff/1/chrome/browser/ui/views/fin... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/1396903003/diff/1/chrome/browser/ui/views/fin... chrome/browser/ui/views/find_bar_view.cc:147: // views::Button actions are only triggered by left and middle mouse clicks. On 2015/10/09 16:01:14, bruthig wrote: > Can you double check that the middle mouse click activates the button? If not > we may need to update this comment, and fix the original comment in > toolbar_button.cc if it's not accurate. Done. CustomButton::OnMousePressed() and CustomButton::OnMouseReleased() use IsTriggerableEvent() to decide if the listener should be notified or not, so I used the same function here and removed the comment. https://codereview.chromium.org/1396903003/diff/1/chrome/browser/ui/views/fin... chrome/browser/ui/views/find_bar_view.cc:188: ink_drop_animation_controller_->AnimateToState(views::InkDropState::HIDDEN); On 2015/10/09 16:01:14, bruthig wrote: > Can you update this to check the event location similar to what I've done here: > https://codereview.chromium.org/1373983002/diff/120001/chrome/browser/ui/view... Done.
lgtm
moshayedi@chromium.org changed reviewers: + pkasting@chromium.org
pkasting@ can you please provide OWNERS review for this CL?
https://codereview.chromium.org/1396903003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/1396903003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:105: FindBarButton(views::ButtonListener* listener) Do not inline any of these methods. Define these out of line just as you would if the class declaration was in a header file. https://codereview.chromium.org/1396903003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:156: ImageButton::OnGestureEvent(event); Why is it that for mouse pressed we do our ink drop handling first and then call the superclass, but for these next functions it's the opposite? This seems inconsistent. https://codereview.chromium.org/1396903003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:161: ink_drop_state = views::InkDropState::ACTION_PENDING; It kinda surprises me we haven't needed a mapping like this in other places already and created a common function for it. I wonder if it would make sense to do some sort of array of pairs of (source, target) enum values and just loop over them. Maybe that wouldn't be shorter. https://codereview.chromium.org/1396903003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:201: }; DISALLOW_COPY_AND_ASSIGN?
https://codereview.chromium.org/1396903003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/1396903003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:105: FindBarButton(views::ButtonListener* listener) On 2015/10/09 22:09:30, Peter Kasting wrote: > Do not inline any of these methods. Define these out of line just as you would > if the class declaration was in a header file. Done. https://codereview.chromium.org/1396903003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:156: ImageButton::OnGestureEvent(event); On 2015/10/09 22:09:30, Peter Kasting wrote: > Why is it that for mouse pressed we do our ink drop handling first and then call > the superclass, but for these next functions it's the opposite? This seems > inconsistent. Done. https://codereview.chromium.org/1396903003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:161: ink_drop_state = views::InkDropState::ACTION_PENDING; On 2015/10/09 22:09:30, Peter Kasting wrote: > It kinda surprises me we haven't needed a mapping like this in other places > already and created a common function for it. > > I wonder if it would make sense to do some sort of array of pairs of (source, > target) enum values and just loop over them. Maybe that wouldn't be shorter. bruthig is going to comment on this. https://codereview.chromium.org/1396903003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:201: }; On 2015/10/09 22:09:30, Peter Kasting wrote: > DISALLOW_COPY_AND_ASSIGN? Done.
https://codereview.chromium.org/1396903003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/1396903003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:161: ink_drop_state = views::InkDropState::ACTION_PENDING; On 2015/10/09 22:09:30, Peter Kasting wrote: > It kinda surprises me we haven't needed a mapping like this in other places > already and created a common function for it. > > I wonder if it would make sense to do some sort of array of pairs of (source, > target) enum values and just loop over them. Maybe that wouldn't be shorter. Ideally we would be able to capture this behavior in a common function somewhere, however it is not as common as one might think. After taking a look at some of the different surfaces that the ripple would be applied to it became clear that each surface had very unique state transitions and we couldn't capture this 'common' behavior in a PreTargetHandler. (A doc outlining this can be found here: https://docs.google.com/document/d/1U4AdR4wZVsZ8g0y95EblY5YiYVbVXcBTJ59XJBF147M). So we decided to apply it directly to each of the surfaces for now. My plan is to clean it up once a good pattern emerges. Having said that, I am more than open to suggestions for possible solutions.
LGTM https://codereview.chromium.org/1396903003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/1396903003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:221: ImageButton::OnGestureEvent(event); You addressed my previous "ordering inconsistency" feedback by making this superclass call in this function last. But that makes things even more inconsistent because now these first two functions call the superclass method last and the next two call it first. I could see an argument that we want press and release to mirror each other in terms of ordering (so one first and the other last), but it seems like at least gesture and click should match each other, and those should probably both match press. Maybe release should match the other three too.
https://codereview.chromium.org/1396903003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/1396903003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:221: ImageButton::OnGestureEvent(event); On 2015/10/13 19:09:23, Peter Kasting wrote: > You addressed my previous "ordering inconsistency" feedback by making this > superclass call in this function last. But that makes things even more > inconsistent because now these first two functions call the superclass method > last and the next two call it first. > > I could see an argument that we want press and release to mirror each other in > terms of ordering (so one first and the other last), but it seems like at least > gesture and click should match each other, and those should probably both match > press. Maybe release should match the other three too. Sorry for not addressing the previous feedback properly. I changed the code to call the super class last in all four functions.
The CQ bit was checked by moshayedi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1396903003/#ps60001 (title: "Addressed feedback from patch set 3.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1396903003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1396903003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/19649dc5691070dde6cde203bd36e4b2a6bbb243 Cr-Commit-Position: refs/heads/master@{#353829} |
