Fix sync promo first run bugs

Fix sync promo first run bugs

This CL fixes two small bugs with showing the sync promo on first run.

First, if a master prefs file specified start URLs (using
"first_run_tabs") then the sync promo wouldn't be shown. Fix was to add
the sync promo even if the start URLs was non-empty.

Second, on Mac OS X, the "On Startup" pref would incorrectly be migrated
to "Continue where I left off". This also caused the sync promo to be
skipped. The problem was the profile version pref was not set yet which
caused the migration code to assume that an old profile was being
migrated. Fix was to check that the pref was actually set.

BUG=180521
TEST=Created the following master prefs file:
{
  "homepage" : "http://www.chromium.org/",
  "homepage_is_newtabpage" : false,
  "first_run_tabs" : [
    "new_tab_page",
    "welcome_page",
    "http://cnn.com"
  ]
}
Placed the file in /Library/Application Support/Chromium/Chromium Master Preferences
Verified that the sync promo showed.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187093

[sail, 2013-03-08 01:22:32]

marja: session_startup_pref.cc
gab: first_run_internal.h
sky: startup/*

[marja, 2013-03-08 11:29:53]

session_startup_pref.cc lgtm

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

LGTM w/ one question below; seems to be the same behavior as the old code so I
guess it's fine, but could be nice to fix.

Cheers!
Gab

https://codereview.chromium.org/12638005/diff/1/chrome/browser/ui/startup/sta...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/12638005/diff/1/chrome/browser/ui/startup/sta...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:275:
tabs->erase(tabs->begin() + 1);
This will remove the NTP even if it was intentionally added as the 1st
first_run_tabs; is that intended?

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

https://codereview.chromium.org/12638005/diff/1/chrome/browser/ui/startup/sta...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/12638005/diff/1/chrome/browser/ui/startup/sta...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:254: bool
IsNewTabURL(const GURL& url, Profile* profile) {
IsNewTabOrHomePage()

https://codereview.chromium.org/12638005/diff/1/chrome/browser/ui/startup/sta...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:260: void
AddSyncPromoTab(StartupTabs* tabs, Profile* profile) {
out params should be last, so profile first then tabs.

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

Addressed review comments. Please take a look.

https://codereview.chromium.org/12638005/diff/1/chrome/browser/ui/startup/sta...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/12638005/diff/1/chrome/browser/ui/startup/sta...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:254: bool
IsNewTabURL(const GURL& url, Profile* profile) {
On 2013/03/08 16:04:19, sky wrote:
> IsNewTabOrHomePage()

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.

https://codereview.chromium.org/12638005/diff/1/chrome/browser/ui/startup/sta...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:260: void
AddSyncPromoTab(StartupTabs* tabs, Profile* profile) {
On 2013/03/08 16:04:19, sky wrote:
> out params should be last, so profile first then tabs.

Done.

https://codereview.chromium.org/12638005/diff/1/chrome/browser/ui/startup/sta...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:275:
tabs->erase(tabs->begin() + 1);
On 2013/03/08 14:39:32, gab wrote:
> This will remove the NTP even if it was intentionally added as the 1st
> first_run_tabs; is that intended?

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.

[sky, 2013-03-08 21:56:43]

LGTM

[commit-bot: I haz the power, 2013-03-08 21:58:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/12638005/8001

[commit-bot: I haz the power, 2013-03-09 00:31:07]

Change committed as 187093