Introduce --cancel-first-run and reduce the strength of --no-first-run.

Introduce --cancel-first-run and reduce the strength of --no-first-run.

--cancel-first-run has the same behavior as the old --no-first-run in that it writes the First Run sentinel (to prevent any later launch from being considered first run) on top of preventing the current run from being considered first run.

In most situations this is too strong and the consumer actually only wants to prevent the current run from being first run; while still allowing a future launch of chrome to be the first run.

This changes the behavior of Chrome Frame which now doesn't write the First Run sentinel (and thus allows a subsequent run of the real Chrome to be first run).

The only case where --cancel-first-run is actually needed is when a user-level Chrome which has already gone through first run self-destructs upon finding a system-level Chrome install on the same machine. After uninstalling itself, it launches system-level Chrome and needs --cancel-first-run to tell it that it should not do first run and just load the current user's prefs straight up (essentially resulting in the same thing as a regular launch of the user-level Chrome from the user's pov).

This is a follow-up on the discussion @ https://codereview.chromium.org/20483002/diff/10002/chrome/browser/first_run/first_run.cc#newcode492 and 
https://codereview.chromium.org/20483002/diff/10002/chrome/browser/chrome_browser_main.cc#oldcode988

Note that InProcessBrowserTest already adds --no-first-run to all tests, so respecifying the flag in sub-test-fixtures is superfluous and such code was removed in this CL as well.

To make sure that no instance of --no-first-run was ignored, patch set 10 actually replaces all instances of --no-first-run by --skip-first-run. It was however deemed too complex (https://codereview.chromium.org/21150006/diff/41001/chrome/common/chrome_switches.cc#oldcode968) to fully replace the flag since many tools outside of Chrome's codebase use --no-first-run and updating all of them to --skip-first-run just to keep the current behavior seemed like overkill. Patch set 11 thus reverted all --skip-first-run flags to --no-first-run.

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

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

Carlos, PTAL, as discussed on https://codereview.chromium.org/20483002/

Greg, do the CF side-effects LGTY? Or should we force --cancel-first-run there
as well?

Thanks!
Gab

[grt (UTC plus 2), 2013-07-30 02:48:16]

Nice idea.

https://codereview.chromium.org/21150006/diff/16001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_mac.mm (right):

https://codereview.chromium.org/21150006/diff/16001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_mac.mm:243: if
(!parsed_command_line().HasSwitch(switches::kCancelFirstRun) &&
how about abstracting the tests for flags away into a helper:
  if (!first_run::IsFirstRunSuppressed(parsed_command_line())) {
    ...
  }

https://codereview.chromium.org/21150006/diff/16001/chrome/browser/first_run/...
File chrome/browser/first_run/first_run.cc (left):

https://codereview.chromium.org/21150006/diff/16001/chrome/browser/first_run/...
chrome/browser/first_run/first_run.cc:486: internal::first_run_ =
internal::FIRST_RUN_CANCEL;
FIRST_RUN_CANCEL doesn't seem to exist in the repo. is this CR based on
un-committed code?

https://codereview.chromium.org/21150006/diff/16001/chrome_frame/chrome_frame...
File chrome_frame/chrome_frame_automation.cc (right):

https://codereview.chromium.org/21150006/diff/16001/chrome_frame/chrome_frame...
chrome_frame/chrome_frame_automation.cc:270:
command_line->AppendSwitch(switches::kSkipFirstRun);
GCF uses a different user data dir than Chrome, so there is no concern that a
subsequent Chrome execution would miss going through first-run. With that in
mind, this should be kCancelFirstRun to maintain existing behavior.

[gab, 2013-07-30 12:58:54]

https://codereview.chromium.org/21150006/diff/16001/chrome/browser/first_run/...
File chrome/browser/first_run/first_run.cc (left):

https://codereview.chromium.org/21150006/diff/16001/chrome/browser/first_run/...
chrome/browser/first_run/first_run.cc:486: internal::first_run_ =
internal::FIRST_RUN_CANCEL;
On 2013/07/30 02:48:16, grt wrote:
> FIRST_RUN_CANCEL doesn't seem to exist in the repo. is this CR based on
> un-committed code?

Ah, yes, can't sneak anything by you ;)! Based on
https://codereview.chromium.org/20743002/

[gab, 2013-07-30 14:10:17]

See comments below.

Thanks!
Gab

https://codereview.chromium.org/21150006/diff/16001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_mac.mm (right):

https://codereview.chromium.org/21150006/diff/16001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_mac.mm:243: if
(!parsed_command_line().HasSwitch(switches::kCancelFirstRun) &&
On 2013/07/30 02:48:16, grt wrote:
> how about abstracting the tests for flags away into a helper:
>   if (!first_run::IsFirstRunSuppressed(parsed_command_line())) {
>     ...
>   }

Done.

https://codereview.chromium.org/21150006/diff/16001/chrome_frame/chrome_frame...
File chrome_frame/chrome_frame_automation.cc (right):

https://codereview.chromium.org/21150006/diff/16001/chrome_frame/chrome_frame...
chrome_frame/chrome_frame_automation.cc:270:
command_line->AppendSwitch(switches::kSkipFirstRun);
On 2013/07/30 02:48:16, grt wrote:
> GCF uses a different user data dir than Chrome, so there is no concern that a
> subsequent Chrome execution would miss going through first-run. With that in
> mind, this should be kCancelFirstRun to maintain existing behavior.

Actually, for user-level the First Run beacon is dropped by chrome.exe so that
argument doesn't apply...

[grt (UTC plus 2), 2013-07-30 14:21:11]

this code lgtm, although i find the change in first_run.cc hard to review since
it's based on uncommitted code which i can't examine easily.

https://codereview.chromium.org/21150006/diff/16001/chrome_frame/chrome_frame...
File chrome_frame/chrome_frame_automation.cc (right):

https://codereview.chromium.org/21150006/diff/16001/chrome_frame/chrome_frame...
chrome_frame/chrome_frame_automation.cc:270:
command_line->AppendSwitch(switches::kSkipFirstRun);
On 2013/07/30 14:10:17, gab wrote:
> On 2013/07/30 02:48:16, grt wrote:
> > GCF uses a different user data dir than Chrome, so there is no concern that
a
> > subsequent Chrome execution would miss going through first-run. With that in
> > mind, this should be kCancelFirstRun to maintain existing behavior.
> 
> Actually, for user-level the First Run beacon is dropped by chrome.exe so that
> argument doesn't apply...

Ah. Right you are, sir. The beacon is put in the Application dir for user-level,
so it doesn't make sense that GCF would prevent user-level Chrome from going
through first run.

[cpu_(ooo_6.6-7.5), 2013-07-30 21:41:36]

lgtm

https://codereview.chromium.org/21150006/diff/41001/chrome/common/chrome_swit...
File chrome/common/chrome_switches.cc (left):

https://codereview.chromium.org/21150006/diff/41001/chrome/common/chrome_swit...
chrome/common/chrome_switches.cc:968: const char kNoFirstRun[]                  
 = "no-first-run";
please check/search if documentation in chromium.org mentions this flag, I
recall a page explaining master prefs.

[gab, 2013-07-31 00:00:51]

https://codereview.chromium.org/21150006/diff/41001/chrome/common/chrome_swit...
File chrome/common/chrome_switches.cc (left):

https://codereview.chromium.org/21150006/diff/41001/chrome/common/chrome_swit...
chrome/common/chrome_switches.cc:968: const char kNoFirstRun[]                  
 = "no-first-run";
On 2013/07/30 21:41:36, cpu (no reviews Aug 1 to 12) wrote:
> please check/search if documentation in http://chromium.org mentions this
flag, I
> recall a page explaining master prefs.

Actually this makes me realize that a bunch of random stuff using chrome might
use this flag (e.g., http://www.chromium.org/developers/testing/fuzzers suggests
using this and in might be hardcoded in some scripts outside of our src/).

The idea behind changing the name was to confirm/prove that I didn't forget an
instance of kNoFirstRun somewhere without deciding which flag it should actually
use.

How about renaming --skip-first-run back to --no-first-run after I get a green
try run with --skip-first-run (to first confirm I didn't forget any...)?
Although I did git grep for kNoFirstRun and --no-first-run to make sure there
were none left...

[gab, 2013-08-01 15:14:49]

ping, see below

https://codereview.chromium.org/21150006/diff/41001/chrome/common/chrome_swit...
File chrome/common/chrome_switches.cc (left):

https://codereview.chromium.org/21150006/diff/41001/chrome/common/chrome_swit...
chrome/common/chrome_switches.cc:968: const char kNoFirstRun[]                  
 = "no-first-run";
On 2013/07/31 00:00:51, gab wrote:
> On 2013/07/30 21:41:36, cpu (no reviews Aug 1 to 12) wrote:
> > please check/search if documentation in http://chromium.org mentions this
> flag, I
> > recall a page explaining master prefs.
> 
> Actually this makes me realize that a bunch of random stuff using chrome might
> use this flag (e.g., http://www.chromium.org/developers/testing/fuzzers
suggests
> using this and in might be hardcoded in some scripts outside of our src/).
> 
> The idea behind changing the name was to confirm/prove that I didn't forget an
> instance of kNoFirstRun somewhere without deciding which flag it should
actually
> use.
> 
> How about renaming --skip-first-run back to --no-first-run after I get a green
> try run with --skip-first-run (to first confirm I didn't forget any...)?
> Although I did git grep for kNoFirstRun and --no-first-run to make sure there
> were none left...

ping, what do you guys think of ^^ ?

[cpu_(ooo_6.6-7.5), 2013-08-01 21:18:20]

no objections from me.

[grt (UTC plus 2), 2013-08-02 15:56:32]

On 2013/08/01 15:14:49, gab wrote:
> ping, see below
> 
>
https://codereview.chromium.org/21150006/diff/41001/chrome/common/chrome_swit...
> File chrome/common/chrome_switches.cc (left):
> 
>
https://codereview.chromium.org/21150006/diff/41001/chrome/common/chrome_swit...
> chrome/common/chrome_switches.cc:968: const char kNoFirstRun[]                
 
>  = "no-first-run";
> On 2013/07/31 00:00:51, gab wrote:
> > On 2013/07/30 21:41:36, cpu (no reviews Aug 1 to 12) wrote:
> > > please check/search if documentation in http://chromium.org mentions this
> > flag, I
> > > recall a page explaining master prefs.
> > 
> > Actually this makes me realize that a bunch of random stuff using chrome
might
> > use this flag (e.g., http://www.chromium.org/developers/testing/fuzzers
> suggests
> > using this and in might be hardcoded in some scripts outside of our src/).
> > 
> > The idea behind changing the name was to confirm/prove that I didn't forget
an
> > instance of kNoFirstRun somewhere without deciding which flag it should
> actually
> > use.
> > 
> > How about renaming --skip-first-run back to --no-first-run after I get a
green
> > try run with --skip-first-run (to first confirm I didn't forget any...)?
> > Although I did git grep for kNoFirstRun and --no-first-run to make sure
there
> > were none left...
> 
> ping, what do you guys think of ^^ ?

sgtm

[grt (UTC plus 2), 2013-08-02 15:58:54]

On 2013/07/31 00:00:51, gab wrote:
>
https://codereview.chromium.org/21150006/diff/41001/chrome/common/chrome_swit...
> File chrome/common/chrome_switches.cc (left):
> 
>
https://codereview.chromium.org/21150006/diff/41001/chrome/common/chrome_swit...
> chrome/common/chrome_switches.cc:968: const char kNoFirstRun[]                
 
>  = "no-first-run";
> On 2013/07/30 21:41:36, cpu (no reviews Aug 1 to 12) wrote:
> > please check/search if documentation in http://chromium.org mentions this
> flag, I
> > recall a page explaining master prefs.
> 
> Actually this makes me realize that a bunch of random stuff using chrome might
> use this flag (e.g., http://www.chromium.org/developers/testing/fuzzers
suggests
> using this and in might be hardcoded in some scripts outside of our src/).
> 
> The idea behind changing the name was to confirm/prove that I didn't forget an
> instance of kNoFirstRun somewhere without deciding which flag it should
actually
> use.
> 
> How about renaming --skip-first-run back to --no-first-run after I get a green
> try run with --skip-first-run (to first confirm I didn't forget any...)?
> Although I did git grep for kNoFirstRun and --no-first-run to make sure there
> were none left...

grep for "no-first-run", too. you missed one in chrome_launcher.cc. ;-)

[gab, 2013-08-06 14:41:18]

On 2013/08/02 15:58:54, grt (no reviews Aug 3-11) wrote:
> On 2013/07/31 00:00:51, gab wrote:
> >
>
https://codereview.chromium.org/21150006/diff/41001/chrome/common/chrome_swit...
> > File chrome/common/chrome_switches.cc (left):
> > 
> >
>
https://codereview.chromium.org/21150006/diff/41001/chrome/common/chrome_swit...
> > chrome/common/chrome_switches.cc:968: const char kNoFirstRun[]              
 
>  
> >  = "no-first-run";
> > On 2013/07/30 21:41:36, cpu (no reviews Aug 1 to 12) wrote:
> > > please check/search if documentation in http://chromium.org mentions this
> > flag, I
> > > recall a page explaining master prefs.
> > 
> > Actually this makes me realize that a bunch of random stuff using chrome
might
> > use this flag (e.g., http://www.chromium.org/developers/testing/fuzzers
> suggests
> > using this and in might be hardcoded in some scripts outside of our src/).
> > 
> > The idea behind changing the name was to confirm/prove that I didn't forget
an
> > instance of kNoFirstRun somewhere without deciding which flag it should
> actually
> > use.
> > 
> > How about renaming --skip-first-run back to --no-first-run after I get a
green
> > try run with --skip-first-run (to first confirm I didn't forget any...)?
> > Although I did git grep for kNoFirstRun and --no-first-run to make sure
there
> > were none left...
> 
> grep for "no-first-run", too. you missed one in chrome_launcher.cc. ;-)

Ah, good point :)!

Running try jobs against latest version, will revert --skip-first-run to
--no-first-run when bots are green.

[gab, 2013-08-07 15:33:13]

@jochen for OWNER on side-effects in

chrome\browser\chrome_browser_main_mac.mm
chrome\browser\chromeos\login\login_browsertest.cc
chrome\test\nacl\nacl_browsertest_util.cc

Thanks!
Gab

[jochen (gone - plz use gerrit), 2013-08-08 00:55:05]

can you explain why you dropped the no first run switches from the browser test
without introducing cancel first run there?

[gab, 2013-08-08 01:29:36]

On 2013/08/08 00:55:05, jochen wrote:
> can you explain why you dropped the no first run switches from the browser
test
> without introducing cancel first run there?

Yes, hidden in the CL description:

"Note that InProcessBrowserTest already adds --no-first-run to all tests, so
respecifying the flag in sub-test-fixtures is superfluous and such code was
removed in this CL as well."

[jochen (gone - plz use gerrit), 2013-08-08 01:32:57]

thanks, lgtm

[commit-bot: I haz the power, 2013-08-08 01:47:25]

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

[commit-bot: I haz the power, 2013-08-08 22:20:04]

Change committed as 216460