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

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 375 matching lines @@
 
     // After the for loop, this contains the browser to activate, if one of the
     // windows has the same id as specified in active_window_id.
     Browser* browser_to_activate = nullptr;
 
     // Determine if there is a visible window, or if the active window exists.
     // Even if all windows are ui::SHOW_STATE_MINIMIZED, if one of them is the
     // active window it will be made visible by the call to
     // browser_to_activate->window()->Activate() later on in this method.
     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)
         chromeos::BootTimesRecorder::Get()->AddLoginTimeMarker(
             "SessionRestore-CreateRestoredBrowser-Start", false);
 #endif
         // Show the first window if none are visible.
         ui::WindowShowState show_state = (*i)->show_state;
         if (!has_visible_browser) {
           show_state = ui::SHOW_STATE_NORMAL;
@@ skipping 227 matching lines @@
     browser->tab_strip_model()->ActivateTabAt(selected_tab_index, true);
 
     if (browser_ == browser)
       return;
 
     browser->window()->Show();
     browser->set_is_session_restore(false);
 
     // 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|.
   void AppendURLsToBrowser(Browser* browser, const std::vector<GURL>& urls) {
     for (size_t i = 0; i < urls.size(); ++i) {
       int add_types = TabStripModel::ADD_FORCE_INDEX;
       if (i == 0)
         add_types |= TabStripModel::ADD_ACTIVE;
       chrome::NavigateParams params(browser, urls[i],
                                     ui::PAGE_TRANSITION_AUTO_TOPLEVEL);
@@ skipping 174 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;