Correct immersive mode gestures under rotation

chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/frame/immersive_mode_controller_ash.h"
 
 #include "ash/ash_switches.h"
 #include "ash/shell.h"
 #include "ash/wm/window_properties.h"
 #include "base/command_line.h"
 #include "chrome/browser/ui/fullscreen/fullscreen_controller.h"
 #include "chrome/browser/ui/immersive_fullscreen_configuration.h"
 #include "chrome/browser/ui/views/bookmarks/bookmark_bar_view.h"
 #include "chrome/browser/ui/views/frame/top_container_view.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "content/public/browser/notification_service.h"
 #include "ui/aura/client/activation_client.h"
 #include "ui/aura/client/aura_constants.h"
 #include "ui/aura/client/capture_client.h"
 #include "ui/aura/env.h"
+#include "ui/aura/root_window.h"
 #include "ui/aura/window.h"
 #include "ui/aura/window_observer.h"
 #include "ui/compositor/layer_animation_observer.h"
 #include "ui/compositor/scoped_layer_animation_settings.h"
 #include "ui/gfx/screen.h"
 #include "ui/gfx/transform.h"
 #include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/window/non_client_view.h"
 
@@ skipping 259 matching lines @@
 
   std::map<views::Widget*, int>::iterator it = widgets_.find(widget);
   DCHECK(it != widgets_.end());
   UpdateWidgetBounds(it->first, it->second);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 
 // Observer to watch for window restore. views::Widget does not provide a hook
 // to observe for window restore, so do this at the Aura level.
 class ImmersiveModeControllerAsh::WindowObserver : public aura::WindowObserver {

[pkotwicz, 2013/05/24 03:27:30]

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).

[rharrison, 2013/05/27 19:00:54]

Done.

  public:
   explicit WindowObserver(ImmersiveModeControllerAsh* controller)
       : controller_(controller) {
-    controller_->native_window_->AddObserver(this);
+    window_ = controller_->native_window_;
+    window_->AddObserver(this);
   }
 
   virtual ~WindowObserver() {
-    controller_->native_window_->RemoveObserver(this);
+    window_->RemoveObserver(this);
   }
 
   // aura::WindowObserver overrides:
   virtual void OnWindowPropertyChanged(aura::Window* window,
                                        const void* key,
                                        intptr_t old) OVERRIDE {
     using aura::client::kShowStateKey;
     if (key == kShowStateKey) {
       // Disable immersive mode when leaving the fullscreen state.
       ui::WindowShowState show_state = static_cast<ui::WindowShowState>(
           window->GetProperty(kShowStateKey));
       if (controller_->IsEnabled() &&
           show_state != ui::SHOW_STATE_FULLSCREEN &&
           show_state != ui::SHOW_STATE_MINIMIZED) {
         controller_->delegate_->FullscreenStateChanged();
       }
       return;
     }
   }
 
+  virtual void OnWindowAddedToRootWindow(aura::Window* window) {
+    DCHECK(window == window_);

[pkotwicz, 2013/05/24 03:27:30]

Nit: DCHECK_EQ

[rharrison, 2013/05/27 19:00:54]

Done.

+    controller_->SetPreTargetHandler();
+  }
+
+  virtual void OnWindowRemovedFromRootWindow(aura::Window* window) {
+    DCHECK(window == window_);

[pkotwicz, 2013/05/24 03:27:30]

DCHECK_EQ

[rharrison, 2013/05/27 19:00:54]

Done.

+    controller_->SetPreTargetHandler();
+  }
+
  private:
   ImmersiveModeControllerAsh* controller_;  // Not owned.
+  aura::Window* window_;  // Not owned.
 
   DISALLOW_COPY_AND_ASSIGN(WindowObserver);
 };
 
 ////////////////////////////////////////////////////////////////////////////////
 
 ImmersiveModeControllerAsh::ImmersiveModeControllerAsh()
     : delegate_(NULL),
       widget_(NULL),
       top_container_(NULL),
@@ skipping 29 matching lines @@
   if (revealed_lock_count_ == 0) {
     // Always animate ending the reveal fast.
     MaybeEndReveal(ANIMATE_FAST);
   }
 }
 
 void ImmersiveModeControllerAsh::MaybeRevealWithoutAnimation() {
   MaybeStartReveal(ANIMATE_NO);
 }
 
+void ImmersiveModeControllerAsh::SetPreTargetHandler() {

[pkotwicz, 2013/05/24 03:27:30]

I don't think that you will need this function once you make
ImmersiveModeControllerAsh inherit from WindowObserver.

[rharrison, 2013/05/27 19:00:54]

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.

+  if (!native_window_)
+    return;
+  aura::RootWindow* root_window = native_window_->GetRootWindow();
+  if (!root_window)
+    return;
+  if (observers_enabled_)
+    root_window->AddPreTargetHandler(this);
+  else
+    root_window->RemovePreTargetHandler(this);
+}
+
 void ImmersiveModeControllerAsh::Init(
     Delegate* delegate,
     views::Widget* widget,
     TopContainerView* top_container) {
   delegate_ = delegate;
   widget_ = widget;
   // Browser view is detached from its widget during destruction. Cache the
   // window pointer so |this| can stop observing during destruction.
   native_window_ = widget_->GetNativeWindow();
   top_container_ = top_container;
@@ skipping 264 matching lines @@
       views::Widget::GetWidgetForNativeWindow(native_window_);
   views::FocusManager* focus_manager = widget->GetFocusManager();
   if (enable) {
     widget->AddObserver(this);
     focus_manager->AddFocusChangeListener(this);
   } else {
     widget->RemoveObserver(this);
     focus_manager->RemoveFocusChangeListener(this);
   }
 
-  if (enable)
-    native_window_->AddPreTargetHandler(this);
-  else
-    native_window_->RemovePreTargetHandler(this);
+  SetPreTargetHandler();
 
   // The window observer adds and removes itself from the native window.
   window_observer_.reset(enable ? new WindowObserver(this) : NULL);
 
   if (enable) {
     registrar_.Add(
         this,
         chrome::NOTIFICATION_FULLSCREEN_CHANGED,
         content::Source<FullscreenController>(
             delegate_->GetFullscreenController()));
@@ skipping 342 matching lines @@
   if (abs(event->details().scroll_y()) <=
       kSwipeVerticalThresholdMultiplier * abs(event->details().scroll_x()))
     return SWIPE_NONE;
   if (event->details().scroll_y() < 0)
     return SWIPE_CLOSE;
   else if (event->details().scroll_y() > 0)
     return SWIPE_OPEN;
   return SWIPE_NONE;
 }
 
 bool ImmersiveModeControllerAsh::IsNearTopContainer(gfx::Point location) const {

[pkotwicz, 2013/05/24 03:27:30]

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

[rharrison, 2013/05/27 19:00:54]

Done.

   gfx::Rect near_bounds = top_container_->GetTargetBoundsInScreen();
   if (reveal_state_ == CLOSED)
     near_bounds.Inset(gfx::Insets(0, 0, -kNearTopContainerDistance, 0));
-  return near_bounds.Contains(location);
+  return near_bounds.Contains(location) ||

[pkotwicz, 2013/05/24 03:27:30]

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.

[rharrison, 2013/05/27 19:00:54]

Done.

+      ((location.y() < near_bounds.y()) &&
+       (location.x() >= near_bounds.x()) &&
+       (location.x() <= near_bounds.x() + near_bounds.width()));

[pkotwicz, 2013/05/24 03:27:30]

Nit: You can use near_bounds.right()

[rharrison, 2013/05/27 19:00:54]

Done.

 }