Fix sync promo first run bugs

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 cb90e206e9241632efd9c31d4a7b172c5f0d6693..c5e9f79e244d7f9a2ea578f2cbdcde2731d930c6 100644
--- a/chrome/browser/ui/startup/startup_browser_creator_impl.cc
+++ b/chrome/browser/ui/startup/startup_browser_creator_impl.cc
@@ -251,6 +251,30 @@ void RecordAppLaunches(Profile* profile,
   }
 }
 
+bool IsNewTabURL(const GURL& url, Profile* profile) {

[sky, 2013/03/08 16:04:19]

IsNewTabOrHomePage()

[sail, 2013/03/08 20:06:43]

The "OrHomePage()" part isn't true. It's only if the url is a home page url
(empty) AND the home page is set to the NTP.

+  GURL ntp_url(chrome::kChromeUINewTabURL);
+  return url == ntp_url ||
+         (url.is_empty() && profile->GetHomePage() == ntp_url);
+}
+
+void AddSyncPromoTab(StartupTabs* tabs, Profile* profile) {

[sky, 2013/03/08 16:04:19]

out params should be last, so profile first then tabs.

[sail, 2013/03/08 20:06:43]

Done.

+  SyncPromoUI::DidShowSyncPromoAtStartup(profile);
+
+  StartupTab sync_promo_tab;
+  GURL continue_url;
+  if (!SyncPromoUI::UseWebBasedSigninFlow())
+    continue_url = GURL(chrome::kChromeUINewTabURL);
+  sync_promo_tab.url = SyncPromoUI::GetSyncPromoURL(
+      continue_url, SyncPromoUI::SOURCE_START_PAGE, false);
+  sync_promo_tab.is_pinned = false;
+  tabs->insert(tabs->begin(), sync_promo_tab);
+
+  // If the next URL is the NTP then remove it, effectively replacing the NTP
+  // with the sync promo.
+  if (tabs->size() > 1 && IsNewTabURL(tabs->at(1).url, profile))
+    tabs->erase(tabs->begin() + 1);

[gab, 2013/03/08 14:39:32]

This will remove the NTP even if it was intentionally added as the 1st
first_run_tabs; is that intended?

[sail, 2013/03/08 20:06:43]

This behavior is intentional. When the sync promo is completed (or skipped) we
redirect to the NTP. Replacing the NTP page means we won't have a duplicate NTP
tab.

+}
+
 class WebContentsCloseObserver : public content::NotificationObserver {
  public:
   WebContentsCloseObserver() : contents_(NULL) {}
@@ -706,6 +730,11 @@ Browser* StartupBrowserCreatorImpl::ProcessSpecifiedURLs(
     NOTREACHED() << "SessionStartupPref has deprecated type HOMEPAGE";
   }
 
+  if (pref.type != SessionStartupPref::LAST &&
+      SyncPromoUI::ShouldShowSyncPromoAtStartup(profile_, is_first_run_)) {
+    AddSyncPromoTab(&tabs, profile_);
+  }
+
   if (tabs.empty())
     return NULL;
 
@@ -909,31 +938,6 @@ void StartupBrowserCreatorImpl::AddStartupURLs(
       prefs->SetString(prefs::kManagedModeLocalPassphrase, "");
       prefs->SetString(prefs::kManagedModeLocalSalt, "");
     }
-  } else if (SyncPromoUI::ShouldShowSyncPromoAtStartup(profile_,
-                                                       is_first_run_)) {
-    // If the sync promo page is going to be displayed then insert it at the
-    // front of the list.
-    GURL continue_url;
-    if (!SyncPromoUI::UseWebBasedSigninFlow()) {
-      continue_url = GURL(chrome::kChromeUINewTabURL);
-    }
-
-    SyncPromoUI::DidShowSyncPromoAtStartup(profile_);
-    GURL old_url = (*startup_urls)[0];
-    (*startup_urls)[0] =
-        SyncPromoUI::GetSyncPromoURL(continue_url,
-                                     SyncPromoUI::SOURCE_START_PAGE,
-                                     false);
-
-    // An empty URL means to go to the home page.
-    if (old_url.is_empty() &&
-        profile_->GetHomePage() == GURL(chrome::kChromeUINewTabURL)) {
-      old_url = GURL(chrome::kChromeUINewTabURL);
-    }
-
-    // If the old URL is not the NTP then insert it right after the sync promo.
-    if (old_url != GURL(chrome::kChromeUINewTabURL))
-      startup_urls->insert(startup_urls->begin() + 1, old_url);
   }
 }