Fixes couple of bugs triggered on chromeos when restoring a tab:

. Makes TabRestoreService go to loaded if asked to restore previous
  session and it already has the max number of tabs.
. Adds method to TabRestoreServiceObserver this is sent when loaded.
. Makes ChromeShellDelegate use a TabRestoreServiceObserver to wait
  for load to complete then restores.

BUG=239194
TEST=add some test coverage
R=jamescook@chromium.org, marja@chromium.org

Review URL: https://chromiumcodereview.appspot.com/14620014

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@199540 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 77 insertions, 2 deletions

diff --git a/chrome/browser/sessions/persistent_tab_restore_service.cc b/chrome/browser/sessions/persistent_tab_restore_service.cc
index d8df61b..8f561f6 100644
--- a/chrome/browser/sessions/persistent_tab_restore_service.cc
+++ b/chrome/browser/sessions/persistent_tab_restore_service.cc
@@ -318,10 +318,17 @@ void PersistentTabRestoreService::Delegate::OnAddEntry() {
 }
 
 void PersistentTabRestoreService::Delegate::LoadTabsFromLastSession() {
-  if (load_state_ != NOT_LOADED ||
-      tab_restore_service_helper_->entries().size() == kMaxEntries)
+  if (load_state_ != NOT_LOADED)
     return;
 
+  if (tab_restore_service_helper_->entries().size() == kMaxEntries) {
+    // We already have the max number of entries we can take. There is no point
+    // in attempting to load since we'll just drop the results. Skip to loaded.
+    load_state_ = (LOADING | LOADED_LAST_SESSION | LOADED_LAST_TABS);
+    LoadStateChanged();
+    return;
+  }
+
 #if !defined(ENABLE_SESSION_SERVICE)
   // If sessions are not stored in the SessionService, default to
   // |LOADED_LAST_SESSION| state.
@@ -818,6 +825,7 @@ void PersistentTabRestoreService::Delegate::LoadStateChanged() {
   const Entries& entries = tab_restore_service_helper_->entries();
   if (staging_entries_.empty() || entries.size() >= kMaxEntries) {
     staging_entries_.clear();
+    tab_restore_service_helper_->NotifyLoaded();
     return;
   }
 
@@ -850,6 +858,8 @@ void PersistentTabRestoreService::Delegate::LoadStateChanged() {
 
   tab_restore_service_helper_->PruneEntries();
   tab_restore_service_helper_->NotifyTabsChanged();
+
+  tab_restore_service_helper_->NotifyLoaded();
 }
 
 void PersistentTabRestoreService::Delegate::RemoveEntryByID(
diff --git a/chrome/browser/sessions/persistent_tab_restore_service_browsertest.cc b/chrome/browser/sessions/persistent_tab_restore_service_browsertest.cc
index b90a8dd..399b35a 100644
--- a/chrome/browser/sessions/persistent_tab_restore_service_browsertest.cc
+++ b/chrome/browser/sessions/persistent_tab_restore_service_browsertest.cc
@@ -12,6 +12,7 @@
 #include "chrome/browser/sessions/session_service_factory.h"
 #include "chrome/browser/sessions/session_types.h"
 #include "chrome/browser/sessions/tab_restore_service_factory.h"
+#include "chrome/browser/sessions/tab_restore_service_observer.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/url_constants.h"
@@ -169,6 +170,33 @@ class PersistentTabRestoreServiceTest : public ChromeRenderViewHostTestHarness {
   content::TestBrowserThread ui_thread_;
 };
 
+namespace {
+
+class TestTabRestoreServiceObserver : public TabRestoreServiceObserver {
+ public:
+  TestTabRestoreServiceObserver() : got_loaded_(false) {}
+
+  void clear_got_loaded() { got_loaded_ = false; }
+  bool got_loaded() const { return got_loaded_; }
+
+  // TabRestoreServiceObserver:
+  virtual void TabRestoreServiceChanged(TabRestoreService* service) OVERRIDE {
+  }
+  virtual void TabRestoreServiceDestroyed(TabRestoreService* service) OVERRIDE {
+  }
+  virtual void TabRestoreServiceLoaded(TabRestoreService* service) OVERRIDE {
+    got_loaded_ = true;
+  }
+
+ private:
+  // Was TabRestoreServiceLoaded() invoked?
+  bool got_loaded_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestTabRestoreServiceObserver);
+};
+
+}  // namespace
+
 TEST_F(PersistentTabRestoreServiceTest, Basic) {
   AddThreeNavigations();
 
@@ -372,7 +400,13 @@ TEST_F(PersistentTabRestoreServiceTest, LoadPreviousSession) {
   SessionServiceFactory::GetForProfile(profile())->
       MoveCurrentSessionToLastSession();
 
+  EXPECT_FALSE(service_->IsLoaded());
+
+  TestTabRestoreServiceObserver observer;
+  service_->AddObserver(&observer);
   service_->LoadTabsFromLastSession();
+  EXPECT_TRUE(observer.got_loaded());
+  service_->RemoveObserver(&observer);
 
   // Make sure we get back one entry with one tab whose url is url1.
   ASSERT_EQ(1U, service_->entries().size());
@@ -695,3 +729,23 @@ TEST_F(PersistentTabRestoreServiceTest, PruneIsCalled) {
   service_->LoadTabsFromLastSession();
   EXPECT_EQ(max_entries, service_->entries().size());
 }
+
+// Makes sure invoking LoadTabsFromLastSession() when the max number of entries
+// have been added results in IsLoaded() returning true and notifies observers.
+TEST_F(PersistentTabRestoreServiceTest, GoToLoadedWhenHaveMaxEntries) {
+  const size_t max_entries = kMaxEntries;
+  for (size_t i = 0; i < max_entries + 5; i++) {
+    NavigateAndCommit(
+        GURL(base::StringPrintf("http://%d", static_cast<int>(i))));
+    service_->CreateHistoricalTab(web_contents(), -1);
+  }
+
+  EXPECT_FALSE(service_->IsLoaded());
+  TestTabRestoreServiceObserver observer;
+  service_->AddObserver(&observer);
+  EXPECT_EQ(max_entries, service_->entries().size());
+  service_->LoadTabsFromLastSession();
+  EXPECT_TRUE(observer.got_loaded());
+  EXPECT_TRUE(service_->IsLoaded());
+  service_->RemoveObserver(&observer);
+}
diff --git a/chrome/browser/sessions/tab_restore_service_helper.cc b/chrome/browser/sessions/tab_restore_service_helper.cc
index 32e3406..0b0e0e5 100644
--- a/chrome/browser/sessions/tab_restore_service_helper.cc
+++ b/chrome/browser/sessions/tab_restore_service_helper.cc
@@ -305,6 +305,11 @@ void TabRestoreServiceHelper::NotifyTabsChanged() {
                     TabRestoreServiceChanged(tab_restore_service_));
 }
 
+void TabRestoreServiceHelper::NotifyLoaded() {
+  FOR_EACH_OBSERVER(TabRestoreServiceObserver, observer_list_,
+                    TabRestoreServiceLoaded(tab_restore_service_));
+}
+
 void TabRestoreServiceHelper::AddEntry(Entry* entry,
                                        bool notify,
                                        bool to_front) {
diff --git a/chrome/browser/sessions/tab_restore_service_helper.h b/chrome/browser/sessions/tab_restore_service_helper.h
index 4f8f1e0..f173648 100644
--- a/chrome/browser/sessions/tab_restore_service_helper.h
+++ b/chrome/browser/sessions/tab_restore_service_helper.h
@@ -90,6 +90,9 @@ class TabRestoreServiceHelper {
   // Notifies observers the tabs have changed.
   void NotifyTabsChanged();
 
+  // Notifies observers the service has loaded.
+  void NotifyLoaded();
+
   // Adds |entry| to the list of entries and takes ownership. If |prune| is true
   // |PruneAndNotify| is invoked. If |to_front| is true the entry is added to
   // the front, otherwise the back. Normal closes go to the front, but
diff --git a/chrome/browser/sessions/tab_restore_service_observer.h b/chrome/browser/sessions/tab_restore_service_observer.h
index e4d21184..328176a 100644
--- a/chrome/browser/sessions/tab_restore_service_observer.h
+++ b/chrome/browser/sessions/tab_restore_service_observer.h
@@ -18,6 +18,9 @@ class TabRestoreServiceObserver {
   // destructor is run.
   virtual void TabRestoreServiceDestroyed(TabRestoreService* service) = 0;
 
+  // Sent when TabRestoreService finishes loading.
+  virtual void TabRestoreServiceLoaded(TabRestoreService* service) {}
+
  protected:
   virtual ~TabRestoreServiceObserver() {}
 };