Tab Search: Remove page handler when Tab Search is hidden

This patch introduces a EmbedderHidden() method that enables bubble
WebUIController subclasses to take appropriate action when their
embedder is no longer visible.

TabSearchUI overrides this to destroy the |page_handler_| when its
embedding bubble is no longer visible to prevent background updates.

Cleaned up use of member variables in WebUIBubbleManagerBase and
WebUIBubbleManager<T>.

Bug: 1099917, 1166967
Change-Id: I374dd74b5a1c3d37a53e7213e64c096adef35bda
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2626114Reviewed-by: Tommy Li <tommycli@chromium.org>
Reviewed-by: dpapad <dpapad@chromium.org>
Reviewed-by: Yuheng Huang <yuhengh@chromium.org>
Commit-Queue: Thomas Lukaszewicz <tluk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844343}

chrome/browser/ui/views/bubble/webui_bubble_manager.cc

@@ -16,17 +16,8 @@ constexpr base::TimeDelta kWebViewRetentionTime =
 
 }  // namespace
 
-WebUIBubbleManagerBase::WebUIBubbleManagerBase(
-    int task_manager_string_id,
-    views::View* anchor_view,
-    content::BrowserContext* browser_context,
-    const GURL& webui_url,
-    bool enable_extension_apis)
-    : task_manager_string_id_(task_manager_string_id),
-      anchor_view_(anchor_view),
-      browser_context_(browser_context),
-      webui_url_(webui_url),
-      enable_extension_apis_(enable_extension_apis),
+WebUIBubbleManagerBase::WebUIBubbleManagerBase(views::View* anchor_view)
+    : anchor_view_(anchor_view),
       cache_timer_(std::make_unique<base::RetainingOneShotTimer>(
           FROM_HERE,
           kWebViewRetentionTime,
@@ -82,6 +73,7 @@ void WebUIBubbleManagerBase::OnWidgetDestroying(views::Widget* widget) {
   bubble_widget_observation_.Reset();
   DCHECK(close_bubble_helper_);
   close_bubble_helper_.reset();
+  WebViewHidden();
   cache_timer_->Reset();
   bubble_using_cached_webview_ = false;
 }

chrome/browser/ui/views/bubble/webui_bubble_manager.h

@@ -28,11 +28,7 @@ class WebUIBubbleDialogView;
 // and caching of the WebView.
 class WebUIBubbleManagerBase : public views::WidgetObserver {
  public:
-  WebUIBubbleManagerBase(int task_manager_string_id,
-                         views::View* anchor_view,
-                         content::BrowserContext* browser_context,
-                         const GURL& webui_url,
-                         bool enable_extension_apis = false);
+  explicit WebUIBubbleManagerBase(views::View* anchor_view);
   WebUIBubbleManagerBase(const WebUIBubbleManagerBase&) = delete;
   const WebUIBubbleManagerBase& operator=(const WebUIBubbleManagerBase&) =
       delete;
@@ -45,10 +41,6 @@ class WebUIBubbleManagerBase : public views::WidgetObserver {
   // views::WidgetObserver:
   void OnWidgetDestroying(views::Widget* widget) override;
 
-  int task_manager_string_id() const { return task_manager_string_id_; }
-  content::BrowserContext* browser_context() { return browser_context_; }
-  const GURL& webui_url() const { return webui_url_; }
-  bool enable_extension_apis() const { return enable_extension_apis_; }
   bool bubble_using_cached_webview() const {
     return bubble_using_cached_webview_;
   }
@@ -58,19 +50,16 @@ class WebUIBubbleManagerBase : public views::WidgetObserver {
     return bubble_view_;
   }
 
+ protected:
+  WebUIBubbleView* cached_web_view() { return cached_web_view_.get(); }
+
  private:
   virtual std::unique_ptr<WebUIBubbleView> CreateWebView() = 0;
+  virtual void WebViewHidden() = 0;
   void ResetWebView();
 
-  // Used for tagging the web contents so that a distinctive name shows up in
-  // the task manager.
-  const int task_manager_string_id_;
-
   views::View* anchor_view_;
-  content::BrowserContext* browser_context_;
-  GURL webui_url_;
   base::WeakPtr<WebUIBubbleDialogView> bubble_view_;
-  const bool enable_extension_apis_;
 
   // Tracks whether the current bubble was created by reusing
   // |cached_web_view_|.
@@ -95,13 +84,23 @@ class WebUIBubbleManagerBase : public views::WidgetObserver {
 template <typename T>
 class WebUIBubbleManager : public WebUIBubbleManagerBase {
  public:
-  using WebUIBubbleManagerBase::WebUIBubbleManagerBase;
+  WebUIBubbleManager(int task_manager_string_id,
+                     views::View* anchor_view,
+                     content::BrowserContext* browser_context,
+                     const GURL& webui_url,
+                     bool enable_extension_apis = false)
+      : WebUIBubbleManagerBase(anchor_view),
+        task_manager_string_id_(task_manager_string_id),
+        browser_context_(browser_context),
+        webui_url_(webui_url),
+        enable_extension_apis_(enable_extension_apis) {}
+  ~WebUIBubbleManager() override = default;
 
  private:
   std::unique_ptr<WebUIBubbleView> CreateWebView() override {
-    auto web_view = std::make_unique<WebUIBubbleView>(browser_context());
+    auto web_view = std::make_unique<WebUIBubbleView>(browser_context_);
     content::WebContents* web_contents = web_view->GetWebContents();
-    if (enable_extension_apis()) {
+    if (enable_extension_apis_) {
       // In order for the WebUI in the renderer to use extensions APIs we must
       // add a ChromeExtensionWebContentsObserver to the WebView's WebContents.
       extensions::ChromeExtensionWebContentsObserver::CreateForWebContents(
@@ -109,11 +108,24 @@ class WebUIBubbleManager : public WebUIBubbleManagerBase {
     }
 
     task_manager::WebContentsTags::CreateForToolContents(
-        web_contents, task_manager_string_id());
-
-    web_view->template LoadURL<T>(webui_url());
+        web_contents, task_manager_string_id_);
+    web_view->template LoadURL<T>(webui_url_);
     return web_view;
   }
+
+  void WebViewHidden() override {
+    DCHECK(cached_web_view());
+    return cached_web_view()
+        ->template GetWebUIController<T>()
+        ->EmbedderHidden();
+  }
+
+  // Used for tagging the web contents so that a distinctive name shows up in
+  // the task manager.
+  const int task_manager_string_id_;
+  content::BrowserContext* browser_context_;
+  const GURL webui_url_;
+  const bool enable_extension_apis_;
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_BUBBLE_WEBUI_BUBBLE_MANAGER_H_

chrome/browser/ui/views/bubble/webui_bubble_manager_browsertest.cc

@@ -16,19 +16,22 @@ namespace {
 class TestWebUIBubbleManager : public WebUIBubbleManagerBase {
  public:
   explicit TestWebUIBubbleManager(Browser* browser)
-      : WebUIBubbleManagerBase(IDS_ACCNAME_TAB_SEARCH,
-                               BrowserView::GetBrowserViewForBrowser(browser),
-                               browser->profile(),
-                               GURL("chrome://about")) {}
+      : WebUIBubbleManagerBase(BrowserView::GetBrowserViewForBrowser(browser)),
+        browser_context_(browser->profile()) {}
   TestWebUIBubbleManager(const TestWebUIBubbleManager&) = delete;
   const TestWebUIBubbleManager& operator=(const TestWebUIBubbleManager&) =
       delete;
   ~TestWebUIBubbleManager() override = default;
 
+  // WebUIBubbleManagerBase:
+  void WebViewHidden() override {}
+
  private:
   std::unique_ptr<WebUIBubbleView> CreateWebView() override {
-    return std::make_unique<WebUIBubbleView>(browser_context());
+    return std::make_unique<WebUIBubbleView>(browser_context_);
   }
+
+  content::BrowserContext* browser_context_;
 };
 
 }  // namespace

chrome/browser/ui/views/bubble/webui_bubble_view.h

@@ -42,11 +42,14 @@ class WebUIBubbleView : public views::WebView,
     GetWebContents()->WasShown();
     SetVisible(true);
     LoadInitialURL(url);
-    T* async_webui_controller =
-        GetWebContents()->GetWebUI()->GetController()->template GetAs<T>();
     // Depends on the WebUIController object being constructed synchronously
     // when the navigation is started in LoadInitialURL().
-    async_webui_controller->set_embedder(weak_ptr_factory_.GetWeakPtr());
+    GetWebUIController<T>()->set_embedder(weak_ptr_factory_.GetWeakPtr());
+  }
+
+  template <typename T>
+  T* GetWebUIController() {
+    return GetWebContents()->GetWebUI()->GetController()->template GetAs<T>();
   }
 
   void set_host(Host* host) { host_ = host; }

chrome/browser/ui/webui/tab_search/tab_search_ui.cc

@@ -90,6 +90,10 @@ TabSearchUI::TabSearchUI(content::WebUI* web_ui)
 
 TabSearchUI::~TabSearchUI() = default;
 
+void TabSearchUI::EmbedderHidden() {
+  page_handler_.reset();
+}
+
 WEB_UI_CONTROLLER_TYPE_IMPL(TabSearchUI)
 
 void TabSearchUI::BindInterface(

chrome/browser/ui/webui/tab_search/tab_search_ui.h

@@ -25,11 +25,18 @@ class TabSearchUI : public ui::MojoBubbleWebUIController,
   TabSearchUI& operator=(const TabSearchUI&) = delete;
   ~TabSearchUI() override;
 
+  // ui::MojoBubbleWebUIController:
+  void EmbedderHidden() override;
+
   // Instantiates the implementor of the mojom::PageHandlerFactory mojo
   // interface passing the pending receiver that will be internally bound.
   void BindInterface(
       mojo::PendingReceiver<tab_search::mojom::PageHandlerFactory> receiver);
 
+  TabSearchPageHandler* page_handler_for_testing() {
+    return page_handler_.get();
+  }
+
  private:
   // tab_search::mojom::PageHandlerFactory
   void CreatePageHandler(

chrome/browser/ui/webui/tab_search/tab_search_ui_browsertest.cc

@@ -10,6 +10,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
 #include "chrome/browser/ui/ui_features.h"
+#include "chrome/browser/ui/webui/tab_search/tab_search_ui.h"
 #include "chrome/common/chrome_isolated_world_ids.h"
 #include "chrome/common/webui_url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
@@ -48,6 +49,12 @@ class TabSearchUIBrowserTest : public InProcessBrowserTest {
     return browser()->tab_strip_model()->GetActiveWebContents();
   }
 
+  TabSearchUI* GetWebUIController() {
+    return webui_contents_->GetWebUI()
+        ->GetController()
+        ->template GetAs<TabSearchUI>();
+  }
+
  protected:
   std::unique_ptr<content::WebContents> webui_contents_;
 
@@ -55,6 +62,14 @@ class TabSearchUIBrowserTest : public InProcessBrowserTest {
   base::test::ScopedFeatureList feature_list_;
 };
 
+IN_PROC_BROWSER_TEST_F(TabSearchUIBrowserTest,
+                       EmbedderHiddenDestroysPageHandler) {
+  EXPECT_NE(nullptr, GetWebUIController());
+  EXPECT_NE(nullptr, GetWebUIController()->page_handler_for_testing());
+  GetWebUIController()->EmbedderHidden();
+  EXPECT_EQ(nullptr, GetWebUIController()->page_handler_for_testing());
+}
+
 // TODO(romanarora): Investigate a way to call WebUI custom methods and refactor
 // JS code below.
 

ui/webui/mojo_bubble_web_ui_controller.h

@@ -35,6 +35,9 @@ class MojoBubbleWebUIController : public MojoWebUIController {
   void set_embedder(base::WeakPtr<Embedder> embedder) { embedder_ = embedder; }
   base::WeakPtr<Embedder> embedder() { return embedder_; }
 
+  // Called when the embedder is hidden and is no longer visible on screen.
+  virtual void EmbedderHidden() {}
+
  private:
   base::WeakPtr<Embedder> embedder_;
 };