Do not display goodies with --no-first-run.

Do not display goodies with --no-first-run.

Make the browsertest parameterized with --no-first-run.

BUG=chromium:667448
TEST=browsertest

Committed: https://crrev.com/97ec92a6e1499fda8e3bf08d515381f5595fee23
Cr-Commit-Position: refs/heads/master@{#434008}

[achuithb, 2016-11-21 23:01:23]

The CQ bit was checked by achuith@chromium.org to run a CQ dry run

[achuithb, 2016-11-21 23:01:56]

achuith@chromium.org changed reviewers:
+ glevin@chromium.org

[achuithb, 2016-11-21 23:01:56]

Gred, PTAL

[commit-bot: I haz the power, 2016-11-21 23:02:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 23:57:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-21 23:57:42]

Dry run: This issue passed the CQ dry run.

[Greg Levin, 2016-11-22 00:13:40]

https://codereview.chromium.org/2519173003/diff/1/chrome/browser/chromeos/fir...
File chrome/browser/chromeos/first_run/goodies_displayer.cc (right):

https://codereview.chromium.org/2519173003/diff/1/chrome/browser/chromeos/fir...
chrome/browser/chromeos/first_run/goodies_displayer.cc:126:
switches::kNoFirstRun);
I'm not sure about all the use cases of this flag, and this may cover all cases
of interest as is, but did you look at DetermineFirstRunState() :

https://cs.chromium.org/chromium/src/chrome/browser/first_run/first_run.cc?l=652

There, --force-first-run supersedes --no-first-run.  I haven't had a chance to
play with the FirstRunSentinel (for example, I don't know what it does on
off-the-record sessions), so I'm not sure if it (and thus
DetermineFirstRunState()) is more appropriate here.

https://codereview.chromium.org/2519173003/diff/1/chrome/browser/chromeos/fir...
File chrome/browser/chromeos/first_run/goodies_displayer_browsertest.cc (right):

https://codereview.chromium.org/2519173003/diff/1/chrome/browser/chromeos/fir...
chrome/browser/chromeos/first_run/goodies_displayer_browsertest.cc:88: if
(NoFirstRunSpecified()) {  // --no-first-run is present by default.
Does this mean that this flag is present in the default browser test setup?  (If
so, there's no need to clarify it... just asking for my own curiosity...)

[achuithb, 2016-11-22 00:38:33]

The CQ bit was checked by achuith@chromium.org to run a CQ dry run

[achuithb, 2016-11-22 00:38:47]

PTAL, Greg

https://codereview.chromium.org/2519173003/diff/1/chrome/browser/chromeos/fir...
File chrome/browser/chromeos/first_run/goodies_displayer.cc (right):

https://codereview.chromium.org/2519173003/diff/1/chrome/browser/chromeos/fir...
chrome/browser/chromeos/first_run/goodies_displayer.cc:126:
switches::kNoFirstRun);
On 2016/11/22 00:13:40, Greg Levin wrote:
> I'm not sure about all the use cases of this flag, and this may cover all
cases
> of interest as is, but did you look at DetermineFirstRunState() :
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/first_run/first_run.cc?l=652
> 
> There, --force-first-run supersedes --no-first-run.  I haven't had a chance to
> play with the FirstRunSentinel (for example, I don't know what it does on
> off-the-record sessions), so I'm not sure if it (and thus
> DetermineFirstRunState()) is more appropriate here.

Done.

https://codereview.chromium.org/2519173003/diff/1/chrome/browser/chromeos/fir...
File chrome/browser/chromeos/first_run/goodies_displayer_browsertest.cc (right):

https://codereview.chromium.org/2519173003/diff/1/chrome/browser/chromeos/fir...
chrome/browser/chromeos/first_run/goodies_displayer_browsertest.cc:88: if
(NoFirstRunSpecified()) {  // --no-first-run is present by default.
On 2016/11/22 00:13:40, Greg Levin wrote:
> Does this mean that this flag is present in the default browser test setup? 
(If
> so, there's no need to clarify it... just asking for my own curiosity...)

Yup, default browser tests specify --no-first-run here:
https://cs.chromium.org/chromium/src/chrome/test/base/test_launcher_utils.cc?...

[commit-bot: I haz the power, 2016-11-22 00:39:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 01:13:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-22 01:13:10]

Dry run: This issue passed the CQ dry run.

[Greg Levin, 2016-11-22 16:57:29]

https://codereview.chromium.org/2519173003/diff/1/chrome/browser/chromeos/fir...
File chrome/browser/chromeos/first_run/goodies_displayer.cc (right):

https://codereview.chromium.org/2519173003/diff/1/chrome/browser/chromeos/fir...
chrome/browser/chromeos/first_run/goodies_displayer.cc:126:
switches::kNoFirstRun);
On 2016/11/22 00:38:47, achuithb wrote:
> On 2016/11/22 00:13:40, Greg Levin wrote:
> > I'm not sure about all the use cases of this flag, and this may cover all
> cases
> > of interest as is, but did you look at DetermineFirstRunState() :
> > 
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/first_run/first_run.cc?l=652
> > 
> > There, --force-first-run supersedes --no-first-run.  I haven't had a chance
to
> > play with the FirstRunSentinel (for example, I don't know what it does on
> > off-the-record sessions), so I'm not sure if it (and thus
> > DetermineFirstRunState()) is more appropriate here.
> 
> Done.

Sorry, didn't mean to imply that you *should* switch to IsChromeFirstRun(), only
that you should investigate the option.  I tested it this morning, and it
doesn't seem to be the right choice for Goodies (at least on Linux desktop debug
version).  The "First Run" flag file gets written even before the OOBE screen is
shown.  If I shut down and restart before doing a proper login (i.e. during
OOBE, or after only doing a guest login), the flag file exists, the system
doesn't think it's a first run, and no Goodies are ever shown.  The rest of the
Goodies code tries to ensure that they will be shown even after such incomplete
startups, so IsChromeFirstRun()'s dependence on the flag file seems like the
wrong choice after all.  I think a better solution would be something like

    const bool no_first_run =
        !base::CommandLine::ForCurrentProcess()->HasSwitch(
            switches::kForceFirstRun) &&
        base::CommandLine::ForCurrentProcess()->HasSwitch(
            switches::kNoFirstRun);

This is consistent with the flag logic in DetermineFirstRunState(), but doesn't
depend on the First Run flag file.

[achuithb, 2016-11-22 21:37:13]

On 2016/11/22 16:57:29, Greg Levin wrote:
>
https://codereview.chromium.org/2519173003/diff/1/chrome/browser/chromeos/fir...
> File chrome/browser/chromeos/first_run/goodies_displayer.cc (right):
> 
>
https://codereview.chromium.org/2519173003/diff/1/chrome/browser/chromeos/fir...
> chrome/browser/chromeos/first_run/goodies_displayer.cc:126:
> switches::kNoFirstRun);
> On 2016/11/22 00:38:47, achuithb wrote:
> > On 2016/11/22 00:13:40, Greg Levin wrote:
> > > I'm not sure about all the use cases of this flag, and this may cover all
> > cases
> > > of interest as is, but did you look at DetermineFirstRunState() :
> > > 
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/first_run/first_run.cc?l=652
> > > 
> > > There, --force-first-run supersedes --no-first-run.  I haven't had a
chance
> to
> > > play with the FirstRunSentinel (for example, I don't know what it does on
> > > off-the-record sessions), so I'm not sure if it (and thus
> > > DetermineFirstRunState()) is more appropriate here.
> > 
> > Done.
> 
> Sorry, didn't mean to imply that you *should* switch to IsChromeFirstRun(),
only
> that you should investigate the option.  I tested it this morning, and it
> doesn't seem to be the right choice for Goodies (at least on Linux desktop
debug
> version).  The "First Run" flag file gets written even before the OOBE screen
is
> shown.  If I shut down and restart before doing a proper login (i.e. during
> OOBE, or after only doing a guest login), the flag file exists, the system
> doesn't think it's a first run, and no Goodies are ever shown.  The rest of
the
> Goodies code tries to ensure that they will be shown even after such
incomplete
> startups, so IsChromeFirstRun()'s dependence on the flag file seems like the
> wrong choice after all.  I think a better solution would be something like
> 
>     const bool no_first_run =
>         !base::CommandLine::ForCurrentProcess()->HasSwitch(
>             switches::kForceFirstRun) &&
>         base::CommandLine::ForCurrentProcess()->HasSwitch(
>             switches::kNoFirstRun);
> 
> This is consistent with the flag logic in DetermineFirstRunState(), but
doesn't
> depend on the First Run flag file.

Looks like IsChromeFirstRun() isn't doing the right thing for chromeos, but I
don't think we want to fix that at the moment.

[achuithb, 2016-11-22 21:37:43]

PTAL, Greg! Thanks for looking into this.

https://codereview.chromium.org/2519173003/diff/1/chrome/browser/chromeos/fir...
File chrome/browser/chromeos/first_run/goodies_displayer.cc (right):

https://codereview.chromium.org/2519173003/diff/1/chrome/browser/chromeos/fir...
chrome/browser/chromeos/first_run/goodies_displayer.cc:126:
switches::kNoFirstRun);
On 2016/11/22 16:57:29, Greg Levin wrote:
> On 2016/11/22 00:38:47, achuithb wrote:
> > On 2016/11/22 00:13:40, Greg Levin wrote:
> > > I'm not sure about all the use cases of this flag, and this may cover all
> > cases
> > > of interest as is, but did you look at DetermineFirstRunState() :
> > > 
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/first_run/first_run.cc?l=652
> > > 
> > > There, --force-first-run supersedes --no-first-run.  I haven't had a
chance
> to
> > > play with the FirstRunSentinel (for example, I don't know what it does on
> > > off-the-record sessions), so I'm not sure if it (and thus
> > > DetermineFirstRunState()) is more appropriate here.
> > 
> > Done.
> 
> Sorry, didn't mean to imply that you *should* switch to IsChromeFirstRun(),
only
> that you should investigate the option.  I tested it this morning, and it
> doesn't seem to be the right choice for Goodies (at least on Linux desktop
debug
> version).  The "First Run" flag file gets written even before the OOBE screen
is
> shown.  If I shut down and restart before doing a proper login (i.e. during
> OOBE, or after only doing a guest login), the flag file exists, the system
> doesn't think it's a first run, and no Goodies are ever shown.  The rest of
the
> Goodies code tries to ensure that they will be shown even after such
incomplete
> startups, so IsChromeFirstRun()'s dependence on the flag file seems like the
> wrong choice after all.  I think a better solution would be something like
> 
>     const bool no_first_run =
>         !base::CommandLine::ForCurrentProcess()->HasSwitch(
>             switches::kForceFirstRun) &&
>         base::CommandLine::ForCurrentProcess()->HasSwitch(
>             switches::kNoFirstRun);
> 
> This is consistent with the flag logic in DetermineFirstRunState(), but
doesn't
> depend on the First Run flag file.

Done.

[achuithb, 2016-11-22 21:37:52]

The CQ bit was checked by achuith@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 21:38:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Greg Levin, 2016-11-22 21:49:21]

LGTM

https://codereview.chromium.org/2519173003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/goodies_displayer.cc (right):

https://codereview.chromium.org/2519173003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/goodies_displayer.cc:127: const bool
is_first_run =
Looks good.  One last optional nit, totally at your discretion:
"is_first_run" is misleading, as that's not really what this variable means at
all.  Maybe something like "switches_allow_first_run" or "first_run_permitted"?

[achuithb, 2016-11-22 22:08:25]

Thanks for the review!

https://codereview.chromium.org/2519173003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/goodies_displayer.cc (right):

https://codereview.chromium.org/2519173003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/goodies_displayer.cc:127: const bool
is_first_run =
On 2016/11/22 21:49:21, Greg Levin wrote:
> Looks good.  One last optional nit, totally at your discretion:
> "is_first_run" is misleading, as that's not really what this variable means at
> all.  Maybe something like "switches_allow_first_run" or
"first_run_permitted"?

I've renamed this. This variable exists purely so the if stmt below is readable,
so I don't think we lose much by being sloppy with the meaning.

[achuithb, 2016-11-22 22:08:30]

The CQ bit was checked by achuith@chromium.org

[achuithb, 2016-11-22 22:08:31]

The patchset sent to the CQ was uploaded after l-g-t-m from glevin@chromium.org
Link to the patchset: https://codereview.chromium.org/2519173003/#ps60001
(title: "rename")

[commit-bot: I haz the power, 2016-11-22 22:09:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 22:39:57]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1479852510729000,
"parent_rev": "6eb73d721b557fe12304e76693e0cf3ac9f683a4", "commit_rev":
"dffb29a91af14971fab116ad71b910855fdb5062"}

[commit-bot: I haz the power, 2016-11-22 22:40:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-22 22:42:55]

Description was changed from

==========
Do not display goodies with --no-first-run.

Make the browsertest parameterized with --no-first-run.

BUG=chromium:667448
TEST=browsertest
==========

to

==========
Do not display goodies with --no-first-run.

Make the browsertest parameterized with --no-first-run.

BUG=chromium:667448
TEST=browsertest

Committed: https://crrev.com/97ec92a6e1499fda8e3bf08d515381f5595fee23
Cr-Commit-Position: refs/heads/master@{#434008}
==========

[commit-bot: I haz the power, 2016-11-22 22:42:56]

Patchset 4 (id:??) landed as
https://crrev.com/97ec92a6e1499fda8e3bf08d515381f5595fee23
Cr-Commit-Position: refs/heads/master@{#434008}