Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2961313003:
Touch gestures for System Tray/ IME/ Stylus/ Notifications (Closed)
DescriptionTouch gestures for System Tray/ IME/ Stylus/ Notifications
Swiping up on the System Tray/ IME/ Stylus/ Notifications buttons in
status area should open the associated bubble.
Swiping down on the opened bubble should close the associated bubble.
Changes:
1.Added the tray_drag_controller to extract the logic of dragging behavior from system_tray.
2.Added help functions in tray_background_view to get the state of associated tray bubble.
3.Added one interface ProcessGestureEventForBubble in TrayBubbleView::Delegate
to help process the dragging that happened on the tray bubble.
BUG=735994, 735996
Review-Url: https://codereview.chromium.org/2961313003
Cr-Commit-Position: refs/heads/master@{#487573}
Committed: https://chromium.googlesource.com/chromium/src/+/d8633937d1ea5d357a182247ae8e977966d6a67e
Patch Set 1 #
Total comments: 8
Patch Set 2 : Create SystemTrayBubbleView instead of Delegate. #
Total comments: 13
Patch Set 3 : Add |is_on_bubble_| instead of. #
Total comments: 12
Patch Set 4 : Fixed comments. #
Total comments: 1
Patch Set 5 : Swiping IME/Stylues/System tray/Notifications tray/bubble. #
Total comments: 83
Patch Set 6 : Swiping IME/Stylues/System tray/Notifications tray/bubble. #Patch Set 7 : Fixed msw's comments. #Patch Set 8 : . #Patch Set 9 : Fixed UT. #
Total comments: 27
Patch Set 10 : Fixed msw's comments. #
Total comments: 5
Patch Set 11 : Fixed msw's comments. #
Total comments: 6
Patch Set 12 : Fixed msw's comments. #
Total comments: 47
Patch Set 13 : Fixed tdanderson's comments. #
Total comments: 4
Patch Set 14 : Fixed nits. #Messages
Total messages: 104 (74 generated)
The CQ bit was checked by minch@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...
Description was changed from ========== Swiping down on the opened system tray bubble to close it. BUG=735994 ========== to ========== Swiping down on the opened system tray bubble to close it. The opened system tray bubble can be dragged everywhere except the slider (will do the logic of slider in a follow-on cl). BUG=735994 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
minch@chromium.org changed reviewers: + sammiequon@chromium.org, xdai@chromium.org
hi,xdai@,sammiequon@, could you help review first? Thanks.
would it make more sense to create a impl of SystemTrayBubbleView and override
OnGestureEvent there instead of using the delegate?
ie.
system_tray_bubble.h
views::SystemTrayBubbleView* bubble_view_;
system_tray_bubble.cc
class SystemTrayBubbleView : views::TrayBubbleView {
void OnGestureEvent(...) override;
}
https://codereview.chromium.org/2961313003/diff/1/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):
https://codereview.chromium.org/2961313003/diff/1/ash/system/tray/system_tray...
ash/system/tray/system_tray.cc:764: return gesture_drag_amount_ <
system_tray_bubble_bounds_.height() / 3.0;
nit:
if (target_view == this)
return ...;
return ...;
https://codereview.chromium.org/2961313003/diff/1/ash/system/tray/system_tray...
File ash/system/tray/system_tray_unittest.cc (right):
https://codereview.chromium.org/2961313003/diff/1/ash/system/tray/system_tray...
ash/system/tray/system_tray_unittest.cc:72: bool is_on_bubble) {
what happens when |is_on_bubble| is true but |start| is not actually on the
bubble? Would it make more sense to compute |is_on_bubble| locally?
i.e.
const bool is_on_bubble = system_tray->GetBoundsInScreen().Contains(start);
https://codereview.chromium.org/2961313003/diff/1/ash/system/tray/system_tray...
ash/system/tray/system_tray_unittest.cc:75: SendScrollStartAndUpdate(start,
delta, begin_scroll_y_hint, timestamp,
AFAICT, |velocity_y| tells the direction as well? Can we instead
SendScrollStartAndUpdate(start, delta, velocity_y > 0.f, ...) ? ignore if this
is not |velocity_y| is for.
The CQ bit was checked by minch@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.
@sammiequon, thanks for your comments. Create SystemTrayBubbleView instead of delegate. Since, only systemtray use this currently and we don't need to change the code under views/ https://codereview.chromium.org/2961313003/diff/1/ash/system/tray/system_tray.cc File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2961313003/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.cc:764: return gesture_drag_amount_ < system_tray_bubble_bounds_.height() / 3.0; On 2017/06/29 21:28:19, sammiequon wrote: > nit: > > if (target_view == this) > return ...; > > return ...; Done. https://codereview.chromium.org/2961313003/diff/1/ash/system/tray/system_tray... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2961313003/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray_unittest.cc:72: bool is_on_bubble) { On 2017/06/29 21:28:19, sammiequon wrote: > what happens when |is_on_bubble| is true but |start| is not actually on the > bubble? Would it make more sense to compute |is_on_bubble| locally? > i.e. > > const bool is_on_bubble = system_tray->GetBoundsInScreen().Contains(start); Since |start| can be point on the system tray, elsewhere except system tray during dragging(i.e, desktop) and on the system tray bubble. And it is a point in the view's own coordinate. I think it is not convenience here to convert it to the screen's coordinate and compare with the bounds of the system tray(we need to know the specific view that start is on). https://codereview.chromium.org/2961313003/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray_unittest.cc:75: SendScrollStartAndUpdate(start, delta, begin_scroll_y_hint, timestamp, On 2017/06/29 21:28:19, sammiequon wrote: > AFAICT, |velocity_y| tells the direction as well? Can we instead > SendScrollStartAndUpdate(start, delta, velocity_y > 0.f, ...) ? ignore if this > is not |velocity_y| is for. I think the |velocity_y| is only for fling event? And, here GestureEventDetails for SCROLL_START has no velocity_y too.
lgtm once xdai@ is happy with it https://codereview.chromium.org/2961313003/diff/1/ash/system/tray/system_tray... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2961313003/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray_unittest.cc:72: bool is_on_bubble) { On 2017/06/30 17:02:30, minch1 wrote: > On 2017/06/29 21:28:19, sammiequon wrote: > > what happens when |is_on_bubble| is true but |start| is not actually on the > > bubble? Would it make more sense to compute |is_on_bubble| locally? > > i.e. > > > > const bool is_on_bubble = system_tray->GetBoundsInScreen().Contains(start); > > Since |start| can be point on the system tray, elsewhere except system tray > during dragging(i.e, desktop) and on the system tray bubble. And it is a point > in the view's own coordinate. I think it is not convenience here to convert it > to the screen's coordinate and compare with the bounds of the system tray(we > need to know the specific view that start is on). Acknowledged. https://codereview.chromium.org/2961313003/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray_unittest.cc:75: SendScrollStartAndUpdate(start, delta, begin_scroll_y_hint, timestamp, On 2017/06/30 17:02:30, minch1 wrote: > On 2017/06/29 21:28:19, sammiequon wrote: > > AFAICT, |velocity_y| tells the direction as well? Can we instead > > SendScrollStartAndUpdate(start, delta, velocity_y > 0.f, ...) ? ignore if this > > is not |velocity_y| is for. > > I think the |velocity_y| is only for fling event? And, here GestureEventDetails > for SCROLL_START has no velocity_y too. Acknowledged. https://codereview.chromium.org/2961313003/diff/20001/ash/system/tray/system_... File ash/system/tray/system_tray_bubble.cc (right): https://codereview.chromium.org/2961313003/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray_bubble.cc:92: class SystemTrayBubbleView : public TrayBubbleView { I feel it makes more sense to move the bubble related gesture events in here. That way the bubble handles gestures on the bubble and the tray handles gestures on the tray, instead of using set_target_view() and passing the gesture event to the tray view. But that should be work for another CL, if you (or someone else more familiar with this code) agree. https://codereview.chromium.org/2961313003/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray_bubble.cc:98: void OnGestureEvent(ui::GestureEvent* event) override { nit: // views::View: https://codereview.chromium.org/2961313003/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray_bubble.cc:109: SystemTray* tray_; nit: // Not owned. https://codereview.chromium.org/2961313003/diff/20001/ash/system/tray/system_... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2961313003/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:67: // based on the view's own cooidinates. nit: coordinates
https://codereview.chromium.org/2961313003/diff/20001/ash/system/tray/system_... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2961313003/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray.cc:630: target_view_ = this; What if you press and drag on a bubble view and then use another finger to press or drag on the system tray? Does it cause problem? https://codereview.chromium.org/2961313003/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray.cc:728: void SystemTray::SetBubbleBounds(const gfx::Point& location) { From the function's name, it may be better to make it accept a (const gfx::Rect& bounds_in_screen) parameter? and move the screen coordinates conversion logic to the upper function? If so, is this possible that we eliminate the necessarily of maintaining |target_view_| and only make it a parameter of SystemTray::ProcessGestureEvent()?
https://codereview.chromium.org/2961313003/diff/20001/ash/system/tray/system_... File ash/system/tray/system_tray_bubble.cc (right): https://codereview.chromium.org/2961313003/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray_bubble.cc:92: class SystemTrayBubbleView : public TrayBubbleView { On 2017/06/30 17:46:02, sammiequon wrote: > I feel it makes more sense to move the bubble related gesture events in here. > That way the bubble handles gestures on the bubble and the tray handles gestures > on the tray, instead of using set_target_view() and passing the gesture event to > the tray view. But that should be work for another CL, if you (or someone else > more familiar with this code) agree. I have the same feeling too. But I also understand that minch@ wants to reuse codes in the current implementation. Thus I will defer the decision to the owner reviewer.
The CQ bit was checked by minch@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/2961313003/diff/20001/ash/system/tray/system_... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2961313003/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray.cc:630: target_view_ = this; On 2017/06/30 17:46:54, xdai1 wrote: > What if you press and drag on a bubble view and then use another finger to press > or drag on the system tray? Does it cause problem? Add |is_on_bubble_| instead, please check. https://codereview.chromium.org/2961313003/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray.cc:728: void SystemTray::SetBubbleBounds(const gfx::Point& location) { On 2017/06/30 17:46:54, xdai1 wrote: > From the function's name, it may be better to make it accept a (const gfx::Rect& > bounds_in_screen) parameter? and move the screen coordinates conversion logic to > the upper function? If so, is this possible that we eliminate the necessarily of > maintaining |target_view_| and only make it a parameter of > SystemTray::ProcessGestureEvent()? Sorry, I found after change the rule for SetBubbleBounds actually we don't need to argument |location|. But we still need to know which view the dragging is triggered on. Add a flag |is_on_bubble_| and always set it in ProcessGestureEvent. Please check. Thanks. https://codereview.chromium.org/2961313003/diff/20001/ash/system/tray/system_... File ash/system/tray/system_tray_bubble.cc (right): https://codereview.chromium.org/2961313003/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray_bubble.cc:92: class SystemTrayBubbleView : public TrayBubbleView { On 2017/06/30 17:51:01, xdai1 wrote: > On 2017/06/30 17:46:02, sammiequon wrote: > > I feel it makes more sense to move the bubble related gesture events in here. > > That way the bubble handles gestures on the bubble and the tray handles > gestures > > on the tray, instead of using set_target_view() and passing the gesture event > to > > the tray view. But that should be work for another CL, if you (or someone else > > more familiar with this code) agree. > > I have the same feeling too. But I also understand that minch@ wants to reuse > codes in the current implementation. Thus I will defer the decision to the owner > reviewer. Let's keep it as-is to let owner take a look. Thanks. https://codereview.chromium.org/2961313003/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray_bubble.cc:98: void OnGestureEvent(ui::GestureEvent* event) override { On 2017/06/30 17:46:02, sammiequon wrote: > nit: // views::View: Done. https://codereview.chromium.org/2961313003/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray_bubble.cc:109: SystemTray* tray_; On 2017/06/30 17:46:02, sammiequon wrote: > nit: // Not owned. Done. https://codereview.chromium.org/2961313003/diff/20001/ash/system/tray/system_... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2961313003/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:67: // based on the view's own cooidinates. On 2017/06/30 17:46:02, sammiequon wrote: > nit: coordinates Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
defer the decision if we should let SystemTrayBubbleView and SystemTray handle their own gesture events to the owner reviewer. elsewhere lgtm https://codereview.chromium.org/2961313003/diff/40001/ash/system/tray/system_... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2961313003/diff/40001/ash/system/tray/system_... ash/system/tray/system_tray.cc:628: if (ProcessGestureEvent(*event, false)) nit: if(ProcessGestureEvent(*event, false /* is_on_bubble */)) https://codereview.chromium.org/2961313003/diff/40001/ash/system/tray/system_... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2961313003/diff/40001/ash/system/tray/system_... ash/system/tray/system_tray.h:157: // be processed any further, false otherwise. Note, the events may triggered nit: may be triggered https://codereview.chromium.org/2961313003/diff/40001/ash/system/tray/system_... ash/system/tray/system_tray.h:217: void SetBubbleBounds(); I think UpdateBubbleBounds() might better describe the function now. https://codereview.chromium.org/2961313003/diff/40001/ash/system/tray/system_... ash/system/tray/system_tray.h:242: bool is_on_bubble_ = false; considering rename it to is_dragging_bubble_? https://codereview.chromium.org/2961313003/diff/40001/ash/system/tray/system_... File ash/system/tray/system_tray_bubble.cc (right): https://codereview.chromium.org/2961313003/diff/40001/ash/system/tray/system_... ash/system/tray/system_tray_bubble.cc:98: // views::View nit: // views::View: https://codereview.chromium.org/2961313003/diff/40001/ash/system/tray/system_... ash/system/tray/system_tray_bubble.cc:100: if (tray_ && tray_->ProcessGestureEvent(*event, true)) nit: tray_->ProcessGestureEvent(*event, true /* is_on_bubble */)
The CQ bit was checked by minch@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...
minch@chromium.org changed reviewers: + tdanderson@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hi, tdanderson@, could you help review? Thanks. https://codereview.chromium.org/2961313003/diff/40001/ash/system/tray/system_... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2961313003/diff/40001/ash/system/tray/system_... ash/system/tray/system_tray.cc:628: if (ProcessGestureEvent(*event, false)) On 2017/06/30 22:27:08, xdai1 wrote: > nit: if(ProcessGestureEvent(*event, false /* is_on_bubble */)) Done. https://codereview.chromium.org/2961313003/diff/40001/ash/system/tray/system_... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2961313003/diff/40001/ash/system/tray/system_... ash/system/tray/system_tray.h:157: // be processed any further, false otherwise. Note, the events may triggered On 2017/06/30 22:27:08, xdai1 wrote: > nit: may be triggered Done. https://codereview.chromium.org/2961313003/diff/40001/ash/system/tray/system_... ash/system/tray/system_tray.h:217: void SetBubbleBounds(); On 2017/06/30 22:27:08, xdai1 wrote: > I think UpdateBubbleBounds() might better describe the function now. Done. https://codereview.chromium.org/2961313003/diff/40001/ash/system/tray/system_... ash/system/tray/system_tray.h:242: bool is_on_bubble_ = false; On 2017/06/30 22:27:08, xdai1 wrote: > considering rename it to is_dragging_bubble_? There is another flag |is_in_drag_| for dragging state. This one is used to tell which view the events is triggered on. I think |is_on_bubble_| should be enough. Let me keep it first. https://codereview.chromium.org/2961313003/diff/40001/ash/system/tray/system_... File ash/system/tray/system_tray_bubble.cc (right): https://codereview.chromium.org/2961313003/diff/40001/ash/system/tray/system_... ash/system/tray/system_tray_bubble.cc:98: // views::View On 2017/06/30 22:27:08, xdai1 wrote: > nit: // views::View: Done. https://codereview.chromium.org/2961313003/diff/40001/ash/system/tray/system_... ash/system/tray/system_tray_bubble.cc:100: if (tray_ && tray_->ProcessGestureEvent(*event, true)) On 2017/06/30 22:27:08, xdai1 wrote: > nit: tray_->ProcessGestureEvent(*event, true /* is_on_bubble */) Done.
https://codereview.chromium.org/2961313003/diff/60001/ash/system/tray/system_... File ash/system/tray/system_tray_bubble.cc (right): https://codereview.chromium.org/2961313003/diff/60001/ash/system/tray/system_... ash/system/tray/system_tray_bubble.cc:92: class SystemTrayBubbleView : public TrayBubbleView { I agree with earlier comments regarding the event-handling logic here; to me it seems out of place that you are performing event-handling logic in SystemTray for the bubble itself. Please correct me if I am mistaken, but as I understand the specs, you would like to do all of the following (is all of this work targeted for M-61?): (1) Swiping up on the following tray surfaces should open the corresponding bubble: - system menu - notifications - palette - IME menu (2) Swiping up anywhere else on the shelf will show the launcher. (3) Swiping down on an open bubble will close the bubble: - system menu - notifications - palette - IME menu Now might be the best time to take a step back and put together a small design doc for what you want the final implementation to look like. Here are some thoughts that may help: * Regarding (3), note that all of the surfaces listed are already subclasses of TrayBubbleView. Instead of creating SystemTrayBubbleView, consider overriding OnGestureEvent() in the TrayBubbleView; that will be a common entry point for gesture events among all four bubble types. * Regarding (1), note that all of the surfaces listed are subclasses of TrayBackgroundView and will all need to have the same swipe-up logic you currently have in SystemTray. Consider overriding OnGestureEvent() in TrayBackgroundView instead. Note that there are some TrayBackgroundView subclasses which have currently-undefined behavior for swipe-up (see https://bugs.chromium.org/p/chromium/issues/detail?id=725977#c4). You (or your TL) should reach out to PM/UX to find out what should happen for swipe-up on all surfaces; for example, should swipe-up on the overview mode button launch overview mode? * Then, to share code, an idea off the top of my head: introduce a new class (for lack of a better name let's call it TrayDragController) that contains all of the drag logic, current drag amount, etc. TrayBubbleView and TrayBackgroundView could each have an instance of TrayDragController, then call into TrayDragController from TrayBubbleView::OnGestureEvent() and TrayBackgroundView::OnGestureEvent(). I am happy to discuss and iterate on this design with you. IMO this will ultimately make it much easier for you to accomplish (1)-(3) for M-61 if that is your goal. But again please let me know if I am mistaken on the scope of what you are trying to accomplish.
The CQ bit was checked by minch@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-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_...)
Description was changed from ========== Swiping down on the opened system tray bubble to close it. The opened system tray bubble can be dragged everywhere except the slider (will do the logic of slider in a follow-on cl). BUG=735994 ========== to ========== Swiping up on the System Tray/ IME/ Stylus/ Notifications buttons in status area should open the bubble. Swiping up on the opened bubble should close the bubble. BUG=735994,735996 ==========
minch@chromium.org changed reviewers: + msw@chromium.org
msw@chromium.org: Please review changes in
The CQ bit was checked by minch@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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
On 2017/07/11 20:56:20, minch1 wrote: > mailto:msw@chromium.org: Please review changes in Sorry, let me check the problem of the trybot first.
The CQ bit was checked by minch@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.
Mostly comments encouraging cleanup and clarification. https://codereview.chromium.org/2961313003/diff/80001/ash/system/ime_menu/ime... File ash/system/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.cc:474: return bubble_.get() != NULL; nit: this should be moot, but use nullptr instead of NULL. https://codereview.chromium.org/2961313003/diff/80001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.cc:486: if (HasBubble()) nit: return bubble_ ? bubble_->bubble_view() : nullptr; (ditto elsewhere for the other impls) https://codereview.chromium.org/2961313003/diff/80001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.cc:494: event->SetHandled(); Please make DragController::ProcessGestureEvent take an event pointer and call SetHandled() on it directly (as needed), so each caller doesn't have to do this, and then you can simplify the callers like this: if (!drag_controller()->ProcessGestureEvent(event, this, false) TrayBackgroundView::OnGestureEvent(event); https://codereview.chromium.org/2961313003/diff/80001/ash/system/ime_menu/ime... File ash/system/ime_menu/ime_menu_tray.h (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.h:40: void ShowImeMenuBubble(); Remove this function, as it's now redundant with ShowBubble(). https://codereview.chromium.org/2961313003/diff/80001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.h:43: void HideImeMenuBubble(); Remove this function, as it's now redundant with CloseBubble(). https://codereview.chromium.org/2961313003/diff/80001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.h:46: bool IsImeMenuBubbleShown(); Remove this function, as it's now redundant with GetBubbleView(). https://codereview.chromium.org/2961313003/diff/80001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.h:71: // ui::EventHandler: nit: Since this interface is inherited via the subclassing of TrayBackgroundView, you can remove this comment and combine this override into the block above. https://codereview.chromium.org/2961313003/diff/80001/ash/system/palette/pale... File ash/system/palette/palette_tray.h (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/palette/pale... ash/system/palette/palette_tray.h:76: bool ShowPalette(); Remove this function, as it's now redundant with ShowBubble(). Callers that checked the return value can call GetBubbleView() instead afterwards. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray.cc:56: #include "ash/wm/maximize_mode/maximize_mode_controller.h" nit: remove if no longer needed https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray.cc:638: bool SystemTray::ProcessGestureEventForBubble(ui::GestureEvent* event) { It would be nice if we didn't need to have so many redundant declarations, but the only way I can imagine that happening is by making a TrayBubbleView::Delegate abstract class in Ash (kinda ugly), or making TrayBackgroundView itself a subclass of TrayBubbleView::Delegate, does the latter option seem reasonable? https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray.h:116: bool CloseSystemBubble() const; Remove this function, as it's now redundant with CloseBubble(). The only callers checking the bool return value are in unit tests, and they can instead check HasSystemBubble() before calling CloseBubble(). It may be okay to delay this cleanup for a followup CL, if you add a TODO and file a bug now, and follow up soon. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... File ash/system/tray/system_tray_bubble.cc (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_bubble.cc:203: // Place the bubble on same display as this system tray if it is not on Update this comment here and move some relevant bits to the declaration comment for GetBubbleWindowContainer, which now creates the clipping window. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... File ash/system/tray/system_tray_bubble.h (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_bubble.h:9: #include <memory> nit: remove if no longer needed https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:68: // based on the view's own coordinates. nit: clarify "the view", do you mean the system tray? https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:145: float scroll_y_hint_ = -1.f; Add explanatory comments for both of these members https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:347: // Begins to scroll downward on the shelf should not show the system tray nit: s/Begins/Beginning/, isn't this scrolling up after the down hint? (delta is still the same as above) https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:356: TEST_F(SystemTrayTest, SwipingOnSystemTrayBubble) { Should this or another new fixture test swiping on open system tray bubbles when the shelf has different alignments? https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:359: shelf->SetAlignment(SHELF_ALIGNMENT_BOTTOM); q: Is this necessary? (it might already have this alignment) https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:360: Shell::Get()->maximize_mode_controller()->EnableMaximizeModeWindowManager( q: Is this necessary? If so, maybe add a comment? https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:364: // Begins to scroll downward and swiping down more than one third of the nit: s/Begins/Beginning/ https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:375: // Begins to scroll upward and swiping down more than one third of the nit: s/Begins/Beginning/, also "and then swiping" https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/tray_ba... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:184: drag_controller_.reset(new TrayDragController(shelf)); nit: use base::MakeUnique https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:435: clipping_window_.reset(new aura::Window(nullptr)); nit: use base::MakeUnique https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/tray_ba... File ash/system/tray/tray_background_view.h (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.h:57: // Dragging releated functions: Add an explanatory comment for each function, nothing here is obviously related to dragging. (also 'related' is spelled incorrectly) https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.h:58: virtual bool HasBubble(); This function seems unnecessary, as callers can instead just check GetBubbleView() for null, right? Avoid adding redundant functionality. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.h:112: // Update the bounds of the tray bubbles. Close the bubble if nit: "of the associated tray bubble.", update line wrapping https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.h:161: std::unique_ptr<TrayDragController> drag_controller_; nit: add a comment. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray_drag_co... File ash/system/tray_drag_controller.cc (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray_drag_co... ash/system/tray_drag_controller.cc:20: !shelf_->IsHorizontalAlignment()) { Ah, this only works for horizontal alignments? Do we have plans for left/right? https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray_drag_co... ash/system/tray_drag_controller.cc:28: if (!target_view_->HasBubble() || !is_in_drag_) { nit: curlies not needed for one-liners https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray_drag_co... ash/system/tray_drag_controller.cc:76: gesture_drag_amount_ += gesture.details().scroll_y(); should we be updating this value before calling UpdateBubbleBounds to preserve the old behavior? https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray_drag_co... ash/system/tray_drag_controller.cc:91: gfx::Rect bounds_on_location = tray_bubble_bounds_; nit: can you imagine a better name for this? https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray_drag_co... File ash/system/tray_drag_controller.h (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray_drag_co... ash/system/tray_drag_controller.h:20: explicit TrayDragController(Shelf* shelf) : shelf_(shelf) {} Move the definition to the .cc file. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray_drag_co... ash/system/tray_drag_controller.h:27: bool is_on_bubble); Please try removing the |is_on_bubble| parameter and replacing that with a value calculated in the function like this: |bool is_on_bubble = event.target() != target_view;|, or this: |bool is_on_bubble = event.target() != target_view->GetWidget()->GetNativeWindow();| The latter should work, and the former might work, and it would simplify the callsites. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray_drag_co... ash/system/tray_drag_controller.h:56: // True if the user is in the process of gesture-dragging to open the system nit: no 'system', right? https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray_drag_co... ash/system/tray_drag_controller.h:60: // True if the dragging happened on the bubble view, false if happened on the nit: "false if it happened" https://codereview.chromium.org/2961313003/diff/80001/ash/system/web_notifica... File ash/system/web_notification/web_notification_tray.h (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/web_notifica... ash/system/web_notification/web_notification_tray.h:75: void ShowMessageCenterBubble(); Remove this redundant function, move its impl to ShowBubble https://codereview.chromium.org/2961313003/diff/80001/ash/system/web_notifica... ash/system/web_notification/web_notification_tray.h:92: // ui::EventHandler: ditto
Also, maybe you meant to say "swiping down" closes the bubble in your CL description? The description could use more detail, describe adding the drag controller (extracting behavior from the system tray bubble), plumbing events, etc.
Description was changed from ========== Swiping up on the System Tray/ IME/ Stylus/ Notifications buttons in status area should open the bubble. Swiping up on the opened bubble should close the bubble. BUG=735994,735996 ========== to ========== Swiping up on the System Tray/ IME/ Stylus/ Notifications buttons in status area should open the bubble. Swiping down on the opened bubble should close the bubble. BUG=735994,735996 ==========
The CQ bit was checked by minch@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...
Description was changed from ========== Swiping up on the System Tray/ IME/ Stylus/ Notifications buttons in status area should open the bubble. Swiping down on the opened bubble should close the bubble. BUG=735994,735996 ========== to ========== Touch gestures for System Tray/ IME/ Stylus/ Notifications Swiping up on the System Tray/ IME/ Stylus/ Notifications buttons in status area should open the associated bubble. Swiping down on the opened bubble should close the associated bubble. Changes: Added the tray_drag_controller to extract the logic of dragging behavior from system_tray. Add help functions in tray_background_view to get the state of associated tray bubble. Add one interface ProcessGestureEventForBubble in TrayBubbleView::Delegate to help process the dragging that happened on the tray bubble. BUG=735994,735996 ==========
Description was changed from ========== Touch gestures for System Tray/ IME/ Stylus/ Notifications Swiping up on the System Tray/ IME/ Stylus/ Notifications buttons in status area should open the associated bubble. Swiping down on the opened bubble should close the associated bubble. Changes: Added the tray_drag_controller to extract the logic of dragging behavior from system_tray. Add help functions in tray_background_view to get the state of associated tray bubble. Add one interface ProcessGestureEventForBubble in TrayBubbleView::Delegate to help process the dragging that happened on the tray bubble. BUG=735994,735996 ========== to ========== Touch gestures for System Tray/ IME/ Stylus/ Notifications Swiping up on the System Tray/ IME/ Stylus/ Notifications buttons in status area should open the associated bubble. Swiping down on the opened bubble should close the associated bubble. Changes: Added the tray_drag_controller to extract the logic of dragging behavior from system_tray. Add help functions in tray_background_view to get the state of associated tray bubble. Add one interface ProcessGestureEventForBubble in TrayBubbleView::Delegate to help process the dragging that happened on the tray bubble. BUG=735994,735996 ==========
Description was changed from ========== Touch gestures for System Tray/ IME/ Stylus/ Notifications Swiping up on the System Tray/ IME/ Stylus/ Notifications buttons in status area should open the associated bubble. Swiping down on the opened bubble should close the associated bubble. Changes: Added the tray_drag_controller to extract the logic of dragging behavior from system_tray. Add help functions in tray_background_view to get the state of associated tray bubble. Add one interface ProcessGestureEventForBubble in TrayBubbleView::Delegate to help process the dragging that happened on the tray bubble. BUG=735994,735996 ========== to ========== Touch gestures for System Tray/ IME/ Stylus/ Notifications Swiping up on the System Tray/ IME/ Stylus/ Notifications buttons in status area should open the associated bubble. Swiping down on the opened bubble should close the associated bubble. Changes: 1.Added the tray_drag_controller to extract the logic of dragging behavior from system_tray. 2.Added help functions in tray_background_view to get the state of associated tray bubble. 3.Added one interface ProcessGestureEventForBubble in TrayBubbleView::Delegate to help process the dragging that happened on the tray bubble. BUG=735994,735996 ==========
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 minch@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by minch@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...
msw@, thanks for your comments. tdanderson@, msw@, please help review the latest ps. https://codereview.chromium.org/2961313003/diff/80001/ash/system/ime_menu/ime... File ash/system/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.cc:474: return bubble_.get() != NULL; On 2017/07/12 05:04:50, msw wrote: > nit: this should be moot, but use nullptr instead of NULL. Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.cc:486: if (HasBubble()) On 2017/07/12 05:04:50, msw wrote: > nit: return bubble_ ? bubble_->bubble_view() : nullptr; > (ditto elsewhere for the other impls) Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.cc:494: event->SetHandled(); On 2017/07/12 05:04:50, msw wrote: > Please make DragController::ProcessGestureEvent take an event pointer and call > SetHandled() on it directly (as needed), so each caller doesn't have to do this, > and then you can simplify the callers like this: > if (!drag_controller()->ProcessGestureEvent(event, this, false) > TrayBackgroundView::OnGestureEvent(event); Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/ime_menu/ime... File ash/system/ime_menu/ime_menu_tray.h (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.h:40: void ShowImeMenuBubble(); On 2017/07/12 05:04:50, msw wrote: > Remove this function, as it's now redundant with ShowBubble(). Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.h:43: void HideImeMenuBubble(); On 2017/07/12 05:04:50, msw wrote: > Remove this function, as it's now redundant with CloseBubble(). Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.h:46: bool IsImeMenuBubbleShown(); On 2017/07/12 05:04:51, msw wrote: > Remove this function, as it's now redundant with GetBubbleView(). Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.h:71: // ui::EventHandler: On 2017/07/12 05:04:51, msw wrote: > nit: Since this interface is inherited via the subclassing of > TrayBackgroundView, you can remove this comment and combine this override into > the block above. Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/palette/pale... File ash/system/palette/palette_tray.h (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/palette/pale... ash/system/palette/palette_tray.h:76: bool ShowPalette(); On 2017/07/12 05:04:51, msw wrote: > Remove this function, as it's now redundant with ShowBubble(). Callers that > checked the return value can call GetBubbleView() instead afterwards. Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray.cc:56: #include "ash/wm/maximize_mode/maximize_mode_controller.h" On 2017/07/12 05:04:51, msw wrote: > nit: remove if no longer needed Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray.cc:638: bool SystemTray::ProcessGestureEventForBubble(ui::GestureEvent* event) { On 2017/07/12 05:04:51, msw wrote: > It would be nice if we didn't need to have so many redundant declarations, but > the only way I can imagine that happening is by making a > TrayBubbleView::Delegate abstract class in Ash (kinda ugly), or making > TrayBackgroundView itself a subclass of TrayBubbleView::Delegate, does the > latter option seem reasonable? For the latter option, since TrayBubbleView::Delegate has some other pure virtual functions (8 functions). If make TrayBackgroundView as a subclass of TrayBubbleView::Delegate, it should have default implementation for all these pure virtual functions, since except SystemTray/ ImeMenuTray/ WebNotificationTray/ PaletteTray what we used in this cl, some other classes which is the subclass of TrayBackgroundView also need to implement these functions. Otherwise, we can make all the functions in TrayBubbleView::Delegate not pure virtual only virtual instead, which should have the default implementation in TrayBubbleView::Delegate instead. Which one do you think is better? (I guess if we really want to do this, maybe just make the functions in TrayBubbleView::Delegate not pure looks better? But we also need to add the default implementation for all of these functions in TrayBubbleView? Or just keep this function like this?) https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray.h:116: bool CloseSystemBubble() const; On 2017/07/12 05:04:51, msw wrote: > Remove this function, as it's now redundant with CloseBubble(). > The only callers checking the bool return value are in unit tests, and they can > instead check HasSystemBubble() before calling CloseBubble(). It may be okay to > delay this cleanup for a followup CL, if you add a TODO and file a bug now, and > follow up soon. Added TODO, will do it in another CL soon.http://crbug.com/741953 https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... File ash/system/tray/system_tray_bubble.cc (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_bubble.cc:203: // Place the bubble on same display as this system tray if it is not on On 2017/07/12 05:04:51, msw wrote: > Update this comment here and move some relevant bits to the declaration comment > for GetBubbleWindowContainer, which now creates the clipping window. Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... File ash/system/tray/system_tray_bubble.h (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_bubble.h:9: #include <memory> On 2017/07/12 05:04:51, msw wrote: > nit: remove if no longer needed Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:68: // based on the view's own coordinates. On 2017/07/12 05:04:51, msw wrote: > nit: clarify "the view", do you mean the system tray? Can be both system tray and system tray bubble. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:145: float scroll_y_hint_ = -1.f; On 2017/07/12 05:04:51, msw wrote: > Add explanatory comments for both of these members Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:347: // Begins to scroll downward on the shelf should not show the system tray On 2017/07/12 05:04:51, msw wrote: > nit: s/Begins/Beginning/, isn't this scrolling up after the down hint? (delta is > still the same as above) Yes, but it starts with scroll downward, so should not show the system tray bubble in this case. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:356: TEST_F(SystemTrayTest, SwipingOnSystemTrayBubble) { On 2017/07/12 05:04:51, msw wrote: > Should this or another new fixture test swiping on open system tray bubbles when > the shelf has different alignments? Acknowledged. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:359: shelf->SetAlignment(SHELF_ALIGNMENT_BOTTOM); On 2017/07/12 05:04:51, msw wrote: > q: Is this necessary? (it might already have this alignment) I am sorry I didn't find the place that show the shelf is already Bottom alignment. Even though it maybe better to set the alignment here to explain that the test is for bottom alignment? https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:360: Shell::Get()->maximize_mode_controller()->EnableMaximizeModeWindowManager( On 2017/07/12 05:04:51, msw wrote: > q: Is this necessary? If so, maybe add a comment? Added one more test case to explain this. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:364: // Begins to scroll downward and swiping down more than one third of the On 2017/07/12 05:04:51, msw wrote: > nit: s/Begins/Beginning/ Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:375: // Begins to scroll upward and swiping down more than one third of the On 2017/07/12 05:04:51, msw wrote: > nit: s/Begins/Beginning/, also "and then swiping" Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/tray_ba... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:184: drag_controller_.reset(new TrayDragController(shelf)); On 2017/07/12 05:04:52, msw wrote: > nit: use base::MakeUnique Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:435: clipping_window_.reset(new aura::Window(nullptr)); On 2017/07/12 05:04:51, msw wrote: > nit: use base::MakeUnique Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/tray_ba... File ash/system/tray/tray_background_view.h (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.h:57: // Dragging releated functions: On 2017/07/12 05:04:52, msw wrote: > Add an explanatory comment for each function, nothing here is obviously related > to dragging. (also 'related' is spelled incorrectly) Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.h:58: virtual bool HasBubble(); On 2017/07/12 05:04:52, msw wrote: > This function seems unnecessary, as callers can instead just check > GetBubbleView() for null, right? Avoid adding redundant functionality. In SystemTray, HasBubble has a little bit different with others, it should be |full_system_tray_menu_| (if there is single line bubble like the volume slider opened, swiping the system tray area should still open the system tray bubble). But I moved the logic to GetBubbleView instead, then we don't need HasBubble. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.h:112: // Update the bounds of the tray bubbles. Close the bubble if On 2017/07/12 05:04:52, msw wrote: > nit: "of the associated tray bubble.", update line wrapping Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.h:161: std::unique_ptr<TrayDragController> drag_controller_; On 2017/07/12 05:04:52, msw wrote: > nit: add a comment. Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray_drag_co... File ash/system/tray_drag_controller.cc (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray_drag_co... ash/system/tray_drag_controller.cc:20: !shelf_->IsHorizontalAlignment()) { On 2017/07/12 05:04:52, msw wrote: > Ah, this only works for horizontal alignments? Do we have plans for left/right? Acknowledged. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray_drag_co... ash/system/tray_drag_controller.cc:28: if (!target_view_->HasBubble() || !is_in_drag_) { On 2017/07/12 05:04:52, msw wrote: > nit: curlies not needed for one-liners Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray_drag_co... ash/system/tray_drag_controller.cc:76: gesture_drag_amount_ += gesture.details().scroll_y(); On 2017/07/12 05:04:52, msw wrote: > should we be updating this value before calling UpdateBubbleBounds to preserve > the old behavior? Thanks. I think we should update this value before calling UpdateBubbleBounds, since update bounds based on |gesture_drag_amount_| now. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray_drag_co... ash/system/tray_drag_controller.cc:91: gfx::Rect bounds_on_location = tray_bubble_bounds_; On 2017/07/12 05:04:52, msw wrote: > nit: can you imagine a better name for this? How about |current_tray_bubble_bounds|? https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray_drag_co... File ash/system/tray_drag_controller.h (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray_drag_co... ash/system/tray_drag_controller.h:20: explicit TrayDragController(Shelf* shelf) : shelf_(shelf) {} On 2017/07/12 05:04:52, msw wrote: > Move the definition to the .cc file. Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray_drag_co... ash/system/tray_drag_controller.h:27: bool is_on_bubble); On 2017/07/12 05:04:52, msw wrote: > Please try removing the |is_on_bubble| parameter and replacing that with a value > calculated in the function like this: |bool is_on_bubble = event.target() != > target_view;|, or this: |bool is_on_bubble = event.target() != > target_view->GetWidget()->GetNativeWindow();| The latter should work, and the > former might work, and it would simplify the callsites. Thanks. Tried the latter, looks that the GetNativeWindow is always the same one? Since |is_on_bubble_| will always be true in this case. The former one works. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray_drag_co... ash/system/tray_drag_controller.h:56: // True if the user is in the process of gesture-dragging to open the system On 2017/07/12 05:04:52, msw wrote: > nit: no 'system', right? Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray_drag_co... ash/system/tray_drag_controller.h:60: // True if the dragging happened on the bubble view, false if happened on the On 2017/07/12 05:04:52, msw wrote: > nit: "false if it happened" Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/web_notifica... File ash/system/web_notification/web_notification_tray.h (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/web_notifica... ash/system/web_notification/web_notification_tray.h:75: void ShowMessageCenterBubble(); On 2017/07/12 05:04:53, msw wrote: > Remove this redundant function, move its impl to ShowBubble Done. https://codereview.chromium.org/2961313003/diff/80001/ash/system/web_notifica... ash/system/web_notification/web_notification_tray.h:92: // ui::EventHandler: On 2017/07/12 05:04:52, msw wrote: > ditto Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Looking better, thanks. I still have some nontrivial comments. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray.cc:638: bool SystemTray::ProcessGestureEventForBubble(ui::GestureEvent* event) { On 2017/07/13 19:10:35, minch1 wrote: > On 2017/07/12 05:04:51, msw wrote: > > It would be nice if we didn't need to have so many redundant declarations, but > > the only way I can imagine that happening is by making a > > TrayBubbleView::Delegate abstract class in Ash (kinda ugly), or making > > TrayBackgroundView itself a subclass of TrayBubbleView::Delegate, does the > > latter option seem reasonable? > > For the latter option, since TrayBubbleView::Delegate has some other pure > virtual functions (8 functions). If make TrayBackgroundView as a subclass of > TrayBubbleView::Delegate, it should have default implementation for all these > pure virtual functions, since except SystemTray/ ImeMenuTray/ > WebNotificationTray/ PaletteTray what we used in this cl, some other classes > which is the subclass of TrayBackgroundView also need to implement these > functions. Otherwise, we can make all the functions in TrayBubbleView::Delegate > not pure virtual only virtual instead, which should have the default > implementation in TrayBubbleView::Delegate instead. > Which one do you think is better? (I guess if we really want to do this, maybe > just make the functions in TrayBubbleView::Delegate not pure looks better? But > we also need to add the default implementation for all of these functions in > TrayBubbleView? Or just keep this function like this?) Thanks for thinking about this, the code will be better if we can consolidate some of the implementations. My preference would be to make TrayBackgroundView inherit from TrayBubbleView::Delegate, but only provide default implementations of the new ProcessGestureEventForBubble function and the common OnGestureEvent override impl pattern. Consolidating more from all the TrayBackgroundView subclasses (like the common PerformAction impl) would be a bonus, but less important for now. I think it should be possible to leave the TrayBubbleView::Delegate interface pure-virtual, and make TrayBackgroundView an abstract class (missing some of the pure-virtual implementations), since nothing instantiates TrayBackgroundView directly. We can leave it up to the TrayBackgroundView subclasses like SystemTray to continue implementing the other pure virtual functions, since those are the classes that are actually instantiated. Does that seem feasible? (otherwise making the interface not pure via TrayBackgroundView seems okay to me). https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray.h:116: bool CloseSystemBubble() const; On 2017/07/13 19:10:35, minch1 wrote: > On 2017/07/12 05:04:51, msw wrote: > > Remove this function, as it's now redundant with CloseBubble(). > > The only callers checking the bool return value are in unit tests, and they > can > > instead check HasSystemBubble() before calling CloseBubble(). It may be okay > to > > delay this cleanup for a followup CL, if you add a TODO and file a bug now, > and > > follow up soon. > > Added TODO, will do it in another CL soon.http://crbug.com/741953 Acknowledged, thanks in advance. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:347: // Begins to scroll downward on the shelf should not show the system tray On 2017/07/13 19:10:35, minch1 wrote: > On 2017/07/12 05:04:51, msw wrote: > > nit: s/Begins/Beginning/, isn't this scrolling up after the down hint? (delta > is > > still the same as above) > > Yes, but it starts with scroll downward, so should not show the system tray > bubble in this case. Hmm, it seems like Android's top system menu/shade will still let you drag it down and make it visible even if the drag begins by moving upwards. I wonder if we'll want to match that, but this is okay for now, I suppose. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:359: shelf->SetAlignment(SHELF_ALIGNMENT_BOTTOM); On 2017/07/13 19:10:35, minch1 wrote: > On 2017/07/12 05:04:51, msw wrote: > > q: Is this necessary? (it might already have this alignment) > > I am sorry I didn't find the place that show the shelf is already Bottom > alignment. Even though it maybe better to set the alignment here to explain that > the test is for bottom alignment? I suggest checking that's the case, via EXPECT_EQ(SHELF_ALIGNMENT_BOTTOM, shelf->alignment()); https://codereview.chromium.org/2961313003/diff/160001/ash/system/palette/pal... File ash/system/palette/palette_tray.cc (right): https://codereview.chromium.org/2961313003/diff/160001/ash/system/palette/pal... ash/system/palette/palette_tray.cc:339: return GetBubbleView() != nullptr; nit: can this assume ShowBubble will actually show, and just return true? https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/system... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/system... ash/system/tray/system_tray.cc:573: // TODO(minch): Remove CloseSystemBubble which is redundant with CloseBubble. nit: move this to the header file. https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/system... ash/system/tray/system_tray.cc:584: return system_bubble_ && full_system_tray_menu_ Please add a comment here like "Only return the bubble view when it's showing the main system tray bubble, not the volume or brightness bubbles etc., to avoid client confusion." https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/system... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:83: ui::Event::DispatcherApi dispatch_helper(&event); Can you extract a helper function for the three similar blocks of code? It might make sense to refactor a bit too, maybe like this? void DispatchGestureEvent(ui::GestureEvent event, bool send_to_bubble) { SystemTray* tray = GetPrimarySystemTray(); auto* target = to_bubble ? tray->GetSystemBubble()->bubble_view() : tray; ui::Event::DispatcherApi(&event).set_target(target); target->OnGestureEvent(&event); } https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:159: float scroll_y_hint_ = -1.f; I think these two members might be better off as arguments to SendGestureEvent, with default values, so we don't always have to specify their values. I'm a bit on the fence, but they seem more like gesture event parameters than test class stateful members. WDYT? https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:162: // false. nit: s/false/the dragging is triggered on the tray button's background view/ (or similar) https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:163: bool is_on_bubble_ = false; nit: maybe rename this to something like |target_bubble_| or |drag_on_bubble_| https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:365: // bubble. nit: "bubble, even if the drag then moves upwards." https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:380: // the bubble's height should close the bubble. Thanks for adding the comment about maximize mode below, but it would be good to append something similar here, like "This only takes effect in maximize mode." https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/tray_b... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.cc:427: // If it is in maximize mode, create an clipping window to hold the tray nit: "Place the bubble in |container|, or in a window clipped to the work area in maximize mode, in order to <I'm not sure why this is needed for maximize mode but not otherwise, can you fill in the blank?>" https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/tray_b... File ash/system/tray/tray_background_view.h (right): https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.h:60: // Closes the associated tray bubble view if the bubble view has been shown. nit: "if it is currently showing."? https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.h:164: // Used to process the dragging that happend on the tray area to show the nit: "Handles touch drag gestures on the tray area and its associated bubble." https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray_drag_c... File ash/system/tray_drag_controller.cc (right): https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray_drag_c... ash/system/tray_drag_controller.cc:16: TrayBackgroundView* target_view) { Maybe rename |target_view| to |tray_view| or similar, since it may not be the actual event target? (it's just a bit confusing to overload the meaning of target while handling events). Please update the function param in the header file, and rename the |target_view_| class member similarly.
The CQ bit was checked by minch@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...
msw@, tdanderson@, oshima@, please help review the latest ps. Thanks. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray.cc:638: bool SystemTray::ProcessGestureEventForBubble(ui::GestureEvent* event) { On 2017/07/14 02:00:45, msw wrote: > On 2017/07/13 19:10:35, minch1 wrote: > > On 2017/07/12 05:04:51, msw wrote: > > > It would be nice if we didn't need to have so many redundant declarations, > but > > > the only way I can imagine that happening is by making a > > > TrayBubbleView::Delegate abstract class in Ash (kinda ugly), or making > > > TrayBackgroundView itself a subclass of TrayBubbleView::Delegate, does the > > > latter option seem reasonable? > > > > For the latter option, since TrayBubbleView::Delegate has some other pure > > virtual functions (8 functions). If make TrayBackgroundView as a subclass of > > TrayBubbleView::Delegate, it should have default implementation for all these > > pure virtual functions, since except SystemTray/ ImeMenuTray/ > > WebNotificationTray/ PaletteTray what we used in this cl, some other classes > > which is the subclass of TrayBackgroundView also need to implement these > > functions. Otherwise, we can make all the functions in > TrayBubbleView::Delegate > > not pure virtual only virtual instead, which should have the default > > implementation in TrayBubbleView::Delegate instead. > > Which one do you think is better? (I guess if we really want to do this, maybe > > just make the functions in TrayBubbleView::Delegate not pure looks better? But > > we also need to add the default implementation for all of these functions in > > TrayBubbleView? Or just keep this function like this?) > > Thanks for thinking about this, the code will be better if we can consolidate > some of the implementations. My preference would be to make TrayBackgroundView > inherit from TrayBubbleView::Delegate, but only provide default implementations > of the new ProcessGestureEventForBubble function and the common OnGestureEvent > override impl pattern. Consolidating more from all the TrayBackgroundView > subclasses (like the common PerformAction impl) would be a bonus, but less > important for now. I think it should be possible to leave the > TrayBubbleView::Delegate interface pure-virtual, and make TrayBackgroundView an > abstract class (missing some of the pure-virtual implementations), since nothing > instantiates TrayBackgroundView directly. We can leave it up to the > TrayBackgroundView subclasses like SystemTray to continue implementing the other > pure virtual functions, since those are the classes that are actually > instantiated. Does that seem feasible? (otherwise making the interface not pure > via TrayBackgroundView seems okay to me). I think TrayBakgroundView is already an abstract class currently. If make TrayBackgroundView a subclass of TrayBubbleView::Delegate, all of its subclasses should implement the pure-virtual functions in TrayBubbleView::Delegate (except ProcessGestureEventForBubble that we want to implement in TrayBackgroundView). Since like OverViewButtonTray is also a subclass of TrayBackgroundView, it will also need to implement these pure-virtual functions in TrayBubbleView::Delegate (if we keep the TrayBubbleView::Delegate pure-virtual). This is totally not what we want. It might feasible to make TrayBubbleView::Delegate non pure-virtual. But ne more thing, I think maybe we cann't implement ProcessGestureEventForBubble in TrayBackgroundview? Since the argument |this| should be SystemTray, not TrayBackgroundView. Then, the added help functions HasBubble, CloseBubble, etc, will call into the correct subclass? https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:347: // Begins to scroll downward on the shelf should not show the system tray On 2017/07/14 02:00:45, msw wrote: > On 2017/07/13 19:10:35, minch1 wrote: > > On 2017/07/12 05:04:51, msw wrote: > > > nit: s/Begins/Beginning/, isn't this scrolling up after the down hint? > (delta > > is > > > still the same as above) > > > > Yes, but it starts with scroll downward, so should not show the system tray > > bubble in this case. > > Hmm, it seems like Android's top system menu/shade will still let you drag it > down and make it visible even if the drag begins by moving upwards. I wonder if > we'll want to match that, but this is okay for now, I suppose. Scroll down first then scroll up will drag the shelf currently. Checked with the PM. We can keep it as this currently. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray_unittest.cc:359: shelf->SetAlignment(SHELF_ALIGNMENT_BOTTOM); On 2017/07/14 02:00:45, msw wrote: > On 2017/07/13 19:10:35, minch1 wrote: > > On 2017/07/12 05:04:51, msw wrote: > > > q: Is this necessary? (it might already have this alignment) > > > > I am sorry I didn't find the place that show the shelf is already Bottom > > alignment. Even though it maybe better to set the alignment here to explain > that > > the test is for bottom alignment? > > I suggest checking that's the case, via EXPECT_EQ(SHELF_ALIGNMENT_BOTTOM, > shelf->alignment()); Done. https://codereview.chromium.org/2961313003/diff/160001/ash/system/palette/pal... File ash/system/palette/palette_tray.cc (right): https://codereview.chromium.org/2961313003/diff/160001/ash/system/palette/pal... ash/system/palette/palette_tray.cc:339: return GetBubbleView() != nullptr; On 2017/07/14 02:00:45, msw wrote: > nit: can this assume ShowBubble will actually show, and just return true? Done. https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/system... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/system... ash/system/tray/system_tray.cc:573: // TODO(minch): Remove CloseSystemBubble which is redundant with CloseBubble. On 2017/07/14 02:00:45, msw wrote: > nit: move this to the header file. Done. https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/system... ash/system/tray/system_tray.cc:584: return system_bubble_ && full_system_tray_menu_ On 2017/07/14 02:00:45, msw wrote: > Please add a comment here like "Only return the bubble view when it's showing > the main system tray bubble, not the volume or brightness bubbles etc., to avoid > client confusion." Done. https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/system... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:83: ui::Event::DispatcherApi dispatch_helper(&event); On 2017/07/14 02:00:46, msw wrote: > Can you extract a helper function for the three similar blocks of code? It might > make sense to refactor a bit too, maybe like this? > void DispatchGestureEvent(ui::GestureEvent event, bool send_to_bubble) { > SystemTray* tray = GetPrimarySystemTray(); > auto* target = to_bubble ? tray->GetSystemBubble()->bubble_view() : tray; > ui::Event::DispatcherApi(&event).set_target(target); > target->OnGestureEvent(&event); > } Done. https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:159: float scroll_y_hint_ = -1.f; On 2017/07/14 02:00:45, msw wrote: > I think these two members might be better off as arguments to SendGestureEvent, > with default values, so we don't always have to specify their values. I'm a bit > on the fence, but they seem more like gesture event parameters than test class > stateful members. WDYT? Personally, I prefer to make them as the arguments of SendGestureEvent with default values. Since, there are just like |start| or |is_fling| in SendGestureEvent. Why I did it like this originally is because don't want to change so many callers' argument. But if with default values, I think we don't need to do that. https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:162: // false. On 2017/07/14 02:00:45, msw wrote: > nit: s/false/the dragging is triggered on the tray button's background view/ (or > similar) Done. https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:163: bool is_on_bubble_ = false; On 2017/07/14 02:00:45, msw wrote: > nit: maybe rename this to something like |target_bubble_| or |drag_on_bubble_| Done. https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:365: // bubble. On 2017/07/14 02:00:46, msw wrote: > nit: "bubble, even if the drag then moves upwards." Done. https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:380: // the bubble's height should close the bubble. On 2017/07/14 02:00:45, msw wrote: > Thanks for adding the comment about maximize mode below, but it would be good to > append something similar here, like "This only takes effect in maximize mode." Done. https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/tray_b... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.cc:427: // If it is in maximize mode, create an clipping window to hold the tray On 2017/07/14 02:00:46, msw wrote: > nit: "Place the bubble in |container|, or in a window clipped to the work area > in maximize mode, in order to <I'm not sure why this is needed for maximize mode > but not otherwise, can you fill in the blank?>" I think it is ok to remove the maximize mode restriction here. It is ok to put the bubbles in the clipped window event not in maximize mode. Why I did this just want to make fewer influence for the laptop mode. Since this feature is only for tablet currently. https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/tray_b... File ash/system/tray/tray_background_view.h (right): https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.h:60: // Closes the associated tray bubble view if the bubble view has been shown. On 2017/07/14 02:00:46, msw wrote: > nit: "if it is currently showing."? Done. https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.h:164: // Used to process the dragging that happend on the tray area to show the On 2017/07/14 02:00:46, msw wrote: > nit: "Handles touch drag gestures on the tray area and its associated bubble." Done. https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray_drag_c... File ash/system/tray_drag_controller.cc (right): https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray_drag_c... ash/system/tray_drag_controller.cc:16: TrayBackgroundView* target_view) { On 2017/07/14 02:00:46, msw wrote: > Maybe rename |target_view| to |tray_view| or similar, since it may not be the > actual event target? (it's just a bit confusing to overload the meaning of > target while handling events). Please update the function param in the header > file, and rename the |target_view_| class member similarly. Done.
minch@chromium.org changed reviewers: + oshima@chromium.org
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_...)
Looking pretty good; please try to consolidate drag controller usage. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray.cc:638: bool SystemTray::ProcessGestureEventForBubble(ui::GestureEvent* event) { On 2017/07/14 19:45:17, minch1 wrote: > On 2017/07/14 02:00:45, msw wrote: > > On 2017/07/13 19:10:35, minch1 wrote: > > > On 2017/07/12 05:04:51, msw wrote: > > > > It would be nice if we didn't need to have so many redundant declarations, > > but > > > > the only way I can imagine that happening is by making a > > > > TrayBubbleView::Delegate abstract class in Ash (kinda ugly), or making > > > > TrayBackgroundView itself a subclass of TrayBubbleView::Delegate, does the > > > > latter option seem reasonable? > > > > > > For the latter option, since TrayBubbleView::Delegate has some other pure > > > virtual functions (8 functions). If make TrayBackgroundView as a subclass of > > > TrayBubbleView::Delegate, it should have default implementation for all > these > > > pure virtual functions, since except SystemTray/ ImeMenuTray/ > > > WebNotificationTray/ PaletteTray what we used in this cl, some other classes > > > which is the subclass of TrayBackgroundView also need to implement these > > > functions. Otherwise, we can make all the functions in > > TrayBubbleView::Delegate > > > not pure virtual only virtual instead, which should have the default > > > implementation in TrayBubbleView::Delegate instead. > > > Which one do you think is better? (I guess if we really want to do this, > maybe > > > just make the functions in TrayBubbleView::Delegate not pure looks better? > But > > > we also need to add the default implementation for all of these functions in > > > TrayBubbleView? Or just keep this function like this?) > > > > Thanks for thinking about this, the code will be better if we can consolidate > > some of the implementations. My preference would be to make TrayBackgroundView > > inherit from TrayBubbleView::Delegate, but only provide default > implementations > > of the new ProcessGestureEventForBubble function and the common OnGestureEvent > > override impl pattern. Consolidating more from all the TrayBackgroundView > > subclasses (like the common PerformAction impl) would be a bonus, but less > > important for now. I think it should be possible to leave the > > TrayBubbleView::Delegate interface pure-virtual, and make TrayBackgroundView > an > > abstract class (missing some of the pure-virtual implementations), since > nothing > > instantiates TrayBackgroundView directly. We can leave it up to the > > TrayBackgroundView subclasses like SystemTray to continue implementing the > other > > pure virtual functions, since those are the classes that are actually > > instantiated. Does that seem feasible? (otherwise making the interface not > pure > > via TrayBackgroundView seems okay to me). > > I think TrayBakgroundView is already an abstract class currently. If make > TrayBackgroundView a subclass of TrayBubbleView::Delegate, all of its subclasses > should implement the pure-virtual functions in TrayBubbleView::Delegate (except > ProcessGestureEventForBubble that we want to implement in TrayBackgroundView). > Since like OverViewButtonTray is also a subclass of TrayBackgroundView, it will > also need to implement these pure-virtual functions in TrayBubbleView::Delegate > (if we keep the TrayBubbleView::Delegate pure-virtual). This is totally not what > we want. > It might feasible to make TrayBubbleView::Delegate non pure-virtual. But ne more > thing, I think maybe we cann't implement ProcessGestureEventForBubble in > TrayBackgroundview? Since the argument |this| should be SystemTray, not > TrayBackgroundView. Then, the added help functions HasBubble, CloseBubble, etc, > will call into the correct subclass? Using |this| in a TrayBackgroundView::ProcessGestureEventForBubble base class implementation would be totally fine (it'll still execute SystemTray, etc. overrides as needed). I definitely favor consolidating more logic in TrayBackgroundView, even if by means of making TrayBubbleView::Delegate non pure-virtual (I'm hope that's not necessary). TrayBackgroundView should help subclasses easily handle or ignore swipe gestures, with minimal per-background overrides. And as TrayBackgroundView owns the DragController, it may as well own the requisite client logic. Someday, views::Button may even own a DragController and offer views::ButtonListener::ButtonSwiped() to let clients and subclasses more easily handle similar gestures. https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/tray_b... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2961313003/diff/160001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.cc:427: // If it is in maximize mode, create an clipping window to hold the tray On 2017/07/14 19:45:18, minch1 wrote: > On 2017/07/14 02:00:46, msw wrote: > > nit: "Place the bubble in |container|, or in a window clipped to the work area > > in maximize mode, in order to <I'm not sure why this is needed for maximize > mode > > but not otherwise, can you fill in the blank?>" > > I think it is ok to remove the maximize mode restriction here. It is ok to put > the bubbles in the clipped window event not in maximize mode. Why I did this > just want to make fewer influence for the laptop mode. Since this feature is > only for tablet currently. What you have is fine, let's leave this as-is for now. https://codereview.chromium.org/2961313003/diff/180001/ash/system/tray/system... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2961313003/diff/180001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:112: ? static_cast<views::View*>( nit: use views::View* target = to avoid the two static casts. https://codereview.chromium.org/2961313003/diff/180001/ash/system/tray/tray_b... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2961313003/diff/180001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.cc:428: // in maximize mode, in order to make sure that the tray bubble during nit: "in maximize mode, to avoid tray bubble and shelf overlap."
The CQ bit was checked by minch@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks, msw@. Please help review the latest ps. https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2961313003/diff/80001/ash/system/tray/system_... ash/system/tray/system_tray.cc:638: bool SystemTray::ProcessGestureEventForBubble(ui::GestureEvent* event) { On 2017/07/14 21:00:30, msw wrote: > On 2017/07/14 19:45:17, minch1 wrote: > > On 2017/07/14 02:00:45, msw wrote: > > > On 2017/07/13 19:10:35, minch1 wrote: > > > > On 2017/07/12 05:04:51, msw wrote: > > > > > It would be nice if we didn't need to have so many redundant > declarations, > > > but > > > > > the only way I can imagine that happening is by making a > > > > > TrayBubbleView::Delegate abstract class in Ash (kinda ugly), or making > > > > > TrayBackgroundView itself a subclass of TrayBubbleView::Delegate, does > the > > > > > latter option seem reasonable? > > > > > > > > For the latter option, since TrayBubbleView::Delegate has some other pure > > > > virtual functions (8 functions). If make TrayBackgroundView as a subclass > of > > > > TrayBubbleView::Delegate, it should have default implementation for all > > these > > > > pure virtual functions, since except SystemTray/ ImeMenuTray/ > > > > WebNotificationTray/ PaletteTray what we used in this cl, some other > classes > > > > which is the subclass of TrayBackgroundView also need to implement these > > > > functions. Otherwise, we can make all the functions in > > > TrayBubbleView::Delegate > > > > not pure virtual only virtual instead, which should have the default > > > > implementation in TrayBubbleView::Delegate instead. > > > > Which one do you think is better? (I guess if we really want to do this, > > maybe > > > > just make the functions in TrayBubbleView::Delegate not pure looks better? > > But > > > > we also need to add the default implementation for all of these functions > in > > > > TrayBubbleView? Or just keep this function like this?) > > > > > > Thanks for thinking about this, the code will be better if we can > consolidate > > > some of the implementations. My preference would be to make > TrayBackgroundView > > > inherit from TrayBubbleView::Delegate, but only provide default > > implementations > > > of the new ProcessGestureEventForBubble function and the common > OnGestureEvent > > > override impl pattern. Consolidating more from all the TrayBackgroundView > > > subclasses (like the common PerformAction impl) would be a bonus, but less > > > important for now. I think it should be possible to leave the > > > TrayBubbleView::Delegate interface pure-virtual, and make TrayBackgroundView > > an > > > abstract class (missing some of the pure-virtual implementations), since > > nothing > > > instantiates TrayBackgroundView directly. We can leave it up to the > > > TrayBackgroundView subclasses like SystemTray to continue implementing the > > other > > > pure virtual functions, since those are the classes that are actually > > > instantiated. Does that seem feasible? (otherwise making the interface not > > pure > > > via TrayBackgroundView seems okay to me). > > > > I think TrayBakgroundView is already an abstract class currently. If make > > TrayBackgroundView a subclass of TrayBubbleView::Delegate, all of its > subclasses > > should implement the pure-virtual functions in TrayBubbleView::Delegate > (except > > ProcessGestureEventForBubble that we want to implement in TrayBackgroundView). > > Since like OverViewButtonTray is also a subclass of TrayBackgroundView, it > will > > also need to implement these pure-virtual functions in > TrayBubbleView::Delegate > > (if we keep the TrayBubbleView::Delegate pure-virtual). This is totally not > what > > we want. > > It might feasible to make TrayBubbleView::Delegate non pure-virtual. But ne > more > > thing, I think maybe we cann't implement ProcessGestureEventForBubble in > > TrayBackgroundview? Since the argument |this| should be SystemTray, not > > TrayBackgroundView. Then, the added help functions HasBubble, CloseBubble, > etc, > > will call into the correct subclass? > > > Using |this| in a TrayBackgroundView::ProcessGestureEventForBubble base class > implementation would be totally fine (it'll still execute SystemTray, etc. > overrides as needed). > > I definitely favor consolidating more logic in TrayBackgroundView, even if by > means of making TrayBubbleView::Delegate non pure-virtual (I'm hope that's not > necessary). TrayBackgroundView should help subclasses easily handle or ignore > swipe gestures, with minimal per-background overrides. And as TrayBackgroundView > owns the DragController, it may as well own the requisite client logic. > > Someday, views::Button may even own a DragController and offer > views::ButtonListener::ButtonSwiped() to let clients and subclasses more easily > handle similar gestures. Done. https://codereview.chromium.org/2961313003/diff/180001/ash/system/tray/system... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2961313003/diff/180001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:112: ? static_cast<views::View*>( On 2017/07/14 21:00:30, msw wrote: > nit: use views::View* target = to avoid the two static casts. I think we still need at least one cast here. Since TrayBubbleView and SystemTray is not the base lass of each other, there will has "incompatible operand types" here without cast. https://codereview.chromium.org/2961313003/diff/180001/ash/system/tray/tray_b... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2961313003/diff/180001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.cc:428: // in maximize mode, in order to make sure that the tray bubble during On 2017/07/14 21:00:30, msw wrote: > nit: "in maximize mode, to avoid tray bubble and shelf overlap." Done.
Nice, thanks for the cleanup! lgtm with some comments. https://codereview.chromium.org/2961313003/diff/180001/ash/system/tray/system... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2961313003/diff/180001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:112: ? static_cast<views::View*>( On 2017/07/15 02:46:29, minch1 wrote: > On 2017/07/14 21:00:30, msw wrote: > > nit: use views::View* target = to avoid the two static casts. > > I think we still need at least one cast here. Since TrayBubbleView and > SystemTray is not the base lass of each other, there will has "incompatible > operand types" here without cast. Acknowledged. https://codereview.chromium.org/2961313003/diff/200001/ash/system/tray/tray_b... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2961313003/diff/200001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.cc:279: if (drag_controller_ && !drag_controller_->ProcessGestureEvent(event, this)) Change this conditional to run the base impl if there is no drag controller: if (!drag_controller || !drag_controller_->ProcessGestureEvent(event, this)) https://codereview.chromium.org/2961313003/diff/200001/ash/system/tray/tray_b... File ash/system/tray/tray_background_view.h (right): https://codereview.chromium.org/2961313003/diff/200001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.h:51: void OnGestureEvent(ui::GestureEvent* event) override; nit: reorder this and the definition to match views::View. https://codereview.chromium.org/2961313003/diff/200001/ui/views/bubble/tray_b... File ui/views/bubble/tray_bubble_view.h (right): https://codereview.chromium.org/2961313003/diff/200001/ui/views/bubble/tray_b... ui/views/bubble/tray_bubble_view.h:55: virtual void BubbleViewDestroyed() {} Move all virtual function definitions to the cc file, per style guide: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i...
The CQ bit was checked by minch@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...
msw@, tdanderson@, please help review the ps#12. Thanks. https://codereview.chromium.org/2961313003/diff/200001/ash/system/tray/tray_b... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2961313003/diff/200001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.cc:279: if (drag_controller_ && !drag_controller_->ProcessGestureEvent(event, this)) On 2017/07/17 19:28:01, msw wrote: > Change this conditional to run the base impl if there is no drag controller: > if (!drag_controller || !drag_controller_->ProcessGestureEvent(event, this)) Done. https://codereview.chromium.org/2961313003/diff/200001/ash/system/tray/tray_b... File ash/system/tray/tray_background_view.h (right): https://codereview.chromium.org/2961313003/diff/200001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.h:51: void OnGestureEvent(ui::GestureEvent* event) override; On 2017/07/17 19:28:01, msw wrote: > nit: reorder this and the definition to match views::View. Done. https://codereview.chromium.org/2961313003/diff/200001/ui/views/bubble/tray_b... File ui/views/bubble/tray_bubble_view.h (right): https://codereview.chromium.org/2961313003/diff/200001/ui/views/bubble/tray_b... ui/views/bubble/tray_bubble_view.h:55: virtual void BubbleViewDestroyed() {} On 2017/07/17 19:28:01, msw wrote: > Move all virtual function definitions to the cc file, per style guide: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i... Done.
lgtm with a minor optional nit. https://codereview.chromium.org/2961313003/diff/220001/ui/views/bubble/tray_b... File ui/views/bubble/tray_bubble_view.h (right): https://codereview.chromium.org/2961313003/diff/220001/ui/views/bubble/tray_b... ui/views/bubble/tray_bubble_view.h:51: virtual ~Delegate() {} optional nit: move this to the cc file too.
Please see my comments below for Patch Set 12. https://codereview.chromium.org/2961313003/diff/220001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2961313003/diff/220001/ash/BUILD.gn#newcode3 ash/BUILD.gn:3: # found in the LICENSE file. side note: please re-ping your PM for a specific listing of the metrics they want collected for all of this new behavior. These can be landed in a follow-on CL. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/system... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:109: void DispatchGestureEvent(ui::GestureEvent event, bool drag_on_bubble) { It looks like |event| is being passed by value from line 85. Is that intentional? https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:113: : static_cast<views::View*>(system_tray); is this static_cast necessary? https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:142: TEST_F(SystemTrayTest, SwipingOnShelfDuringAnimation) { This CL lets any TrayBackgroundView with an associated bubble have the swipe up/down behavior while in maximize mode. So ideally, all of the tests you've added in SystemTrayTest for the swipe up/down behavior should probably be testing this on TrayBackgroundView / TrayDragController directly rather than for a specific surface. I suggest you add a TODO(minch) for this work, and that is OK to handle as a follow-on. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:343: // Swiping on opened system tray bubble. Suggested: "Tests for swiping down on an open system tray bubble in order to close it." https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/tray_b... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.cc:184: drag_controller_ = base::MakeUnique<TrayDragController>(shelf); Instantiating |drag_controller_| here means that every TBV will have a TrayDragController. But certain subclasses shouldn't have them, e.g., such as OverviewButtonTray. Shouldn't you only be instantiating the member for system tray, notifications, ime, and palette? https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/tray_b... File ash/system/tray/tray_background_view.h (right): https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.h:62: // Gets the associated tray bubble view. nit: "Returns the associated tray bubble view, if one exists. Otherwise returns nullptr." https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.h:64: nit: "if it exists and is currently showing" https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.h:66: virtual void CloseBubble() {} The no-op implementations of CloseBubble() and ShowBubble() should go in the .cc, not here in the .h. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.h:68: // Shows the associated tray bubble. nit: "if one exists" https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... File ash/system/tray_drag_controller.cc (right): https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... ash/system/tray_drag_controller.cc:27: bool should_start_dragging = StartGestureDrag(*event); nit: const https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... ash/system/tray_drag_controller.cc:103: int bounds_y = nit: const https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... ash/system/tray_drag_controller.cc:114: // fling was fast enough and in the correct direction. If |is_on_bubble_| is true and I'm flinging downward with sufficient velocity, shouldn't that close the bubble? https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... File ash/system/tray_drag_controller.h (right): https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... ash/system/tray_drag_controller.h:15: class ASH_EXPORT TrayDragController { Please add brief class-level documentation here. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... ash/system/tray_drag_controller.h:23: // appropriate. Returns true if the gesture has been handled and it should not It looks like you return true if and only if |event| is being marked as handled. Can you change ProcessGestureEvent() to a void return type, and instead just check if the event is handled after the ProcessGestureEvent() call site(s)? https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... ash/system/tray_drag_controller.h:34: // Update the bounds of the tray bubbles according to nit: 'bubble' instead of 'bubbles'. Similar for line 38. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... ash/system/tray_drag_controller.h:47: TrayBackgroundView* tray_view_ = nullptr; nit: please add brief documentation for these two members https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... ash/system/tray_drag_controller.h:55: // True if the user is in the process of gesture-dragging to open the tray Isn't this also true if the user is gesture-dragging to close an already-open bubble? https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... ash/system/tray_drag_controller.h:61: bool is_on_bubble_ = false; Should you also say "Only valid if |is_in_drag_| is true" here as well? https://codereview.chromium.org/2961313003/diff/220001/ash/system/web_notific... File ash/system/web_notification/web_notification_tray.cc (left): https://codereview.chromium.org/2961313003/diff/220001/ash/system/web_notific... ash/system/web_notification/web_notification_tray.cc:403: void WebNotificationTray::ShowMessageCenterBubble() { This also needs to be removed from the .h https://codereview.chromium.org/2961313003/diff/220001/ui/views/bubble/tray_b... File ui/views/bubble/tray_bubble_view.h (right): https://codereview.chromium.org/2961313003/diff/220001/ui/views/bubble/tray_b... ui/views/bubble/tray_bubble_view.h:87: virtual bool ProcessGestureEventForBubble(ui::GestureEvent* event); Please add some documentation here for this new function.
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 minch@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.
Thanks tdanderson@, please help review ps#13. https://codereview.chromium.org/2961313003/diff/220001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2961313003/diff/220001/ash/BUILD.gn#newcode3 ash/BUILD.gn:3: # found in the LICENSE file. On 2017/07/17 22:44:36, tdanderson wrote: > side note: please re-ping your PM for a specific listing of the metrics they > want collected for all of this new behavior. These can be landed in a follow-on > CL. Thanks for mention this. I synced with PM about this. He preferred to have different metrics for different results of one swiping. e.g, swiping up on the system tray, finally close or open the system tray bubble should have different metrics (or one metric that can be split into two?). Will do this in a follow-on cl. Thanks. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/system... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:109: void DispatchGestureEvent(ui::GestureEvent event, bool drag_on_bubble) { On 2017/07/17 22:44:36, tdanderson wrote: > It looks like |event| is being passed by value from line 85. Is that > intentional? Changed it to pointer. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:113: : static_cast<views::View*>(system_tray); On 2017/07/17 22:44:36, tdanderson wrote: > is this static_cast necessary? I think so. Even though SystemTray and TrayBubbleView both are the subclass of views::View. But they are not the superclass of each other. The expression can't determine the type before assignment without static_cast here. There is an "incompatible operand types" compiler error here without cast. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:142: TEST_F(SystemTrayTest, SwipingOnShelfDuringAnimation) { On 2017/07/17 22:44:36, tdanderson wrote: > This CL lets any TrayBackgroundView with an associated bubble have the swipe > up/down behavior while in maximize mode. So ideally, all of the tests you've > added in SystemTrayTest for the swipe up/down behavior should probably be > testing this on TrayBackgroundView / TrayDragController directly rather than for > a specific surface. I suggest you add a TODO(minch) for this work, and that is > OK to handle as a follow-on. Thanks. Added TODO for this. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:343: // Swiping on opened system tray bubble. On 2017/07/17 22:44:36, tdanderson wrote: > Suggested: "Tests for swiping down on an open system tray bubble in order to > close it." Done. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/tray_b... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.cc:184: drag_controller_ = base::MakeUnique<TrayDragController>(shelf); On 2017/07/17 22:44:36, tdanderson wrote: > Instantiating |drag_controller_| here means that every TBV will have a > TrayDragController. But certain subclasses shouldn't have them, e.g., such as > OverviewButtonTray. Shouldn't you only be instantiating the member for system > tray, notifications, ime, and palette? Done. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/tray_b... File ash/system/tray/tray_background_view.h (right): https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.h:62: // Gets the associated tray bubble view. On 2017/07/17 22:44:37, tdanderson wrote: > nit: "Returns the associated tray bubble view, if one exists. Otherwise returns > nullptr." Done. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.h:64: On 2017/07/17 22:44:36, tdanderson wrote: > nit: "if it exists and is currently showing" Done. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.h:66: virtual void CloseBubble() {} On 2017/07/17 22:44:37, tdanderson wrote: > The no-op implementations of CloseBubble() and ShowBubble() should go in the > .cc, not here in the .h. Done. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/tray_b... ash/system/tray/tray_background_view.h:68: // Shows the associated tray bubble. On 2017/07/17 22:44:37, tdanderson wrote: > nit: "if one exists" Done. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... File ash/system/tray_drag_controller.cc (right): https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... ash/system/tray_drag_controller.cc:27: bool should_start_dragging = StartGestureDrag(*event); On 2017/07/17 22:44:37, tdanderson wrote: > nit: const Done. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... ash/system/tray_drag_controller.cc:103: int bounds_y = On 2017/07/17 22:44:37, tdanderson wrote: > nit: const Done. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... ash/system/tray_drag_controller.cc:114: // fling was fast enough and in the correct direction. On 2017/07/17 22:44:37, tdanderson wrote: > If |is_on_bubble_| is true and I'm flinging downward with sufficient velocity, > shouldn't that close the bubble? Yes, that should close the bubble. And the logic here will achieve that. I am sorry, I don't quite get the concern here. Should I update the comment or logic here? https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... File ash/system/tray_drag_controller.h (right): https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... ash/system/tray_drag_controller.h:15: class ASH_EXPORT TrayDragController { On 2017/07/17 22:44:37, tdanderson wrote: > Please add brief class-level documentation here. Done. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... ash/system/tray_drag_controller.h:23: // appropriate. Returns true if the gesture has been handled and it should not On 2017/07/17 22:44:37, tdanderson wrote: > It looks like you return true if and only if |event| is being marked as handled. > Can you change ProcessGestureEvent() to a void return type, and instead just > check if the event is handled after the ProcessGestureEvent() call site(s)? Done. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... ash/system/tray_drag_controller.h:34: // Update the bounds of the tray bubbles according to On 2017/07/17 22:44:37, tdanderson wrote: > nit: 'bubble' instead of 'bubbles'. Similar for line 38. Done. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... ash/system/tray_drag_controller.h:47: TrayBackgroundView* tray_view_ = nullptr; On 2017/07/17 22:44:37, tdanderson wrote: > nit: please add brief documentation for these two members Done. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... ash/system/tray_drag_controller.h:55: // True if the user is in the process of gesture-dragging to open the tray On 2017/07/17 22:44:37, tdanderson wrote: > Isn't this also true if the user is gesture-dragging to close an already-open > bubble? Done. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... ash/system/tray_drag_controller.h:61: bool is_on_bubble_ = false; On 2017/07/17 22:44:37, tdanderson wrote: > Should you also say "Only valid if |is_in_drag_| is true" here as well? Done. https://codereview.chromium.org/2961313003/diff/220001/ash/system/web_notific... File ash/system/web_notification/web_notification_tray.cc (left): https://codereview.chromium.org/2961313003/diff/220001/ash/system/web_notific... ash/system/web_notification/web_notification_tray.cc:403: void WebNotificationTray::ShowMessageCenterBubble() { On 2017/07/17 22:44:37, tdanderson wrote: > This also needs to be removed from the .h Done. https://codereview.chromium.org/2961313003/diff/220001/ui/views/bubble/tray_b... File ui/views/bubble/tray_bubble_view.h (right): https://codereview.chromium.org/2961313003/diff/220001/ui/views/bubble/tray_b... ui/views/bubble/tray_bubble_view.h:51: virtual ~Delegate() {} On 2017/07/17 20:45:03, msw wrote: > optional nit: move this to the cc file too. Done. https://codereview.chromium.org/2961313003/diff/220001/ui/views/bubble/tray_b... ui/views/bubble/tray_bubble_view.h:87: virtual bool ProcessGestureEventForBubble(ui::GestureEvent* event); On 2017/07/17 22:44:38, tdanderson wrote: > Please add some documentation here for this new function. Done.
Patch Set 13 LGTM. Thanks for all your work in arriving at a good design and persisting through all the changes! https://codereview.chromium.org/2961313003/diff/220001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2961313003/diff/220001/ash/BUILD.gn#newcode3 ash/BUILD.gn:3: # found in the LICENSE file. On 2017/07/18 03:58:58, minch1 wrote: > On 2017/07/17 22:44:36, tdanderson wrote: > > side note: please re-ping your PM for a specific listing of the metrics they > > want collected for all of this new behavior. These can be landed in a > follow-on > > CL. > > Thanks for mention this. I synced with PM about this. He preferred to have > different metrics for different results of one swiping. e.g, swiping up on the > system tray, finally close or open the system tray bubble should have different > metrics (or one metric that can be split into two?). > Will do this in a follow-on cl. Thanks. Acknowledged. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/system... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:113: : static_cast<views::View*>(system_tray); On 2017/07/18 03:58:58, minch1 wrote: > On 2017/07/17 22:44:36, tdanderson wrote: > > is this static_cast necessary? > > I think so. Even though SystemTray and TrayBubbleView both are the subclass of > views::View. But they are not the superclass of each other. The expression can't > determine the type before assignment without static_cast here. There is an > "incompatible operand types" compiler error here without cast. Acknowledged. https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... File ash/system/tray_drag_controller.cc (right): https://codereview.chromium.org/2961313003/diff/220001/ash/system/tray_drag_c... ash/system/tray_drag_controller.cc:114: // fling was fast enough and in the correct direction. On 2017/07/18 03:58:59, minch1 wrote: > On 2017/07/17 22:44:37, tdanderson wrote: > > If |is_on_bubble_| is true and I'm flinging downward with sufficient velocity, > > shouldn't that close the bubble? > > Yes, that should close the bubble. And the logic here will achieve that. I am > sorry, I don't quite get the concern here. Should I update the comment or logic > here? My mistake, sorry, thank you for clarifying. I had mis-read some of the code. This looks good. https://codereview.chromium.org/2961313003/diff/240001/ash/system/tray_drag_c... File ash/system/tray_drag_controller.h (right): https://codereview.chromium.org/2961313003/diff/240001/ash/system/tray_drag_c... ash/system/tray_drag_controller.h:18: // WebNotificationTray and their's accosiated tray bubbles can be dragged. Thanks, this is a nice description. One small typo nit, though: "their associated".
nice CL. just one nit. lgtm https://codereview.chromium.org/2961313003/diff/240001/ash/system/palette/pal... File ash/system/palette/palette_tray.cc (right): https://codereview.chromium.org/2961313003/diff/240001/ash/system/palette/pal... ash/system/palette/palette_tray.cc:391: bubble_.reset(new ash::TrayBubbleWrapper(this, bubble_view)); nit: makeunique
The CQ bit was checked by minch@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 minch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sammiequon@chromium.org, xdai@chromium.org, msw@chromium.org, tdanderson@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2961313003/#ps260001 (title: "Fixed nits.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for review. https://codereview.chromium.org/2961313003/diff/240001/ash/system/palette/pal... File ash/system/palette/palette_tray.cc (right): https://codereview.chromium.org/2961313003/diff/240001/ash/system/palette/pal... ash/system/palette/palette_tray.cc:391: bubble_.reset(new ash::TrayBubbleWrapper(this, bubble_view)); On 2017/07/18 17:20:38, oshima wrote: > nit: makeunique Done. https://codereview.chromium.org/2961313003/diff/240001/ash/system/tray_drag_c... File ash/system/tray_drag_controller.h (right): https://codereview.chromium.org/2961313003/diff/240001/ash/system/tray_drag_c... ash/system/tray_drag_controller.h:18: // WebNotificationTray and their's accosiated tray bubbles can be dragged. On 2017/07/18 16:38:05, tdanderson wrote: > Thanks, this is a nice description. One small typo nit, though: "their > associated". Done.
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1500408079420900,
"parent_rev": "fb1364df3ad900158e6822cd05d3d1114138058c", "commit_rev":
"d8633937d1ea5d357a182247ae8e977966d6a67e"}
Message was sent while issue was closed.
Description was changed from ========== Touch gestures for System Tray/ IME/ Stylus/ Notifications Swiping up on the System Tray/ IME/ Stylus/ Notifications buttons in status area should open the associated bubble. Swiping down on the opened bubble should close the associated bubble. Changes: 1.Added the tray_drag_controller to extract the logic of dragging behavior from system_tray. 2.Added help functions in tray_background_view to get the state of associated tray bubble. 3.Added one interface ProcessGestureEventForBubble in TrayBubbleView::Delegate to help process the dragging that happened on the tray bubble. BUG=735994,735996 ========== to ========== Touch gestures for System Tray/ IME/ Stylus/ Notifications Swiping up on the System Tray/ IME/ Stylus/ Notifications buttons in status area should open the associated bubble. Swiping down on the opened bubble should close the associated bubble. Changes: 1.Added the tray_drag_controller to extract the logic of dragging behavior from system_tray. 2.Added help functions in tray_background_view to get the state of associated tray bubble. 3.Added one interface ProcessGestureEventForBubble in TrayBubbleView::Delegate to help process the dragging that happened on the tray bubble. BUG=735994,735996 Review-Url: https://codereview.chromium.org/2961313003 Cr-Commit-Position: refs/heads/master@{#487573} Committed: https://chromium.googlesource.com/chromium/src/+/d8633937d1ea5d357a182247ae8e... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/d8633937d1ea5d357a182247ae8e... |
