FIXME: LifecycleUnits are now used for tab discarding

FIXME: bring "temporary workaround for pinned and unloaded tabs" https://crrev.com/a271a5c03ac2
brave · Apr 24, 2018 · 4db5e92 · 4db5e92
1 parent 52b697e
commit 4db5e92
Show file tree

Hide file tree

Showing 6 changed files with 109 additions and 106 deletions.
diff --git a/atom/browser/extensions/tab_helper.cc b/atom/browser/extensions/tab_helper.cc
@@ -184,26 +184,26 @@ bool TabHelper::AttachGuest(int window_id, int index) {
 content::WebContents* TabHelper::DetachGuest() {
   if (guest()->attached()) {
     // create temporary null placeholder
-    auto null_contents = GetTabManager()->CreateNullContents(
-        browser_->tab_strip_model(), web_contents());
+    // auto null_contents = GetTabManager()->CreateNullContents(
+    //     browser_->tab_strip_model(), web_contents());
 
-    null_contents->GetController().CopyStateFrom(
-        web_contents()->GetController(), false);
+    // null_contents->GetController().CopyStateFrom(
+    //     web_contents()->GetController(), false);
 
-    auto null_helper = FromWebContents(null_contents);
-    null_helper->index_ = get_index();
-    null_helper->pinned_ = pinned_;
-    // transfer window closing state
-    null_helper->window_closing_ = window_closing_;
-    window_closing_ = false;
+    // auto null_helper = FromWebContents(null_contents);
+    // null_helper->index_ = get_index();
+    // null_helper->pinned_ = pinned_;
+    //// transfer window closing state
+    // null_helper->window_closing_ = window_closing_;
+    // window_closing_ = false;
 
-    null_helper->SetPlaceholder(true);
+    // null_helper->SetPlaceholder(true);
 
-    // Replace the detached tab with the null placeholder
-    browser_->tab_strip_model()->ReplaceWebContentsAt(
-        get_index(), null_contents);
+    //// Replace the detached tab with the null placeholder
+    // browser_->tab_strip_model()->ReplaceWebContentsAt(
+    //     get_index(), null_contents);
 
-    return null_contents;
+    // return null_contents;
   }
   return nullptr;
 }
@@ -480,9 +480,10 @@ bool TabHelper::Discard() {
     return true;
   } else {
     if (guest()->attached()) {
-      int64_t web_contents_id = TabManager::IdFromWebContents(web_contents());
-      return !!GetTabManager()->DiscardTabById(
-          web_contents_id, resource_coordinator::DiscardReason::kProactive);
+      // int64_t web_contents_id =
+      // TabManager::IdFromWebContents(web_contents());
+      // return !!GetTabManager()->DiscardTabById(
+      //     web_contents_id, resource_coordinator::DiscardReason::kProactive);
     }
   }
 }
@@ -491,7 +492,7 @@ bool TabHelper::IsDiscarded() {
   if (discarded_) {
     return true;
   }
-  return GetTabManager()->IsTabDiscarded(web_contents());
+  // return GetTabManager()->IsTabDiscarded(web_contents());
 }
 
 void TabHelper::SetPinned(bool pinned) {

diff --git a/brave/browser/resource_coordinator/guest_tab_manager.cc b/brave/browser/resource_coordinator/guest_tab_manager.cc
@@ -37,37 +37,40 @@ namespace resource_coordinator {
 
 GuestTabManager::GuestTabManager() : TabManager() {}
 
-WebContents* GuestTabManager::CreateNullContents(
-    TabStripModel* model, WebContents* old_contents) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-
-  auto tab_helper = extensions::TabHelper::FromWebContents(old_contents);
-  DCHECK(tab_helper && tab_helper->guest());
-
-  auto embedder = tab_helper->guest()->embedder_web_contents();
-
-  WebContents::CreateParams params(old_contents->GetBrowserContext());
-  params.initially_hidden = true;
-  auto contents = extensions::TabHelper::CreateTab(embedder, params);
-  content::RestoreHelper::CreateForWebContents(contents);
-  return contents;
-}
-
-void GuestTabManager::DestroyOldContents(WebContents* old_contents) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-
-  auto tab_helper = extensions::TabHelper::FromWebContents(old_contents);
-  DCHECK(tab_helper && tab_helper->guest());
-  // Let the guest destroy itself after the detach message has been received
-  tab_helper->guest()->SetCanRunInDetachedState(false);
-}
+// WebContents* GuestTabManager::CreateNullContents(
+//     TabStripModel* model, WebContents* old_contents) {
+//   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+// 
+//   auto tab_helper = extensions::TabHelper::FromWebContents(old_contents);
+//   DCHECK(tab_helper && tab_helper->guest());
+// 
+//   auto embedder = tab_helper->guest()->embedder_web_contents();
+// 
+//   WebContents::CreateParams params(old_contents->GetBrowserContext());
+//   params.initially_hidden = true;
+//   auto contents = extensions::TabHelper::CreateTab(embedder, params);
+//   content::RestoreHelper::CreateForWebContents(contents);
+//   return contents;
+// }
+// 
+// void GuestTabManager::DestroyOldContents(WebContents* old_contents) {
+//   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+// 
+//   auto tab_helper = extensions::TabHelper::FromWebContents(old_contents);
+//   DCHECK(tab_helper && tab_helper->guest());
+//   // Let the guest destroy itself after the detach message has been
+//   received
+//   tab_helper->guest()->SetCanRunInDetachedState(false);
+// }
 
 void GuestTabManager::TabReplacedAt(TabStripModel* tab_strip_model,
                                content::WebContents* old_contents,
                                content::WebContents* new_contents,
                                int index) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
+  TabManager::TabReplacedAt(tab_strip_model, old_contents, new_contents, index);
+
   auto helper = content::RestoreHelper::FromWebContents(new_contents);
   // prevent the navigation controller from trying to autoload on
   // controller->SetActive(true)

diff --git a/brave/browser/resource_coordinator/guest_tab_manager.h b/brave/browser/resource_coordinator/guest_tab_manager.h
@@ -42,9 +42,9 @@ class GuestTabManager : public TabManager {
                        content::WebContents* new_contents,
                        int index) override;
 
-  content::WebContents* CreateNullContents(
-      TabStripModel* model, content::WebContents* old_contents) override;
-  void DestroyOldContents(content::WebContents* old_contents) override;
+  // content::WebContents* CreateNullContents(
+  //     TabStripModel* model, content::WebContents* old_contents) override;
+  // void DestroyOldContents(content::WebContents* old_contents) override;
 
   DISALLOW_COPY_AND_ASSIGN(GuestTabManager);
 };

diff --git a/chromium_src/BUILD.gn b/chromium_src/BUILD.gn
@@ -1080,8 +1080,6 @@ source_set("tab_manager") {
     "chrome/browser/engagement/site_engagement_service.cc",
     "//chrome/browser/memory/oom_memory_details.cc",
     "//chrome/browser/memory/oom_memory_details.h",
-    "//chrome/browser/resource_coordinator/discard_metrics_util.cc",
-    "//chrome/browser/resource_coordinator/discard_metrics_util.h",
     "//chrome/browser/resource_coordinator/page_signal_receiver.cc",
     "//chrome/browser/resource_coordinator/page_signal_receiver.h",
     "//chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc",

diff --git a/chromium_src/chrome/browser/browser_process_impl.cc b/chromium_src/chrome/browser/browser_process_impl.cc
@@ -23,6 +23,7 @@
 #include "chrome/browser/printing/background_printing_manager.h"
 #include "chrome/browser/printing/print_job_manager.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/resource_coordinator/tab_lifecycle_unit_source.h"
 #include "chrome/browser/shell_integration.h"
 #include "chrome/browser/status_icons/status_tray.h"
 #include "chrome/browser/ui/user_manager.h"
@@ -357,8 +358,12 @@ void BrowserProcessImpl::PreMainMessageLoopRun() {
 resource_coordinator::TabManager* BrowserProcessImpl::GetTabManager() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX)
-  if (!tab_manager_.get())
-    tab_manager_.reset(new resource_coordinator::GuestTabManager());
+  if (!tab_manager_) {
+    tab_manager_ = std::make_unique<resource_coordinator::GuestTabManager>();
+    tab_lifecycle_unit_source_ =
+        std::make_unique<resource_coordinator::TabLifecycleUnitSource>();
+    tab_lifecycle_unit_source_->AddObserver(tab_manager_.get());
+  }
   return tab_manager_.get();
 #else
   return nullptr;

diff --git a/patches/master_patch.patch b/patches/master_patch.patch
@@ -414,83 +414,79 @@ index a5ea92f2878459b49f88d4895c7d18f2e157dd6c..9cffca0352713bce12b4035c02612cd7
      }
    }
 
-diff --git a/chrome/browser/resource_coordinator/tab_manager.cc b/chrome/browser/resource_coordinator/tab_manager.cc
-index 7bcce4951f872e0c68f39abbebab736ccc54509f..0b55725bef29558e8a9f98069f99983a091f2daa 100644
---- a/chrome/browser/resource_coordinator/tab_manager.cc
-+++ b/chrome/browser/resource_coordinator/tab_manager.cc
-@@ -660,7 +660,8 @@ void TabManager::AddTabStats(const BrowserInfo& browser_info,
-   TabStripModel* tab_strip_model = browser_info.tab_strip_model;
-   for (int i = 0; i < tab_strip_model->count(); i++) {
-     WebContents* contents = tab_strip_model->GetWebContentsAt(i);
--    if (!contents->IsCrashed()) {
-+    // TODO(bridiver) - temporary workaround for pinned and unloaded tabs
-+    if (contents && !contents->IsCrashed()) {
-       TabStats stats;
-       stats.is_app = browser_info.browser_is_app;
-       stats.is_internal_page = IsInternalPage(contents->GetLastCommittedURL());
-@@ -765,6 +766,17 @@ void TabManager::PurgeBackgroundedTabsIfNeeded() {
-   }
+diff --git a/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc b/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc
+index ae5aaf93fc0e07fd6896cd33f702afd751b0041b..1b855ac47cb3fc796deb1bd69e888a0958d712a2 100644
+--- a/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc
++++ b/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc
+@@ -202,6 +202,20 @@ bool TabLifecycleUnitSource::TabLifecycleUnit::CanDiscard(
+   return true;
  }
 
-+// MUON(bridiver): see tab_manager.h
-+WebContents* TabManager::CreateNullContents(
-+    TabStripModel* model, WebContents* old_contents) {
-+  return WebContents::Create(WebContents::CreateParams(model->profile()));
++// MUON(bridiver): see tab_lifecycle_unit.h
++WebContents* TabLifecycleUnitSource::TabLifecycleUnit::CreateNullContents(
++    content::WebContents::CreateParams create_params,
++    WebContents* old_contents) {
++  return WebContents::Create(WebContents::CreateParams(create_params));
 +}
 +
-+// MUON(bridiver): see tab_manager.h
-+void TabManager::DestroyOldContents(WebContents* old_contents) {
++// MUON(bridiver): see tab_lifecycle_unit.h
++void TabLifecycleUnitSource::TabLifecycleUnit::DestroyOldContents(
++    WebContents* old_contents) {
 +  delete old_contents;
 +}
 +
- WebContents* TabManager::DiscardWebContentsAt(int index,
-                                               TabStripModel* model,
-                                               DiscardReason reason) {
-@@ -780,8 +792,7 @@ WebContents* TabManager::DiscardWebContentsAt(int index,
-       "TabManager.Discarding.DiscardedTabHasBeforeUnloadHandler",
-       old_contents->NeedToFireBeforeUnload());
-
--  WebContents* null_contents =
--      WebContents::Create(WebContents::CreateParams(model->profile()));
-+  WebContents* null_contents = CreateNullContents(model, old_contents);
++
+ bool TabLifecycleUnitSource::TabLifecycleUnit::Discard(
+     DiscardReason discard_reason) {
+   if (!tab_strip_model_ || IsDiscarded())
+@@ -219,7 +233,7 @@ bool TabLifecycleUnitSource::TabLifecycleUnit::Discard(
+   create_params.initially_hidden =
+       old_contents->GetVisibility() == content::Visibility::HIDDEN;
+   content::WebContents* const null_contents =
+-      content::WebContents::Create(create_params);
++      CreateNullContents(create_params, old_contents);
    // Copy over the state from the navigation controller to preserve the
    // back/forward history and to continue to display the correct title/favicon.
    //
-@@ -841,7 +852,8 @@ WebContents* TabManager::DiscardWebContentsAt(int index,
-   // TODO(jamescook): This breaks script connections with other tabs.
-   // Find a different approach that doesn't do that, perhaps based on
+@@ -270,7 +284,8 @@ bool TabLifecycleUnitSource::TabLifecycleUnit::Discard(
+   // TODO(jamescook): This breaks script connections with other tabs. Find a
+   // different approach that doesn't do that, perhaps based on
    // RenderFrameProxyHosts.
 -  delete old_contents;
-+  // MUON(bridiver): see tab_manager.h
++  // MUON(bridiver): see tab_lifecycle_unit.h
 +  DestroyOldContents(old_contents);
 
-   return null_contents;
- }
-diff --git a/chrome/browser/resource_coordinator/tab_manager.h b/chrome/browser/resource_coordinator/tab_manager.h
-index 440fdf8df66efce1b68a0463189986a6a0b0353a..7cc8fb11ecb5e2caa67e4e92d6d4526edb2d2001 100644
---- a/chrome/browser/resource_coordinator/tab_manager.h
-+++ b/chrome/browser/resource_coordinator/tab_manager.h
-@@ -93,6 +93,11 @@ class TabManager : public TabStripModelObserver, public BrowserListObserver {
-   TabManager();
-   ~TabManager() override;
+   SetState(State::DISCARDED);
+   ++discard_count_;
+diff --git a/chrome/browser/resource_coordinator/tab_lifecycle_unit.h b/chrome/browser/resource_coordinator/tab_lifecycle_unit.h
+index 6fb7f50f4661c0d695920ee00cd1fbe330e0ddf7..8b452e7626986732caad283145b94ee9bdc3668e 100644
+--- a/chrome/browser/resource_coordinator/tab_lifecycle_unit.h
++++ b/chrome/browser/resource_coordinator/tab_lifecycle_unit.h
+@@ -93,6 +93,12 @@ class TabLifecycleUnitSource::TabLifecycleUnit
+   bool IsDiscarded() const override;
+   int GetDiscardCount() const override;
 
 +  // MUON(bridiver): override to create/destroy guests webcontents
 +  virtual content::WebContents* CreateNullContents(
-+      TabStripModel* model, content::WebContents* old_contents);
++      content::WebContents::CreateParams create_params,
++      content::WebContents* old_contents);
 +  virtual void DestroyOldContents(content::WebContents* old_contents);
 +
-   // Number of discard events since Chrome started.
-   int discard_count() const { return discard_count_; }
-
-@@ -300,6 +305,8 @@ class TabManager : public TabStripModelObserver, public BrowserListObserver {
-   // min time to purge times this value.
-   const int kDefaultMinMaxTimeToPurgeRatio = 4;
+  private:
+   // Invoked when the state goes from DISCARDED to non-DISCARDED and vice-versa.
+   void OnDiscardedStateChange();
+diff --git a/chrome/browser/resource_coordinator/tab_manager.h b/chrome/browser/resource_coordinator/tab_manager.h
+index 25dea18db1339ad6bf351870675bb1de15fcd60f..665366319856bc5ea4ca638e6d7ad78e1c5b924a 100644
+--- a/chrome/browser/resource_coordinator/tab_manager.h
++++ b/chrome/browser/resource_coordinator/tab_manager.h
+@@ -207,6 +207,7 @@ class TabManager : public LifecycleUnitObserver,
+       base::TimeDelta::FromMinutes(10);
 
+  private:
 +  friend class GuestTabManager;
-+
-   // Finds TabStripModel which has a WebContents whose id is the given
-   // |tab_id|, and returns the WebContents index and the TabStripModel.
-   int FindTabStripModelById(int32_t tab_id, TabStripModel** model) const;
+   friend class TabManagerStatsCollectorTest;
+
+   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, PurgeBackgroundRenderer);
 diff --git a/chrome/common/BUILD.gn b/chrome/common/BUILD.gn
 index 7ca361112ae6553b5d55c0118868b6592466e466..b3cbde0364a0de8e75fe06fdf86a72321078145a 100644
 --- a/chrome/common/BUILD.gn