Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15734011:
Correct immersive mode gestures under rotation (Closed)
|
Created:
7 years, 7 months ago by rharrison Modified:
7 years, 6 months ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCorrect immersive mode gestures under rotation
This code does to things to make sure that immersive mode gestures operate
correctly regardless of the screen orientation. The first is to move the
registration of the filter to the root window, which guarantees that all of the
events will be seen by the filter.
The second change is to extend the bounds the filter is watching above by the
maximum bezel side. This makes sure that under rotation swipes that start on the
bezel are still handled.
BUG=241545, 244269
TEST=Rotated screen through all rotations and confirmed that swipe in/out works
as expected in immersive mode.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203005
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed sadrul's comments #
Total comments: 2
Patch Set 3 : Addressed pkotwicz's comments #Patch Set 4 : Other part of the change to address pkotwicz's comments #
Total comments: 6
Patch Set 5 : Rebased and cleaned up CL #Patch Set 6 : Added in OnWindowRemovedFromRootWindow #
Total comments: 14
Patch Set 7 : Responded to pkotwicz's comments #Patch Set 8 : Moved SetPreTargetHandler() to private section #
Total comments: 8
Patch Set 9 : Fixed outstanding nits #Patch Set 10 : Corrected incorrect overridden method names #Patch Set 11 : Fix misnamed overrides correctly this time :-/ #
Messages
Total messages: 17 (0 generated)
PTAL
https://codereview.chromium.org/15734011/diff/1/chrome/browser/ui/views/frame... File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (left): https://codereview.chromium.org/15734011/diff/1/chrome/browser/ui/views/frame... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:1001: return near_bounds.Contains(location); Could you just do: return near_bounds.Contains(location) || location.y() < near_bounds.y(); https://codereview.chromium.org/15734011/diff/1/chrome/browser/ui/views/frame... File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right): https://codereview.chromium.org/15734011/diff/1/chrome/browser/ui/views/frame... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:703: root_window_->RemovePreTargetHandler(this); Is it possible for root_window_ to become invalid (e.g. when the display the window is currently on is removed)?
https://codereview.chromium.org/15734011/diff/1/chrome/browser/ui/views/frame... File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (left): https://codereview.chromium.org/15734011/diff/1/chrome/browser/ui/views/frame... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:1001: return near_bounds.Contains(location); On 2013/05/22 18:22:45, sadrul wrote: > Could you just do: > > return near_bounds.Contains(location) || > location.y() < near_bounds.y(); Done. https://codereview.chromium.org/15734011/diff/1/chrome/browser/ui/views/frame... File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right): https://codereview.chromium.org/15734011/diff/1/chrome/browser/ui/views/frame... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:703: root_window_->RemovePreTargetHandler(this); On 2013/05/22 18:22:45, sadrul wrote: > Is it possible for root_window_ to become invalid (e.g. when the display the > window is currently on is removed)? Done.
I checked and it is possible for the root window to become invalid. The common way of checking if the root window has changed is via WindowObserver::OnWindowAddedToRootWindow() and WindowObserver::OnWindowRemovingFromRootWindow()
https://codereview.chromium.org/15734011/diff/16001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right): https://codereview.chromium.org/15734011/diff/16001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:358: bezel_inset_(0) { You don't use this?
Only part of my change was in this upload, will upload the rest soon. https://codereview.chromium.org/15734011/diff/16001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right): https://codereview.chromium.org/15734011/diff/16001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:358: bezel_inset_(0) { On 2013/05/23 03:29:35, pkotwicz wrote: > You don't use this? Done.
I have resolved all outstanding comments. PTAL
I'll take a look tonight
Looks pretty close. You can probably send to OWNERS in the next iteration. https://codereview.chromium.org/15734011/diff/28001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right): https://codereview.chromium.org/15734011/diff/28001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:29: #include "ui/base/ui_base_switches.h" I think you don't need most of the new #includes https://codereview.chromium.org/15734011/diff/28001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:339: if (window == window_) DCHECK_EQ(window_, window) instead. https://codereview.chromium.org/15734011/diff/28001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:340: controller_->SetPreTargetHandler(); Do you need to override OnWindowRemovedFromRootWindow() too?
Adding jamescook@ for OWNERS https://codereview.chromium.org/15734011/diff/28001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right): https://codereview.chromium.org/15734011/diff/28001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:29: #include "ui/base/ui_base_switches.h" On 2013/05/23 18:22:07, sadrul wrote: > I think you don't need most of the new #includes Done. https://codereview.chromium.org/15734011/diff/28001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:339: if (window == window_) On 2013/05/23 18:22:07, sadrul wrote: > DCHECK_EQ(window_, window) instead. Done. https://codereview.chromium.org/15734011/diff/28001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:340: controller_->SetPreTargetHandler(); On 2013/05/23 18:22:07, sadrul wrote: > Do you need to override OnWindowRemovedFromRootWindow() too? Done.
Looks really good. Just nits. https://codereview.chromium.org/15734011/diff/47001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right): https://codereview.chromium.org/15734011/diff/47001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:301: class ImmersiveModeControllerAsh::WindowObserver : public aura::WindowObserver { I think we should make ImmersiveModeControllerAsh inherit from aura::WindowObserver. The fact that this is a separate class is a remnant from when ImmersiveModeControllerAsh was not ash only (and the class had a different name). https://codereview.chromium.org/15734011/diff/47001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:332: DCHECK(window == window_); Nit: DCHECK_EQ https://codereview.chromium.org/15734011/diff/47001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:337: DCHECK(window == window_); DCHECK_EQ https://codereview.chromium.org/15734011/diff/47001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:393: void ImmersiveModeControllerAsh::SetPreTargetHandler() { I don't think that you will need this function once you make ImmersiveModeControllerAsh inherit from WindowObserver. https://codereview.chromium.org/15734011/diff/47001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:1052: bool ImmersiveModeControllerAsh::IsNearTopContainer(gfx::Point location) const { Nits: - We should rename this method to something more representative of what it does. How about: bool ImmersiveModeControllerAsh::ShouldHandleEvent(const gfx::Point& location) / ImmersiveModeController::ShouldHandleGestureEvent(const gfx::Point& location) - We should pass |location| by reference https://codereview.chromium.org/15734011/diff/47001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:1056: return near_bounds.Contains(location) || Can you please add a comment as to why the condition is as it is? I am unsure if I will remember why a year from now. https://codereview.chromium.org/15734011/diff/47001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:1059: (location.x() <= near_bounds.x() + near_bounds.width())); Nit: You can use near_bounds.right()
https://codereview.chromium.org/15734011/diff/47001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right): https://codereview.chromium.org/15734011/diff/47001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:301: class ImmersiveModeControllerAsh::WindowObserver : public aura::WindowObserver { On 2013/05/24 03:27:30, pkotwicz wrote: > I think we should make ImmersiveModeControllerAsh inherit from > aura::WindowObserver. The fact that this is a separate class is a remnant from > when ImmersiveModeControllerAsh was not ash only (and the class had a different > name). Done. https://codereview.chromium.org/15734011/diff/47001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:332: DCHECK(window == window_); On 2013/05/24 03:27:30, pkotwicz wrote: > Nit: DCHECK_EQ Done. https://codereview.chromium.org/15734011/diff/47001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:337: DCHECK(window == window_); On 2013/05/24 03:27:30, pkotwicz wrote: > DCHECK_EQ Done. https://codereview.chromium.org/15734011/diff/47001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:393: void ImmersiveModeControllerAsh::SetPreTargetHandler() { On 2013/05/24 03:27:30, pkotwicz wrote: > I don't think that you will need this function once you make > ImmersiveModeControllerAsh inherit from WindowObserver. I think it will still be needed to avoid code duplication, but I don't think it will need to in the public interface anymore. https://codereview.chromium.org/15734011/diff/47001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:1052: bool ImmersiveModeControllerAsh::IsNearTopContainer(gfx::Point location) const { On 2013/05/24 03:27:30, pkotwicz wrote: > Nits: > - We should rename this method to something more representative of what it does. > How about: > bool ImmersiveModeControllerAsh::ShouldHandleEvent(const gfx::Point& location) / > ImmersiveModeController::ShouldHandleGestureEvent(const gfx::Point& location) > - We should pass |location| by reference Done. https://codereview.chromium.org/15734011/diff/47001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:1056: return near_bounds.Contains(location) || On 2013/05/24 03:27:30, pkotwicz wrote: > Can you please add a comment as to why the condition is as it is? I am unsure if > I will remember why a year from now. Done. https://codereview.chromium.org/15734011/diff/47001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:1059: (location.x() <= near_bounds.x() + near_bounds.width())); On 2013/05/24 03:27:30, pkotwicz wrote: > Nit: You can use near_bounds.right() Done.
LGTM with nits https://codereview.chromium.org/15734011/diff/53001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right): https://codereview.chromium.org/15734011/diff/53001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:1025: // hit the bounds due to small on screen size, so the hit target needs to be The small size of the tab strip? https://codereview.chromium.org/15734011/diff/53001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:1035: (location.x() <= near_bounds.x() + near_bounds.right())); This is supposed to be near_bounds.right() instead of near_bounds.x() + near_bounds.right() https://codereview.chromium.org/15734011/diff/53001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/immersive_mode_controller_ash.h (right): https://codereview.chromium.org/15734011/diff/53001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.h:209: // True when |location| is "near" to the top container. When the top container Nit: update the comment
LGTM with nit https://codereview.chromium.org/15734011/diff/53001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/immersive_mode_controller_ash.h (right): https://codereview.chromium.org/15734011/diff/53001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.h:218: void SetPreTargetHandler(); nit: Consider making this UpdatePreTargetHandler(bool add), since "Set" implies to me that you are always adding, never removing. This also makes it more clear what's happening during the sequence of setting observers_enabled_ and installing the handler.
https://codereview.chromium.org/15734011/diff/53001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right): https://codereview.chromium.org/15734011/diff/53001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:1025: // hit the bounds due to small on screen size, so the hit target needs to be On 2013/05/27 21:32:46, pkotwicz wrote: > The small size of the tab strip? Done. https://codereview.chromium.org/15734011/diff/53001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:1035: (location.x() <= near_bounds.x() + near_bounds.right())); On 2013/05/27 21:32:46, pkotwicz wrote: > This is supposed to be near_bounds.right() instead of near_bounds.x() + > near_bounds.right() Done. https://codereview.chromium.org/15734011/diff/53001/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/immersive_mode_controller_ash.h (right): https://codereview.chromium.org/15734011/diff/53001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.h:209: // True when |location| is "near" to the top container. When the top container On 2013/05/27 21:32:46, pkotwicz wrote: > Nit: update the comment Done. https://codereview.chromium.org/15734011/diff/53001/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/immersive_mode_controller_ash.h:218: void SetPreTargetHandler(); On 2013/05/28 16:29:42, James Cook (Chromium) wrote: > nit: Consider making this UpdatePreTargetHandler(bool add), since "Set" implies > to me that you are always adding, never removing. This also makes it more clear > what's happening during the sequence of setting observers_enabled_ and > installing the handler. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rharrison@chromium.org/15734011/71002
Message was sent while issue was closed.
Change committed as 203005 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
