Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
545393002:
Adding split view divider widget. (Closed)
|
Created:
6 years, 3 months ago by mfomitchev Modified:
6 years, 2 months ago CC:
chromium-reviews, dcheng Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdding split view divider widget.
Adding split view divider widget which could be dragged to exit the split view.
BUG=403207, 408691
Committed: https://crrev.com/b47c16517f0882cbabf8c9aa9ce66711f026fd5b
Cr-Commit-Position: refs/heads/master@{#296351}
Patch Set 1 #Patch Set 2 : git cl format #
Total comments: 22
Patch Set 3 : Implementing targeting for the divider, addressing mukai's feedback. #
Total comments: 22
Patch Set 4 : Rebase #Patch Set 5 : Minor format fixes. #Patch Set 6 : Addressing sadrul's feedback #
Total comments: 12
Patch Set 7 : Addressing sadrul's feedback (contd) #Patch Set 8 : Getting rid of divider_window, addressing tdanderson's feedback. #
Total comments: 59
Patch Set 9 : Addressing sadrul's feedback (contd). #
Total comments: 2
Patch Set 10 : Addressing Peter's feedback. #Patch Set 11 : Code format #
Total comments: 21
Patch Set 12 : Addressing pkotwicz 's feedback (contd) #
Total comments: 1
Patch Set 13 : Rebase + a couple of TODOs #
Total comments: 1
Patch Set 14 : Addressing pkotwicz feedback (contd) #Patch Set 15 : Fixing the unit test #Patch Set 16 : Fixing a NOTREACHED in the test #
Messages
Total messages: 45 (6 generated)
mfomitchev@chromium.org changed reviewers: + mukai@chromium.org, oshima@chromium.org, sadrul@chromium.org
PTAL tl;dr: Any thoughts about unifying DragHandle::ScrollDelegate and BezelController::ScrollDelegate? DragHandle::ScrollDelegate is obviously very similar to BezelController::ScrollDelegate. I think it would be nice to unify these interfaces. However if we unify combine these interfaces into one, the delegate would need to know whether the callback is coming from the bezel (and which bezel), or the "drag handle". One way would be to add some sort of an id to all the callback methods. E.g. a string id with fully qualified class name as the prefix to the id to avoid collision. Another would be to add the scroll origin coordinates - for the case of the split view this should be sufficient, but perhaps not in a general case. Also, we'd need to add the origin to CanScroll() which seems a bit unfortunate. Any thoughts?
Apart from the coding, I am wondering if using a widget for the splitter is the best idea. As far as I tested on device, it is really hard to grab the splitter when shown. Maybe, simply using a layer for splitter and handling all gesture events in bezel_controller could be a better solution (in that case that is not a 'bezel' controller anymore...). https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:146: const int kDividerWidth = kDragHandleWidth + 2 * kDragHandleInnerMargin; These const values should be in the anonymous namespace, usually appearing at the top of the file. https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:165: widget->SetSize(size); why not simply SetSize(gfx::Size(...))? Also, use param.initial_bounds instead of SetSize. https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... File athena/wm/split_view_controller.h (right): https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... athena/wm/split_view_controller.h:108: virtual void OnActivityOrderHasChanged() OVERRIDE; You'll have to update and reimplement the new observer methods. https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... athena/wm/split_view_controller.h:126: scoped_ptr<views::Widget> divider_widget_; Usually views::Widget is used as self-owned, not scoped_ptr with WIDGET_OWNS_NATIVE_WIDGET.
On 2014/09/08 19:32:00, Jun Mukai wrote: > Apart from the coding, I am wondering if using a widget for the splitter is the > best idea. > > As far as I tested on device, it is really hard to grab the splitter when shown. > Maybe, simply using a layer for splitter and handling all gesture events in > bezel_controller could be a better solution (in that case that is not a 'bezel' > controller anymore...). Thanks for pointing this out Jun! What if instead of removing the widget completely, we made the widget's size to be just the size of the visible handle and used WindowTargeter to extend the touch bounds of the drag handle widget? Something similar to EasyResizeWindowTargeter, but applied to a single window (or a set of windows). It seems to me that would be a more elegant solution than implementing all of the targeting logic inside the BezelController, and it would also be reusable if we wanted to have similar functionality in other places. What do you think?
On 2014/09/09 14:40:30, mfomitchev wrote: > On 2014/09/08 19:32:00, Jun Mukai wrote: > > Apart from the coding, I am wondering if using a widget for the splitter is > the > > best idea. > > > > As far as I tested on device, it is really hard to grab the splitter when > shown. > > Maybe, simply using a layer for splitter and handling all gesture events in > > bezel_controller could be a better solution (in that case that is not a > 'bezel' > > controller anymore...). > > Thanks for pointing this out Jun! What if instead of removing the widget > completely, > we made the widget's size to be just the size of the visible handle and used > WindowTargeter to extend the touch bounds of the drag handle widget? Something > similar to EasyResizeWindowTargeter, but applied to a single window (or a set of > windows). > > It seems to me that would be a more elegant solution than implementing all of > the > targeting logic inside the BezelController, and it would also be reusable if we > wanted to have similar functionality in other places. > > What do you think? It is up to you, but WindowTargeter sounds like an overkill to me. Also, we already have the gesture handling logic in athena/wm, keeping single place for split-view gesture control would be a bit more reasonable.
https://codereview.chromium.org/545393002/diff/20001/athena/common/drag_handl... File athena/common/drag_handle.cc (right): https://codereview.chromium.org/545393002/diff/20001/athena/common/drag_handl... athena/common/drag_handle.cc:137: delegate_->HandleScrollEnd(); Do we not want to take the velocity into consideration to decide whether to complete the action or not? https://codereview.chromium.org/545393002/diff/20001/athena/common/drag_handle.h File athena/common/drag_handle.h (right): https://codereview.chromium.org/545393002/diff/20001/athena/common/drag_handl... athena/common/drag_handle.h:39: virtual bool HandleCanScroll() = 0; Maybe HandleScrollBegin() could return a bool indicating whether the delegate is going to handle the drag gesture or not. https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:108: Show() should be outside of the if? https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:164: gfx::Size(kDividerWidth, container_->bounds().height()); The minimized home-card allows swiping anywhere (i.e. not just the white-bar in the middle). I think we would want the same behaviour for this divider widget too? (i.e. the divider should have the full height of the screen, and allow dragging from anywhere, not just the white part in the center) https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:223: divider_widget_->Show(); Consider ShowInactive(), but perhaps the widget shouldn't be activate-able? https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:286: if (divider_widget_) { There's a CHECK in DeactivateSplitMode()/UpdateLayout() for divider_widget_. So I guess we expect divider_widget_ to be non-null here too, and remove the if? https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:305: if (divider_widget_) ditto
https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:146: const int kDividerWidth = kDragHandleWidth + 2 * kDragHandleInnerMargin; On 2014/09/08 19:32:00, Jun Mukai wrote: > These const values should be in the anonymous namespace, usually appearing at > the top of the file. Done. https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:165: widget->SetSize(size); On 2014/09/08 19:32:00, Jun Mukai wrote: > why not simply SetSize(gfx::Size(...))? > > Also, use param.initial_bounds instead of SetSize. Done. There's no intial_bounds, I just used bounds. https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... File athena/wm/split_view_controller.h (right): https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... athena/wm/split_view_controller.h:108: virtual void OnActivityOrderHasChanged() OVERRIDE; On 2014/09/08 19:32:00, Jun Mukai wrote: > You'll have to update and reimplement the new observer methods. Done. https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... athena/wm/split_view_controller.h:126: scoped_ptr<views::Widget> divider_widget_; On 2014/09/08 19:32:00, Jun Mukai wrote: > Usually views::Widget is used as self-owned, not scoped_ptr with > WIDGET_OWNS_NATIVE_WIDGET. Done. The widget is now created once and never destroyed. I had to add a check for the state to OnOverviewModeExit().
https://codereview.chromium.org/545393002/diff/20001/athena/common/drag_handl... File athena/common/drag_handle.cc (right): https://codereview.chromium.org/545393002/diff/20001/athena/common/drag_handl... athena/common/drag_handle.cc:137: delegate_->HandleScrollEnd(); That sounds like a good idea. I was thinking of doing this for the bezel gestures as well - swiping hard should probably switch to the next activity even if you don't drag the handle far enough to the other side. I created a separate bug to track this: crbug.com/413880 https://codereview.chromium.org/545393002/diff/20001/athena/common/drag_handle.h File athena/common/drag_handle.h (right): https://codereview.chromium.org/545393002/diff/20001/athena/common/drag_handl... athena/common/drag_handle.h:39: virtual bool HandleCanScroll() = 0; Yeah.. if we are not going to unify DragHandle::ScrollDelegate with BezelController::ScrollDelegate, it might be worth it. Although it might come in handy in the future (e.g. for using a different highlight color when HandleCanScroll() is false). Also being able to short-circuit in DragHandle::OnGestureEvent is kind of nice... https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:108: I changed the code so that Show/Hid is only done from SetState or when we enter/exit overview mode. https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:164: gfx::Size(kDividerWidth, container_->bounds().height()); I was trying to minimize the height of the widget, because bigger widget means we are stealing more pixels from the content. Especially now with priority window targeter installed. "Content not chrome" and all that... It should be easy enough to change if needed. https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:223: divider_widget_->Show(); ShowInactive() explicitly changes the state to inactive if it was active. I wanted this to just show the widget w/o changing its "active" state. If the widget is non-activatable, will it still be able to receive events? BTW, I have changed the code so that this is only done in SetState() and on overview mode enter/exit, so it's less of an issue now. https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:286: if (divider_widget_) { On 2014/09/12 04:30:36, sadrul wrote: > There's a CHECK in DeactivateSplitMode()/UpdateLayout() for divider_widget_. So > I guess we expect divider_widget_ to be non-null here too, and remove the if? Done. https://codereview.chromium.org/545393002/diff/20001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:305: if (divider_widget_) On 2014/09/12 04:30:36, sadrul wrote: > ditto Done.
mfomitchev@chromium.org changed reviewers: + tdanderson@chromium.org
https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:108: virtual void OnWindowDestroying(aura::Window* window) OVERRIDE {} Some have {} in the same line, some have these in separate lines. It'd be good to be consistent (maybe run through 'git cl format') https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:314: divider_window_.reset(new aura::Window(new EmptyWindowDelegate())); What's the purpose of |divider_window_|? https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:333: params.activatable = views::Widget::InitParams::ACTIVATABLE_YES; Make this ACTIVATABLE_NO (it should still be able to receive events after that). https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:354: scoped_ptr<views::ViewTargeter>(targeter)); |divider_view| is the only view (other than the internal root-view; but |divider_view| should completely overlap the root-view) in the Widget. So you shouldn't need this targeter. What target do the gesture events go to without this targeter? https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:641: if (divider_widget_) { Remove {} https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:647: if (state_ != INACTIVE) { ditto https://codereview.chromium.org/545393002/diff/40001/athena/wm/window_manager... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/545393002/diff/40001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:158: split_view_controller_.reset(); RemoveObserver() first? https://codereview.chromium.org/545393002/diff/40001/ui/views/widget/native_w... File ui/views/widget/native_widget_aura.cc (right): https://codereview.chromium.org/545393002/diff/40001/ui/views/widget/native_w... ui/views/widget/native_widget_aura.cc:862: input_method->DispatchKeyEvent(*event); Remove from this patch.
https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:108: virtual void OnWindowDestroying(aura::Window* window) OVERRIDE {} On 2014/09/15 14:32:29, sadrul wrote: > Some have {} in the same line, some have these in separate lines. It'd be good > to be consistent (maybe run through 'git cl format') Done. All methods that don't do anything have {} on the same line now. https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:314: divider_window_.reset(new aura::Window(new EmptyWindowDelegate())); It covers the empty space between two windows in split view (that is not covered by the handle) and consumes events. If it wasn't there, the window underneath (or the background) would show through and could be interacted with. I have added a comment to .h to clarify. https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:333: params.activatable = views::Widget::InitParams::ACTIVATABLE_YES; On 2014/09/15 14:32:29, sadrul wrote: > Make this ACTIVATABLE_NO (it should still be able to receive events after that). Done. Thanks. https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:354: scoped_ptr<views::ViewTargeter>(targeter)); It doesn't work if I remove this view targeter. It returns the root view itself. It rejects divider_view when the center of the touch is outside the view, because the overlap with the touch area is <60% (the view is relatively tall). I guess root view is its default fallback. I have expanded on the TODO comment above this code to highlight this issue. I think we should fix this, but I'd rather we did it outside of this CL. https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:641: if (divider_widget_) { On 2014/09/15 14:32:29, sadrul wrote: > Remove {} Done. https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:647: if (state_ != INACTIVE) { On 2014/09/15 14:32:29, sadrul wrote: > ditto Done. https://codereview.chromium.org/545393002/diff/40001/athena/wm/window_manager... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/545393002/diff/40001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:158: split_view_controller_.reset(); On 2014/09/15 14:32:29, sadrul wrote: > RemoveObserver() first? Done. https://codereview.chromium.org/545393002/diff/40001/ui/views/widget/native_w... File ui/views/widget/native_widget_aura.cc (right): https://codereview.chromium.org/545393002/diff/40001/ui/views/widget/native_w... ui/views/widget/native_widget_aura.cc:862: input_method->DispatchKeyEvent(*event); Ok, I will create a separate one for this. And actually after making the widget ACTIVATABLE_NO, it doesn't crash here anymore, so we can submit the two patches in any order.
[Note that a couple of the replies below are to the comments in patchset 3] https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:314: divider_window_.reset(new aura::Window(new EmptyWindowDelegate())); On 2014/09/15 16:37:23, mfomitchev wrote: > It covers the empty space between two windows in split view (that is not covered > by the handle) and consumes events. If it wasn't there, the window underneath > (or the background) would show through and could be interacted with. I have > added a comment to .h to clarify. You don't need a separate window for this. You should have the widget span the entire height of the screen instead. (we do this for the minimized home-card at the bottom) https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:354: scoped_ptr<views::ViewTargeter>(targeter)); On 2014/09/15 16:37:23, mfomitchev wrote: > It doesn't work if I remove this view targeter. It returns the root view itself. > It rejects divider_view when the center of the touch is outside the view, > because the overlap with the touch area is <60% (the view is relatively tall). I > guess root view is its default fallback. I have expanded on the TODO comment > above this code to highlight this issue. I think we should fix this, but I'd > rather we did it outside of this CL. This is a pretty bad bug. You should file a crbug and reference it from the TODO here. https://codereview.chromium.org/545393002/diff/100001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/100001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:381: InitializeDivider(); Remove {}
https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:314: divider_window_.reset(new aura::Window(new EmptyWindowDelegate())); If we do that, we will steal the entire right edge from the content in the left window and the entire left edge from the content in the right window because of the "priority" window targeting for the widget's window. I think it's good to minimize number of pixels we steal from content. Content not chrome, and all that. Minimized home-card is different - the UX actually requires us to respond on any swipe from the bottom. Plus we don't use window targeting on the minimized home card. (Hopefully we never do that and figure out how to leverage the bezel instead if we need to improve the targeting there). https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:354: scoped_ptr<views::ViewTargeter>(targeter)); On 2014/09/15 16:53:31, sadrul wrote: > On 2014/09/15 16:37:23, mfomitchev wrote: > > It doesn't work if I remove this view targeter. It returns the root view > itself. > > It rejects divider_view when the center of the touch is outside the view, > > because the overlap with the touch area is <60% (the view is relatively tall). > I > > guess root view is its default fallback. I have expanded on the TODO comment > > above this code to highlight this issue. I think we should fix this, but I'd > > rather we did it outside of this CL. > > This is a pretty bad bug. You should file a crbug and reference it from the TODO > here. Done. https://codereview.chromium.org/545393002/diff/100001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/100001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:381: InitializeDivider(); On 2014/09/15 16:53:31, sadrul wrote: > Remove {} Done.
View targeting stuff lgtm with the TODO suggested (I will look into that). Came across a few nits while reading through other parts of the CL, though. https://codereview.chromium.org/545393002/diff/100001/athena/util/drag_handle.cc File athena/util/drag_handle.cc (right): https://codereview.chromium.org/545393002/diff/100001/athena/util/drag_handle... athena/util/drag_handle.cc:18: class DragHandleView : public views::View { nit: include brief class-level documentation. https://codereview.chromium.org/545393002/diff/100001/athena/util/drag_handle... athena/util/drag_handle.cc:106: } nit: newline after 106 https://codereview.chromium.org/545393002/diff/100001/athena/util/drag_handle... athena/util/drag_handle.cc:134: } else if (event->type() == ui::ET_SCROLL_FLING_START) { nit: SCROLL_END || SCROLL_FLING_START can be combined into one case. https://codereview.chromium.org/545393002/diff/100001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/100001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:49: // views::ViewTargeterDelegate nit: missing : at end. https://codereview.chromium.org/545393002/diff/100001/athena/wm/split_view_co... File athena/wm/split_view_controller.h (right): https://codereview.chromium.org/545393002/diff/100001/athena/wm/split_view_co... athena/wm/split_view_controller.h:140: scoped_ptr<aura::Window> divider_window_; nit: newline after 140.
https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:314: divider_window_.reset(new aura::Window(new EmptyWindowDelegate())); On 2014/09/15 17:44:20, mfomitchev wrote: > If we do that, we will steal the entire right edge from the content in the left > window and the entire left edge from the content in the right window because of > the "priority" window targeting for the widget's window. I think it's good to > minimize number of pixels we steal from content. Content not chrome, and all > that. It still doesn't make sense to have to maintain two separate windows at the same time for the divider. The window-targeter should be more clever about its targeting so it doesn't steal pixels from the content. > > Minimized home-card is different - the UX actually requires us to respond on any > swipe from the bottom. Plus we don't use window targeting on the minimized home > card. (Hopefully we never do that and figure out how to leverage the bezel > instead if we need to improve the targeting there).
https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:314: divider_window_.reset(new aura::Window(new EmptyWindowDelegate())); > It still doesn't make sense to have to maintain two separate windows at the same > time for the divider. The window-targeter should be more clever about its > targeting so it doesn't steal pixels from the content. I guess we could do that.. - I guess PriorityWindowTargeter would need to take divider_view as its argument and use its bounds within the widget to determine if the event should be targeted to the widget's window. - We'd need to put divider_view into another view with a BoxLayout to have it centered within the widget. - We'd also need to make the view targeter smarter - it shouldn't just target the divider_view every time anymore - it would need to consider the location of the gesture, since gestures inside the widget but away from the drag handle should't target divider_view. - We wouldn't be able to use WINDOW_LAYER_SOLID_COLOR anymore, so the divider layers might become more expensive to paint/store in memory. I undertsand it's a hassle to manipulate two divider windows instead of one in split view controller, but we'd be adding complexity elsewhere, so I am not sure it's a definite improvement. If you still want me to go ahead with this change - do you mind if I make it in a separate CL? We may not need to do the view targeting here depending on what we do in core views.
On 2014/09/15 18:34:21, mfomitchev wrote: > https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... > File athena/wm/split_view_controller.cc (right): > > https://codereview.chromium.org/545393002/diff/40001/athena/wm/split_view_con... > athena/wm/split_view_controller.cc:314: divider_window_.reset(new > aura::Window(new EmptyWindowDelegate())); > > It still doesn't make sense to have to maintain two separate windows at the > same > > time for the divider. The window-targeter should be more clever about its > > targeting so it doesn't steal pixels from the content. > > I guess we could do that.. > - I guess PriorityWindowTargeter would need to take divider_view as its argument > and use its bounds within the widget to determine if the event should be > targeted to the widget's window. > - We'd need to put divider_view into another view with a BoxLayout to have it > centered within the widget. > - We'd also need to make the view targeter smarter - it shouldn't just target > the divider_view every time anymore - it would need to consider the location of > the gesture, since gestures inside the widget but away from the drag handle > should't target divider_view. > - We wouldn't be able to use WINDOW_LAYER_SOLID_COLOR anymore, so the divider > layers might become more expensive to paint/store in memory. These should be mostly trivial changes. I don't think the memory-cost here is prohibitive. (we will be using one less layer, so there's that too) > > I undertsand it's a hassle to manipulate two divider windows instead of one in > split view controller, but we'd be adding complexity elsewhere, so I am not sure > it's a definite improvement. > > If you still want me to go ahead with this change - do you mind if I make it in > a separate CL? We may not need to do the view targeting here depending on what > we do in core views. Since the changes are mostly trivial, and it would improve code clarity/readability/maintainability, I would rather land with the updated code.
Ok, I got rid of the divider window. I didn't change the view-level targeting, since it would just be throw away work if we address the issue in core views, and also I don't think it's too bad the way it is for now. Basically you can drag anywhere from the divider if you hit it right on, but the additional touch area is only added around the handle. https://codereview.chromium.org/545393002/diff/100001/athena/util/drag_handle.cc File athena/util/drag_handle.cc (right): https://codereview.chromium.org/545393002/diff/100001/athena/util/drag_handle... athena/util/drag_handle.cc:18: class DragHandleView : public views::View { On 2014/09/15 17:48:19, tdanderson wrote: > nit: include brief class-level documentation. Done. https://codereview.chromium.org/545393002/diff/100001/athena/util/drag_handle... athena/util/drag_handle.cc:106: } On 2014/09/15 17:48:19, tdanderson wrote: > nit: newline after 106 Done. https://codereview.chromium.org/545393002/diff/100001/athena/util/drag_handle... athena/util/drag_handle.cc:134: } else if (event->type() == ui::ET_SCROLL_FLING_START) { On 2014/09/15 17:48:19, tdanderson wrote: > nit: SCROLL_END || SCROLL_FLING_START can be combined into one case. Done. https://codereview.chromium.org/545393002/diff/100001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/100001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:49: // views::ViewTargeterDelegate On 2014/09/15 17:48:19, tdanderson wrote: > nit: missing : at end. Done. https://codereview.chromium.org/545393002/diff/100001/athena/wm/split_view_co... File athena/wm/split_view_controller.h (right): https://codereview.chromium.org/545393002/diff/100001/athena/wm/split_view_co... athena/wm/split_view_controller.h:140: scoped_ptr<aura::Window> divider_window_; On 2014/09/15 17:48:19, tdanderson wrote: > nit: newline after 140. Done.
Perhaps, this is for another CL, but if Javascript opens a popup window while a user is dragging the splitter around, one would expect the newly opened window to replace one of the windows in split view. This does not occur.
Aren't popups completely broken on Athena right now? I just tried http://www.w3schools.com/js/tryit.asp?filename=tryjs_confirm (in normal mode) - the popup window isn't even appearing.
JS alerts are completely broken in athena. Popups opened via window.open() are not.
For the sake of completeness, http://crbug.com/405594 tracks alert() and confirm() being broken on athena
On 2014/09/16 16:05:36, pkotwicz wrote: > Perhaps, this is for another CL, but if Javascript opens a popup window while a > user is dragging the splitter around, one would expect the newly opened window > to replace one of the windows in split view. This does not occur. Considering the drag gestures on the divider widget are expected to be of short-duration, it might make sense to just not show the window until the gesture ends? (I suspect we will also need to figure out the case where one of the split windows is a window.open()ed window, and the parent window closes it during the drag). But yeah, we should take care of these issues in a separate CL. https://codereview.chromium.org/545393002/diff/140001/athena/util/drag_handle.cc File athena/util/drag_handle.cc (right): https://codereview.chromium.org/545393002/diff/140001/athena/util/drag_handle... athena/util/drag_handle.cc:21: explicit DragHandleView(DragHandle::ScrollDirection scroll_direction, No explicit https://codereview.chromium.org/545393002/diff/140001/athena/wm/bezel_control... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/545393002/diff/140001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:63: } You would normally just do: if (...) return ...; if (...) return ...; return ...; https://codereview.chromium.org/545393002/diff/140001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:128: if (scroll_bezel_ != BEZEL_NONE) { Remove {} https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:42: // TODO(mfomitchev): Should this be moved to ui/views? No. This should be removed when crbug.com/414339 is fixed. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:64: // TODO(mfomitchev): This is a copy of EmptyWindowDelegate in You no longer need EmptyWindowDelegate. Remove. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:137: bool ShouldProcessEvent(ui::EventType event_type) { This is never used. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:158: gfx::Rect view_bounds_in_root = gfx::Rect(view_origin_in_root, view_size); Can you just use View::GetBoundsInScreen() instead? https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:181: event->root_location().y())) { Just .Contains(event->root_location()) https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:343: params.opacity = views::Widget::InitParams::TRANSLUCENT_WINDOW; Do we need the widget to be translucent? https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:362: new StaticViewTargeterDelegate(drag_handle_), I think this StaticViewTargeterDelegate() is leaking?
Ah, I see. It's actually worse than just opening the window in the background - the new window is overlayed on top of the drag handle window and it shows through between the left and the right windows. I've created crbug.com/414792 to track that. I'd prefer to fix this in a separate CL.
https://codereview.chromium.org/545393002/diff/140001/athena/util/drag_handle.cc File athena/util/drag_handle.cc (right): https://codereview.chromium.org/545393002/diff/140001/athena/util/drag_handle... athena/util/drag_handle.cc:21: explicit DragHandleView(DragHandle::ScrollDirection scroll_direction, On 2014/09/16 17:11:36, sadrul wrote: > No explicit Done. https://codereview.chromium.org/545393002/diff/140001/athena/wm/bezel_control... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/545393002/diff/140001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:63: } On 2014/09/16 17:11:36, sadrul wrote: > You would normally just do: > if (...) > return ...; > if (...) > return ...; > return ...; Done. https://codereview.chromium.org/545393002/diff/140001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:128: if (scroll_bezel_ != BEZEL_NONE) { On 2014/09/16 17:11:36, sadrul wrote: > Remove {} Done. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:42: // TODO(mfomitchev): Should this be moved to ui/views? On 2014/09/16 17:11:37, sadrul wrote: > No. This should be removed when crbug.com/414339 is fixed. Acknowledged. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:64: // TODO(mfomitchev): This is a copy of EmptyWindowDelegate in On 2014/09/16 17:11:37, sadrul wrote: > You no longer need EmptyWindowDelegate. Remove. Done. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:137: bool ShouldProcessEvent(ui::EventType event_type) { On 2014/09/16 17:11:36, sadrul wrote: > This is never used. Done. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:158: gfx::Rect view_bounds_in_root = gfx::Rect(view_origin_in_root, view_size); On 2014/09/16 17:11:36, sadrul wrote: > Can you just use View::GetBoundsInScreen() instead? Done. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:181: event->root_location().y())) { On 2014/09/16 17:11:36, sadrul wrote: > Just .Contains(event->root_location()) Done. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:343: params.opacity = views::Widget::InitParams::TRANSLUCENT_WINDOW; On 2014/09/16 17:11:37, sadrul wrote: > Do we need the widget to be translucent? Done. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:362: new StaticViewTargeterDelegate(drag_handle_), On 2014/09/16 17:11:37, sadrul wrote: > I think this StaticViewTargeterDelegate() is leaking? Done.
lgtm
lgtm https://codereview.chromium.org/545393002/diff/160001/athena/wm/bezel_control... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/545393002/diff/160001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:53: aura::Window* window) { nit: Doesn't this fit into the previous line?
pkotwicz@chromium.org changed reviewers: + pkotwicz@chromium.org
https://codereview.chromium.org/545393002/diff/140001/athena/util/drag_handle.cc File athena/util/drag_handle.cc (right): https://codereview.chromium.org/545393002/diff/140001/athena/util/drag_handle... athena/util/drag_handle.cc:43: State state_; The enum has two values, can this be a boolean instead? https://codereview.chromium.org/545393002/diff/140001/athena/util/drag_handle.h File athena/util/drag_handle.h (right): https://codereview.chromium.org/545393002/diff/140001/athena/util/drag_handle... athena/util/drag_handle.h:17: class DragHandle { Nuke the DragHandle class. No one instantiates it. Maybe rename DragHandle::ScrollDelegate to DragHandleScrollDelegate https://codereview.chromium.org/545393002/diff/140001/athena/util/drag_handle... athena/util/drag_handle.h:39: virtual bool HandleCanScroll() = 0; I wonder whether this is overkill. In the bezel case, we just ignore ScrollBegin(), ScrollUpdate(), ScrollEnd() if scrolling is not allowed https://codereview.chromium.org/545393002/diff/140001/athena/wm/bezel_control... File athena/wm/bezel_controller.h (right): https://codereview.chromium.org/545393002/diff/140001/athena/wm/bezel_control... athena/wm/bezel_controller.h:40: virtual void ScrollBegin(Bezel bezel, float delta) = 0; For the sake of clarity, you use rename ScrollBegin(), ScrollUpdate() and ScrollEnd() to BezelScrollBegin(), BezelScrollUpdate and BezelScrollEnd() https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:280: divider_position_ = container_width / 2; You should add a new method GetDefaultDividerPosition(). You compute this value often enough that I think that it is worth it https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:339: views::Widget::InitParams params(views::Widget::InitParams::TYPE_CONTROL); The widget should be of views::Widget::InitParams::TYPE_POPUP Widgets of TYPE_POPUP are not activatable by default. (So you won't need to specify params.activatable) https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:341: params.accept_events = true; No need to specify as params.accept_events == true is the default https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:383: gfx::Rect SplitViewController::GetLeftAreaBounds() { We have to use the work area height. Otherwise, the window ends below the minimized home card. (Try http://x20web.corp.google.com/~pkotwicz/unimportant/bottom.html) https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:403: InitializeDivider(); Would it make sense to move InitializeDivider() into ShowDivider()? https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:421: aura::Window* active_window = window_list_provider_->GetWindowList().back(); Nit: |active_window| -> |topmost_window| https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:424: right_window_->SetBounds(gfx::Rect(container_->bounds())); You probably meant: container_->bounds().size() https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:458: gfx::Transform right_transform; Optional: It might be clearer if you first computed the desired window bounds and then always called GetTransformForBounds(). https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:516: divider_widget_->GetNativeWindow()->SetTransform(divider_transform); Nit: Set the transforms on the windows in the same order in the animate and the non-animate case e.g. |left_window_|, then |divider_widget_|, then |right_window_| https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:536: gfx::Rect container_bounds = container_->GetBoundsInScreen(); Can we use |container_|->bounds() instead of |display_bounds|. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:586: SetState(INACTIVE); Should the divider hide at the end of the animation? I am ok if you do this as part of another CL. I have filed crbug.com/414877 w.r.t to the divider animation when entering split screen from overview mode https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:618: divider_position_ = delta + divider_position_; Optional: It might be useful to cache |divider_start_position_|. That would allow HandleScrollUpdate() and ScrollUpdate() to use more similar code.
Peter, can you PTAL? https://codereview.chromium.org/545393002/diff/140001/athena/util/drag_handle.cc File athena/util/drag_handle.cc (right): https://codereview.chromium.org/545393002/diff/140001/athena/util/drag_handle... athena/util/drag_handle.cc:43: State state_; On 2014/09/16 20:02:47, pkotwicz wrote: > The enum has two values, can this be a boolean instead? Done. https://codereview.chromium.org/545393002/diff/140001/athena/util/drag_handle.h File athena/util/drag_handle.h (right): https://codereview.chromium.org/545393002/diff/140001/athena/util/drag_handle... athena/util/drag_handle.h:17: class DragHandle { On 2014/09/16 20:02:47, pkotwicz wrote: > Nuke the DragHandle class. No one instantiates it. Maybe rename > DragHandle::ScrollDelegate to DragHandleScrollDelegate Done. https://codereview.chromium.org/545393002/diff/140001/athena/util/drag_handle... athena/util/drag_handle.h:39: virtual bool HandleCanScroll() = 0; Actually we don't ignore update/end once we receive a begin. Start the drag-from-the-bezel, then rotate the device - you can still continue dragging. We don't mark the events as handled in this case though, which I think is a bug, I created crbug.com/416564 for this. https://codereview.chromium.org/545393002/diff/140001/athena/wm/bezel_control... File athena/wm/bezel_controller.h (right): https://codereview.chromium.org/545393002/diff/140001/athena/wm/bezel_control... athena/wm/bezel_controller.h:40: virtual void ScrollBegin(Bezel bezel, float delta) = 0; On 2014/09/16 20:02:47, pkotwicz wrote: > For the sake of clarity, you use rename ScrollBegin(), ScrollUpdate() and > ScrollEnd() to BezelScrollBegin(), BezelScrollUpdate and BezelScrollEnd() Done. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:280: divider_position_ = container_width / 2; On 2014/09/16 20:02:47, pkotwicz wrote: > You should add a new method GetDefaultDividerPosition(). You compute this value > often enough that I think that it is worth it Done. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:339: views::Widget::InitParams params(views::Widget::InitParams::TYPE_CONTROL); CONTROL is also not activatable by default. I have removed the explicit setting of activatable to ACTIVATABLE_NO. It's not a popup, so I'd rather keep it control unless there's a reason not to. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:341: params.accept_events = true; On 2014/09/16 20:02:47, pkotwicz wrote: > No need to specify as params.accept_events == true is the default Done. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:383: gfx::Rect SplitViewController::GetLeftAreaBounds() { On 2014/09/16 20:02:47, pkotwicz wrote: > We have to use the work area height. Otherwise, the window ends below the > minimized home card. > (Try http://x20web.corp.google.com/~pkotwicz/unimportant/bottom.html) Done. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:403: InitializeDivider(); Well, it would work, but I don't know if one is better than the other. A bunch of methods need the divider to be non-null, e.g. HideDivider(). Ideally I'd like to put InitializeDivider into the constructor, but that doesn't work. I could create GetDividerWidget() but seems like a bit of an overkill. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:421: aura::Window* active_window = window_list_provider_->GetWindowList().back(); On 2014/09/16 20:02:48, pkotwicz wrote: > Nit: |active_window| -> |topmost_window| Done. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:424: right_window_->SetBounds(gfx::Rect(container_->bounds())); On 2014/09/16 20:02:48, pkotwicz wrote: > You probably meant: container_->bounds().size() Done. Good catch! https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:458: gfx::Transform right_transform; It's a bit more complicated than that though - I only do GetTransformForBounds when I need to scale up. Otherwise I simply translate the window (GetTransformForBounds would scale the window down, which I don't want). https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:516: divider_widget_->GetNativeWindow()->SetTransform(divider_transform); On 2014/09/16 20:02:47, pkotwicz wrote: > Nit: Set the transforms on the windows in the same order in the animate and the > non-animate case e.g. |left_window_|, then |divider_widget_|, then > |right_window_| Done. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:536: gfx::Rect container_bounds = container_->GetBoundsInScreen(); |delta| is relative to the bezel, while divider_position_ is relative to the container bounds. If we don't have display_bounds, how will we get the display width (or display_bounds.right())? https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:586: SetState(INACTIVE); Agreed, it would be good to fade it out or something like that. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:618: divider_position_ = delta + divider_position_; On 2014/09/16 20:02:47, pkotwicz wrote: > Optional: It might be useful to cache |divider_start_position_|. That would > allow HandleScrollUpdate() and ScrollUpdate() to use more similar code. Done. https://codereview.chromium.org/545393002/diff/160001/athena/wm/bezel_control... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/545393002/diff/160001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:53: aura::Window* window) { On 2014/09/16 19:58:48, Jun Mukai wrote: > nit: Doesn't this fit into the previous line? Nope, 5 chars over 80.
A few more comments. Thank you for being patient with me https://codereview.chromium.org/545393002/diff/140001/athena/util/drag_handle.h File athena/util/drag_handle.h (right): https://codereview.chromium.org/545393002/diff/140001/athena/util/drag_handle... athena/util/drag_handle.h:39: virtual bool HandleCanScroll() = 0; Just to double check, we are planning on having HandleCanScroll() eventually sometimes return false? If we are, then I am ok with having this method. I can't think of a case where we would want HandleCanScroll() to be false. We want to prevent multiple gestures from occurring simultaneously. http://crbug.com/416725 (e.g. title drag and bezel drag) but I think we will end up preventing this in some other way. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:339: views::Widget::InitParams params(views::Widget::InitParams::TYPE_CONTROL); It does not matter in this case. Generally TYPE_CONTROL is used for views::Widgets which have a views::Widget parent For TYPE_CONTROL windows Widget::is_top_level() returns false and for TYPE_POPUP windows Widget::is_top_level() returns true. This matters if the window can take focus. See Widget::GetFocusManager() Popup windows (e.g. JS alerts) use TYPE_WINDOW https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:536: gfx::Rect container_bounds = container_->GetBoundsInScreen(); From reading ShelfBezelEventFilter::OnGestureEvent() it looks like the bezel does not contribute to the size of the screen. Maybe its different on Athena or I am not reading the code correctly https://codereview.chromium.org/545393002/diff/200001/athena/util/drag_handle.cc File athena/util/drag_handle.cc (right): https://codereview.chromium.org/545393002/diff/200001/athena/util/drag_handle... athena/util/drag_handle.cc:8: #include "ui/views/layout/box_layout.h" Nit: remove include https://codereview.chromium.org/545393002/diff/200001/athena/util/drag_handle... athena/util/drag_handle.cc:10: #include "ui/views/widget/widget.h" Nit: Remove include https://codereview.chromium.org/545393002/diff/200001/athena/util/drag_handle... athena/util/drag_handle.cc:86: void DragHandleView::OnMouseEntered(const ui::MouseEvent& event) { Given that this class does not yet handle mouse drags, it may not be worth handling mouse enters and exits yet either https://codereview.chromium.org/545393002/diff/200001/athena/wm/bezel_control... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/545393002/diff/200001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:51: // Only implemented for LEFT and RIGHT bezels ATM. Nit: Remove 'ATM' https://codereview.chromium.org/545393002/diff/200001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:118: SetState(NONE); I am sure that I am missing something. Can you explain why this is needed? I initially thought that this was to make the unit tests pass but that's not the case. Based on the description, it sounds like the changes that I am making in BezelController in https://codereview.chromium.org/574113004/ accomplish the same task (If the two do the same thing I think my implementation is slightly better) https://codereview.chromium.org/545393002/diff/200001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/200001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:307: gfx::Screen::GetNativeScreen()->GetPrimaryDisplay().work_area(); Shouldn't this use |divider_position_| ? How about this: int width = 0; int x = 0; if (divider_position_ <= GetDefaultDividerPosition()) { width = GetDefaultDividerPosition() - kDividerWidth / 2; x = divider_position_ - kDividerWidth / 2 - width; } else { width = divider_position_ - kDividerWidth / 2; } int height = gfx::Screen::GetNativeScreen()->GetPrimaryDisplay().work_area().height(); return gfx::Rect(x, 0, width, height);
https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:536: gfx::Rect container_bounds = container_->GetBoundsInScreen(); Leaving as is probably OK. The new code has the same behavior as the old code. I should not be asking about potential suboptimalities which are not related to the current CL
https://codereview.chromium.org/545393002/diff/140001/athena/util/drag_handle.h File athena/util/drag_handle.h (right): https://codereview.chromium.org/545393002/diff/140001/athena/util/drag_handle... athena/util/drag_handle.h:39: virtual bool HandleCanScroll() = 0; Yes, good point. I added it because I thought we were going to unify DragHandleScrollDelegate with BezelController::ScrollDelegate, but it doesn't look like that's going to happen. Done. https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:339: views::Widget::InitParams params(views::Widget::InitParams::TYPE_CONTROL); Ok, changed to TYPE_POPUP https://codereview.chromium.org/545393002/diff/140001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:536: gfx::Rect container_bounds = container_->GetBoundsInScreen(); This was meant to support the case for when container doesn't start at the edge of the screen. See discussion here: https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.c... I don't mind discussing this further, but it obviously shouldn't block landing of this CL :) https://codereview.chromium.org/545393002/diff/200001/athena/util/drag_handle.cc File athena/util/drag_handle.cc (right): https://codereview.chromium.org/545393002/diff/200001/athena/util/drag_handle... athena/util/drag_handle.cc:8: #include "ui/views/layout/box_layout.h" On 2014/09/23 05:13:39, pkotwicz wrote: > Nit: remove include Done. https://codereview.chromium.org/545393002/diff/200001/athena/util/drag_handle... athena/util/drag_handle.cc:10: #include "ui/views/widget/widget.h" On 2014/09/23 05:13:39, pkotwicz wrote: > Nit: Remove include Done. https://codereview.chromium.org/545393002/diff/200001/athena/util/drag_handle... athena/util/drag_handle.cc:86: void DragHandleView::OnMouseEntered(const ui::MouseEvent& event) { On 2014/09/23 05:13:39, pkotwicz wrote: > Given that this class does not yet handle mouse drags, it may not be worth > handling mouse enters and exits yet either Done. https://codereview.chromium.org/545393002/diff/200001/athena/wm/bezel_control... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/545393002/diff/200001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:51: // Only implemented for LEFT and RIGHT bezels ATM. On 2014/09/23 05:13:40, pkotwicz wrote: > Nit: Remove 'ATM' Done. https://codereview.chromium.org/545393002/diff/200001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:118: SetState(NONE); When you drag the divider handle, it becomes the target of the gesture event sequence. The handle is hidden on ET_GESTURE_SCROLL_END. This prevents ET_GESTURE_END from being dispatched (because the target is hidden). Thus the bezel controller never receives ET_GESTURE_END and it doesn't change its state to NONE. More generally, we can't rely on receiving ET_GESTURE_END for every ET_GESTURE_BEGIN because the target can be hidden or even deleted mid-gesture. Thus we do a state sanity check on every ET_GESTURE_BEGIN. I've updated the comment to make it a bit more clear/detailed. https://codereview.chromium.org/545393002/diff/200001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/200001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:307: gfx::Screen::GetNativeScreen()->GetPrimaryDisplay().work_area(); Oops, I totally messed this up by copy/pasting old code, thanks for catching! The if() here wouldn't do too much good though - we'd still need the if in UpdateLayout() because the window width can be half the screen width or full screen width (depending on whether the window was in split mode) and our logic for scale vs. translate depends on whether the target area width is greater than the window width. We could pass the window into this method, but that would complicate explaining what this method does, and I am not sure it would be better than what we have now. The way it is now the method is very straightforward - the bounds of the screen area to the left of the divider, and I like that.
LGTM with comments https://codereview.chromium.org/545393002/diff/200001/athena/wm/bezel_control... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/545393002/diff/200001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:118: SetState(NONE); From talking to tdresser@ we are supposed to get ET_GESTURE_END. However, I cannot get ET_GESTURE_END to be sent This is ok for now for the sake of this CL. However, it would be useful to figure out why ET_GESTURE_END is not received https://codereview.chromium.org/545393002/diff/200001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/200001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:307: gfx::Screen::GetNativeScreen()->GetPrimaryDisplay().work_area(); I understand now, thanks! https://codereview.chromium.org/545393002/diff/200001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:345: right_window_->SetBounds(gfx::Rect(container_->bounds().size())); This is wrong. It needs to into account the work area height. I don't mind if you fix this in a hacky way for now. I will fix this in a better way in combination with properly sending OnSplitViewModeEnter() when entering split view via the bezel https://codereview.chromium.org/545393002/diff/200001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:348: left_window_->SetBounds(gfx::Rect(container_->bounds().size())); Ditto https://codereview.chromium.org/545393002/diff/200001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:483: // Calculate divider_scroll_start_position_ Nit: "|" around variable name and period at the end of the sentence https://codereview.chromium.org/545393002/diff/220001/athena/util/drag_handle... File athena/util/drag_handle_unittest.cc (right): https://codereview.chromium.org/545393002/diff/220001/athena/util/drag_handle... athena/util/drag_handle_unittest.cc:104: CreateGestureEvent(ui::ET_GESTURE_SCROLL_END, begin_x + update_delta, 0); In a separate CL, it would be useful to make constructing GestureEventDetials with ui::ET_GESTURE_SCROLL_END not fire the NOTREACHED() macro. https://codereview.chromium.org/545393002/diff/240001/athena/util/drag_handle... File athena/util/drag_handle_unittest.cc (right): https://codereview.chromium.org/545393002/diff/240001/athena/util/drag_handle... athena/util/drag_handle_unittest.cc:7: #include "testing/gtest/include/gtest/gtest.h" Nit: Remove this include
https://codereview.chromium.org/545393002/diff/200001/athena/wm/bezel_control... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/545393002/diff/200001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:118: SetState(NONE); I started a thread with sadrul on this, but it didn't get anywhere. I'll add you. Also, if the target was deleted (which could happen)I understand it is expected that ET_GESTURE_END won't be send. https://codereview.chromium.org/545393002/diff/200001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/545393002/diff/200001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:345: right_window_->SetBounds(gfx::Rect(container_->bounds().size())); Ok, updated. I wish all activity windows were inside a container which had it's bounds set to the work area bounds. Do you know why that is not done? It seems pretty hacky the way we just use work area now. https://codereview.chromium.org/545393002/diff/200001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:348: left_window_->SetBounds(gfx::Rect(container_->bounds().size())); On 2014/09/23 19:44:46, pkotwicz wrote: > Ditto Done. https://codereview.chromium.org/545393002/diff/200001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:483: // Calculate divider_scroll_start_position_ On 2014/09/23 19:44:46, pkotwicz wrote: > Nit: "|" around variable name and period at the end of the sentence Done.
The CQ bit was checked by mfomitchev@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/545393002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mfomitchev@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/545393002/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as 3f398207ca94ba6354b3efdfe0cdf19d0e2019fe
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/b47c16517f0882cbabf8c9aa9ce66711f026fd5b Cr-Commit-Position: refs/heads/master@{#296351}
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:280001) has been created in https://codereview.chromium.org/602603003/ by thakis@chromium.org. The reason for reverting is: DragHandleTest.ScrollTest is failing on the bots: http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
