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

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

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199540

[sky, 2013-05-09 00:16:43]

marja: sessions changes
james: chrome_shell_delegate

[James Cook, 2013-05-09 16:36:31]

https://codereview.chromium.org/14620014/diff/2001/chrome/browser/ui/ash/chro...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/14620014/diff/2001/chrome/browser/ui/ash/chro...
chrome/browser/ui/ash/chrome_shell_delegate.cc:210: Browser* browser =
chrome::FindBrowserWithWindow(ash::wm::GetActiveWindow());
I think the old code will create a new browser window if none are open (via
GetTargetBrowser).  I think we want to keep that behavior, since the user can
hit Ctrl-Shift-T with no windows open.

[sky, 2013-05-09 16:42:12]

https://codereview.chromium.org/14620014/diff/2001/chrome/browser/ui/ash/chro...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/14620014/diff/2001/chrome/browser/ui/ash/chro...
chrome/browser/ui/ash/chrome_shell_delegate.cc:210: Browser* browser =
chrome::FindBrowserWithWindow(ash::wm::GetActiveWindow());
On 2013/05/09 16:36:31, James Cook (Chromium) wrote:
> I think the old code will create a new browser window if none are open (via
> GetTargetBrowser).  I think we want to keep that behavior, since the user can
> hit Ctrl-Shift-T with no windows open.

This is done at line 65 now. Specifically:
service->RestoreMostRecentEntry(NULL, chrome::HOST_DESKTOP_TYPE_ASH);
does that.

GetTargetBrowser is hugely problematic (177542, which keeps getting pushed). The
old code could create a browser and never end up using it (it would not be shown
either).

[James Cook, 2013-05-09 16:48:30]

LGTM

https://codereview.chromium.org/14620014/diff/2001/chrome/browser/ui/ash/chro...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/14620014/diff/2001/chrome/browser/ui/ash/chro...
chrome/browser/ui/ash/chrome_shell_delegate.cc:210: Browser* browser =
chrome::FindBrowserWithWindow(ash::wm::GetActiveWindow());
On 2013/05/09 16:42:12, sky wrote:
> On 2013/05/09 16:36:31, James Cook (Chromium) wrote:
> > I think the old code will create a new browser window if none are open (via
> > GetTargetBrowser).  I think we want to keep that behavior, since the user
can
> > hit Ctrl-Shift-T with no windows open.
> 
> This is done at line 65 now. Specifically:
> service->RestoreMostRecentEntry(NULL, chrome::HOST_DESKTOP_TYPE_ASH);
> does that.
> 
> GetTargetBrowser is hugely problematic (177542, which keeps getting pushed).
The
> old code could create a browser and never end up using it (it would not be
shown
> either).

Ah, I didn't notice RestoreMostRecentEntry did that.  Damn, I thought I found a
bug in your code, which is so rare I was getting excited. :-)

[marja, 2013-05-10 08:18:36]

chrome/browser/sessions lgtm

[commit-bot: I haz the power, 2013-05-10 15:00:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/14620014/2001

[commit-bot: I haz the power, 2013-05-10 15:00:44]

Failed to apply patch for chrome/browser/ui/ash/chrome_shell_delegate.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/ash/chrome_shell_delegate.cc
  Hunk #3 FAILED at 202.
  1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/ui/ash/chrome_shell_delegate.cc.rej

Patch:       chrome/browser/ui/ash/chrome_shell_delegate.cc
Index: chrome/browser/ui/ash/chrome_shell_delegate.cc
diff --git a/chrome/browser/ui/ash/chrome_shell_delegate.cc
b/chrome/browser/ui/ash/chrome_shell_delegate.cc
index
56c448655cb0a831386b920aa20a56b564a2c121..a5d39c114d5f1122ed9793ce39ed22e8405f258f
100644
--- a/chrome/browser/ui/ash/chrome_shell_delegate.cc
+++ b/chrome/browser/ui/ash/chrome_shell_delegate.cc
@@ -23,6 +23,7 @@
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/sessions/tab_restore_service.h"
 #include "chrome/browser/sessions/tab_restore_service_factory.h"
+#include "chrome/browser/sessions/tab_restore_service_observer.h"
 #include "chrome/browser/ui/app_list/app_list_view_delegate.h"
 #include "chrome/browser/ui/ash/app_list/app_list_controller_ash.h"
 #include "chrome/browser/ui/ash/ash_keyboard_controller_proxy.h"
@@ -57,6 +58,57 @@
 // static
 ChromeShellDelegate* ChromeShellDelegate::instance_ = NULL;
 
+namespace {
+
+void RestoreTabUsingProfile(Profile* profile) {
+  TabRestoreService* service =
TabRestoreServiceFactory::GetForProfile(profile);
+  service->RestoreMostRecentEntry(NULL, chrome::HOST_DESKTOP_TYPE_ASH);
+}
+
+}  // namespace
+
+// TabRestoreHelper is used to restore a tab. In particular when the user
+// attempts to a restore a tab if the TabRestoreService hasn't finished loading
+// this waits for it. Once the TabRestoreService finishes loading the tab is
+// restored.
+class ChromeShellDelegate::TabRestoreHelper : public TabRestoreServiceObserver
{
+ public:
+  TabRestoreHelper(ChromeShellDelegate* delegate,
+                   Profile* profile,
+                   TabRestoreService* service)
+      : delegate_(delegate),
+        profile_(profile),
+        tab_restore_service_(service) {
+    tab_restore_service_->AddObserver(this);
+  }
+
+  virtual ~TabRestoreHelper() {
+    tab_restore_service_->RemoveObserver(this);
+  }
+
+  TabRestoreService* tab_restore_service() { return tab_restore_service_; }
+
+  virtual void TabRestoreServiceChanged(TabRestoreService* service) OVERRIDE {
+  }
+  virtual void TabRestoreServiceDestroyed(TabRestoreService* service) OVERRIDE
{
+    // This destroys us.
+    delegate_->tab_restore_helper_.reset();
+  }
+
+  virtual void TabRestoreServiceLoaded(TabRestoreService* service) OVERRIDE {
+    RestoreTabUsingProfile(profile_);
+    // This destroys us.
+    delegate_->tab_restore_helper_.reset();
+  }
+
+ private:
+  ChromeShellDelegate* delegate_;
+  Profile* profile_;
+  TabRestoreService* tab_restore_service_;
+
+  DISALLOW_COPY_AND_ASSIGN(TabRestoreHelper);
+};
+
 ChromeShellDelegate::ChromeShellDelegate()
     : window_positioner_(new ash::WindowPositioner()),
       weak_factory_(this),
@@ -150,25 +202,27 @@ void ChromeShellDelegate::ToggleMaximized() {
 }
 
 void ChromeShellDelegate::RestoreTab() {
-  Browser* browser = GetTargetBrowser();
-  // Do not restore tabs while in the incognito mode.
-  if (browser->profile()->IsOffTheRecord())
+  if (tab_restore_helper_.get()) {
+    DCHECK(!tab_restore_helper_->tab_restore_service()->IsLoaded());
+    return;
+  }
+
+  Browser* browser = chrome::FindBrowserWithWindow(ash::wm::GetActiveWindow());
+  Profile* profile = browser ? browser->profile() : NULL;
+  if (!profile)
+    profile = ProfileManager::GetDefaultProfileOrOffTheRecord();
+  if (profile->IsOffTheRecord())
     return;
   TabRestoreService* service =
-      TabRestoreServiceFactory::GetForProfile(browser->profile());
+      TabRestoreServiceFactory::GetForProfile(profile);
   if (!service)
     return;
+
   if (service->IsLoaded()) {
-    chrome::RestoreTab(browser);
+    RestoreTabUsingProfile(profile);
   } else {
+    tab_restore_helper_.reset(new TabRestoreHelper(this, profile, service));
     service->LoadTabsFromLastSession();
-    // LoadTabsFromLastSession is asynchronous, so TabRestoreService has not
-    // finished loading the entries at this point. Wait for next event cycle
-    // which loads the restored tab entries.
-    MessageLoop::current()->PostTask(
-        FROM_HERE,
-        base::Bind(&ChromeShellDelegate::RestoreTab,
-                   weak_factory_.GetWeakPtr()));
   }
 }

[commit-bot: I haz the power, 2013-05-10 18:29:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/14620014/12001

[commit-bot: I haz the power, 2013-05-10 21:15:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/14620014/12001

[commit-bot: I haz the power, 2013-05-10 21:19:29]

Change committed as 199540