- Remove redundant views::View::accelerator_registration_delayed_

- Remove unused parameters to views::View::NativeViewHierarchyChanged()
- Split views::Widget::NotifyNativeViewHierarchyWillChange() into views::Widget::NotifyNativeViewHierarchyWillChange() and views::Widget::NotifyNativeViewHierarchyChanged()

BUG=276729
TEST=ViewTest.NativeViewHierarchyChanged

Review URL: https://chromiumcodereview.appspot.com/23068029

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@219363 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 113 insertions, 182 deletions

diff --git a/ui/views/controls/native/native_view_host_unittest.cc b/ui/views/controls/native/native_view_host_unittest.cc
index 0f8eca1..f30980b 100644
--- a/ui/views/controls/native/native_view_host_unittest.cc
+++ b/ui/views/controls/native/native_view_host_unittest.cc
@@ -72,33 +72,23 @@ namespace {
 // times NativeViewHierarchyChanged() is invoked.
 class NativeViewHierarchyChangedTestView : public View {
  public:
-  NativeViewHierarchyChangedTestView()
-      : attached_count_(0),
-        detached_count_(0) {
+  NativeViewHierarchyChangedTestView() : notification_count_(0) {
   }
 
-  void ResetCounts() {
-    attached_count_ = detached_count_ = 0;
+  void ResetCount() {
+    notification_count_ = 0;
   }
 
-  int attached_count() const { return attached_count_; }
-  int detached_count() const { return detached_count_; }
+  int notification_count() const { return notification_count_; }
 
   // Overriden from View:
-  virtual void NativeViewHierarchyChanged(
-      bool attached,
-      gfx::NativeView native_view,
-      internal::RootView* root_view) OVERRIDE {
-    if (attached)
-      attached_count_++;
-    else
-      detached_count_++;
-    View::NativeViewHierarchyChanged(attached, native_view, root_view);
+  virtual void NativeViewHierarchyChanged() OVERRIDE {
+    ++notification_count_;
+    View::NativeViewHierarchyChanged();
   }
 
  private:
-  int attached_count_;
-  int detached_count_;
+  int notification_count_;
 
   DISALLOW_COPY_AND_ASSIGN(NativeViewHierarchyChangedTestView);
 };
@@ -158,22 +148,21 @@ TEST_F(NativeViewHostTest, NativeViewHierarchyChanged) {
                                               test_view,
                                               host));
 
-  EXPECT_EQ(0, test_view->attached_count());
-  EXPECT_EQ(0, test_view->detached_count());
-  test_view->ResetCounts();
+  EXPECT_EQ(0, test_view->notification_count());
+  test_view->ResetCount();
 
-  // Detach should send both an attach and detach as well as changing parent.
+  // Detaching should send a NativeViewHierarchyChanged() notification and
+  // change the parent.
   host->Detach();
-  EXPECT_EQ(1, test_view->attached_count());
-  EXPECT_EQ(1, test_view->detached_count());
+  EXPECT_EQ(1, test_view->notification_count());
   EXPECT_NE(toplevel()->GetNativeView(),
             GetNativeParent(child->GetNativeView()));
-  test_view->ResetCounts();
+  test_view->ResetCount();
 
-  // Attaching should send both an attach and detach and reset parent.
+  // Attaching should send a NativeViewHierarchyChanged() notification and
+  // reset the parent.
   host->Attach(child->GetNativeView());
-  EXPECT_EQ(1, test_view->attached_count());
-  EXPECT_EQ(1, test_view->detached_count());
+  EXPECT_EQ(1, test_view->notification_count());
   EXPECT_EQ(toplevel()->GetNativeView(),
             GetNativeParent(child->GetNativeView()));
 }
diff --git a/ui/views/view.cc b/ui/views/view.cc
index 73b1e4e..6fa85e6 100644
--- a/ui/views/view.cc
+++ b/ui/views/view.cc
@@ -177,7 +177,6 @@ View::View()
       focus_border_(FocusBorder::CreateDashedFocusBorder()),
       flip_canvas_on_paint_for_rtl_ui_(false),
       paint_to_layer_(false),
-      accelerator_registration_delayed_(false),
       accelerator_focus_manager_(NULL),
       registered_accelerator_count_(0),
       next_focusable_view_(NULL),
@@ -1305,21 +1304,13 @@ void View::ViewHierarchyChanged(const ViewHierarchyChangedDetails& details) {
 void View::VisibilityChanged(View* starting_from, bool is_visible) {
 }
 
-void View::NativeViewHierarchyChanged(bool attached,
-                                      gfx::NativeView native_view,
-                                      internal::RootView* root_view) {
+void View::NativeViewHierarchyChanged() {
   FocusManager* focus_manager = GetFocusManager();
-  if (!accelerator_registration_delayed_ &&
-      accelerator_focus_manager_ &&
-      accelerator_focus_manager_ != focus_manager) {
+  if (accelerator_focus_manager_ != focus_manager) {
     UnregisterAccelerators(true);
-    accelerator_registration_delayed_ = true;
-  }
-  if (accelerator_registration_delayed_ && attached) {
-    if (focus_manager) {
+
+    if (focus_manager)
       RegisterPendingAccelerators();
-      accelerator_registration_delayed_ = false;
-    }
   }
 }
 
@@ -1821,14 +1812,10 @@ void View::PropagateAddNotifications(
   ViewHierarchyChangedImpl(true, details);
 }
 
-void View::PropagateNativeViewHierarchyChanged(bool attached,
-                                               gfx::NativeView native_view,
-                                               internal::RootView* root_view) {
+void View::PropagateNativeViewHierarchyChanged() {
   for (int i = 0, count = child_count(); i < count; ++i)
-    child_at(i)->PropagateNativeViewHierarchyChanged(attached,
-                                                           native_view,
-                                                           root_view);
-  NativeViewHierarchyChanged(attached, native_view, root_view);
+    child_at(i)->PropagateNativeViewHierarchyChanged();
+  NativeViewHierarchyChanged();
 }
 
 void View::ViewHierarchyChangedImpl(
@@ -1838,13 +1825,8 @@ void View::ViewHierarchyChangedImpl(
     if (details.is_add) {
       // If you get this registration, you are part of a subtree that has been
       // added to the view hierarchy.
-      if (GetFocusManager()) {
+      if (GetFocusManager())
         RegisterPendingAccelerators();
-      } else {
-        // Delay accelerator registration until visible as we do not have
-        // focus manager until then.
-        accelerator_registration_delayed_ = true;
-      }
     } else {
       if (details.child == this)
         UnregisterAccelerators(true);
@@ -1857,7 +1839,7 @@ void View::ViewHierarchyChangedImpl(
     if (widget)
       widget->UpdateRootLayers();
   } else if (!details.is_add && details.child == this) {
-    // Make sure the layers beloning to the subtree rooted at |child| get
+    // Make sure the layers belonging to the subtree rooted at |child| get
     // removed from layers that do not belong in the same subtree.
     OrphanLayers();
     if (use_acceleration_when_possible) {
@@ -2235,9 +2217,6 @@ void View::UnregisterAccelerators(bool leave_data_intact) {
 
   if (GetWidget()) {
     if (accelerator_focus_manager_) {
-      // We may not have a FocusManager if the window containing us is being
-      // closed, in which case the FocusManager is being deleted so there is
-      // nothing to unregister.
       accelerator_focus_manager_->UnregisterAccelerators(this);
       accelerator_focus_manager_ = NULL;
     }
diff --git a/ui/views/view.h b/ui/views/view.h
index 337012c..2b0a742 100644
--- a/ui/views/view.h
+++ b/ui/views/view.h
@@ -1032,17 +1032,13 @@ class VIEWS_EXPORT View : public ui::LayerDelegate,
   // invoked for that view as well as all the children recursively.
   virtual void VisibilityChanged(View* starting_from, bool is_visible);
 
-  // Called when the native view hierarchy changed.
-  // |attached| is true if that view has been attached to a new NativeView
-  // hierarchy, false if it has been detached.
-  // |native_view| is the NativeView this view was attached/detached from, and
-  // |root_view| is the root view associated with the NativeView.
-  // Views created without a native view parent don't have a focus manager.
-  // When this function is called they could do the processing that requires
-  // it - like registering accelerators, for example.
-  virtual void NativeViewHierarchyChanged(bool attached,
-                                          gfx::NativeView native_view,
-                                          internal::RootView* root_view);
+  // This method is invoked when the parent NativeView of the widget that the
+  // view is attached to has changed and the view hierarchy has not changed.
+  // ViewHierarchyChanged() is called when the parent NativeView of the widget
+  // that the view is attached to is changed as a result of changing the view
+  // hierarchy. Overriding this method is useful for tracking which
+  // FocusManager manages this view.
+  virtual void NativeViewHierarchyChanged();
 
   // Painting ------------------------------------------------------------------
 
@@ -1260,9 +1256,7 @@ class VIEWS_EXPORT View : public ui::LayerDelegate,
 
   // Propagates NativeViewHierarchyChanged() notification through all the
   // children.
-  void PropagateNativeViewHierarchyChanged(bool attached,
-                                           gfx::NativeView native_view,
-                                           internal::RootView* root_view);
+  void PropagateNativeViewHierarchyChanged();
 
   // Takes care of registering/unregistering accelerators if
   // |register_accelerators| true and calls ViewHierarchyChanged().
@@ -1503,10 +1497,6 @@ class VIEWS_EXPORT View : public ui::LayerDelegate,
 
   // Accelerators --------------------------------------------------------------
 
-  // true if when we were added to hierarchy we were without focus manager
-  // attempt addition when ancestor chain changed.
-  bool accelerator_registration_delayed_;
-
   // Focus manager accelerators registered on.
   FocusManager* accelerator_focus_manager_;
 
diff --git a/ui/views/view_unittest.cc b/ui/views/view_unittest.cc
index dfb604e..6cd2a00 100644
--- a/ui/views/view_unittest.cc
+++ b/ui/views/view_unittest.cc
@@ -1545,98 +1545,71 @@ TEST_F(ViewTest, DISABLED_RerouteMouseWheelTest) {
 ////////////////////////////////////////////////////////////////////////////////
 // Native view hierachy
 ////////////////////////////////////////////////////////////////////////////////
-class TestNativeViewHierarchy : public View {
+class ToplevelWidgetObserverView : public View {
  public:
-  TestNativeViewHierarchy() {
+  ToplevelWidgetObserverView() : toplevel_(NULL) {
   }
-
-  virtual void NativeViewHierarchyChanged(
-      bool attached,
-      gfx::NativeView native_view,
-      internal::RootView* root_view) OVERRIDE {
-    NotificationInfo info;
-    info.attached = attached;
-    info.native_view = native_view;
-    info.root_view = root_view;
-    notifications_.push_back(info);
-  };
-  struct NotificationInfo {
-    bool attached;
-    gfx::NativeView native_view;
-    internal::RootView* root_view;
-  };
-  static const size_t kTotalViews = 2;
-  std::vector<NotificationInfo> notifications_;
-};
-
-class TestChangeNativeViewHierarchy {
- public:
-  explicit TestChangeNativeViewHierarchy(ViewTest *view_test) {
-    view_test_ = view_test;
-    native_host_ = new NativeViewHost();
-    host_ = new Widget;
-    Widget::InitParams params =
-        view_test->CreateParams(Widget::InitParams::TYPE_POPUP);
-    params.bounds = gfx::Rect(0, 0, 500, 300);
-    host_->Init(params);
-    host_->GetRootView()->AddChildView(native_host_);
-    for (size_t i = 0; i < TestNativeViewHierarchy::kTotalViews; ++i) {
-      windows_[i] = new Widget;
-      Widget::InitParams params(Widget::InitParams::TYPE_CONTROL);
-      params.parent = host_->GetNativeView();
-      params.bounds = gfx::Rect(0, 0, 500, 300);
-      windows_[i]->Init(params);
-      root_views_[i] = windows_[i]->GetRootView();
-      test_views_[i] = new TestNativeViewHierarchy;
-      root_views_[i]->AddChildView(test_views_[i]);
-    }
+  virtual ~ToplevelWidgetObserverView() {
   }
 
-  ~TestChangeNativeViewHierarchy() {
-    for (size_t i = 0; i < TestNativeViewHierarchy::kTotalViews; ++i) {
-      windows_[i]->Close();
+  // View overrides:
+  virtual void ViewHierarchyChanged(
+      const ViewHierarchyChangedDetails& details) OVERRIDE {
+    if (details.is_add) {
+      toplevel_ = GetWidget() ? GetWidget()->GetTopLevelWidget() : NULL;
+    } else {
+      toplevel_ = NULL;
     }
-    host_->Close();
-    // Will close and self-delete widgets - no need to manually delete them.
-    view_test_->RunPendingMessages();
   }
-
-  void CheckChangingHierarhy() {
-    size_t i;
-    for (i = 0; i < TestNativeViewHierarchy::kTotalViews; ++i) {
-      // TODO(georgey): use actual hierarchy changes to send notifications.
-      static_cast<internal::RootView*>(root_views_[i])->
-          NotifyNativeViewHierarchyChanged(false, host_->GetNativeView());
-      static_cast<internal::RootView*>(root_views_[i])->
-          NotifyNativeViewHierarchyChanged(true, host_->GetNativeView());
-    }
-    for (i = 0; i < TestNativeViewHierarchy::kTotalViews; ++i) {
-      ASSERT_EQ(static_cast<size_t>(2), test_views_[i]->notifications_.size());
-      EXPECT_FALSE(test_views_[i]->notifications_[0].attached);
-      EXPECT_EQ(host_->GetNativeView(),
-          test_views_[i]->notifications_[0].native_view);
-      EXPECT_EQ(root_views_[i], test_views_[i]->notifications_[0].root_view);
-      EXPECT_TRUE(test_views_[i]->notifications_[1].attached);
-      EXPECT_EQ(host_->GetNativeView(),
-          test_views_[i]->notifications_[1].native_view);
-      EXPECT_EQ(root_views_[i], test_views_[i]->notifications_[1].root_view);
-    }
+  virtual void NativeViewHierarchyChanged() OVERRIDE {
+    toplevel_ = GetWidget() ? GetWidget()->GetTopLevelWidget() : NULL;
   }
 
-  NativeViewHost* native_host_;
-  Widget* host_;
-  Widget* windows_[TestNativeViewHierarchy::kTotalViews];
-  View* root_views_[TestNativeViewHierarchy::kTotalViews];
-  TestNativeViewHierarchy* test_views_[TestNativeViewHierarchy::kTotalViews];
-  ViewTest* view_test_;
+  Widget* toplevel() { return toplevel_; }
+
+ private:
+  Widget* toplevel_;
+
+  DISALLOW_COPY_AND_ASSIGN(ToplevelWidgetObserverView);
 };
 
-TEST_F(ViewTest, ChangeNativeViewHierarchyChangeHierarchy) {
-  // TODO(georgey): Fix the test for Linux
-#if defined(OS_WIN)
-  TestChangeNativeViewHierarchy test(this);
-  test.CheckChangingHierarhy();
-#endif
+// Test that a view can track the current top level widget by overriding
+// View::ViewHierarchyChanged() and View::NativeViewHierarchyChanged().
+TEST_F(ViewTest, NativeViewHierarchyChanged) {
+  scoped_ptr<Widget> toplevel1(new Widget);
+  Widget::InitParams toplevel1_params =
+      CreateParams(Widget::InitParams::TYPE_POPUP);
+  toplevel1_params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+  toplevel1->Init(toplevel1_params);
+
+  scoped_ptr<Widget> toplevel2(new Widget);
+  Widget::InitParams toplevel2_params =
+      CreateParams(Widget::InitParams::TYPE_POPUP);
+  toplevel2_params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+  toplevel2->Init(toplevel2_params);
+
+  Widget* child = new Widget;
+  Widget::InitParams child_params(Widget::InitParams::TYPE_CONTROL);
+  child_params.parent = toplevel1->GetNativeView();
+  child->Init(child_params);
+
+  ToplevelWidgetObserverView* observer_view =
+      new ToplevelWidgetObserverView();
+  EXPECT_EQ(NULL, observer_view->toplevel());
+
+  child->SetContentsView(observer_view);
+  EXPECT_EQ(toplevel1, observer_view->toplevel());
+
+  Widget::ReparentNativeView(child->GetNativeView(),
+                             toplevel2->GetNativeView());
+  EXPECT_EQ(toplevel2, observer_view->toplevel());
+
+  observer_view->parent()->RemoveChildView(observer_view);
+  EXPECT_EQ(NULL, observer_view->toplevel());
+
+  // Make |observer_view| |child|'s contents view again so that it gets deleted
+  // with the widget.
+  child->SetContentsView(observer_view);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
diff --git a/ui/views/widget/native_widget_aura.cc b/ui/views/widget/native_widget_aura.cc
index 5348f24..f9ebed7 100644
--- a/ui/views/widget/native_widget_aura.cc
+++ b/ui/views/widget/native_widget_aura.cc
@@ -1089,7 +1089,7 @@ void NativeWidgetPrivate::ReparentNativeView(gfx::NativeView native_view,
   // from their previous parent.
   for (Widget::Widgets::iterator it = widgets.begin();
       it != widgets.end(); ++it) {
-    (*it)->NotifyNativeViewHierarchyChanged(false, previous_parent);
+    (*it)->NotifyNativeViewHierarchyWillChange();
   }
 
   if (new_parent) {
@@ -1114,7 +1114,7 @@ void NativeWidgetPrivate::ReparentNativeView(gfx::NativeView native_view,
   // And now, notify them that they have a brand new parent.
   for (Widget::Widgets::iterator it = widgets.begin();
       it != widgets.end(); ++it) {
-    (*it)->NotifyNativeViewHierarchyChanged(true, new_parent);
+    (*it)->NotifyNativeViewHierarchyChanged();
   }
 }
 
diff --git a/ui/views/widget/native_widget_win.cc b/ui/views/widget/native_widget_win.cc
index c0b84b6..472ff9c 100644
--- a/ui/views/widget/native_widget_win.cc
+++ b/ui/views/widget/native_widget_win.cc
@@ -1009,9 +1009,7 @@ void NativeWidgetPrivate::ReparentNativeView(gfx::NativeView native_view,
   // from their previous parent.
   for (Widget::Widgets::iterator it = widgets.begin();
        it != widgets.end(); ++it) {
-    // TODO(beng): Rename this notification to NotifyNativeViewChanging()
-    // and eliminate the bool parameter.
-    (*it)->NotifyNativeViewHierarchyChanged(false, previous_parent);
+    (*it)->NotifyNativeViewHierarchyWillChange();
   }
 
   ::SetParent(native_view, new_parent);
@@ -1019,7 +1017,7 @@ void NativeWidgetPrivate::ReparentNativeView(gfx::NativeView native_view,
   // And now, notify them that they have a brand new parent.
   for (Widget::Widgets::iterator it = widgets.begin();
        it != widgets.end(); ++it) {
-    (*it)->NotifyNativeViewHierarchyChanged(true, new_parent);
+    (*it)->NotifyNativeViewHierarchyChanged();
   }
 }
 
diff --git a/ui/views/widget/root_view.cc b/ui/views/widget/root_view.cc
index b304c52..d580e45 100644
--- a/ui/views/widget/root_view.cc
+++ b/ui/views/widget/root_view.cc
@@ -102,9 +102,8 @@ View* RootView::GetContentsView() {
   return child_count() > 0 ? child_at(0) : NULL;
 }
 
-void RootView::NotifyNativeViewHierarchyChanged(bool attached,
-                                                gfx::NativeView native_view) {
-  PropagateNativeViewHierarchyChanged(attached, native_view, this);
+void RootView::NotifyNativeViewHierarchyChanged() {
+  PropagateNativeViewHierarchyChanged();
 }
 
 // Input -----------------------------------------------------------------------
diff --git a/ui/views/widget/root_view.h b/ui/views/widget/root_view.h
index b89ab8f..053e7d1 100644
--- a/ui/views/widget/root_view.h
+++ b/ui/views/widget/root_view.h
@@ -60,8 +60,7 @@ class VIEWS_EXPORT RootView : public View,
   View* GetContentsView();
 
   // Called when parent of the host changed.
-  void NotifyNativeViewHierarchyChanged(bool attached,
-                                        gfx::NativeView native_view);
+  void NotifyNativeViewHierarchyChanged();
 
   // Input ---------------------------------------------------------------------
 
diff --git a/ui/views/widget/widget.cc b/ui/views/widget/widget.cc
index 0de8ccf..4bf2a85 100644
--- a/ui/views/widget/widget.cc
+++ b/ui/views/widget/widget.cc
@@ -427,16 +427,16 @@ void Widget::ViewHierarchyChanged(
   }
 }
 
-void Widget::NotifyNativeViewHierarchyChanged(bool attached,
-                                              gfx::NativeView native_view) {
-  if (!attached) {
-    FocusManager* focus_manager = GetFocusManager();
-    // We are being removed from a window hierarchy.  Treat this as
-    // the root_view_ being removed.
-    if (focus_manager)
-      focus_manager->ViewRemoved(root_view_.get());
-  }
-  root_view_->NotifyNativeViewHierarchyChanged(attached, native_view);
+void Widget::NotifyNativeViewHierarchyWillChange() {
+  FocusManager* focus_manager = GetFocusManager();
+  // We are being removed from a window hierarchy.  Treat this as
+  // the root_view_ being removed.
+  if (focus_manager)
+    focus_manager->ViewRemoved(root_view_.get());
+}
+
+void Widget::NotifyNativeViewHierarchyChanged() {
+  root_view_->NotifyNativeViewHierarchyChanged();
 }
 
 // Converted methods (see header) ----------------------------------------------
diff --git a/ui/views/widget/widget.h b/ui/views/widget/widget.h
index ed64f3b..e6067eb 100644
--- a/ui/views/widget/widget.h
+++ b/ui/views/widget/widget.h
@@ -331,9 +331,13 @@ class VIEWS_EXPORT Widget : public internal::NativeWidgetDelegate,
   // Forwarded from the RootView so that the widget can do any cleanup.
   void ViewHierarchyChanged(const View::ViewHierarchyChangedDetails& details);
 
-  // Performs any necessary cleanup and forwards to RootView.
-  void NotifyNativeViewHierarchyChanged(bool attached,
-                                        gfx::NativeView native_view);
+  // Called right before changing the widget's parent NativeView to do any
+  // cleanup.
+  void NotifyNativeViewHierarchyWillChange();
+
+  // Called after changing the widget's parent NativeView. Notifies the RootView
+  // about the change.
+  void NotifyNativeViewHierarchyChanged();
 
   // Returns the top level widget in a hierarchy (see is_top_level() for
   // the definition of top level widget.) Will return NULL if called