Refactor WebAppFrameToolbarView ownership 3/x: Linux.

This makes PWAs work on Linux with the WebAppFrameToolbarInBrowserView
feature enabled. Besides changes guarded by that flag, this also
includes one small change when the flag is disabled: when calculating
the space taken up by window controls overlay in "web" coordinates,
the code did subtract top insets from the y coordinate, but did not
subtract the same distance from the height of the area. This CL fixes
this (since replicating the buggy behavior in the new implementation
would be harder). While this doesn't change the layout of anything
directly, since it does change what values are exposed via the web
API, websites using Window Controls Overlay might look slightly
different.

Bug: 1407240
Change-Id: I4c3cdc98a626aa421d1451973b4a35b4444a0869
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4179737
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1099019}

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

@@ -84,6 +84,7 @@ class TestLayoutDelegate : public OpaqueBrowserFrameViewLayoutDelegate {
 #if BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_CHROMEOS_LACROS)
   ui::WindowTiledEdges GetTiledEdges() const override { return {}; }
 #endif
+  int WebAppButtonHeight() const override { return 0; }
 };
 
 class TestNavButtonProvider : public ui::NavButtonProvider {

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

@@ -51,6 +51,7 @@ class TestLayoutDelegate : public OpaqueBrowserFrameViewLayoutDelegate {
 #if BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_CHROMEOS_LACROS)
   ui::WindowTiledEdges GetTiledEdges() const override { return tiled_edges_; }
 #endif
+  int WebAppButtonHeight() const override { return 0; }
 
   ui::WindowTiledEdges tiled_edges_;
 };

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

@@ -25,6 +25,7 @@
 #include "chrome/browser/ui/views/toolbar/toolbar_view.h"
 #include "chrome/browser/ui/views/web_apps/frame_toolbar/web_app_frame_toolbar_view.h"
 #include "chrome/browser/ui/web_applications/app_browser_controller.h"
+#include "chrome/common/chrome_features.h"
 #include "chrome/grit/generated_resources.h"
 #include "chrome/grit/theme_resources.h"
 #include "components/strings/grit/components_strings.h"
@@ -215,20 +216,27 @@ void OpaqueBrowserFrameView::InitViews() {
                      .Build());
   }
 
-  if (controller) {
+  if (controller && !base::FeatureList::IsEnabled(
+                        features::kWebAppFrameToolbarInBrowserView)) {
     set_web_app_frame_toolbar(
         AddChildView(std::make_unique<WebAppFrameToolbarView>(browser_view())));
   }
 
-  // The window title appears above the web app frame toolbar (if present),
-  // which surrounds the title with minimal-ui buttons on the left,
-  // and other controls (such as the app menu button) on the right.
-  window_title_ = new views::Label(browser_view()->GetWindowTitle());
-  window_title_->SetVisible(browser_view()->ShouldShowWindowTitle());
-  window_title_->SetSubpixelRenderingEnabled(false);
-  window_title_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-  window_title_->SetID(VIEW_ID_WINDOW_TITLE);
-  AddChildView(window_title_.get());
+  // If kWebappFrameToolbarInBrowserView is enabled and this is a web app
+  // window, the window title will be part of the BrowserView and thus we don't
+  // need to create another one here.
+  if (!controller || !base::FeatureList::IsEnabled(
+                         features::kWebAppFrameToolbarInBrowserView)) {
+    // The window title appears above the web app frame toolbar (if present),
+    // which surrounds the title with minimal-ui buttons on the left,
+    // and other controls (such as the app menu button) on the right.
+    window_title_ = new views::Label(browser_view()->GetWindowTitle());
+    window_title_->SetVisible(browser_view()->ShouldShowWindowTitle());
+    window_title_->SetSubpixelRenderingEnabled(false);
+    window_title_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+    window_title_->SetID(VIEW_ID_WINDOW_TITLE);
+    AddChildView(window_title_.get());
+  }
 
 #if BUILDFLAG(IS_WIN)
   if (browser_view()->AppUsesWindowControlsOverlay())
@@ -246,16 +254,13 @@ gfx::Rect OpaqueBrowserFrameView::GetBoundsForTabStripRegion(
 
 gfx::Rect OpaqueBrowserFrameView::GetBoundsForWebAppFrameToolbar(
     const gfx::Size& toolbar_preferred_size) const {
-  // TODO(https://crbug.com/1407240): Implement this method to make
-  // WebAppFrameToolbar-in-BrowserView work.
-  return gfx::Rect();
+  return layout_->GetBoundsForWebAppFrameToolbar(toolbar_preferred_size);
 }
 
 void OpaqueBrowserFrameView::LayoutWebAppWindowTitle(
     const gfx::Rect& available_space,
     views::Label& window_title_label) const {
-  // TODO(https://crbug.com/1407240): Implement this method to make
-  // WebAppFrameToolbar-in-BrowserView work.
+  layout_->LayoutWebAppWindowTitle(available_space, window_title_label);
 }
 
 int OpaqueBrowserFrameView::GetTopInset(bool restored) const {
@@ -421,7 +426,9 @@ void OpaqueBrowserFrameView::UpdateWindowIcon() {
 void OpaqueBrowserFrameView::UpdateWindowTitle() {
   if (!frame()->IsFullscreen() && ShouldShowWindowTitle()) {
     Layout();
-    window_title_->SchedulePaint();
+    if (window_title_) {
+      window_title_->SchedulePaint();
+    }
   }
 }
 
@@ -543,13 +550,22 @@ gfx::Size OpaqueBrowserFrameView::GetTabstripMinimumSize() const {
 }
 
 int OpaqueBrowserFrameView::GetTopAreaHeight() const {
-  const int non_client_top_height = layout_->NonClientTopHeight(false);
-  if (!browser_view()->GetTabStripVisible())
-    return non_client_top_height;
-  return std::max(
-      non_client_top_height,
-      GetBoundsForTabStripRegion(GetTabstripMinimumSize()).bottom() -
-          GetLayoutConstant(TABSTRIP_TOOLBAR_OVERLAP));
+  int top_height = layout_->NonClientTopHeight(false);
+  if (browser_view()->GetTabStripVisible()) {
+    top_height =
+        std::max(top_height,
+                 GetBoundsForTabStripRegion(GetTabstripMinimumSize()).bottom() -
+                     GetLayoutConstant(TABSTRIP_TOOLBAR_OVERLAP));
+  }
+  if (base::FeatureList::IsEnabled(
+          features::kWebAppFrameToolbarInBrowserView)) {
+    gfx::Rect web_app_toolbar_bounds = GetBoundsForWebAppFrameToolbar(
+        browser_view()->GetWebAppFrameToolbarPreferredSize());
+    if (!web_app_toolbar_bounds.IsEmpty()) {
+      top_height = std::max(top_height, web_app_toolbar_bounds.bottom());
+    }
+  }
+  return top_height;
 }
 
 bool OpaqueBrowserFrameView::UseCustomFrame() const {
@@ -598,6 +614,10 @@ ui::WindowTiledEdges OpaqueBrowserFrameView::GetTiledEdges() const {
 }
 #endif
 
+int OpaqueBrowserFrameView::WebAppButtonHeight() const {
+  return browser_view()->GetWebAppFrameToolbarPreferredSize().height();
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // OpaqueBrowserFrameView, protected:
 
@@ -609,9 +629,11 @@ void OpaqueBrowserFrameView::OnPaint(gfx::Canvas* canvas) {
 
   const bool active = ShouldPaintAsActive();
   SkColor frame_color = GetFrameColor(BrowserFrameActiveState::kUseCurrent);
-  window_title_->SetEnabledColor(
-      GetCaptionColor(BrowserFrameActiveState::kUseCurrent));
-  window_title_->SetBackgroundColor(frame_color);
+  if (window_title_) {
+    window_title_->SetEnabledColor(
+        GetCaptionColor(BrowserFrameActiveState::kUseCurrent));
+    window_title_->SetBackgroundColor(frame_color);
+  }
   frame_background_->set_frame_color(frame_color);
   frame_background_->set_use_custom_frame(frame()->UseCustomFrame());
   frame_background_->set_is_active(active);

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

@@ -120,6 +120,7 @@ class OpaqueBrowserFrameView : public BrowserNonClientFrameView,
 #if BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_CHROMEOS_LACROS)
   ui::WindowTiledEdges GetTiledEdges() const override;
 #endif
+  int WebAppButtonHeight() const override;
 
  protected:
   views::Button* minimize_button() const { return minimize_button_; }

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

@@ -17,6 +17,7 @@
 #include "build/build_config.h"
 #include "chrome/browser/ui/views/frame/caption_button_placeholder_container.h"
 #include "chrome/browser/ui/views/web_apps/frame_toolbar/web_app_frame_toolbar_view.h"
+#include "chrome/common/chrome_features.h"
 #include "chrome/common/chrome_switches.h"
 #include "ui/compositor/layer.h"
 #include "ui/gfx/font.h"
@@ -105,6 +106,34 @@ gfx::Rect OpaqueBrowserFrameViewLayout::GetBoundsForTabStripRegion(
                    tabstrip_minimum_size.height());
 }
 
+gfx::Rect OpaqueBrowserFrameViewLayout::GetBoundsForWebAppFrameToolbar(
+    const gfx::Size& toolbar_preferred_size) const {
+  if (delegate_->IsFullscreen()) {
+    return gfx::Rect();
+  }
+
+  // Adding 2px of vertical padding puts at least 1 px of space on the top and
+  // bottom of the element.
+  constexpr int kVerticalPadding = 2;
+
+  const int x = available_space_leading_x_;
+  const int available_width = available_space_trailing_x_ - x;
+  return gfx::Rect(x, FrameEdgeInsets(false).top(),
+                   std::max(0, available_width),
+                   toolbar_preferred_size.height() + kVerticalPadding +
+                       kContentEdgeShadowThickness);
+}
+
+void OpaqueBrowserFrameViewLayout::LayoutWebAppWindowTitle(
+    const gfx::Rect& available_space,
+    views::Label& window_title_label) const {
+  gfx::Rect bounds = available_space;
+  bounds.Inset(gfx::Insets::TLBR(0, kIconTitleSpacing, 0, kCaptionSpacing));
+  window_title_label.SetSubpixelRenderingEnabled(false);
+  window_title_label.SetHorizontalAlignment(gfx::ALIGN_LEFT);
+  window_title_label.SetBoundsRect(bounds);
+}
+
 gfx::Size OpaqueBrowserFrameViewLayout::GetMinimumSize(
     const views::View* host) const {
   // Ensure that we can fit the main browser view.
@@ -161,8 +190,16 @@ int OpaqueBrowserFrameViewLayout::NonClientTopHeight(bool restored) const {
   const int caption_button_height = DefaultCaptionButtonY(restored) +
                                     kCaptionButtonHeight +
                                     kCaptionButtonBottomPadding;
+
   int web_app_button_height = 0;
-  if (web_app_frame_toolbar_) {
+  if (base::FeatureList::IsEnabled(
+          features::kWebAppFrameToolbarInBrowserView)) {
+    web_app_button_height = delegate_->WebAppButtonHeight();
+    if (web_app_button_height > 0) {
+      web_app_button_height +=
+          FrameEdgeInsets(restored).top() + kVerticalPadding;
+    }
+  } else if (web_app_frame_toolbar_) {
     web_app_button_height =
         FrameEdgeInsets(restored).top() +
         web_app_frame_toolbar_->GetPreferredSize().height() + kVerticalPadding;
@@ -210,6 +247,11 @@ gfx::Rect OpaqueBrowserFrameViewLayout::CalculateClientAreaBounds(
       (is_window_controls_overlay_enabled_ || is_borderless_mode_enabled_)
           ? border_thickness.top()
           : NonClientTopHeight(false);
+  if (base::FeatureList::IsEnabled(
+          features::kWebAppFrameToolbarInBrowserView) &&
+      delegate_->WebAppButtonHeight() > 0) {
+    top_height = border_thickness.top();
+  }
   return gfx::Rect(
       border_thickness.left(), top_height,
       std::max(0, width - border_thickness.width()),
@@ -345,7 +387,8 @@ void OpaqueBrowserFrameViewLayout::LayoutTitleBar() {
   // potentially be replaced with checks that |web_app_frame_toolbar_| is
   // non-null.
   bool should_show_toolbar =
-      !delegate_->IsFullscreen() && web_app_frame_toolbar_;
+      (!delegate_->IsFullscreen() && web_app_frame_toolbar_) ||
+      delegate_->WebAppButtonHeight() > 0;
   absl::optional<int> icon_spacing;
 
   if (should_show_icon || should_show_title || should_show_toolbar) {
@@ -384,7 +427,7 @@ void OpaqueBrowserFrameViewLayout::LayoutTitleBar() {
     available_space_leading_x_ += size;
     minimum_size_for_buttons_ += size;
 
-    if (should_show_toolbar) {
+    if (should_show_toolbar && web_app_frame_toolbar_) {
       std::pair<int, int> remaining_bounds =
           web_app_frame_toolbar_->LayoutInContainer(
               available_space_leading_x_, available_space_trailing_x_,
@@ -659,8 +702,10 @@ void OpaqueBrowserFrameViewLayout::LayoutTitleBarForWindowControlsOverlay(
 
     web_app_frame_toolbar_view_width = available_space_trailing_x_;
 
-    available_space_trailing_x_ -=
-        web_app_frame_toolbar_->GetPreferredSize().width();
+    if (web_app_frame_toolbar_) {
+      available_space_trailing_x_ -=
+          web_app_frame_toolbar_->GetPreferredSize().width();
+    }
   }
 
   auto insets = FrameBorderInsets(/*restored=*/false);
@@ -669,16 +714,18 @@ void OpaqueBrowserFrameViewLayout::LayoutTitleBarForWindowControlsOverlay(
       container_x, insets.top(), minimum_size_for_buttons_ - insets.width(),
       height - insets.top());
 
-  web_app_frame_toolbar_->LayoutForWindowControlsOverlay(
-      gfx::Rect(x, insets.top(), web_app_frame_toolbar_view_width,
-                height - insets.top()));
-
-  int bounding_rect_width =
-      web_app_frame_toolbar_->bounds().x() - available_space_leading_x_;
-  // Set y to 0 for the bounding_rect as this is web contents coordinates and
-  // so, FrameBorderThickness should not be included.
-  delegate_->UpdateWindowControlsOverlay(
-      host->GetMirroredRect(gfx::Rect(x, 0, bounding_rect_width, height)));
+  if (web_app_frame_toolbar_) {
+    web_app_frame_toolbar_->LayoutForWindowControlsOverlay(
+        gfx::Rect(x, insets.top(), web_app_frame_toolbar_view_width,
+                  height - insets.top()));
+
+    int bounding_rect_width =
+        web_app_frame_toolbar_->bounds().x() - available_space_leading_x_;
+    // Set y to 0 for the bounding_rect as this is web contents coordinates and
+    // so, FrameBorderThickness should not be included.
+    delegate_->UpdateWindowControlsOverlay(host->GetMirroredRect(
+        gfx::Rect(x, 0, bounding_rect_width, height - insets.top())));
+  }
 }
 
 ///////////////////////////////////////////////////////////////////////////////

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

@@ -59,6 +59,10 @@ class OpaqueBrowserFrameViewLayout : public views::LayoutManager {
 
   gfx::Rect GetBoundsForTabStripRegion(const gfx::Size& tabstrip_minimum_size,
                                        int total_width) const;
+  gfx::Rect GetBoundsForWebAppFrameToolbar(
+      const gfx::Size& toolbar_preferred_size) const;
+  void LayoutWebAppWindowTitle(const gfx::Rect& available_space,
+                               views::Label& window_title_label) const;
 
   // Returns the bounds of the window required to display the content area at
   // the specified bounds.

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

@@ -103,6 +103,11 @@ class OpaqueBrowserFrameViewLayoutDelegate {
   virtual ui::WindowTiledEdges GetTiledEdges() const = 0;
 #endif
 
+  // Returns the (preferred) heights of buttons in the web app frame toolbar. If
+  // the toolbar isn't visible, or if features::kWebAppFrameToolbarInBrowserView
+  // isn't enabled, this returns 0.
+  virtual int WebAppButtonHeight() const = 0;
+
  protected:
   virtual ~OpaqueBrowserFrameViewLayoutDelegate() = default;
 };

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

@@ -96,6 +96,7 @@ class TestLayoutDelegate : public OpaqueBrowserFrameViewLayoutDelegate {
 #if BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_CHROMEOS_LACROS)
   ui::WindowTiledEdges GetTiledEdges() const override { return {}; }
 #endif
+  int WebAppButtonHeight() const override { return 0; }
 
  private:
   std::u16string window_title_;