Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2178163002:
Add ink drop ripple to shelf overflow button (Closed)
|
Created:
4 years, 5 months ago by mohsen Modified:
4 years, 3 months ago CC:
bruthig+ink_drop_chromium.org, chromium-reviews, dcheng, kalyank, sadrul, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ink drop ripple to shelf overflow button
It uses a flood fill ripple that remains active while the overflow shelf
is active. InkDropHostView and CustomButton are updated to handle active
case:
- CustomButton does not hide the ripple if it is not a pending ripple;
- InkDropHostView does not show or hide a pending ripple if it is
showing an active ripple.
BUG=612579
TEST=OverflowButtonInkDropTest.* in ash_unittests, manual
Committed: https://crrev.com/623c7e4e56e93b450090d0dbd68e4d9221a49a18
Cr-Commit-Position: refs/heads/master@{#419232}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed review comments #
Total comments: 2
Patch Set 3 : Rebased + Added tests #Patch Set 4 : Used scoped task runner #Patch Set 5 : Fixed OnClickCanceled #Patch Set 6 : Rebased #Patch Set 7 : Fixed a failing test #Patch Set 8 : Fixed CustomButton's OnClickCanceled test #Patch Set 9 : Fixed scoped task runner #
Total comments: 12
Patch Set 10 : Addressed review comments #Patch Set 11 : Created separate fixtures for overflow button tests + Cleanup #
Total comments: 2
Patch Set 12 : Rebased #Patch Set 13 : Fixed OnClickCanceled #
Total comments: 8
Patch Set 14 : Addressed review comments #
Total comments: 35
Patch Set 15 : Addressed review comments #
Total comments: 4
Messages
Total messages: 85 (65 generated)
The CQ bit was checked by mohsen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mohsen@chromium.org changed reviewers: + bruthig@chromium.org
Can you take a first look, please?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2178163002/diff/1/ash/common/shelf/overflow_b... File ash/common/shelf/overflow_button.cc (right): https://codereview.chromium.org/2178163002/diff/1/ash/common/shelf/overflow_b... ash/common/shelf/overflow_button.cc:62: set_hide_ink_drop_when_showing_context_menu(false); Does this need to be toggled or can it just be initialized to false? https://codereview.chromium.org/2178163002/diff/1/ui/views/animation/ink_drop... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2178163002/diff/1/ui/views/animation/ink_drop... ui/views/animation/ink_drop_host_view.cc:61: if (current_ink_drop_state == InkDropState::ACTIVATED) Can you double check this logic still works well with the BookmarkButtons? e.g. 1. Open a folder button 2. Tap and hold for a delay. I expect the ripple to be visible 3. If you release the tap after a long enough delay the menu is re-shown and the ripple transitions to ACTIVATED. https://codereview.chromium.org/2178163002/diff/1/ui/views/controls/button/cu... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2178163002/diff/1/ui/views/controls/button/cu... ui/views/controls/button/custom_button.cc:200: if (state_ == STATE_PRESSED && notify_action_ == NOTIFY_ON_RELEASE) What's the use case for this? AFAICT this will never evaluate to true because the previous block will either set the state to STATE_NORMAL, STATE_HOVERED, or state_ == STATE_DISABLED.
The CQ bit was checked by mohsen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2178163002/diff/1/ash/common/shelf/overflow_b... File ash/common/shelf/overflow_button.cc (right): https://codereview.chromium.org/2178163002/diff/1/ash/common/shelf/overflow_b... ash/common/shelf/overflow_button.cc:62: set_hide_ink_drop_when_showing_context_menu(false); On 2016/07/26 at 13:57:15, bruthig wrote: > Does this need to be toggled or can it just be initialized to false? My OverflowButton::ShouldEnterPushedState() was wrong, which I fixed it. Now this can just be initialized to false. Thanks for pointing this out! https://codereview.chromium.org/2178163002/diff/1/ui/views/animation/ink_drop... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2178163002/diff/1/ui/views/animation/ink_drop... ui/views/animation/ink_drop_host_view.cc:61: if (current_ink_drop_state == InkDropState::ACTIVATED) On 2016/07/26 at 13:57:15, bruthig wrote: > Can you double check this logic still works well with the BookmarkButtons? e.g. > 1. Open a folder button > 2. Tap and hold for a delay. I expect the ripple to be visible > 3. If you release the tap after a long enough delay the menu is re-shown and the ripple transitions to ACTIVATED. This seems to work. The activated ripple is hidden before the tap-down reaches here, so the early out does not happen. https://codereview.chromium.org/2178163002/diff/1/ui/views/controls/button/cu... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2178163002/diff/1/ui/views/controls/button/cu... ui/views/controls/button/custom_button.cc:200: if (state_ == STATE_PRESSED && notify_action_ == NOTIFY_ON_RELEASE) On 2016/07/26 at 13:57:16, bruthig wrote: > What's the use case for this? AFAICT this will never evaluate to true because the previous block will either set the state to STATE_NORMAL, STATE_HOVERED, or state_ == STATE_DISABLED. Sorry! This is totally wrong! The use case is that when the user presses on the button when it is active, no pending ink drop is shown, so when click is cancelled, ink drop should not get hidden. Here I wanted not to call OnClickCanceled() when button is not in the pressed state (button is not pressed, so there is nothing to cancel). But, that's not that easy and it is better to be fixed separately. For now, I just changed the code to hide the ripple only if it is not in activated state. (see CustomButton::OnClickCanceled())
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2178163002/diff/20001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2178163002/diff/20001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:473: if (ink_drop()->GetTargetInkDropState() != views::InkDropState::ACTIVATED) { I think I'd feel better if this were configurable on the InkDropHostView. i.e. InkDropHostView::should_hide_ripple_on_click_cancel_. WDYT?
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by mohsen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mohsen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by mohsen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mohsen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mohsen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mohsen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2178163002/diff/20001/ui/views/controls/butto... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2178163002/diff/20001/ui/views/controls/butto... ui/views/controls/button/custom_button.cc:473: if (ink_drop()->GetTargetInkDropState() != views::InkDropState::ACTIVATED) { On 2016/07/26 at 21:24:27, bruthig (OOO-not avail 14-22) wrote: > I think I'd feel better if this were configurable on the InkDropHostView. i.e. InkDropHostView::should_hide_ripple_on_click_cancel_. > > WDYT? I changed the code such that OnClickCanceled() is not called at all if button is not in the pressed state. So, this is not needed anymore.
https://codereview.chromium.org/2178163002/diff/180001/ash/shelf/shelf_view_u... File ash/shelf/shelf_view_unittest.cc (right): https://codereview.chromium.org/2178163002/diff/180001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2162: mock_task_runner_->ClearPendingTasks(); It might be a good idea to DCHECK_EQ(mock_task_runner_, base::MessageLoop::current()->task_runner()) here to make sure we don't have multiple clients who aren't playing nice. https://codereview.chromium.org/2178163002/diff/180001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2650: EXPECT_FALSE(test_api_->overflow_bubble() && I know you are just copying this style of expectation from other tests but it is not a great format because the it makes it impossible to know which condition is actually false in the output (see example below). I think it would be better to re-write these as: ASSERT_TRUE(test_api_->overflow_bubble()); EXPECT_TRUE(test_api_->overflow_bubble()->IsShowing()); Example expectation: EXPECT_TRUE(foo && bar); Example Log: Value of: foo && bar Actual: false Expected: true https://codereview.chromium.org/2178163002/diff/180001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2666: EXPECT_THAT(overflow_button_ink_drop_->GetAndResetRequestedStates(), I really like how these expectations work with the matchers!! https://codereview.chromium.org/2178163002/diff/180001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:3128: views::InkDropState::HIDDEN)); I kind of expect the ALTERNATE_ACTION_PENDING to be followed by a ALTERNATE_ACTION_TRIGGERED here when showing the context menu. How much of a change would that be? It might be difficult to do, and not worthwhile, because it should behave nicely when showing the context menu when the button is already activated. https://codereview.chromium.org/2178163002/diff/180001/ui/views/animation/ink... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2178163002/diff/180001/ui/views/animation/ink... ui/views/animation/ink_drop_host_view.cc:61: if (current_ink_drop_state == InkDropState::ACTIVATED) These might benefit from some test cases if they are easy to write (I wouldn't spend more than an hour writing them). Unfortunately the one for the ET_GESTURE_END+ACTIVATED is missing...
The CQ bit was checked by mohsen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by mohsen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by mohsen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mohsen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2178163002/diff/180001/ash/shelf/shelf_view_u... File ash/shelf/shelf_view_unittest.cc (right): https://codereview.chromium.org/2178163002/diff/180001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2162: mock_task_runner_->ClearPendingTasks(); On 2016/08/25 at 19:18:35, bruthig (OOO Aug 29th) wrote: > It might be a good idea to DCHECK_EQ(mock_task_runner_, base::MessageLoop::current()->task_runner()) here to make sure we don't have multiple clients who aren't playing nice. Done. https://codereview.chromium.org/2178163002/diff/180001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2650: EXPECT_FALSE(test_api_->overflow_bubble() && On 2016/08/25 at 19:18:35, bruthig (OOO Aug 29th) wrote: > I know you are just copying this style of expectation from other tests but it is not a great format because the it makes it impossible to know which condition is actually false in the output (see example below). I think it would be better to re-write these as: > > ASSERT_TRUE(test_api_->overflow_bubble()); > EXPECT_TRUE(test_api_->overflow_bubble()->IsShowing()); > > Example expectation: > > EXPECT_TRUE(foo && bar); > > Example Log: > > Value of: foo && bar > Actual: false > Expected: true I updated this for EXPECT_TRUE(foo && bar) cases, but I don't think we can do that for EXPECT_FALSE(foo && bar) (similarly, EXPECT_TRUE(foo || bar)). https://codereview.chromium.org/2178163002/diff/180001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:3128: views::InkDropState::HIDDEN)); On 2016/08/25 at 19:18:35, bruthig (OOO Aug 29th) wrote: > I kind of expect the ALTERNATE_ACTION_PENDING to be followed by a ALTERNATE_ACTION_TRIGGERED here when showing the context menu. How much of a change would that be? > > It might be difficult to do, and not worthwhile, because it should behave nicely when showing the context menu when the button is already activated. Currently, ALTERNATE_ACTION_PENDING is used on a long-press and ALTERNATE_ACTION_TRIGGERED is used on the following long-tap (i.e., when the user lifts the finger from the screen). It kind of makes sense to me. Here, the context menu interrupts the long-tap action, so it makes sense to me that no ALTERNATE_ACTION_TRIGGERED is seen. WDYT? https://codereview.chromium.org/2178163002/diff/180001/ui/views/animation/ink... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2178163002/diff/180001/ui/views/animation/ink... ui/views/animation/ink_drop_host_view.cc:61: if (current_ink_drop_state == InkDropState::ACTIVATED) On 2016/08/25 at 19:18:35, bruthig (OOO Aug 29th) wrote: > These might benefit from some test cases if they are easy to write (I wouldn't spend more than an hour writing them). Unfortunately the one for the ET_GESTURE_END+ACTIVATED is missing... I believe these are already tested in gesture tests for the overflow button. If you mean, adding some tests for the InkDropHostView itself, I don't see any tests for other gestures for the InkDropHostView, either (at least in ink_drop_host_view_unittest.cc).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Feel free to add other OWNER's. https://codereview.chromium.org/2178163002/diff/180001/ash/shelf/shelf_view_u... File ash/shelf/shelf_view_unittest.cc (right): https://codereview.chromium.org/2178163002/diff/180001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2650: EXPECT_FALSE(test_api_->overflow_bubble() && On 2016/08/30 18:03:32, mohsen wrote: > On 2016/08/25 at 19:18:35, bruthig (OOO Aug 29th) wrote: > > I know you are just copying this style of expectation from other tests but it > is not a great format because the it makes it impossible to know which condition > is actually false in the output (see example below). I think it would be better > to re-write these as: > > > > ASSERT_TRUE(test_api_->overflow_bubble()); > > EXPECT_TRUE(test_api_->overflow_bubble()->IsShowing()); > > > > Example expectation: > > > > EXPECT_TRUE(foo && bar); > > > > Example Log: > > > > Value of: foo && bar > > Actual: false > > Expected: true > > I updated this for EXPECT_TRUE(foo && bar) cases, but I don't think we can do > that for EXPECT_FALSE(foo && bar) (similarly, EXPECT_TRUE(foo || bar)). Acknowledged. https://codereview.chromium.org/2178163002/diff/180001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:3128: views::InkDropState::HIDDEN)); On 2016/08/30 18:03:32, mohsen wrote: > On 2016/08/25 at 19:18:35, bruthig (OOO Aug 29th) wrote: > > I kind of expect the ALTERNATE_ACTION_PENDING to be followed by a > ALTERNATE_ACTION_TRIGGERED here when showing the context menu. How much of a > change would that be? > > > > It might be difficult to do, and not worthwhile, because it should behave > nicely when showing the context menu when the button is already activated. > > Currently, ALTERNATE_ACTION_PENDING is used on a long-press and > ALTERNATE_ACTION_TRIGGERED is used on the following long-tap (i.e., when the > user lifts the finger from the screen). It kind of makes sense to me. Here, the > context menu interrupts the long-tap action, so it makes sense to me that no > ALTERNATE_ACTION_TRIGGERED is seen. WDYT? Seems reasonable, if we wanted it to perform an ALTERNATE_ACTION_TRIGGERED we would have to update CustomButton::ShowContextMenu() and that would be too much of a change to include here. https://codereview.chromium.org/2178163002/diff/180001/ui/views/animation/ink... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2178163002/diff/180001/ui/views/animation/ink... ui/views/animation/ink_drop_host_view.cc:61: if (current_ink_drop_state == InkDropState::ACTIVATED) On 2016/08/30 18:03:32, mohsen wrote: > On 2016/08/25 at 19:18:35, bruthig (OOO Aug 29th) wrote: > > These might benefit from some test cases if they are easy to write (I wouldn't > spend more than an hour writing them). Unfortunately the one for the > ET_GESTURE_END+ACTIVATED is missing... > > I believe these are already tested in gesture tests for the overflow button. If > you mean, adding some tests for the InkDropHostView itself, I don't see any > tests for other gestures for the InkDropHostView, either (at least in > ink_drop_host_view_unittest.cc). Yeah, I meant in ink_drop_host_view_unittest.cc. But you are correct, no gesture tests exist currently. Forget I said anything ;) https://codereview.chromium.org/2178163002/diff/220001/ui/views/controls/butt... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2178163002/diff/220001/ui/views/controls/butt... ui/views/controls/button/custom_button.cc:202: if (notify_action_ == NOTIFY_ON_RELEASE && was_pressed) Correct me if I'm wrong but you said that OnClickCanceled() can sometimes be called when the mouse is released with no corresponding mouse press correct? If so, can you just add a comment here stating that. https://codereview.chromium.org/2178163002/diff/260001/ash/shelf/shelf_view_u... File ash/shelf/shelf_view_unittest.cc (right): https://codereview.chromium.org/2178163002/diff/260001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2134: if (!ash_test_helper()->test_shell_delegate()) { When does a ShellDelegate already exist? https://codereview.chromium.org/2178163002/diff/260001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2671: void InitOverflowButtonInkDrop() { Does this need to be protected? https://codereview.chromium.org/2178163002/diff/260001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2929: mock_task_runner.FastForwardUntilNoTasksRemain(); This causes a context menu to be shown? https://codereview.chromium.org/2178163002/diff/260001/ash/test/shelf_view_te... File ash/test/shelf_view_test_api.cc (right): https://codereview.chromium.org/2178163002/diff/260001/ash/test/shelf_view_te... ash/test/shelf_view_test_api.cc:76: if (shelf_view_->IsShowingOverflowBubble()) Can you make this a DCHECK instead of an if? It is better to be explicit, especially in tests.
The CQ bit was checked by mohsen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2178163002/diff/220001/ui/views/controls/butt... File ui/views/controls/button/custom_button.cc (right): https://codereview.chromium.org/2178163002/diff/220001/ui/views/controls/butt... ui/views/controls/button/custom_button.cc:202: if (notify_action_ == NOTIFY_ON_RELEASE && was_pressed) On 2016/09/01 at 16:12:54, bruthig wrote: > Correct me if I'm wrong but you said that OnClickCanceled() can sometimes be called when the mouse is released with no corresponding mouse press correct? If so, can you just add a comment here stating that. Not exactly. Probably, what I meant was that OnClickCanceled() can be called even if mouse pressed did not change the button state to PRESSED (e.g. when the button is disabled). This can be deduced from the comments above OnClickCanceled() declaration ("Called when a button gets released without triggering an action"). https://codereview.chromium.org/2178163002/diff/260001/ash/shelf/shelf_view_u... File ash/shelf/shelf_view_unittest.cc (right): https://codereview.chromium.org/2178163002/diff/260001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2134: if (!ash_test_helper()->test_shell_delegate()) { On 2016/09/01 at 16:12:54, bruthig wrote: > When does a ShellDelegate already exist? When it is set by a subclass (e.g. OverflowButtonInkDropTest) before calling this SetUp() function. https://codereview.chromium.org/2178163002/diff/260001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2671: void InitOverflowButtonInkDrop() { On 2016/09/01 at 16:12:54, bruthig wrote: > Does this need to be protected? Nope. Moved to private section. https://codereview.chromium.org/2178163002/diff/260001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2929: mock_task_runner.FastForwardUntilNoTasksRemain(); On 2016/09/01 at 16:12:54, bruthig wrote: > This causes a context menu to be shown? Eventually. This causes long-press timer to expire which will bring up the context menu. https://codereview.chromium.org/2178163002/diff/260001/ash/test/shelf_view_te... File ash/test/shelf_view_test_api.cc (right): https://codereview.chromium.org/2178163002/diff/260001/ash/test/shelf_view_te... ash/test/shelf_view_test_api.cc:76: if (shelf_view_->IsShowingOverflowBubble()) On 2016/09/01 at 16:12:54, bruthig wrote: > Can you make this a DCHECK instead of an if? It is better to be explicit, especially in tests. Done. Also, for the ShowOverflowBubble().
mohsen@chromium.org changed reviewers: + jamescook@chromium.org
jamescook@: Can you take a look at changes in ash/ please?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please add a TEST= line to the CL description with at least the test binary that includes the tests (ash_unittests) and the test suite names. Also, it would be nice if the CL description talked a little more about the changes you made in //ui https://codereview.chromium.org/2178163002/diff/280001/ash/common/shelf/overf... File ash/common/shelf/overflow_bubble.h (right): https://codereview.chromium.org/2178163002/diff/280001/ash/common/shelf/overf... ash/common/shelf/overflow_bubble.h:59: OverflowButton* button_; // Owned by ShelfView. nit: |button| is too generic. In particular, it's not immediately obvious whether it is a button inside this view or in the parent shelf view. How about |overflow_button_| or |anchor_button_| or similar? https://codereview.chromium.org/2178163002/diff/280001/ash/common/shelf/overf... File ash/common/shelf/overflow_button.cc (right): https://codereview.chromium.org/2178163002/diff/280001/ash/common/shelf/overf... ash/common/shelf/overflow_button.cc:86: return base::WrapUnique(new views::FloodFillInkDropRipple( nit: IWYU #include ptr_util.h https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... File ash/shelf/shelf_view_unittest.cc (right): https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2135: if (!ash_test_helper()->test_shell_delegate()) { This is odd. I see from the discussion that a subclass might set a delegate. It might be better to have a virtual SetShellDelegate() or CreateShellDelegate() function that the subclass can override, or have the subclass just not call this base method (and call ShelfViewTest directly). Either way, the function needs a comment. https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2667: EXPECT_FALSE(test_api_->overflow_bubble() && this is odd. I think you should ASSERT_TRUE(test_api_->overflow_bubble()) since you expect it to exist, then EXPECT_FALSE() is showing. https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2686: void InitOverflowButtonInkDrop() { nit: I think it would be clearer to just inline this in SetUp() above, especially since it initializes a member variable. https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2691: overflow_button_ink_drop_ = new InkDropSpy(base::WrapUnique(ink_drop_impl)); nit: either base::MakeUnique<>, or inline the call to new inside WrapUnique(). Otherwise I have to think about ownership transfers. :-) https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2711: EXPECT_FALSE(test_api_->overflow_bubble() && I don't think you need the part before the &&. If it's important, ASSERT it separately. Otherwise skip it. https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2766: EXPECT_FALSE(test_api_->overflow_bubble() && ditto https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2789: generator.MoveMouseTo(GetScreenPointInsideOverflowButton()); btw, I like how you used GetScreenPointInside and Outside. It's easy to read. https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2823: EXPECT_FALSE(test_api_->overflow_bubble() && ditto https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2828: // There is no ink drop effect for gesture events on Windows. nit: move comment above #if https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2876: EXPECT_FALSE(test_api_->overflow_bubble() && ditto https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2910: EXPECT_FALSE(test_api_->overflow_bubble() && ditto and below https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2921: RunAllPendingInMessageLoop(); super nit: Move this above { so the first line has the ScopedFoo, which makes it super obvious why you opened a new scope. https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:3082: // There is no ink drop effect for gesture events on Windows. ditto https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:3131: ASSERT_TRUE(test_api_->overflow_bubble()); having seen many of these lines, it might be easier to read if you just left them out. If the test crashes it'll be pretty obvious which pointer is null. https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:3202: #endif // !defined(OS_WIN) Very thorough tests.
Description was changed from ========== Add ink drop ripple to shelf overflow button It is a flood fill ripple that remains active while the overflow is active. BUG=612579 ========== to ========== Add ink drop ripple to shelf overflow button It is a flood fill ripple that remains active while the overflow is active. BUG=612579 TEST=OverflowButtonInkDropTest.* in ash_unittests, manual ==========
Description was changed from ========== Add ink drop ripple to shelf overflow button It is a flood fill ripple that remains active while the overflow is active. BUG=612579 TEST=OverflowButtonInkDropTest.* in ash_unittests, manual ========== to ========== Add ink drop ripple to shelf overflow button It uses a flood fill ripple that remains active while the overflow shelf is active. InkDropHostView and CustomButton are updated to handle active case: - CustomButton does not hide the ripple if it is not a pending ripple; - InkDropHostView does not show or hide a pending ripple if it is showing an active ripple. BUG=612579 TEST=OverflowButtonInkDropTest.* in ash_unittests, manual ==========
Patchset #15 (id:300001) has been deleted
https://codereview.chromium.org/2178163002/diff/280001/ash/common/shelf/overf... File ash/common/shelf/overflow_bubble.h (right): https://codereview.chromium.org/2178163002/diff/280001/ash/common/shelf/overf... ash/common/shelf/overflow_bubble.h:59: OverflowButton* button_; // Owned by ShelfView. On 2016/09/12 at 02:54:47, James Cook wrote: > nit: |button| is too generic. In particular, it's not immediately obvious whether it is a button inside this view or in the parent shelf view. How about |overflow_button_| or |anchor_button_| or similar? Done. https://codereview.chromium.org/2178163002/diff/280001/ash/common/shelf/overf... File ash/common/shelf/overflow_button.cc (right): https://codereview.chromium.org/2178163002/diff/280001/ash/common/shelf/overf... ash/common/shelf/overflow_button.cc:86: return base::WrapUnique(new views::FloodFillInkDropRipple( On 2016/09/12 at 02:54:47, James Cook wrote: > nit: IWYU #include ptr_util.h Done. Also, replaced with base::MakeUnique(). https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... File ash/shelf/shelf_view_unittest.cc (right): https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2135: if (!ash_test_helper()->test_shell_delegate()) { On 2016/09/12 at 02:54:47, James Cook wrote: > This is odd. I see from the discussion that a subclass might set a delegate. It might be better to have a virtual SetShellDelegate() or CreateShellDelegate() function that the subclass can override, or have the subclass just not call this base method (and call ShelfViewTest directly). Either way, the function needs a comment. Done. https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2667: EXPECT_FALSE(test_api_->overflow_bubble() && On 2016/09/12 at 02:54:47, James Cook wrote: > this is odd. I think you should ASSERT_TRUE(test_api_->overflow_bubble()) since you expect it to exist, then EXPECT_FALSE() is showing. I think overflow_bubble is not necessarily existing here. The ShelfView itself uses a similar expression to check if a bubble is showing or not. Maybe, I should just add an IsShowingOverflowBubble() to the test api that delegates to the one defined in the ShelfView itself. Done. https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2686: void InitOverflowButtonInkDrop() { On 2016/09/12 at 02:54:47, James Cook wrote: > nit: I think it would be clearer to just inline this in SetUp() above, especially since it initializes a member variable. Done. https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2691: overflow_button_ink_drop_ = new InkDropSpy(base::WrapUnique(ink_drop_impl)); On 2016/09/12 at 02:54:47, James Cook wrote: > nit: either base::MakeUnique<>, or inline the call to new inside WrapUnique(). Otherwise I have to think about ownership transfers. :-) Not sure what you exactly mean. I updated these lines to make ownerships more clear (also in two other similar places). Does that address your concerns? https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2711: EXPECT_FALSE(test_api_->overflow_bubble() && On 2016/09/12 at 02:54:47, James Cook wrote: > I don't think you need the part before the &&. If it's important, ASSERT it separately. Otherwise skip it. Fixed. https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2766: EXPECT_FALSE(test_api_->overflow_bubble() && On 2016/09/12 at 02:54:47, James Cook wrote: > ditto Fixed. https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2789: generator.MoveMouseTo(GetScreenPointInsideOverflowButton()); On 2016/09/12 at 02:54:47, James Cook wrote: > btw, I like how you used GetScreenPointInside and Outside. It's easy to read. Thanks :-) https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2823: EXPECT_FALSE(test_api_->overflow_bubble() && On 2016/09/12 at 02:54:47, James Cook wrote: > ditto Fixed. https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2828: // There is no ink drop effect for gesture events on Windows. On 2016/09/12 at 02:54:47, James Cook wrote: > nit: move comment above #if Done. Also in other similar places in this file. https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2876: EXPECT_FALSE(test_api_->overflow_bubble() && On 2016/09/12 at 02:54:47, James Cook wrote: > ditto Fixed. https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2910: EXPECT_FALSE(test_api_->overflow_bubble() && On 2016/09/12 at 02:54:47, James Cook wrote: > ditto and below Fixed. https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2921: RunAllPendingInMessageLoop(); On 2016/09/12 at 02:54:47, James Cook wrote: > super nit: Move this above { so the first line has the ScopedFoo, which makes it super obvious why you opened a new scope. Right. Done. https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:3082: // There is no ink drop effect for gesture events on Windows. On 2016/09/12 at 02:54:47, James Cook wrote: > ditto Done. https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:3131: ASSERT_TRUE(test_api_->overflow_bubble()); On 2016/09/12 at 02:54:47, James Cook wrote: > having seen many of these lines, it might be easier to read if you just left them out. If the test crashes it'll be pretty obvious which pointer is null. Fixed. https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:3202: #endif // !defined(OS_WIN) On 2016/09/12 at 02:54:47, James Cook wrote: > Very thorough tests. Thanks :-)
The CQ bit was checked by mohsen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
//ash LGTM https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... File ash/shelf/shelf_view_unittest.cc (right): https://codereview.chromium.org/2178163002/diff/280001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2691: overflow_button_ink_drop_ = new InkDropSpy(base::WrapUnique(ink_drop_impl)); On 2016/09/14 18:34:43, mohsen wrote: > On 2016/09/12 at 02:54:47, James Cook wrote: > > nit: either base::MakeUnique<>, or inline the call to new inside WrapUnique(). > Otherwise I have to think about ownership transfers. :-) > > Not sure what you exactly mean. I updated these lines to make ownerships more > clear (also in two other similar places). Does that address your concerns? Yes. MakeUnique<Foo> makes me happy. WrapUnique(new Foo) with the "new" inside the () also makes me happy. "new" on a separate line makes me think too much. :-) https://codereview.chromium.org/2178163002/diff/320001/ash/shelf/shelf_view_u... File ash/shelf/shelf_view_unittest.cc (right): https://codereview.chromium.org/2178163002/diff/320001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:2137: // returned object. Nice docs. https://codereview.chromium.org/2178163002/diff/320001/ash/test/shelf_view_te... File ash/test/shelf_view_test_api.h (right): https://codereview.chromium.org/2178163002/diff/320001/ash/test/shelf_view_te... ash/test/shelf_view_test_api.h:69: bool IsShowingOverflowBubble() const; Nice, makes tests easier to read.
mohsen@chromium.org changed reviewers: + sadrul@chromium.org
sadrul@: Can you please take a look at changes in ui/views/?
https://codereview.chromium.org/2178163002/diff/320001/ui/views/animation/ink... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2178163002/diff/320001/ui/views/animation/ink... ui/views/animation/ink_drop_host_view.cc:82: break; This is getting fairly complex. Can you look into adding test coverage in ink_drop_host_view_unittest.cc for this? (ok if you want to do that in a follow up CL)
lgtm
https://codereview.chromium.org/2178163002/diff/320001/ui/views/animation/ink... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2178163002/diff/320001/ui/views/animation/ink... ui/views/animation/ink_drop_host_view.cc:82: break; On 2016/09/15 at 20:42:00, sadrul wrote: > This is getting fairly complex. Can you look into adding test coverage in ink_drop_host_view_unittest.cc for this? (ok if you want to do that in a follow up CL) Sure. Filed https://crbug.com/647729
The CQ bit was checked by mohsen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #15 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Add ink drop ripple to shelf overflow button It uses a flood fill ripple that remains active while the overflow shelf is active. InkDropHostView and CustomButton are updated to handle active case: - CustomButton does not hide the ripple if it is not a pending ripple; - InkDropHostView does not show or hide a pending ripple if it is showing an active ripple. BUG=612579 TEST=OverflowButtonInkDropTest.* in ash_unittests, manual ========== to ========== Add ink drop ripple to shelf overflow button It uses a flood fill ripple that remains active while the overflow shelf is active. InkDropHostView and CustomButton are updated to handle active case: - CustomButton does not hide the ripple if it is not a pending ripple; - InkDropHostView does not show or hide a pending ripple if it is showing an active ripple. BUG=612579 TEST=OverflowButtonInkDropTest.* in ash_unittests, manual Committed: https://crrev.com/623c7e4e56e93b450090d0dbd68e4d9221a49a18 Cr-Commit-Position: refs/heads/master@{#419232} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/623c7e4e56e93b450090d0dbd68e4d9221a49a18 Cr-Commit-Position: refs/heads/master@{#419232} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
