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

chrome/browser/sessions/session_restore.cc

 // Copyright 2012 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/sessions/session_restore.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <list>
@@ skipping 557 matching lines @@
           base::TimeDelta delta = highest_time - tab.last_active_time;
           contents->SetLastActiveTime(now - delta);
           RestoredTab restored_tab(contents, false,
                                    tab.extension_app_id.empty(), tab.pinned);
           created_contents->push_back(restored_tab);
         }
       }
     }
   }
 
+  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
   // false.
   WebContents* RestoreTab(const sessions::SessionTab& tab,
                           const int tab_index,
                           Browser* browser,
                           bool is_selected_tab) {
     // It's possible (particularly for foreign sessions) to receive a tab
     // without valid navigations. In that case, just skip it.
     // 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);
 
     // Associate sessionStorage (if any) to the restored tab.
     scoped_refptr<content::SessionStorageNamespace> session_storage_namespace;
     if (!tab.session_storage_persistent_id.empty()) {
       session_storage_namespace =
           content::BrowserContext::GetDefaultStoragePartition(profile_)
               ->GetDOMStorageContext()
               ->RecreateSessionStorage(tab.session_storage_persistent_id);
@@ skipping 231 matching lines @@
 // static
 SessionRestore::CallbackSubscription
     SessionRestore::RegisterOnSessionRestoredCallback(
         const base::Callback<void(int)>& callback) {
   return on_session_restored_callbacks()->Add(callback);
 }
 
 // static
 base::CallbackList<void(int)>*
     SessionRestore::on_session_restored_callbacks_ = nullptr;