Revert "Make ClientView a child of the NonClientFrameView"

This reverts commit ec39b1d.

Reason for revert: Event dispatch is broken on cros. please see crbug.com/1190023

Original change's description:
> Make ClientView a child of the NonClientFrameView
>
> This refactor changes the hierarchy of views under
> NonClientView. Previously, NonClientFrameView and ClientView were
> siblings under NonClientView. This will change the hierarchy to:
> NonClientView > NonClientFrameView > ClientView.
>
> This change also enables a cleaner implementation of the Window
> Controls Overlay (see before (https://crrev.com/c/2504573) vs
> after (https://crrev.com/c/2545685))
>
> Window controls overlay links:
> Explainer: https://github.com/WICG/window-controls-overlay/blob/master/explainer.md
> Design Doc: https://docs.google.com/document/d/1k0YL_-VMLIfjYCgJ2v6cMvuUv2qMKg4BgLI2tJ4qtyo/edit?usp=sharing
> I2P: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/cper6nNLFRQ/hU91kfCWBQAJ
>
> Bug: 1175276, 937121
> Change-Id: If16de54a858f571c628b66c7801ef3777c6cc924
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2522616
> Commit-Queue: Amanda Baker <ambake@microsoft.com>
> Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
> Reviewed-by: Matt Giuca <mgiuca@chromium.org>
> Reviewed-by: David Tseng <dtseng@chromium.org>
> Reviewed-by: Leonard Grey <lgrey@chromium.org>
> Reviewed-by: Peter Kasting <pkasting@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#864068}

Bug: 1175276
Bug: 937121
Change-Id: I329a9b46f3144c97682cccf072d88b9730ffeb60
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2774966
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Commit-Queue: Sergey Ulanov <sergeyu@chromium.org>
Commit-Queue: Chase Phillips <cmp@chromium.org>
Auto-Submit: Mitsuru Oshima <oshima@chromium.org>
Owners-Override: Sergey Ulanov <sergeyu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#864795}

apps/ui/views/app_window_frame_view.cc

@@ -249,11 +249,8 @@ gfx::Size AppWindowFrameView::CalculatePreferredSize() const {
 }
 
 void AppWindowFrameView::Layout() {
-  NonClientFrameView::Layout();
-
   if (!draw_frame_)
     return;
-
   gfx::Size close_size = close_button_->GetPreferredSize();
   const int kButtonOffsetY = 0;
   const int kButtonSpacing = 1;

ash/app_list/views/search_box_view_unittest.cc

@@ -91,9 +91,9 @@ class SearchBoxViewTest : public views::test::WidgetTest,
   }
 
   void TearDown() override {
+    view_.reset();
     app_list_view_->GetWidget()->Close();
     widget_->CloseNow();
-    view_.reset();
     views::test::WidgetTest::TearDown();
   }
 

ash/frame/default_frame_header_unittest.cc

@@ -277,7 +277,7 @@ TEST_F(DefaultFrameHeaderTest, ResizeAndReorderDuringAnimation) {
     LayerDestroyedChecker checker(animating_layer);
 
     // Change the view's stacking order should stop the animation.
-    ASSERT_EQ(3u, frame_view_1->children().size());
+    ASSERT_EQ(2u, frame_view_1->children().size());
     frame_view_1->ReorderChildView(extra_view_1, 0);
 
     EXPECT_EQ(

ash/frame/non_client_frame_view_ash.cc

@@ -327,16 +327,16 @@ gfx::Size NonClientFrameViewAsh::CalculatePreferredSize() const {
 }
 
 void NonClientFrameViewAsh::Layout() {
-  views::NonClientFrameView::Layout();
-  if (!GetFrameEnabled())
+  if (!GetEnabled())
     return;
+  views::NonClientFrameView::Layout();
   aura::Window* frame_window = frame_->GetNativeWindow();
   frame_window->SetProperty(aura::client::kTopViewInset,
                             NonClientTopBorderHeight());
 }
 
 gfx::Size NonClientFrameViewAsh::GetMinimumSize() const {
-  if (!GetFrameEnabled())
+  if (!GetEnabled())
     return gfx::Size();
 
   gfx::Size min_client_view_size(frame_->client_view()->GetMinimumSize());
@@ -358,14 +358,21 @@ gfx::Size NonClientFrameViewAsh::GetMaximumSize() const {
   return gfx::Size(width, height);
 }
 
+void NonClientFrameViewAsh::SetVisible(bool visible) {
+  overlay_view_->SetVisible(visible);
+  views::View::SetVisible(visible);
+  // We need to re-layout so that client view will occupy entire window.
+  InvalidateLayout();
+}
+
 void NonClientFrameViewAsh::SetShouldPaintHeader(bool paint) {
   header_view_->SetShouldPaintHeader(paint);
 }
 
 int NonClientFrameViewAsh::NonClientTopBorderHeight() const {
   // The frame should not occupy the window area when it's in fullscreen,
   // not visible or disabled.
-  if (frame_->IsFullscreen() || !GetFrameEnabled() ||
+  if (frame_->IsFullscreen() || !GetVisible() || !GetEnabled() ||
       header_view_->in_immersive_mode()) {
     return 0;
   }
@@ -388,15 +395,6 @@ SkColor NonClientFrameViewAsh::GetInactiveFrameColorForTest() const {
   return frame_->GetNativeWindow()->GetProperty(kFrameInactiveColorKey);
 }
 
-void NonClientFrameViewAsh::SetFrameEnabled(bool enabled) {
-  if (enabled == frame_enabled_)
-    return;
-
-  frame_enabled_ = enabled;
-  overlay_view_->SetVisible(frame_enabled_);
-  InvalidateLayout();
-}
-
 void NonClientFrameViewAsh::OnDidSchedulePaint(const gfx::Rect& r) {
   // We may end up here before |header_view_| has been added to the Widget.
   if (header_view_->GetWidget()) {

ash/frame/non_client_frame_view_ash.h

@@ -90,6 +90,7 @@ class ASH_EXPORT NonClientFrameViewAsh : public views::NonClientFrameView {
   void Layout() override;
   gfx::Size GetMinimumSize() const override;
   gfx::Size GetMaximumSize() const override;
+  void SetVisible(bool visible) override;
 
   // If |paint| is false, we should not paint the header. Used for overview mode
   // with OnOverviewModeStarting() and OnOverviewModeEnded() to hide/show the
@@ -110,9 +111,6 @@ class ASH_EXPORT NonClientFrameViewAsh : public views::NonClientFrameView {
 
   views::Widget* frame() { return frame_; }
 
-  bool GetFrameEnabled() const { return frame_enabled_; }
-  virtual void SetFrameEnabled(bool enabled);
-
  protected:
   // views::View:
   void OnDidSchedulePaint(const gfx::Rect& r) override;
@@ -143,8 +141,6 @@ class ASH_EXPORT NonClientFrameViewAsh : public views::NonClientFrameView {
 
   OverlayView* overlay_view_ = nullptr;
 
-  bool frame_enabled_ = true;
-
   std::unique_ptr<NonClientFrameViewAshImmersiveHelper> immersive_helper_;
 
   base::CallbackListSubscription paint_as_active_subscription_ =

ash/frame/non_client_frame_view_ash_unittest.cc

@@ -543,19 +543,19 @@ TEST_F(NonClientFrameViewAshTest, FrameVisibility) {
       delegate->non_client_frame_view();
   EXPECT_EQ(client_bounds, widget->client_view()->GetLocalBounds().size());
 
-  non_client_frame_view->SetFrameEnabled(false);
+  non_client_frame_view->SetVisible(false);
   widget->GetRootView()->Layout();
   EXPECT_EQ(gfx::Size(200, 100),
             widget->client_view()->GetLocalBounds().size());
-  EXPECT_FALSE(non_client_frame_view->GetFrameEnabled());
+  EXPECT_FALSE(widget->non_client_view()->frame_view()->GetVisible());
   EXPECT_EQ(
       window_bounds,
       non_client_frame_view->GetClientBoundsForWindowBounds(window_bounds));
 
-  non_client_frame_view->SetFrameEnabled(true);
+  non_client_frame_view->SetVisible(true);
   widget->GetRootView()->Layout();
   EXPECT_EQ(client_bounds, widget->client_view()->GetLocalBounds().size());
-  EXPECT_TRUE(non_client_frame_view->GetFrameEnabled());
+  EXPECT_TRUE(widget->non_client_view()->frame_view()->GetVisible());
   EXPECT_EQ(32, delegate->GetNonClientFrameViewTopBorderHeight());
   EXPECT_EQ(
       gfx::Rect(gfx::Point(10, 42), client_bounds),

ash/system/holding_space/holding_space_tray_bubble.cc

@@ -319,8 +319,6 @@ HoldingSpaceTrayBubble::HoldingSpaceTrayBubble(
 
   // Create and customize bubble view.
   TrayBubbleView* bubble_view = new TrayBubbleView(init_params);
-  // Ensure bubble frame does not draw background behind bubble view.
-  bubble_view->set_color(SK_ColorTRANSPARENT);
   child_bubble_container_ =
       bubble_view->AddChildView(std::make_unique<ChildBubbleContainer>());
   child_bubble_container_->SetMaxHeight(CalculateMaxHeight());
@@ -341,6 +339,12 @@ HoldingSpaceTrayBubble::HoldingSpaceTrayBubble(
   bubble_wrapper_ = std::make_unique<TrayBubbleWrapper>(
       holding_space_tray, bubble_view, false /* is_persistent */);
 
+  // Set bubble frame to be invisible.
+  bubble_wrapper_->GetBubbleWidget()
+      ->non_client_view()
+      ->frame_view()
+      ->SetVisible(false);
+
   event_handler_ =
       std::make_unique<HoldingSpaceTrayBubbleEventHandler>(this, &delegate_);
 

ash/wm/system_modal_container_layout_manager_unittest.cc

@@ -547,12 +547,11 @@ TEST_F(SystemModalContainerLayoutManagerTest, ShowModalWhileHidden) {
 TEST_F(SystemModalContainerLayoutManagerTest, ChangeCapture) {
   std::unique_ptr<aura::Window> widget_window(ShowToplevelTestWindow(false));
   views::test::CaptureTrackingView* view = new views::test::CaptureTrackingView;
-  views::View* client_view =
+  views::View* contents_view =
       views::Widget::GetWidgetForNativeView(widget_window.get())
-          ->non_client_view()
-          ->client_view();
-  client_view->AddChildView(view);
-  view->SetBoundsRect(client_view->bounds());
+          ->GetContentsView();
+  contents_view->AddChildView(view);
+  view->SetBoundsRect(contents_view->bounds());
 
   gfx::Point center(view->width() / 2, view->height() / 2);
   views::View::ConvertPointToScreen(view, &center);

chrome/browser/resources/chromeos/accessibility/chromevox/background/background_test.js

@@ -3053,7 +3053,7 @@ TEST_F('ChromeVoxBackgroundTest', 'ImageAnnotations', function() {
 
 TEST_F('ChromeVoxBackgroundTest', 'VolumeChanges', function() {
   const mockFeedback = this.createMockFeedback();
-  this.runWithLoadedTree('<p>test</p>', function() {
+  this.runWithLoadedTree(``, function() {
     const bounds = ChromeVoxState.instance.getFocusBounds();
     mockFeedback.call(press(KeyCode.VOLUME_UP))
         .expectSpeech('Volume', 'Slider', /\d+%/)

chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_container.cc

@@ -89,6 +89,21 @@ class FullSizeBubbleFrameView : public views::BubbleFrameView {
   ~FullSizeBubbleFrameView() override = default;
 
  private:
+  // Overridden from views::ViewTargeterDelegate:
+  bool DoesIntersectRect(const View* target,
+                         const gfx::Rect& rect) const override {
+    // Make sure click events can still reach the close button, even if the
+    // ClientView overlaps it.
+    // NOTE: |rect| is in the mirrored coordinate space, so we must use the
+    // close button's mirrored bounds to correctly target the close button when
+    // in RTL mode.
+    if (IsCloseButtonVisible() &&
+        GetCloseButtonMirroredBounds().Intersects(rect)) {
+      return true;
+    }
+    return views::BubbleFrameView::DoesIntersectRect(target, rect);
+  }
+
   // Overridden from views::BubbleFrameView:
   bool ExtendClientIntoTitle() const override { return true; }
 };

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

@@ -66,11 +66,6 @@ BrowserNonClientFrameView::~BrowserNonClientFrameView() {
     g_browser_process->profile_manager()->
         GetProfileAttributesStorage().RemoveObserver(this);
   }
-
-  // WebAppFrameToolbarView::ToolbarButtonContainer is an
-  // ImmersiveModeController::Observer, so it must be destroyed before the
-  // BrowserView destroys the ImmersiveModeController.
-  delete web_app_frame_toolbar_;
 }
 
 void BrowserNonClientFrameView::OnBrowserViewInitViewsComplete() {
@@ -299,6 +294,68 @@ void BrowserNonClientFrameView::ChildPreferredSizeChanged(views::View* child) {
     Layout();
 }
 
+bool BrowserNonClientFrameView::DoesIntersectRect(const views::View* target,
+                                                  const gfx::Rect& rect) const {
+  DCHECK_EQ(target, this);
+  if (!views::ViewTargeterDelegate::DoesIntersectRect(this, rect)) {
+    // |rect| is outside the frame's bounds.
+    return false;
+  }
+
+  bool should_leave_to_top_container = false;
+#if BUILDFLAG(IS_CHROMEOS_ASH) || BUILDFLAG(IS_CHROMEOS_LACROS)
+  // In immersive mode, the caption buttons container is reparented to the
+  // TopContainerView and hence |rect| should not be claimed here.  See
+  // BrowserNonClientFrameViewChromeOS::OnImmersiveRevealStarted().
+  should_leave_to_top_container =
+      browser_view_->immersive_mode_controller()->IsRevealed();
+#endif
+
+  if (!browser_view_->GetTabStripVisible()) {
+    // Claim |rect| if it is above the top of the topmost client area view.
+    return !should_leave_to_top_container && (rect.y() < GetTopInset(false));
+  }
+
+  // If the rect is outside the bounds of the client area, claim it.
+  gfx::RectF rect_in_client_view_coords_f(rect);
+  View::ConvertRectToTarget(this, frame_->client_view(),
+                            &rect_in_client_view_coords_f);
+  gfx::Rect rect_in_client_view_coords =
+      gfx::ToEnclosingRect(rect_in_client_view_coords_f);
+  if (!frame_->client_view()->HitTestRect(rect_in_client_view_coords))
+    return true;
+
+  // Otherwise, claim |rect| only if it is above the bottom of the tab strip
+  // region view in a non-tab portion.
+  TabStripRegionView* tab_strip_region_view =
+      browser_view_->tab_strip_region_view();
+
+  // The |tab_strip_region_view| may not be in a Widget (e.g. when switching
+  // into immersive reveal the BrowserView's TopContainerView is reparented).
+  if (tab_strip_region_view->GetWidget()) {
+    gfx::RectF rect_in_region_view_coords_f(rect);
+    View::ConvertRectToTarget(this, tab_strip_region_view,
+                              &rect_in_region_view_coords_f);
+    gfx::Rect rect_in_region_view_coords =
+        gfx::ToEnclosingRect(rect_in_region_view_coords_f);
+    if (rect_in_region_view_coords.y() >=
+        tab_strip_region_view->GetLocalBounds().bottom()) {
+      // |rect| is below the tab_strip_region_view.
+      return false;
+    }
+
+    if (tab_strip_region_view->HitTestRect(rect_in_region_view_coords)) {
+      // Claim |rect| if it is in a non-tab portion of the tabstrip.
+      return tab_strip_region_view->IsRectInWindowCaption(
+          rect_in_region_view_coords);
+    }
+  }
+
+  // We claim |rect| because it is above the bottom of the tabstrip, but
+  // not in the tabstrip itself.
+  return !should_leave_to_top_container;
+}
+
 void BrowserNonClientFrameView::OnProfileAdded(
     const base::FilePath& profile_path) {
   OnProfileAvatarChanged(profile_path);

chrome/browser/ui/views/frame/browser_non_client_frame_view.h

@@ -157,6 +157,8 @@ class BrowserNonClientFrameView : public views::NonClientFrameView,
 
   // views::NonClientFrameView:
   void ChildPreferredSizeChanged(views::View* child) override;
+  bool DoesIntersectRect(const views::View* target,
+                         const gfx::Rect& rect) const override;
 
   // ProfileAttributesStorage::Observer:
   void OnProfileAdded(const base::FilePath& profile_path) override;

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

@@ -401,27 +401,6 @@ void BrowserNonClientFrameViewChromeOS::ChildPreferredSizeChanged(
   }
 }
 
-bool BrowserNonClientFrameViewChromeOS::DoesIntersectRect(
-    const views::View* target,
-    const gfx::Rect& rect) const {
-  DCHECK_EQ(target, this);
-  if (!views::ViewTargeterDelegate::DoesIntersectRect(this, rect)) {
-    // |rect| is outside the frame's bounds.
-    return false;
-  }
-
-  bool should_leave_to_top_container = false;
-#if BUILDFLAG(IS_CHROMEOS_ASH) || BUILDFLAG(IS_CHROMEOS_LACROS)
-  // In immersive mode, the caption buttons container is reparented to the
-  // TopContainerView and hence |rect| should not be claimed here.  See
-  // BrowserNonClientFrameViewChromeOS::OnImmersiveRevealStarted().
-  should_leave_to_top_container =
-      browser_view()->immersive_mode_controller()->IsRevealed();
-#endif
-
-  return !should_leave_to_top_container;
-}
-
 SkColor BrowserNonClientFrameViewChromeOS::GetTitleColor() {
   return browser_view()->GetRegularOrGuestSession()
              ? kNormalWindowTitleTextColor
@@ -582,13 +561,7 @@ void BrowserNonClientFrameViewChromeOS::OnImmersiveRevealStarted() {
 }
 
 void BrowserNonClientFrameViewChromeOS::OnImmersiveRevealEnded() {
-  // Ensure the caption button container receives events before the browser view
-  // by placing it higher in the z-order.
-  // [0] - FrameAnimatorView
-  // [1] - BrowserView
-  // [2] - FrameCaptionButtonContainerView
-  const int kCaptionButtonContainerIndex = 2;
-  AddChildViewAt(caption_button_container_, kCaptionButtonContainerIndex);
+  AddChildViewAt(caption_button_container_, 0);
   if (web_app_frame_toolbar())
     AddChildViewAt(web_app_frame_toolbar(), 0);
   Layout();

chrome/browser/ui/views/frame/browser_non_client_frame_view_chromeos.h

@@ -79,8 +79,6 @@ class BrowserNonClientFrameViewChromeOS
   gfx::Size GetMinimumSize() const override;
   void OnThemeChanged() override;
   void ChildPreferredSizeChanged(views::View* child) override;
-  bool DoesIntersectRect(const views::View* target,
-                         const gfx::Rect& rect) const override;
 
   // BrowserFrameHeaderChromeOS::AppearanceProvider:
   SkColor GetTitleColor() override;

chrome/browser/ui/views/frame/browser_non_client_frame_view_mac.mm

@@ -338,8 +338,6 @@ FullscreenToolbarStyle GetUserPreferredToolbarStyle(bool always_show) {
 }
 
 void BrowserNonClientFrameViewMac::Layout() {
-  NonClientFrameView::Layout();
-
   const int available_height = GetTopInset(true);
   int leading_x = kFramePaddingLeft;
   int trailing_x = width();

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

@@ -61,15 +61,12 @@ class BrowserNonClientFrameViewPopupTest
 #define MAYBE_HitTestPopupTopChrome HitTestPopupTopChrome
 #endif
 TEST_F(BrowserNonClientFrameViewPopupTest, MAYBE_HitTestPopupTopChrome) {
-  constexpr gfx::Rect kLeftOfFrame(-1, 4, 1, 1);
-  EXPECT_FALSE(frame_view_->HitTestRect(kLeftOfFrame));
-
-  constexpr gfx::Rect kAboveFrame(4, -1, 1, 1);
-  EXPECT_FALSE(frame_view_->HitTestRect(kAboveFrame));
-
+  EXPECT_FALSE(frame_view_->HitTestRect(gfx::Rect(-1, 4, 1, 1)));
+  EXPECT_FALSE(frame_view_->HitTestRect(gfx::Rect(4, -1, 1, 1)));
   const int top_inset = frame_view_->GetTopInset(false);
-  const gfx::Rect in_browser_view(4, top_inset, 1, 1);
-  EXPECT_TRUE(frame_view_->HitTestRect(in_browser_view));
+  EXPECT_FALSE(frame_view_->HitTestRect(gfx::Rect(4, top_inset, 1, 1)));
+  if (top_inset > 0)
+    EXPECT_TRUE(frame_view_->HitTestRect(gfx::Rect(4, top_inset - 1, 1, 1)));
 }
 
 class BrowserNonClientFrameViewTabbedTest
@@ -111,9 +108,7 @@ TEST_F(BrowserNonClientFrameViewTabbedTest, MAYBE_HitTestTabstrip) {
 
   // Hits client portions of the tabstrip (near the bottom left corner of the
   // first tab).
-  EXPECT_TRUE(frame_view_->HitTestRect(gfx::Rect(
-      tabstrip_bounds.x() + 10, tabstrip_bounds.bottom() - 10, 1, 1)));
-  EXPECT_TRUE(frame_view_->browser_view()->HitTestRect(gfx::Rect(
+  EXPECT_FALSE(frame_view_->HitTestRect(gfx::Rect(
       tabstrip_bounds.x() + 10, tabstrip_bounds.bottom() - 10, 1, 1)));
 
 // Tabs extend to the top of the tabstrip everywhere in this test context on