Merge pull request brave#23429 from brave/sko/spt-resurrect

Fix SharedPinnedTab functionality
jagadeshjai · May 5, 2024 · d40db35 · d40db35
2 parents 92a592b + 9b5d754
commit d40db35
Show file tree

Hide file tree

Showing 3 changed files with 27 additions and 14 deletions.
diff --git a/browser/ui/tabs/shared_pinned_tab_service.cc b/browser/ui/tabs/shared_pinned_tab_service.cc
@@ -14,6 +14,8 @@
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_tabrestore.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
+#include "chrome/browser/ui/tabs/tab_enums.h"
+#include "chrome/browser/ui/tabs/tab_model.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/web_contents_user_data.h"
 
@@ -134,7 +136,7 @@ SharedPinnedTabService::GetTabRendererDataForDummyContents(
 
 void SharedPinnedTabService::CacheWebContentsIfNeeded(
     Browser* browser,
-    const std::vector<std::unique_ptr<DetachedWebContents>>& web_contents) {
+    std::vector<std::unique_ptr<tabs::TabModel>> pinned_tabs) {
   DVLOG(2) << __FUNCTION__;
   DCHECK(!profile_will_be_destroyed_);
 
@@ -149,19 +151,18 @@ void SharedPinnedTabService::CacheWebContentsIfNeeded(
     return;
   }
 
-  for (auto& detached_web_contents : web_contents) {
-    if (!detached_web_contents->tab ||
-        !detached_web_contents->tab->contents()) {
+  for (auto& pinned_tab : pinned_tabs) {
+    if (!pinned_tab->contents()) {
       // Could be already cached by another component.
       continue;
     }
 
-    if (!SharedContentsData::FromWebContents(detached_web_contents->contents)) {
+    if (!SharedContentsData::FromWebContents(pinned_tab->contents())) {
       continue;
     }
 
     cached_shared_contentses_from_closing_browser_.insert(
-        detached_web_contents->tab->ReplaceContents(nullptr));
+        tabs::TabModel::DestroyAndTakeWebContents(std::move(pinned_tab)));
   }
 }
 
@@ -252,6 +253,14 @@ void SharedPinnedTabService::OnBrowserClosing(Browser* browser) {
       }
     }
   } else {
+    // Try caching shared contents from the closing browser.
+    auto* tab_strip_model = browser->tab_strip_model();
+    std::vector<std::unique_ptr<tabs::TabModel>> pinned_tabs;
+    for (int i = tab_strip_model->IndexOfFirstNonPinnedTab() - 1; i >= 0; --i) {
+      pinned_tabs.push_back(tab_strip_model->DetachTabAtForInsertion(i));
+    }
+    CacheWebContentsIfNeeded(browser, std::move(pinned_tabs));
+
     for (auto& pinned_tab_data : pinned_tab_data_) {
       if (pinned_tab_data.contents_owner_model == browser->tab_strip_model()) {
         pinned_tab_data.contents_owner_model = nullptr;
@@ -723,6 +732,11 @@ void SharedPinnedTabService::MoveSharedWebContentsToBrowser(
     DCHECK(dummy_contents_data);
     dummy_contents_data->stop_propagation();
 
+    const int add_type =
+        ADD_PINNED | (is_last_closing_browser ? ADD_ACTIVE : 0);
+    tab_strip_model->InsertWebContentsAt(
+        index, std::move(unique_shared_contents), add_type);
+
     // In order to prevent browser from being closed, we should close the dummy
     // contents after we restore the tab.
     tab_strip_model->CloseWebContentsAt(index + 1, /* close_type */ 0);

diff --git a/browser/ui/tabs/shared_pinned_tab_service.h b/browser/ui/tabs/shared_pinned_tab_service.h
@@ -12,6 +12,7 @@
 #include "base/scoped_observation.h"
 #include "chrome/browser/profiles/profile_observer.h"
 #include "chrome/browser/ui/browser_list_observer.h"
+#include "chrome/browser/ui/tabs/tab_model.h"
 #include "chrome/browser/ui/tabs/tab_renderer_data.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
@@ -55,10 +56,6 @@ class SharedPinnedTabService : public KeyedService,
       int index,
       content::WebContents* maybe_dummy_contents);
 
-  void CacheWebContentsIfNeeded(
-      Browser* browser,
-      const std::vector<std::unique_ptr<DetachedWebContents>>& web_contents);
-
   // KeyedService:
   void Shutdown() override;
 
@@ -84,6 +81,10 @@ class SharedPinnedTabService : public KeyedService,
   void OnProfileWillBeDestroyed(Profile* profile) override;
 
  private:
+  void CacheWebContentsIfNeeded(
+      Browser* browser,
+      std::vector<std::unique_ptr<tabs::TabModel>> pinned_tabs);
+
   void OnTabAdded(TabStripModel* tab_strip_model,
                   const TabStripModelChange::Insert* insert);
   void OnTabMoved(TabStripModel* tab_strip_model,

diff --git a/browser/ui/tabs/test/shared_pinned_tab_service_browsertest.cc b/browser/ui/tabs/test/shared_pinned_tab_service_browsertest.cc
@@ -131,8 +131,7 @@ IN_PROC_BROWSER_TEST_F(SharedPinnedTabServiceBrowserTest, PinAndUnpinTabs) {
       tab_strip_model_2->GetWebContentsAt(0)));
 }
 
-IN_PROC_BROWSER_TEST_F(SharedPinnedTabServiceBrowserTest,
-                       DISABLED_ActivatePinnedTab) {
+IN_PROC_BROWSER_TEST_F(SharedPinnedTabServiceBrowserTest, ActivatePinnedTab) {
   // Precondition
   auto* browser_1 = browser();
   auto* tab_strip_model_1 = browser_1->tab_strip_model();
@@ -200,8 +199,7 @@ IN_PROC_BROWSER_TEST_F(SharedPinnedTabServiceBrowserTest, NewBrowser) {
       tab_strip_model_2->GetWebContentsAt(0)));
 }
 
-IN_PROC_BROWSER_TEST_F(SharedPinnedTabServiceBrowserTest,
-                       DISABLED_BringAllTabs) {
+IN_PROC_BROWSER_TEST_F(SharedPinnedTabServiceBrowserTest, BringAllTabs) {
   // Given that there're multiple windows with shared pinned tabs
   auto* browser_1 = browser();
   auto* tab_strip_model_1 = browser_1->tab_strip_model();