cros: Fix restoring session windows after crash inconsistent minimized window counts

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 eeafc2d4fed5bd8ac73abeea509a04cf4f662b5f..b6d19f0a417d48e25c8f280c4daed5d0c21b317c 100644
--- a/chrome/browser/sessions/session_restore.cc
+++ b/chrome/browser/sessions/session_restore.cc
@@ -395,20 +395,45 @@ class SessionRestoreImpl : public content::NotificationObserver {
     bool has_visible_browser = false;
     for (const auto& window : *windows) {

[sky, 2017/03/07 22:41:33]

This function is getting rather long and hard to reason about. Please move your
new conditions into this loop. (current code loops through windows 3 times, that
feels unnecessary).

       if (window->show_state != ui::SHOW_STATE_MINIMIZED ||
-          window->window_id.id() == active_window_id)
+          window->window_id.id() == active_window_id) {
         has_visible_browser = true;
+        break;
+      }
+    }
+
+    // If |browser_| exists, is on-record, and tabbed, we need to firstly
+    // consider restoring tabs from active window to |browser_|. If active
+    // tabbed window doesn't exist, restore the first set of tabs instead.
+    bool active_window_on_existing_browser = false;

[sky, 2017/03/07 22:41:33]

Renane to browser_is_active_window.

+    bool first_window_on_existing_browser = false;

[sky, 2017/03/07 22:41:32]

Rename to restore_first_window_to_browser.

+    if (browser_ && browser_->is_type_tabbed() &&
+        !browser_->profile()->IsOffTheRecord()) {
+      for (auto i = windows->begin(); i != windows->end(); ++i) {
+        if ((*i)->type != sessions::SessionWindow::TYPE_TABBED)
+          continue;
+
+        if ((*i)->window_id.id() == active_window_id) {
+          active_window_on_existing_browser = true;

[sky, 2017/03/07 22:41:33]

I think you should only do this logic if the browser window is not visible.
Otherwise you're changing the order that I mentioned previously. What state is
the browser's window in when you get here?

[Qiang(Joe) Xu, 2017/03/08 04:59:38]

I checked the visibility of |browser_|'s native window, it is visible. I think
this makes sense since |browser_| is already opened and active before session
restore starts in the reporter's case.

hmm, I am a little out of solution right now. I will firstly send this comment
back, and see if I can figure out another solution later.

+          break;
+        }
+      }
+
+      if (!active_window_on_existing_browser &&
+          (*windows->begin())->type == sessions::SessionWindow::TYPE_TABBED) {
+        first_window_on_existing_browser = true;
+      }
     }
 
     for (auto i = windows->begin(); i != windows->end(); ++i) {
       Browser* browser = nullptr;
       if (!has_tabbed_browser &&
-          (*i)->type == sessions::SessionWindow::TYPE_TABBED)
+          (*i)->type == sessions::SessionWindow::TYPE_TABBED) {
         has_tabbed_browser = true;
-      if (i == windows->begin() &&
-          (*i)->type == sessions::SessionWindow::TYPE_TABBED && browser_ &&
-          browser_->is_type_tabbed() &&
-          !browser_->profile()->IsOffTheRecord()) {
-        // The first set of tabs is added to the existing browser.
+      }
+
+      if ((active_window_on_existing_browser &&
+           (*i)->window_id.id() == active_window_id) ||
+          (first_window_on_existing_browser && i == windows->begin())) {

[sky, 2017/03/07 22:41:33]

That you have to check windows->begin() here is confusing. How about removing
first_window_on_existing_browser and checking the type?

         browser = browser_;
       } else {
 #if defined(OS_CHROMEOS)
@@ -656,7 +681,8 @@ class SessionRestoreImpl : public content::NotificationObserver {
 
     // TODO(jcampan): http://crbug.com/8123 we should not need to set the
     //                initial focus explicitly.
-    browser->tab_strip_model()->GetActiveWebContents()->SetInitialFocus();
+    if (browser->window()->IsActive())
+      browser->tab_strip_model()->GetActiveWebContents()->SetInitialFocus();
   }
 
   // Appends the urls in |urls| to |browser|.