Try saving pinned tabs on OnBrowserRemoved

Bug:71939

PinnedTabService is observing two events:
NOTIFICATION_CLOSE_ALL_BROWSERS_REQUEST and BrowserList::OnBrowserClosing.

The first is when users want to shut down entire app and
the latter is when users close a browser at once even if it has multiple tabs.

But the bug is edge case which doesn't belong to any case above for now.
When tab strip is empty, BrowserListObserver::OnBrowserClosing is not called even
this will end up with closing the browser.

So try saving pinned tabs again on BrowserListObserver::OnBrowserRemoved.

Change-Id: Id73891b9509918bca3b1dcaf0b34cc32a7a687a7
Test: PinnedTabServiceBrowserTest.*
Reviewed-on: https://chromium-review.googlesource.com/892539Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537442}

chrome/browser/ui/tabs/pinned_tab_service.cc

@@ -25,6 +25,16 @@ bool IsOnlyNormalBrowser(Browser* browser) {
   return true;
 }
 
+// Returns true if there's at lease one tabbed browser associated with
+// |profile|.
+bool BrowserListHasNormalBrowser(Profile* profile) {
+  for (auto* b : *BrowserList::GetInstance()) {
+    if (b->is_type_tabbed() && b->profile() == profile)
+      return true;
+  }
+  return false;
+}
+
 }  // namespace
 
 PinnedTabService::PinnedTabService(Profile* profile)
@@ -99,3 +109,19 @@ void PinnedTabService::OnBrowserClosing(Browser* browser) {
     PinnedTabCodec::WritePinnedTabs(profile_);
   }
 }
+
+void PinnedTabService::OnBrowserRemoved(Browser* browser) {
+  if (!browser->is_type_tabbed() || browser->profile() != profile_)
+    return;
+
+  if (save_pinned_tabs_ && has_normal_browser_ &&
+      !BrowserListHasNormalBrowser(browser->profile())) {
+    // This happens when user closes each tabs manually via the close button on
+    // them. In this case OnBrowserClosing() above is not called. This causes
+    // pinned tabs to repopen on the next startup. So we should call
+    // WritePinnedTab() to clear the data.
+    // http://crbug.com/71939
+    has_normal_browser_ = false;
+    PinnedTabCodec::WritePinnedTabs(profile_);
+  }
+}

chrome/browser/ui/tabs/pinned_tab_service.h

@@ -34,6 +34,7 @@ class PinnedTabService : public content::NotificationObserver,
 
   // BrowserListObserver:
   void OnBrowserClosing(Browser* browser) override;
+  void OnBrowserRemoved(Browser* browser) override;
 
   Profile* profile_;
 

chrome/browser/ui/tabs/pinned_tab_service_browsertest.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/tabs/pinned_tab_service.h"
+
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_navigator_params.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/tabs/pinned_tab_codec.h"
+#include "chrome/browser/ui/tabs/pinned_tab_service_factory.h"
+#include "chrome/browser/ui/tabs/pinned_tab_test_utils.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/test/test_utils.h"
+
+namespace {
+
+// Wait until a browser is removed from BrowserList.
+class BrowserRemovalWaiter : public BrowserListObserver {
+ public:
+  explicit BrowserRemovalWaiter(const Browser* browser) : browser_(browser) {
+    BrowserList::AddObserver(this);
+  }
+  ~BrowserRemovalWaiter() override = default;
+
+  void WaitForRemoval() {
+    message_loop_runner_ = new content::MessageLoopRunner;
+    message_loop_runner_->Run();
+  }
+
+ private:
+  // BrowserListObserver override:
+  void OnBrowserRemoved(Browser* browser) override {
+    if (browser != browser_)
+      return;
+
+    BrowserList::RemoveObserver(this);
+    if (message_loop_runner_.get() && message_loop_runner_->loop_running())
+      message_loop_runner_->Quit();
+  }
+
+  const Browser* const browser_;
+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(BrowserRemovalWaiter);
+};
+
+using PinnedTabServiceBrowserTest = InProcessBrowserTest;
+
+}  // namespace
+
+// Makes sure pinned tabs are updated when tabstrip is empty.
+// http://crbug.com/71939
+IN_PROC_BROWSER_TEST_F(PinnedTabServiceBrowserTest, TabStripEmpty) {
+  Profile* profile = browser()->profile();
+  GURL url("http://www.google.com");
+  NavigateParams params(browser(), url, ui::PAGE_TRANSITION_TYPED);
+  ui_test_utils::NavigateToURL(&params);
+
+  TabStripModel* tab_strip_model = browser()->tab_strip_model();
+  tab_strip_model->SetTabPinned(0, true);
+
+  PinnedTabCodec::WritePinnedTabs(profile);
+  std::string result =
+      PinnedTabTestUtils::TabsToString(PinnedTabCodec::ReadPinnedTabs(profile));
+  EXPECT_EQ("http://www.google.com/:pinned", result);
+
+  // When tab strip is empty, browser window will be closed and PinnedTabService
+  // must update data on this event.
+  BrowserRemovalWaiter waiter(browser());
+  tab_strip_model->SetTabPinned(0, false);
+  EXPECT_TRUE(
+      tab_strip_model->CloseWebContentsAt(0, TabStripModel::CLOSE_NONE));
+  EXPECT_TRUE(tab_strip_model->empty());
+  waiter.WaitForRemoval();
+
+  // Let's see it's cleared out properly.
+  result =
+      PinnedTabTestUtils::TabsToString(PinnedTabCodec::ReadPinnedTabs(profile));
+  EXPECT_TRUE(result.empty());
+}
+
+IN_PROC_BROWSER_TEST_F(PinnedTabServiceBrowserTest, CloseWindow) {
+  Profile* profile = browser()->profile();
+  EXPECT_TRUE(PinnedTabServiceFactory::GetForProfile(profile));
+  EXPECT_TRUE(profile->GetPrefs());
+
+  GURL url("http://www.google.com");
+  NavigateParams params(browser(), url, ui::PAGE_TRANSITION_TYPED);
+  ui_test_utils::NavigateToURL(&params);
+
+  TabStripModel* tab_strip_model = browser()->tab_strip_model();
+  tab_strip_model->SetTabPinned(0, true);
+
+  BrowserRemovalWaiter waiter(browser());
+  browser()->window()->Close();
+  waiter.WaitForRemoval();
+
+  std::string result =
+      PinnedTabTestUtils::TabsToString(PinnedTabCodec::ReadPinnedTabs(profile));
+  EXPECT_EQ("http://www.google.com/:pinned", result);
+}

chrome/test/BUILD.gn

@@ -183,6 +183,10 @@ static_library("test_support") {
     ]
   } else {
     public_deps += [ "//components/zoom:test_support" ]
+    sources += [
+      "../browser/ui/tabs/pinned_tab_test_utils.cc",
+      "../browser/ui/tabs/pinned_tab_test_utils.h",
+    ]
   }
 
   if (is_linux) {
@@ -687,6 +691,7 @@ test("browser_tests") {
       "../browser/ui/blocked_content/popup_tracker_browsertest.cc",
       "../browser/ui/blocked_content/safe_browsing_triggered_popup_blocker_browsertest.cc",
       "../browser/ui/blocked_content/tab_under_blocker_browsertest.cc",
+      "../browser/ui/tabs/pinned_tab_service_browsertest.cc",
 
       # If this list is used on Android in the future, these browser/speech/*
       # files will probably not be applicable.
@@ -2924,8 +2929,6 @@ test("unit_tests") {
       "../browser/ui/tab_contents/tab_contents_iterator_unittest.cc",
       "../browser/ui/tabs/pinned_tab_codec_unittest.cc",
       "../browser/ui/tabs/pinned_tab_service_unittest.cc",
-      "../browser/ui/tabs/pinned_tab_test_utils.cc",
-      "../browser/ui/tabs/pinned_tab_test_utils.h",
       "../browser/ui/tabs/tab_menu_model_unittest.cc",
       "../browser/ui/tabs/tab_strip_model_stats_recorder_unittest.cc",
       "../browser/ui/tabs/tab_strip_model_unittest.cc",