Merge 3 different ways of obtaining first run state into a single one.

Merge 3 different ways of obtaining first run state (BrowserMain's |is_first_run|/|do_first_run_tasks_| and first_run::IsChromeFirstRun()) into a single one.

We have switches to force the browser in/out of thinking it's first run (--force-first-run and --no-first-run). Yet first_run::IsChromeFirstRun() didn't use those and thus some code entry points weren't respecting --force-first-run correctly.

We also had |is_first_run| and |do_first_run_tasks_| in chrome_browser_main.cc... I forget why I thought that was a good idea originally, but I no longer do..! Perhaps some things changed since..?

There are basically 3 first run state that matter:
1) It is first run => do the first run things everywhere (can be forced by --force-first-run)
2) It's not first run (neither forced, nor is first run sentinel present) => Don't do first run things.
3) --no-first-run => Just like (2), but we also create the first run sentinel to prevent any subsequent launches from being a natural first run.

This logic is now all coded up in a single place in first_run::IsChromeFirstRun().

Other changes (highlighted inline in patch set 1):
1) Some first run things used to be skipped on CHROME_OS based on a hidden ifdef; moved that ifdef up where it makes sense in chrome_browser_main.cc
2) Running first run in App mode or App Launcher mode would prevent a bunch of silent things from happening, based on an if which was originally there to prevent the import UI I believe (which is now silent...), reverted that trend: running App mode now only prevents adding first run tabs.

Also found issue 264694 while working on this CL, addressed with further cleanup in https://codereview.chromium.org/20743002/. 

BUG=234647, 264694
TEST=
A) Launch chrome.exe with no First Run sentinel, make sure first run happens once (relaunch and it shouldn't happen twice).
B) Launch chrome.exe with First Run sentinel, but with --force-first-run, make sure first run happens anyways.
C) Launch chrome.exe with no First Run sentinel, but with --no-first-run, make sure first run doesn't happen (and doesn't happen in subsequent relaunches either without the flag).

R=cpu@chromium.org, jam@chromium.org

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

[gab, 2013-07-26 15:41:13]

Carlos, please take a look, I tried to make the description as descriptive as
possible (+ inline comments on patch set 1).

Basically moving things around to fix the mess that accumulated over the years
about what should happen on first run.

Also put further cleanup in https://codereview.chromium.org/20743002/ to avoid
bloating this CL too much... (please take a look at that one after this one!)

Cheers!
Gab

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (left):

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:988:
!parsed_command_line().HasSwitch(switches::kApp) &&
We would previously set do_first_run_tasks_ to false if first run was in App
mode of AppLauncher mode.

I think this was previously done to avoid the import UI (now silent in
background -- i.e., AutoImport()) and avoid adding first run tabs...

Now a bunch of things have come under this logic which I don't think belong
here.... besides adding first run tabs, every other first run initialization
should happen even if running App Launcher or App mode first.

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:1027: first_run::CreateSentinel();
Now handled inside first_run::IsChromeFirstRun(); no point having browser main
care about setting this internal state of first_run.

Note: first_run::CreateSentinel() is further moved to first_run::internal:: in
https://codereview.chromium.org/20743002/

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:832: is_first_run_ =
first_run::IsChromeFirstRun();
We could in fact completely get rid of is_first_run_ state and use static getter
first_run::IsChromeFirstRun() everywhere if that's preferable..?

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:960: #if !defined(OS_ANDROID) &&
!defined(OS_CHROMEOS)
This comes from the old (removed in this CL) hidden side-effect in
ProcessMasterPreferences which I moved up here.

I think it's better to explicitly ifdef out things that shouldn't happen on
Chrome OS than to have do_first_run_tasks_ be set to false through some hidden
ifdef.

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/ui/startup...
File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (left):

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/ui/startup...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:469:
SyncPromoUI::DidShowSyncPromoAtStartup(browser()->profile());
This was originally required when I wrote these tests, but I don't see why it is
anymore, tests seem to pass without it.

[cpu_(ooo_6.6-7.5), 2013-07-26 21:35:00]

Haven't started actual review but I wonder about case #3, should it really
create the sentinel?

I guess what is the use case for --no-first-run?

[gab, 2013-07-26 23:14:01]

--no-first-run creates the sentinel by definition (see comment by
declaration of kNoFirstRun)

It's used by tests and chrome frame I believe, perhaps it would make sense
that it wouldn't create the sentinel, I haven't looked into that, but it
always has.
On Jul 26, 2013 5:35 PM, <cpu@chromium.org> wrote:

> Haven't started actual review but I wonder about case #3, should it really
> create the sentinel?
>
> I guess what is the use case for --no-first-run?
>
>
https://codereview.chromium.**org/20483002/<https://codereview.chromium.org/2...
>

[cpu_(ooo_6.6-7.5), 2013-07-27 21:16:31]

lgtm

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (left):

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:988:
!parsed_command_line().HasSwitch(switches::kApp) &&
On 2013/07/26 15:41:13, gab wrote:
> We would previously set do_first_run_tasks_ to false if first run was in App
> mode of AppLauncher mode.
> 
> I think this was previously done to avoid the import UI (now silent in
> background -- i.e., AutoImport()) and avoid adding first run tabs...
> 
> Now a bunch of things have come under this logic which I don't think belong
> here.... besides adding first run tabs, every other first run initialization
> should happen even if running App Launcher or App mode first.

In general I agree, but you should fire an email to benwells, it might be
possible to have a machine that has only IE and install a google calculator that
installs chrome under the hood. I don't know if that is ready or if we don't
plan to do that anymore. I guess if it works for a chromeframe only install it
should work with apps. This is just a suggestion.

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:832: is_first_run_ =
first_run::IsChromeFirstRun();
On 2013/07/26 15:41:13, gab wrote:
> We could in fact completely get rid of is_first_run_ state and use static
getter
> first_run::IsChromeFirstRun() everywhere if that's preferable..?

Yes please. You can have a function local is_first_run in case
IsChromeFirstRun() is doing disk IO but my last recollection is that it caches
the value.

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:960: #if !defined(OS_ANDROID) &&
!defined(OS_CHROMEOS)
On 2013/07/26 15:41:13, gab wrote:
> This comes from the old (removed in this CL) hidden side-effect in
> ProcessMasterPreferences which I moved up here.
> 
> I think it's better to explicitly ifdef out things that shouldn't happen on
> Chrome OS than to have do_first_run_tasks_ be set to false through some hidden
> ifdef.

Sure, It more along the lines of other things in this horrible horrible file.

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:978: // master_preferences into
local_state, as a "local_state" section in
btw, I pinged him on his 3 bugs (130598, 180304, 230115)

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/first_run/...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/first_run/...
chrome/browser/first_run/first_run.cc:492: CreateSentinel();
yeah it would be ideal if we don't create the sentinel when kNoFirstRun,
hopefully tests don't explode.

[gab, 2013-07-29 17:39:49]

@jam: This gets rid of do_first_run_tasks_ which I gather from
https://codereview.chromium.org/19689006 you wanted to move. Would you mind
stamping this for me please?

Thanks!
Gab

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (left):

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:988:
!parsed_command_line().HasSwitch(switches::kApp) &&
On 2013/07/27 21:16:31, cpu wrote:
> On 2013/07/26 15:41:13, gab wrote:
> > We would previously set do_first_run_tasks_ to false if first run was in App
> > mode of AppLauncher mode.
> > 
> > I think this was previously done to avoid the import UI (now silent in
> > background -- i.e., AutoImport()) and avoid adding first run tabs...
> > 
> > Now a bunch of things have come under this logic which I don't think belong
> > here.... besides adding first run tabs, every other first run initialization
> > should happen even if running App Launcher or App mode first.
> 
> In general I agree, but you should fire an email to benwells, it might be
> possible to have a machine that has only IE and install a google calculator
that
> installs chrome under the hood. I don't know if that is ready or if we don't
> plan to do that anymore. I guess if it works for a chromeframe only install it
> should work with apps. This is just a suggestion.

[cc'ing benwells here]

Right, so AFAIK this has been put to sleep for now while AppsV2 is brought up,
it will be revisited later.

Ideally we would have --no-first-run and --cancel-first-run. In most cases users
of that flag simply don't want first run now, but don't want to cancel it the
first time Chrome actually runs (i.e. CF, apps, tests, etc.).

We still need --cancel-first-run in some scenarios (i.e., self-destruct where we
don't want the system-level chrome being launched after destroying the
user-level chrome to launch in first run).

For now we only have --no-first-run which actually cancels first run from ever
happening in future runs of chrome and is likely too strong in most scenarios.

It will be very easy after this CL and https://codereview.chromium.org/20743002/
to implement separate --no-first-run and --cancel-first-run if required.

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:832: is_first_run_ =
first_run::IsChromeFirstRun();
On 2013/07/27 21:16:31, cpu wrote:
> On 2013/07/26 15:41:13, gab wrote:
> > We could in fact completely get rid of is_first_run_ state and use static
> getter
> > first_run::IsChromeFirstRun() everywhere if that's preferable..?
> 
> Yes please. You can have a function local is_first_run in case
> IsChromeFirstRun() is doing disk IO but my last recollection is that it caches
> the value.

Done, indeed IsChromeFirstRun() caches the value.

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/first_run/...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/first_run/...
chrome/browser/first_run/first_run.cc:492: CreateSentinel();
On 2013/07/27 21:16:31, cpu wrote:
> yeah it would be ideal if we don't create the sentinel when kNoFirstRun,
> hopefully tests don't explode.

It always has, but I agree that it would make sense to have --no-first-run do
what you think it should do and --cancel-first-run as a stronger flag when
cancelling first run is actually desired.

I'll look into that as a follow-up CL.

[benwells1, 2013-07-29 22:17:27]

--
from a tablet: terse and error prone
On Jul 30, 2013 3:39 AM, <gab@chromium.org> wrote:
>
> @jam: This gets rid of do_first_run_tasks_ which I gather from
> https://codereview.chromium.org/19689006 you wanted to move. Would you
mind
> stamping this for me please?
>
> Thanks!
> Gab
>
>
>
https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
> File chrome/browser/chrome_browser_main.cc (left):
>
>
https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
> chrome/browser/chrome_browser_main.cc:988:
> !parsed_command_line().HasSwitch(switches::kApp) &&
> On 2013/07/27 21:16:31, cpu wrote:
>>
>> On 2013/07/26 15:41:13, gab wrote:
>> > We would previously set do_first_run_tasks_ to false if first run
>
> was in App
>>
>> > mode of AppLauncher mode.
>> >
>> > I think this was previously done to avoid the import UI (now silent
>
> in
>>
>> > background -- i.e., AutoImport()) and avoid adding first run tabs...
>> >
>> > Now a bunch of things have come under this logic which I don't think
>
> belong
>>
>> > here.... besides adding first run tabs, every other first run
>
> initialization
>>
>> > should happen even if running App Launcher or App mode first.
>
>
>> In general I agree, but you should fire an email to benwells, it might
>
> be
>>
>> possible to have a machine that has only IE and install a google
>
> calculator that
>>
>> installs chrome under the hood. I don't know if that is ready or if we
>
> don't
>>
>> plan to do that anymore. I guess if it works for a chromeframe only
>
> install it
>>
>> should work with apps. This is just a suggestion.
>
>
> [cc'ing benwells here]
>
> Right, so AFAIK this has been put to sleep for now while AppsV2 is
> brought up, it will be revisited later.

That's right, chrome apps are only available from chrome at the moment.
Well probably get back to non-chrome in the next few months.

>
> Ideally we would have --no-first-run and --cancel-first-run. In most
> cases users of that flag simply don't want first run now, but don't want
> to cancel it the first time Chrome actually runs (i.e. CF, apps, tests,
> etc.).
>
> We still need --cancel-first-run in some scenarios (i.e., self-destruct
> where we don't want the system-level chrome being launched after
> destroying the user-level chrome to launch in first run).
>
> For now we only have --no-first-run which actually cancels first run
> from ever happening in future runs of chrome and is likely too strong in
> most scenarios.
>
> It will be very easy after this CL and
> https://codereview.chromium.org/20743002/ to implement separate
> --no-first-run and --cancel-first-run if required.

That sounds like it will be great for apps when we support non chrome usage.

>
>
https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
> File chrome/browser/chrome_browser_main.cc (right):
>
>
https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
> chrome/browser/chrome_browser_main.cc:832: is_first_run_ =
> first_run::IsChromeFirstRun();
> On 2013/07/27 21:16:31, cpu wrote:
>>
>> On 2013/07/26 15:41:13, gab wrote:
>> > We could in fact completely get rid of is_first_run_ state and use
>
> static
>>
>> getter
>> > first_run::IsChromeFirstRun() everywhere if that's preferable..?
>
>
>> Yes please. You can have a function local is_first_run in case
>> IsChromeFirstRun() is doing disk IO but my last recollection is that
>
> it caches
>>
>> the value.
>
>
> Done, indeed IsChromeFirstRun() caches the value.
>
>
https://codereview.chromium.org/20483002/diff/10002/chrome/browser/first_run/...
> File chrome/browser/first_run/first_run.cc (right):
>
>
https://codereview.chromium.org/20483002/diff/10002/chrome/browser/first_run/...
> chrome/browser/first_run/first_run.cc:492: CreateSentinel();
> On 2013/07/27 21:16:31, cpu wrote:
>>
>> yeah it would be ideal if we don't create the sentinel when
>
> kNoFirstRun,
>>
>> hopefully tests don't explode.
>
>
> It always has, but I agree that it would make sense to have
> --no-first-run do what you think it should do and --cancel-first-run as
> a stronger flag when cancelling first run is actually desired.
>
> I'll look into that as a follow-up CL.
>
> https://codereview.chromium.org/20483002/

[gab, 2013-07-29 22:22:29]

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (left):

https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:988:
!parsed_command_line().HasSwitch(switches::kApp) &&
On 2013/07/29 17:39:50, gab wrote:
> On 2013/07/27 21:16:31, cpu wrote:
> > On 2013/07/26 15:41:13, gab wrote:
> > > We would previously set do_first_run_tasks_ to false if first run was in
App
> > > mode of AppLauncher mode.
> > > 
> > > I think this was previously done to avoid the import UI (now silent in
> > > background -- i.e., AutoImport()) and avoid adding first run tabs...
> > > 
> > > Now a bunch of things have come under this logic which I don't think
belong
> > > here.... besides adding first run tabs, every other first run
initialization
> > > should happen even if running App Launcher or App mode first.
> > 
> > In general I agree, but you should fire an email to benwells, it might be
> > possible to have a machine that has only IE and install a google calculator
> that
> > installs chrome under the hood. I don't know if that is ready or if we don't
> > plan to do that anymore. I guess if it works for a chromeframe only install
it
> > should work with apps. This is just a suggestion.
> 
> [cc'ing benwells here]
> 
> Right, so AFAIK this has been put to sleep for now while AppsV2 is brought up,
> it will be revisited later.
> 
> Ideally we would have --no-first-run and --cancel-first-run. In most cases
users
> of that flag simply don't want first run now, but don't want to cancel it the
> first time Chrome actually runs (i.e. CF, apps, tests, etc.).
> 
> We still need --cancel-first-run in some scenarios (i.e., self-destruct where
we
> don't want the system-level chrome being launched after destroying the
> user-level chrome to launch in first run).
> 
> For now we only have --no-first-run which actually cancels first run from ever
> happening in future runs of chrome and is likely too strong in most scenarios.
> 
> It will be very easy after this CL and
https://codereview.chromium.org/20743002/
> to implement separate --no-first-run and --cancel-first-run if required.

> benwells: That sounds like it will be great for apps when we support non
chrome usage.

Here https://codereview.chromium.org/21150006/ :)!

[jam, 2013-07-29 23:09:10]

not really familiar with this code, rubberstamp lgtm based on cpu's review

[commit-bot: I haz the power, 2013-07-30 20:47:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/20483002/56001

[gab, 2013-07-31 02:14:09]

Committed patchset #4 manually as r214514 (presubmit successful).