Fix for URL opening code

chrome/browser/ui/startup/startup_browser_creator_impl.cc

 // Copyright (c) 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/ui/startup/startup_browser_creator_impl.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 112 matching lines @@
 #if BUILDFLAG(ENABLE_RLZ)
 #include "components/rlz/rlz_tracker.h"  // nogncheck
 #endif
 
 using content::ChildProcessSecurityPolicy;
 using content::WebContents;
 using extensions::Extension;
 
 namespace {
 
+std::vector<GURL>* g_startup_urls = nullptr;

[Avi (use Gerrit), 2017/04/06 15:54:42]

Why would this need to be global? Can't it be an instance variable?

[eugenebng, 2017/04/10 11:54:08]

This cannot be an instance variable, because different instances of this class
are used.
For opening URLSs, local StartupBrowserCreatorImpl is used:
https://cs.chromium.org/chromium/src/chrome/browser/app_controller_mac.mm?rcl...
For restoring session - other instance.

+
 // Utility functions ----------------------------------------------------------
 
 enum LaunchMode {
   LM_TO_BE_DECIDED = 0,       // Possibly direct launch or via a shortcut.
   LM_AS_WEBAPP,               // Launched as a installed web application.
   LM_WITH_URLS,               // Launched with urls in the cmd line.
   LM_SHORTCUT_NONE,           // Not launched from a shortcut.
   LM_SHORTCUT_NONAME,         // Launched from shortcut but no name available.
   LM_SHORTCUT_UNKNOWN,        // Launched from user-defined shortcut.
   LM_SHORTCUT_QUICKLAUNCH,    // Launched from the quick launch bar.
@@ skipping 285 matching lines @@
 
 Browser* StartupBrowserCreatorImpl::OpenURLsInBrowser(
     Browser* browser,
     bool process_startup,
     const std::vector<GURL>& urls) {
   StartupTabs tabs;
   UrlsToTabs(urls, &tabs);
   return OpenTabsInBrowser(browser, process_startup, tabs);
 }
 
+void StartupBrowserCreatorImpl::OpenURLsAfterSessionRestore(
+    Browser* browser,
+    bool process_startup,
+    const std::vector<GURL>& urls) {
+  // Save URLs for deferred opening
+  if (!g_startup_urls)
+    g_startup_urls = new std::vector<GURL>();
+  *g_startup_urls = urls;
+}
+
+bool StartupBrowserCreatorImpl::WillRestoreSession(

[Avi (use Gerrit), 2017/04/06 15:54:42]

This seems like a bunch of logic that must already exist somewhere. Surely
Chrome already knows that it's going to restore a session, so I worry that this
will fall out of state with the logic elsewhere.

[gab, 2017/04/06 16:02:21]

That's probably true and if you can avoid touching
startup_browser_creator(_impl) as much as possible that'd be great, it's already
*such* a mess..!

[gab, 2017/04/06 16:02:21]

That's probably true and if you can avoid touching
startup_browser_creator(_impl) as much as possible that'd be great, it's already
*such* a mess..!

[eugenebng, 2017/04/10 11:54:08]

True, this logic was taken from conditions for calling session restore in this
class:
https://cs.chromium.org/chromium/src/chrome/browser/ui/startup/startup_browse...
https://cs.chromium.org/chromium/src/chrome/browser/ui/startup/startup_browse...

It explains what I want to change and works as a proof of concept, but I
absolutely agree that this code should be significantly improved or rewritten.
The problem is that, before changes, app_controller_mac creates browser and
shows if necessary. Either browser creating and showing code should be changed
or app_controller_mac should know if session will be restored or not (which
resulted in a complicated duplicated logic in first iteration) and behave
differently according to that - that's what is shown in this CL.
Improving this logic looks like the Most Important point here.
I'll think on what can be done here, but don't hesitate to give an advise if you
have any ideas on this.

+    Browser* browser,
+    Profile* profile,
+    bool process_startup,
+    const std::vector<GURL>& urls) {
+  // First condition.
+  BrowserOpenBehaviorOptions behavior_options = 0;
+  bool is_post_crash_launch = HasPendingUncleanExit(profile);
+  if (process_startup)
+    behavior_options |= PROCESS_STARTUP;
+  if (is_post_crash_launch)
+    behavior_options |= IS_POST_CRASH_LAUNCH;
+  if (command_line_.HasSwitch(switches::kRestoreLastSession))
+    behavior_options |= HAS_RESTORE_SWITCH;
+  if (command_line_.HasSwitch(switches::kOpenInNewWindow))
+    behavior_options |= HAS_NEW_WINDOW_SWITCH;
+  if (!urls.empty())
+    behavior_options |= HAS_CMD_LINE_TABS;
+  BrowserOpenBehavior behavior = DetermineBrowserOpenBehavior(
+      StartupBrowserCreator::GetSessionStartupPref(command_line_, profile),
+      behavior_options);
+  if (behavior == BrowserOpenBehavior::SYNCHRONOUS_RESTORE)
+    return true;
+  // Second contition.
+  SessionStartupPref pref =
+      StartupBrowserCreator::GetSessionStartupPref(command_line_, profile);
+  if (pref.type == SessionStartupPref::LAST && !is_first_run_&&
+      (!(profile->GetLastSessionExitType() == Profile::EXIT_CRASHED) ||
+       command_line_.HasSwitch(switches::kRestoreLastSession)))
+    return true;
+  return false;
+}
+
+void StartupBrowserCreatorImpl::OpenDeferredURLs(Browser* browser) {
+  if (g_startup_urls && !g_startup_urls->empty()) {
+    StartupBrowserCreator::in_synchronous_profile_launch_ = true;
+    OpenURLsInBrowser(browser, false, *g_startup_urls);
+    StartupBrowserCreator::in_synchronous_profile_launch_ = false;
+    delete g_startup_urls;
+    g_startup_urls = nullptr;
+  }
+}
+
 Browser* StartupBrowserCreatorImpl::OpenTabsInBrowser(Browser* browser,
                                                       bool process_startup,
                                                       const StartupTabs& tabs) {
   DCHECK(!tabs.empty());
 
   // If we don't yet have a profile, try to use the one we're given from
   // |browser|. While we may not end up actually using |browser| (since it
   // could be a popup window), we can at least use the profile.
   if (!profile_ && browser)
     profile_ = browser->profile();
@@ skipping 307 matching lines @@
 Browser* StartupBrowserCreatorImpl::RestoreOrCreateBrowser(
     const StartupTabs& tabs,
     BrowserOpenBehavior behavior,
     SessionRestore::BehaviorBitmask restore_options,
     bool process_startup,
     bool is_post_crash_launch) {
   Browser* browser = nullptr;
   if (behavior == BrowserOpenBehavior::SYNCHRONOUS_RESTORE) {
     browser = SessionRestore::RestoreSession(profile_, nullptr, restore_options,
                                              TabsToUrls(tabs));
-    if (browser)
+    if (browser) {
+      OpenDeferredURLs(browser);
       return browser;
+    }
   } else if (behavior == BrowserOpenBehavior::USE_EXISTING) {
     browser = chrome::FindTabbedBrowser(profile_, process_startup);
   }
 
   base::AutoReset<bool> synchronous_launch_resetter(
       &StartupBrowserCreator::in_synchronous_profile_launch_, true);
 
   // OpenTabsInBrowser requires at least one tab be passed. As a fallback to
   // prevent a crash, use the NTP if |tabs| is empty. This could happen if
   // we expected a session restore to happen but it did not occur/succeed.
@@ skipping 243 matching lines @@
     }
 #endif
 
     std::vector<GURL> adjusted_urls(urls_to_open);
     AddSpecialURLs(&adjusted_urls);
 
     // The startup code only executes for browsers launched in desktop mode. Ash
     // should never get here.
     Browser* browser = SessionRestore::RestoreSession(
         profile_, NULL, restore_behavior, adjusted_urls);
-
+    OpenDeferredURLs(browser);
     AddInfoBarsIfNecessary(browser, chrome::startup::IS_PROCESS_STARTUP);
     return true;
   }
 
   Browser* browser = ProcessSpecifiedURLs(urls_to_open);
   if (!browser)
     return false;
 
   AddInfoBarsIfNecessary(browser, chrome::startup::IS_PROCESS_STARTUP);
 
@@ skipping 216 matching lines @@
 #if defined(OS_WIN)
   TriggeredProfileResetter* triggered_profile_resetter =
       TriggeredProfileResetterFactory::GetForBrowserContext(profile_);
   // TriggeredProfileResetter instance will be nullptr for incognito profiles.
   if (triggered_profile_resetter) {
     has_reset_trigger = triggered_profile_resetter->HasResetTrigger();
   }
 #endif  // defined(OS_WIN)
   return has_reset_trigger;
 }