[omnibox][webui] Size by ResizeObserver not EnableSizingFromWebContents

This CL eliminates use of EnableSizingFromWebContents and the related
FrameSizeChanged method because the sizing behavior observed on
different systems was unpredictable and generally incorrect. With this
change, the browser's ResizeObserver API is used to detect the actual
size of the webui dropdown content element. The actual size is plumbed
through the RealboxHandler to the OmniboxPopupPresenter so it can size
the containing widget accurately.

Bug: 1479181
Change-Id: I3caa1dbbba7d3910db2718573db2a702e7bf1334
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4923182
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: John Lee <johntlee@chromium.org>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Commit-Queue: Orin Jaworski <orinj@chromium.org>
Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1212402}

chrome/browser/ui/views/omnibox/omnibox_popup_presenter.cc

@@ -60,7 +60,10 @@ void OmniboxPopupPresenter::Show() {
     widget_->SetContentsView(
         std::make_unique<RoundedOmniboxResultsFrame>(this, location_bar_view_));
     widget_->AddObserver(this);
-    FrameSizeChanged(nullptr, gfx::Size(0, 0));
+  }
+  RealboxHandler* handler = GetHandler();
+  if (handler && !handler->HasObserver(this)) {
+    handler->AddObserver(this);
   }
 }
 
@@ -90,40 +93,32 @@ RealboxHandler* OmniboxPopupPresenter::GetHandler() {
   return omnibox_popup_ui->handler();
 }
 
+void OmniboxPopupPresenter::OnWidgetDestroyed(views::Widget* widget) {
+  if (widget == widget_) {
+    widget_ = nullptr;
+  }
+}
+
 // TODO(crbug.com/1396174): This should also be called when LocationBarView
 //  size is changed.
-void OmniboxPopupPresenter::FrameSizeChanged(
-    content::RenderFrameHost* render_frame_host,
-    const gfx::Size& frame_size) {
+void OmniboxPopupPresenter::OnPopupElementSizeChanged(gfx::Size size) {
   if (widget_) {
+    // The width is known, and is the basis for consistent web content rendering
+    // so width is specified exactly; then only height adjusts dynamically.
     gfx::Rect widget_bounds = location_bar_view_->GetBoundsInScreen();
     widget_bounds.Inset(
         -RoundedOmniboxResultsFrame::GetLocationBarAlignmentInsets());
 
-    // The width is known, and is the basis for consistent web content rendering
-    // so width is specified exactly; then only height adjusts dynamically.
     // TODO(crbug.com/1396174): Change max height according to max suggestion
     //  count and calculated row height, or use a more general maximum value.
-    const int width = widget_bounds.width();
-    constexpr int kMaxHeight = 480;
-    EnableSizingFromWebContents(gfx::Size(width, 1),
-                                gfx::Size(width, kMaxHeight));
-
+    constexpr int kMaxHeight = 600;
     widget_bounds.set_height(widget_bounds.height() +
-                             std::min(kMaxHeight, frame_size.height()));
+                             std::min(kMaxHeight, size.height()));
     widget_bounds.Inset(-RoundedOmniboxResultsFrame::GetShadowInsets());
     widget_->SetBounds(widget_bounds);
   }
 }
 
-void OmniboxPopupPresenter::OnWidgetDestroyed(views::Widget* widget) {
-  // TODO(crbug.com/1445142): Consider restoring if not closed logically by
-  // omnibox.
-  if (widget == widget_) {
-    widget_ = nullptr;
-  }
-}
-
 bool OmniboxPopupPresenter::IsHandlerReady() {
   OmniboxPopupUI* omnibox_popup_ui = static_cast<OmniboxPopupUI*>(
       GetWebContents()->GetWebUI()->GetController());
@@ -132,6 +127,10 @@ bool OmniboxPopupPresenter::IsHandlerReady() {
 }
 
 void OmniboxPopupPresenter::ReleaseWidget(bool close) {
+  RealboxHandler* handler = GetHandler();
+  if (handler && handler->HasObserver(this)) {
+    handler->RemoveObserver(this);
+  }
   if (widget_) {
     // Avoid possibility of dangling raw_ptr by nulling before cleanup.
     views::Widget* widget = widget_;

chrome/browser/ui/views/omnibox/omnibox_popup_presenter.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_UI_VIEWS_OMNIBOX_OMNIBOX_POPUP_PRESENTER_H_
 #define CHROME_BROWSER_UI_VIEWS_OMNIBOX_OMNIBOX_POPUP_PRESENTER_H_
 
+#include "chrome/browser/ui/webui/realbox/realbox_handler.h"
 #include "content/public/browser/render_frame_host.h"
 #include "ui/base/metadata/metadata_header_macros.h"
 #include "ui/gfx/geometry/rect.h"
@@ -15,7 +16,6 @@
 
 class LocationBarView;
 class OmniboxController;
-class RealboxHandler;
 
 // An assistant class for OmniboxPopupViewWebUI, this manages a WebView and a
 // Widget to present WebUI suggestions.  This class is an implementation detail
@@ -24,7 +24,8 @@ class RealboxHandler;
 // logic concerns and communication between native omnibox code and the WebUI
 // code, work with OmniboxPopupViewWebUI directly.
 class OmniboxPopupPresenter : public views::WebView,
-                              public views::WidgetObserver {
+                              public views::WidgetObserver,
+                              public OmniboxWebUIPopupChangeObserver {
  public:
   METADATA_HEADER(OmniboxPopupPresenter);
   explicit OmniboxPopupPresenter(LocationBarView* location_bar_view,
@@ -44,13 +45,12 @@ class OmniboxPopupPresenter : public views::WebView,
   // Returns nullptr if handler is not ready.
   RealboxHandler* GetHandler();
 
-  // views::WebView
-  void FrameSizeChanged(content::RenderFrameHost* render_frame_host,
-                        const gfx::Size& frame_size) override;
-
   // views::WidgetObserver:
   void OnWidgetDestroyed(views::Widget* widget) override;
 
+  // RealboxWebUIChangeClient:
+  void OnPopupElementSizeChanged(gfx::Size size) override;
+
  private:
   friend class OmniboxPopupViewWebUITest;
 

chrome/browser/ui/views/omnibox/omnibox_popup_view_webui_interactive_uitest.cc

@@ -68,10 +68,12 @@ IN_PROC_BROWSER_TEST_F(OmniboxPopupViewWebUITest,
 
 IN_PROC_BROWSER_TEST_F(OmniboxPopupViewWebUITest, PopupLoadsAndAcceptsCalls) {
   WaitForHandler();
+  popup_view()->presenter_->Show();
   popup_view()->UpdatePopupAppearance();
   OmniboxPopupSelection selection(OmniboxPopupSelection::kNoMatch);
   popup_view()->OnSelectionChanged(selection, selection);
   popup_view()->ProvideButtonFocusHint(0);
+  popup_view()->presenter_->Hide();
 }
 
 #endif  // BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC)

chrome/browser/ui/webui/realbox/realbox_handler.cc

@@ -800,6 +800,20 @@ bool RealboxHandler::IsRemoteBound() const {
   return page_set_;
 }
 
+void RealboxHandler::AddObserver(OmniboxWebUIPopupChangeObserver* observer) {
+  observers_.AddObserver(observer);
+  observer->OnPopupElementSizeChanged(webui_size_);
+}
+
+void RealboxHandler::RemoveObserver(OmniboxWebUIPopupChangeObserver* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+bool RealboxHandler::HasObserver(
+    const OmniboxWebUIPopupChangeObserver* observer) const {
+  return observers_.HasObserver(observer);
+}
+
 void RealboxHandler::SetPage(
     mojo::PendingRemote<omnibox::mojom::Page> pending_page) {
   page_.Bind(std::move(pending_page));
@@ -890,6 +904,13 @@ void RealboxHandler::OnNavigationLikely(
   }
 }
 
+void RealboxHandler::PopupElementSizeChanged(const gfx::Size& size) {
+  webui_size_ = size;
+  for (OmniboxWebUIPopupChangeObserver& observer : observers_) {
+    observer.OnPopupElementSizeChanged(size);
+  }
+}
+
 void RealboxHandler::DeleteAutocompleteMatch(uint8_t line, const GURL& url) {
   const AutocompleteMatch* match = GetMatchWithUrl(line, url);
   if (!match || !match->SupportsDeletion()) {

chrome/browser/ui/webui/realbox/realbox_handler.h

@@ -10,6 +10,8 @@
 
 #include "base/memory/raw_ptr.h"
 #include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
+#include "base/observer_list_types.h"
 #include "base/scoped_observation.h"
 #include "components/omnibox/browser/autocomplete_controller.h"
 #include "components/omnibox/browser/location_bar_model.h"
@@ -19,6 +21,7 @@
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/receiver.h"
 #include "mojo/public/cpp/bindings/remote.h"
+#include "ui/gfx/geometry/size.h"
 #include "ui/gfx/vector_icon_types.h"
 
 class GURL;
@@ -32,6 +35,13 @@ class WebContents;
 class WebUIDataSource;
 }  // namespace content
 
+// An observer interface for changes to the WebUI Omnibox popup.
+class OmniboxWebUIPopupChangeObserver : public base::CheckedObserver {
+ public:
+  // Called when a ResizeObserver detects the popup element changed size.
+  virtual void OnPopupElementSizeChanged(gfx::Size size) = 0;
+};
+
 // Handles bidirectional communication between NTP realbox JS and the browser.
 class RealboxHandler : public omnibox::mojom::PageHandler,
                        public AutocompleteController::Observer,
@@ -71,6 +81,11 @@ class RealboxHandler : public omnibox::mojom::PageHandler,
   // Returns true if the page remote is bound and ready to receive calls.
   bool IsRemoteBound() const;
 
+  // Handle observers to be notified of WebUI changes.
+  void AddObserver(OmniboxWebUIPopupChangeObserver* observer);
+  void RemoveObserver(OmniboxWebUIPopupChangeObserver* observer);
+  bool HasObserver(const OmniboxWebUIPopupChangeObserver* observer) const;
+
   // omnibox::mojom::PageHandler:
   void SetPage(mojo::PendingRemote<omnibox::mojom::Page> pending_page) override;
   void OnFocusChanged(bool focused) override;
@@ -100,6 +115,7 @@ class RealboxHandler : public omnibox::mojom::PageHandler,
       uint8_t line,
       const GURL& url,
       omnibox::mojom::NavigationPredictor navigation_predictor) override;
+  void PopupElementSizeChanged(const gfx::Size& size) override;
 
   // AutocompleteController::Observer:
   void OnResultChanged(AutocompleteController* controller,
@@ -142,6 +158,10 @@ class RealboxHandler : public omnibox::mojom::PageHandler,
   std::atomic<bool> page_set_;
   mojo::Remote<omnibox::mojom::Page> page_;
   mojo::Receiver<omnibox::mojom::PageHandler> page_handler_;
+  base::ObserverList<OmniboxWebUIPopupChangeObserver> observers_;
+
+  // Size of the WebUI popup element, as reported by ResizeObserver.
+  gfx::Size webui_size_;
 
   // This is unused, it's just needed for LocationBarModel implementation.
   gfx::VectorIcon vector_icon_{nullptr, 0u, ""};

chrome/browser/ui/webui/realbox/realbox_handler_unittest.cc

@@ -15,6 +15,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
+
 class MockPage : public omnibox::mojom::Page {
  public:
   MockPage() = default;
@@ -35,6 +36,16 @@ class MockPage : public omnibox::mojom::Page {
               UpdateSelection,
               (omnibox::mojom::OmniboxPopupSelectionPtr));
 };
+
+class TestObserver : public OmniboxWebUIPopupChangeObserver {
+ public:
+  void OnPopupElementSizeChanged(gfx::Size size) override { called_ = true; }
+  bool called() const { return called_; }
+
+ private:
+  bool called_ = false;
+};
+
 }  // namespace
 
 class RealboxHandlerTest : public ::testing::Test {
@@ -150,3 +161,13 @@ TEST_F(RealboxHandlerTest, RealboxUpdatesSelection) {
   EXPECT_EQ(omnibox::mojom::SelectionLineState::kFocusedButtonRemoveSuggestion,
             selection->state);
 }
+
+TEST_F(RealboxHandlerTest, RealboxObservationWorks) {
+  TestObserver observer;
+  EXPECT_FALSE(observer.called());
+  handler_->AddObserver(&observer);
+  EXPECT_TRUE(handler_->HasObserver(&observer));
+  handler_->RemoveObserver(&observer);
+  EXPECT_FALSE(handler_->HasObserver(&observer));
+  EXPECT_TRUE(observer.called());
+}

chrome/test/data/webui/new_tab_page/realbox/test_realbox_browser_proxy.ts

@@ -5,6 +5,7 @@
 import {NavigationPredictor, PageCallbackRouter, PageHandlerInterface, PageRemote} from 'chrome://resources/cr_components/omnibox/omnibox.mojom-webui.js';
 import {String16} from 'chrome://resources/mojo/mojo/public/mojom/base/string16.mojom-webui.js';
 import {TimeTicks} from 'chrome://resources/mojo/mojo/public/mojom/base/time.mojom-webui.js';
+import {Size} from 'chrome://resources/mojo/ui/gfx/geometry/mojom/geometry.mojom-webui.js';
 import {Url} from 'chrome://resources/mojo/url/mojom/url.mojom-webui.js';
 import {TestBrowserProxy} from 'chrome://webui-test/test_browser_proxy.js';
 
@@ -26,6 +27,7 @@ class FakePageHandler extends TestBrowserProxy implements PageHandlerInterface {
       'stopAutocomplete',
       'toggleSuggestionGroupIdVisibility',
       'onFocusChanged',
+      'popupElementSizeChanged',
     ]);
   }
 
@@ -37,6 +39,10 @@ class FakePageHandler extends TestBrowserProxy implements PageHandlerInterface {
     this.methodCalled('onFocusChanged', {focused});
   }
 
+  popupElementSizeChanged(size: Size) {
+    this.methodCalled('popupElementSizeChanged', {size});
+  }
+
   deleteAutocompleteMatch(line: number, url: Url) {
     this.methodCalled('deleteAutocompleteMatch', {line, url});
   }

components/omnibox/browser/BUILD.gn

@@ -929,6 +929,7 @@ mojom("mojo_bindings") {
 
   public_deps = [
     "//mojo/public/mojom/base",
+    "//ui/gfx/mojom",
     "//url/mojom:url_mojom_gurl",
   ]
 }

components/omnibox/browser/omnibox.mojom

@@ -6,6 +6,7 @@ module omnibox.mojom;
 
 import "mojo/public/mojom/base/string16.mojom";
 import "mojo/public/mojom/base/time.mojom";
+import "ui/gfx/geometry/mojom/geometry.mojom";
 import "url/mojom/url.mojom";
 
 // Events that indicate that the user is about to navigate to a suggestion.
@@ -164,6 +165,8 @@ interface PageHandler {
                 bool ctrl_key,
                 bool meta_key,
                 bool shift_key);
+  // Called when the popup results view element size is changed.
+  PopupElementSizeChanged(gfx.mojom.Size size);
 };
 
 // WebUI-side handler for requests from the browser.

ui/webui/resources/cr_components/omnibox/realbox_dropdown.ts

@@ -24,6 +24,12 @@ const remainder = (lhs: number, rhs: number) => ((lhs % rhs) + rhs) % rhs;
 const CHAR_TYPED_TO_PAINT = 'Realbox.CharTypedToRepaintLatency.ToPaint';
 const RESULT_CHANGED_TO_PAINT = 'Realbox.ResultChangedToRepaintLatency.ToPaint';
 
+export interface RealboxDropdownElement {
+  $: {
+    content: HTMLElement,
+  };
+}
+
 // A dropdown element that contains autocomplete matches. Provides an API for
 // the embedder (i.e., <ntp-realbox>) to change the selection.
 export class RealboxDropdownElement extends PolymerElement {
@@ -118,14 +124,32 @@ export class RealboxDropdownElement extends PolymerElement {
   private hiddenGroupIds_: number[];
   private selectableMatchElements_: RealboxMatchElement[];
   private showSecondarySide_: boolean;
-
+  private resizeObserver_: ResizeObserver|null = null;
   private pageHandler_: PageHandlerInterface;
 
   constructor() {
     super();
     this.pageHandler_ = RealboxBrowserProxy.getInstance().handler;
   }
 
+  override connectedCallback() {
+    super.connectedCallback();
+    this.resizeObserver_ = new ResizeObserver(
+        (entries: ResizeObserverEntry[]) =>
+            this.pageHandler_.popupElementSizeChanged({
+              width: entries[0].contentRect.width,
+              height: entries[0].contentRect.height,
+            }));
+    this.resizeObserver_.observe(this.$.content);
+  }
+
+  override disconnectedCallback() {
+    if (this.resizeObserver_) {
+      this.resizeObserver_.disconnect();
+    }
+    super.disconnectedCallback();
+  }
+
   //============================================================================
   // Public methods
   //============================================================================