Refactoring startup logic for upcoming FRE changes (non-Win 10).

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>
 #include <memory>
 #include <vector>
 
 #include "apps/app_restore_service.h"
 #include "apps/app_restore_service_factory.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/environment.h"
+#include "base/feature_list.h"
 #include "base/lazy_instance.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/statistics_recorder.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_restrictions.h"
 #include "build/build_config.h"
 #include "chrome/browser/apps/install_chrome_app.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/custom_handlers/protocol_handler_registry.h"
 #include "chrome/browser/custom_handlers/protocol_handler_registry_factory.h"
 #include "chrome/browser/defaults.h"
 #include "chrome/browser/extensions/extension_creator.h"
 #include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/extensions/launch_util.h"
 #include "chrome/browser/extensions/pack_extension_job.h"
 #include "chrome/browser/first_run/first_run.h"
+#include "chrome/browser/first_run/first_run_features.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/prefs/incognito_mode_prefs.h"
 #include "chrome/browser/prefs/session_startup_pref.h"
 #include "chrome/browser/profile_resetter/triggered_profile_resetter.h"
 #include "chrome/browser/profile_resetter/triggered_profile_resetter_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_io_data.h"
 #include "chrome/browser/sessions/session_restore.h"
 #include "chrome/browser/sessions/session_service.h"
 #include "chrome/browser/sessions/session_service_factory.h"
@@ skipping 440 matching lines @@
       WebContents* app_tab = ::OpenAppShortcutWindow(profile, url);
       return (app_tab != NULL);
     }
   }
   return false;
 }
 
 void StartupBrowserCreatorImpl::ProcessLaunchURLs(
     bool process_startup,
     const std::vector<GURL>& urls_to_open) {
+  if (base::FeatureList::IsEnabled(features::kUseConsolidatedFirstRun)) {
+    ProcessLaunchURLsUsingConsolidatedFlow(process_startup, urls_to_open);
+    return;
+  }
+  // TODO(tmartino): Remainder of this function is deprecated. Remove when
+  // kUseConsolidatedFirstRun is on by default.
+
   // Don't open any browser windows if we're starting up in "background mode".
   if (process_startup && command_line_.HasSwitch(switches::kNoStartupWindow))
     return;
 
   // Determine whether or not this launch must include the welcome page.
   InitializeWelcomeRunType(urls_to_open);
 
-// TODO(tapted): Move this to startup_browser_creator_win.cc after refactor.
-#if defined(OS_WIN)
-  if (base::win::GetVersion() >= base::win::VERSION_WIN8) {
-    // See if there are apps for this profile that should be launched on startup
-    // due to a switch from Metro mode.
-    app_metro_launch::HandleAppLaunchForMetroRestart(profile_);
-  }
-#endif
-
   if (process_startup && ProcessStartupURLs(urls_to_open)) {
     // ProcessStartupURLs processed the urls, nothing else to do.
     return;
   }
 
   chrome::startup::IsProcessStartup is_process_startup = process_startup ?
       chrome::startup::IS_PROCESS_STARTUP :
       chrome::startup::IS_NOT_PROCESS_STARTUP;
   if (!process_startup) {
     // Even if we're not starting a new process, this may conceptually be
@@ skipping 31 matching lines @@
     // Always open a list of urls in a window on the native desktop.
     browser = chrome::FindTabbedBrowser(profile_, false);
   }
   // This will launch a browser; prevent session restore.
   StartupBrowserCreator::in_synchronous_profile_launch_ = true;
   browser = OpenURLsInBrowser(browser, process_startup, adjusted_urls);
   StartupBrowserCreator::in_synchronous_profile_launch_ = false;
   AddInfoBarsIfNecessary(browser, is_process_startup);
 }
 
+void StartupBrowserCreatorImpl::ProcessLaunchURLsUsingConsolidatedFlow(
+    bool process_startup,
+    const std::vector<GURL>& urls_to_open) {
+  std::vector<GURL> adjusted_urls(urls_to_open);
+
+  // Don't open any browser windows if we're starting up in "background mode".

[grt (UTC plus 2), 2016/08/23 10:36:02]

nit: i'm in the "no personal pronouns" camp
(https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/BYMa_...).
if it makes sense, please see if any comments with "we" and such in them can be
reworded for clarity without the pronoun. for example, this could be:
  // Don't open any browser windows when the process is being started in
background mode.

[tmartino, 2016/09/08 21:33:17]

Done.

+  if (process_startup && command_line_.HasSwitch(switches::kNoStartupWindow))
+    return;
+
+  // If Master Prefs contains any first run tabs, use only those.

[grt (UTC plus 2), 2016/08/23 10:36:02]

in the old logic, URLs on the command-line suppress first_run_tabs.

[tmartino, 2016/09/08 21:33:16]

Good catch; done.

+  std::vector<GURL> tabs_to_insert;
+  if (browser_creator_) {

[grt (UTC plus 2), 2016/08/23 10:36:02]

 && !browser_creator_->first_run_tabs_.empty()

[tmartino, 2016/09/08 21:33:16]

Ack, see thread about ProcessMasterPrefsTabs in first_run.cc.

+    tabs_to_insert =

[grt (UTC plus 2), 2016/08/23 10:36:02]

if you're assuming that first_run_tabs_ will never be non-empty on
non-first-run, please add a DCHECK for this:
    DCHECK(first_run_);

[grt (UTC plus 2), 2016/08/23 10:36:02]

also add:
    DCHECK(process_startup);
to document/assert the expectation that first_run_tabs_ will only be non-empty
during process startup and not during rendezvous

[tmartino, 2016/09/08 21:33:16]

Note that this logic has been moved to startup_tab_provider, but I'll respond
here anyway.

On 2016/08/23 at 10:36:02, grt (UTC plus 2) wrote:
My goal here is to enforce the policy "master preferences only apply on first
run." I'm not assuming that they're empty at other times--they should be, but
I'm not relying on that assumption.

On 2016/08/23 at 10:36:02, grt (UTC plus 2) wrote:
Can you elaborate on that assertion? Is this based on the idea that we clear
their values in this function, and the first call to this function should always
occur during process startup? Or is there some other reason to believe they're
only populated at process startup that would be risky to ignore?

[grt (UTC plus 2), 2016/09/09 11:40:29]

when i wrote that, i must have been thinking "how can you have first_run_tabs if
this isn't process startup since you'd have already gone through first run?" i
can imagine it's possible that the first launch of the browser does something
like start background mode or launch an app or something, and that it's only
during rendezvous that the user has what we think of as the "first run" browser
experience. it's a bit odd to think about, but as long as there are ways to
start the browser without bringing the user through the FRE then you're right,
first_run_tabs may be non-empty in the case of rendezvous. this also means we
shouldn't drop the First Run sentinel until we actually bring the user through
the experience. maybe this is already the case, i don't remember.

[tmartino, 2016/09/10 00:19:40]

Two things of note:
1. IsChromeFirstRun uses a cached value, so even after the sentinel is created,
IsChromeFirstRun will return true until the process terminates.
2. For Onboarding Tabs, I'm increasingly convinced we don't actually want to
base our logic entirely on this sentinel. We've got a lot of edge cases, for
example: 
  * We recently decided we want to show the welcome page to all new profiles,
not just the first execution on this machine.
  * We want to defer until second run if URLs are passed in via command line.
I think the proper logic might be to add a local state value that represents
"has this profile been shown the welcome page?". The only problem with this
approach is how we avoid showing this to upgrading users. WDYT?

+        first_run::ProcessMasterPrefsTabs(browser_creator_->first_run_tabs_);
+    browser_creator_->first_run_tabs_.clear();
+  }
+  // Otherwise, use the standard Onboarding logic to determine which, if any,
+  // tabs need to be surfaced.
+  if (tabs_to_insert.empty()) {
+    first_run::FirstRunSystemStatus status =
+        first_run::GetFirstRunSystemStatus();
+    tabs_to_insert = first_run::GetOnboardingTabs(status);
+  }
+

[grt (UTC plus 2), 2016/08/23 10:36:02]

is it intentional that the NTP is no longer opened?

[tmartino, 2016/09/08 21:33:16]

No, good catch. Thanks.

+  // If this profile is marked for a reset prompt, put this before all other
+  // tabs.
+  if (ProfileHasResetTrigger()) {
+    tabs_to_insert.insert(tabs_to_insert.begin(),
+                          internals::GetTriggeredResetSettingsURL());
+  }
+
+  // TODO(tmartino): Function for processing pinned tabs.
+
+  // Prepend the resulting list of tabs to those passed in.
+  if (!tabs_to_insert.empty()) {
+    adjusted_urls.insert(adjusted_urls.begin(), tabs_to_insert.begin(),
+                         tabs_to_insert.end());
+  }
+
+  // TODO(tmartino): If this is not process startup, we need to restore
+  // asynchronously and return here.
+
+  // See if we can or should restore an old session synchronously.
+  Browser* browser = MaybeRestoreSession(process_startup, adjusted_urls);

[grt (UTC plus 2), 2016/08/23 10:36:02]

can you break this function into two: one that computes the urls and what to do,
and another that opens them in a Browser? this way, you can exhaustively
unittest the former without having to rely on browser tests. not sure how much
of a gain that would be, but it's worth considering.

+
+  // Otherwise, open.
+  if (!browser) {

[grt (UTC plus 2), 2016/08/23 10:36:02]

nit: omit braces

[tmartino, 2016/09/08 21:33:16]

N/A

+    browser = OpenURLsInBrowser(browser, process_startup, adjusted_urls);
+  }
+
+  // Finally, add info bars.
+  chrome::startup::IsProcessStartup is_process_startup =
+      process_startup ? chrome::startup::IS_PROCESS_STARTUP
+                      : chrome::startup::IS_NOT_PROCESS_STARTUP;
+  AddInfoBarsIfNecessary(browser, is_process_startup);
+}
+
+Browser* StartupBrowserCreatorImpl::MaybeRestoreSession(
+    bool process_startup,
+    const std::vector<GURL>& urls) {
+  // TODO(tmartino): Move session restore logic into this function.
+  return NULL;

[grt (UTC plus 2), 2016/08/23 10:36:02]

nit: nullptr

[tmartino, 2016/09/08 21:33:17]

Done (throughout)

+}
+
 bool StartupBrowserCreatorImpl::ProcessStartupURLs(
     const std::vector<GURL>& urls_to_open) {
   VLOG(1) << "StartupBrowserCreatorImpl::ProcessStartupURLs";
   SessionStartupPref pref =
       StartupBrowserCreator::GetSessionStartupPref(command_line_, profile_);
   if (pref.type == SessionStartupPref::LAST)
     VLOG(1) << "Pref: last";
   else if (pref.type == SessionStartupPref::URLS)
     VLOG(1) << "Pref: urls";
   else if (pref.type == SessionStartupPref::DEFAULT)
@@ skipping 62 matching lines @@
       !HasPendingUncleanExit(profile_)) {
     content::BrowserContext::GetDefaultStoragePartition(profile_)->
         GetDOMStorageContext()->StartScavengingUnusedSessionStorage();
   }
 
   return true;
 }
 
 Browser* StartupBrowserCreatorImpl::ProcessSpecifiedURLs(
     const std::vector<GURL>& urls_to_open) {
+  // TODO(tmartino): Deprecated, remove this once UseConsolidatedFirstRun is
+  // enabled.
+
   SessionStartupPref pref =
       StartupBrowserCreator::GetSessionStartupPref(command_line_, profile_);
   StartupTabs tabs;
   // Pinned tabs should not be displayed when chrome is launched in incognito
   // mode. Also, no pages should be opened automatically if the session
   // crashed. Otherwise it might trigger another crash, locking the user out of
   // chrome. The crash infobar is shown in this case.
   if (!IncognitoModePrefs::ShouldLaunchIncognito(command_line_,
                                                  profile_->GetPrefs()) &&
       !HasPendingUncleanExit(profile_)) {
@@ skipping 158 matching lines @@
            browser_creator_->is_default_browser_dialog_suppressed())) {
         chrome::ShowDefaultBrowserPrompt(profile_);
       }
     }
 #endif
   }
 }
 
 void StartupBrowserCreatorImpl::AddStartupURLs(
     std::vector<GURL>* startup_urls) const {
-  // TODO(atwilson): Simplify the logic that decides which tabs to open on
-  // start-up and make it more consistent. http://crbug.com/248883
+  // TODO(tmartino): Deprecated, remove this once UseConsolidatedFirstRun is
+  // enabled.
 
   // If we have urls specified by the first run master preferences use them
   // and nothing else.
   if (browser_creator_ && startup_urls->empty()) {
     if (!browser_creator_->first_run_tabs_.empty()) {
       std::vector<GURL>::iterator it =
           browser_creator_->first_run_tabs_.begin();
       while (it != browser_creator_->first_run_tabs_.end()) {
         // Replace magic names for the actual urls.
         if (it->host() == "new_tab_page") {
@@ skipping 45 matching lines @@
           startup_urls->at(0) == GURL(chrome::kChromeUINewTabURL))
         startup_urls->at(0) = sync_promo_url;
       else
         startup_urls->insert(startup_urls->begin(), sync_promo_url);
     }
   }
 }
 
 void StartupBrowserCreatorImpl::AddSpecialURLs(
     std::vector<GURL>* url_list) const {
+  // TODO(tmartino): Deprecated, remove this once UseConsolidatedFirstRun is
+  // enabled.
+
   // Optionally include the welcome page.
   if (welcome_run_type_ == WelcomeRunType::FIRST_TAB)
     url_list->insert(url_list->begin(), internals::GetWelcomePageURL());
 
   // If this Profile is marked for a reset prompt, ensure the reset
   // settings dialog appears.
   if (ProfileHasResetTrigger()) {
     url_list->insert(url_list->begin(),
                      internals::GetTriggeredResetSettingsURL());
   }
 }
 
 // For first-run, the type will be FIRST_RUN_LAST for all systems except for
 // Windows 10+, where it will be FIRST_RUN_FIRST. For non-first run, the type
 // will be NONE for all systems except for Windows 10+, where it will be
 // ANY_RUN_FIRST if this is the first somewhat normal launch since an OS
 // upgrade.
+
+// TODO(tmartino): Deprecated, remove this once UseConsolidatedFirstRun is
+// enabled.
 void StartupBrowserCreatorImpl::InitializeWelcomeRunType(
     const std::vector<GURL>& urls_to_open) {
   DCHECK_EQ(static_cast<int>(WelcomeRunType::NONE),
             static_cast<int>(welcome_run_type_));
 #if defined(OS_WIN)
   // Do not welcome if there are any URLs to open.
   if (!urls_to_open.empty())
     return;
 
   base::win::OSInfo* const os_info = base::win::OSInfo::GetInstance();
@@ skipping 66 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;
 }