Fix for URL opening code

chrome/browser/ui/startup/startup_browser_creator_impl.cc

Index: chrome/browser/ui/startup/startup_browser_creator_impl.cc
diff --git a/chrome/browser/ui/startup/startup_browser_creator_impl.cc b/chrome/browser/ui/startup/startup_browser_creator_impl.cc
index 92e1e9bcbef548cfef54c3fc71bbd5c7e94bc40c..d98b5765441bdc6ff089f8b79bedddab78cb3157 100644
--- a/chrome/browser/ui/startup/startup_browser_creator_impl.cc
+++ b/chrome/browser/ui/startup/startup_browser_creator_impl.cc
@@ -130,6 +130,8 @@ 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 {
@@ -435,6 +437,59 @@ Browser* StartupBrowserCreatorImpl::OpenURLsInBrowser(
   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) {
@@ -762,8 +817,10 @@ Browser* StartupBrowserCreatorImpl::RestoreOrCreateBrowser(
   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);
   }
@@ -1027,7 +1084,7 @@ bool StartupBrowserCreatorImpl::ProcessStartupURLs(
     // 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;
   }