Make sure that the browser will always restore settings page instead of sign out page after user si…

chrome/browser/sessions/session_restore.cc

Index: chrome/browser/sessions/session_restore.cc
diff --git a/chrome/browser/sessions/session_restore.cc b/chrome/browser/sessions/session_restore.cc
index 87a44c4fb57615b643b7c68324c40ebcf61fa76f..bac02f29e030e0ca785cfe4b2a5e977032576c5f 100644
--- a/chrome/browser/sessions/session_restore.cc
+++ b/chrome/browser/sessions/session_restore.cc
@@ -575,6 +575,29 @@ class SessionRestoreImpl : public content::NotificationObserver {
     }
   }
 
+  int GetSelectedIndex(const sessions::SessionTab& tab) {

[sky, 2016/11/09 22:24:04]

I have a mild preference for moving this to
SessionService::OnGotSessionCommands.

[sky, 2016/11/09 22:24:04]

This only effects session restore, what about tab restore?

[zmin, 2016/11/10 22:36:07]

I want to scope the affect of this change as little as possible. The only
scenario is "user sign out + browser close and then sign in again" which is
covered by this CL very well based on my test. Including browser automatically
restore everything or manually restore the setting page by click
History->Recently closed. 

And any other restore process should still restore the sign out page as what we
have now.

[zmin, 2016/11/10 22:36:07]

OnGotSessionCommands is a simple function. Moving this to there means I have to
go through each tabs again which has additional performance cost. 

+    int selected_index = tab.current_navigation_index;
+    selected_index = std::max(
+        0,
+        std::min(selected_index, static_cast<int>(tab.navigations.size() - 1)));
+
+    // After user sign out, Chrome may navigate to the setting page from the
+    // sign out page asynchronously. The browser may be closed before the
+    // navigation callback finished.
+    std::string setting_page_url = std::string(chrome::kChromeUISettingsURL);
+    std::string sign_out_page_url =
+        setting_page_url + std::string(chrome::kSignOutSubPage);

[sky, 2016/11/09 22:24:04]

Is there a reason you care about this case, and say not always removing the sign
out page?

[zmin, 2016/11/10 22:36:07]

There're some edge cases I'm thinking about. For example, there're multiple sign
out sub pages in the navigation list so moving the index does nothing. Or the
previous page is totally unrelated as the sign out page url is typed manually by
user. 

"Always removing" might be too complicated here and I just want to keep the
thing simple here because it's enough to cover the use case which is "user sign
out then sign in again with force sign in"

+    if (selected_index > 0 &&
+        tab.navigations.at(selected_index).virtual_url().spec() ==

[sky, 2016/11/09 22:24:04]

at() -> [] (same comment below).

[zmin, 2016/11/10 22:36:07]

Done.

+            sign_out_page_url &&
+        tab.navigations.at(selected_index - 1).virtual_url().spec() ==
+            setting_page_url) {
+      selected_index -= 1;
+    }
+
+    return selected_index;
+  }
+
   // |tab_index| is ignored for pinned tabs which will always be pushed behind
   // the last existing pinned tab.
   // |tab_loader_| will schedule this tab for loading if |is_selected_tab| is
@@ -588,10 +611,7 @@ class SessionRestoreImpl : public content::NotificationObserver {
     // See crbug.com/154129.
     if (tab.navigations.empty())
       return nullptr;
-    int selected_index = tab.current_navigation_index;
-    selected_index = std::max(
-        0,
-        std::min(selected_index, static_cast<int>(tab.navigations.size() - 1)));
+    int selected_index = GetSelectedIndex(tab);
 
     RecordAppLaunchForTab(browser, tab, selected_index);