Make --keep-alive-for-test respect the kBackgroundModeEnabled flag.

Make --keep-alive-for-test respect the kBackgroundModeEnabled preference.

The motivation for this is that we may one day want to make background mode be
the default, which this flag currently achieves, but in that case we'd need to
respect the user's desire to disable background mode altogether.

TEST=Starting chrome with --keep-alive-for-test should display a chrome icon in the system tray unless background mode is disabled from the advanced tab in chrome://settings. Toggling that setting should toggle the presence of the icon.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151674

[Andrew T Wilson (Slow), 2012-08-13 21:13:06]

Generally looks good, but I'm wondering if we should add a unit test to verify
this new behavior?

[Andrew T Wilson (Slow), 2012-08-13 21:20:35]

On 2012/08/13 21:13:06, Andrew T Wilson wrote:
> Generally looks good, but I'm wondering if we should add a unit test to verify
> this new behavior?

And, actually, I'm wondering if maybe a simpler solution is just to fail a
CHECK() if the user passes both --keep-alive-for-test=true and
--kBackgroundModeEnabled=false since they are operating at cross purposes. Just
ignoring one over the other seems like a mistake (and if I were to make a
change, I'd probably change it to give --keep-alive-for-test priority (so it
overrides any attempt to disable background mode).

[koz (OOO until 15th September), 2012-08-14 05:31:55]

On 2012/08/13 21:20:35, Andrew T Wilson wrote:
> On 2012/08/13 21:13:06, Andrew T Wilson wrote:
> > Generally looks good, but I'm wondering if we should add a unit test to
verify
> > this new behavior?
> 
> And, actually, I'm wondering if maybe a simpler solution is just to fail a
> CHECK() if the user passes both --keep-alive-for-test=true and
> --kBackgroundModeEnabled=false since they are operating at cross purposes.
Just
> ignoring one over the other seems like a mistake (and if I were to make a
> change, I'd probably change it to give --keep-alive-for-test priority (so it
> overrides any attempt to disable background mode).

Sorry, I think my commit message was misleading - kBackgroundModeEnabled is a
preference, not a flag and so changes dynamically as the user changes it from
chrome://settings. The use case I had in mind for this change was if we decide
to make background mode the default operating mode for chrome (even without any
extensions with the "background" permission) then we could just set the
--keep-alive-for-test flag to true (with a rename of course), in which case the
user should be able to turn it off via their preferences.

Also, I tried adding a unit test, but setting preferences via
g_browser_process->local_state() from the unit test seems to do bizarre things
(eg: local_state()->SetBoolean(kBackgroundModeEnabled, false) causes a
notification that the preference changed to true). Do you know anything about
that?

[Andrew T Wilson (Slow), 2012-08-14 21:09:43]

On 2012/08/14 05:31:55, koz wrote:
> On 2012/08/13 21:20:35, Andrew T Wilson wrote:
> > On 2012/08/13 21:13:06, Andrew T Wilson wrote:
> > > Generally looks good, but I'm wondering if we should add a unit test to
> verify
> > > this new behavior?
> > 
> > And, actually, I'm wondering if maybe a simpler solution is just to fail a
> > CHECK() if the user passes both --keep-alive-for-test=true and
> > --kBackgroundModeEnabled=false since they are operating at cross purposes.
> Just
> > ignoring one over the other seems like a mistake (and if I were to make a
> > change, I'd probably change it to give --keep-alive-for-test priority (so it
> > overrides any attempt to disable background mode).
> 
> Sorry, I think my commit message was misleading - kBackgroundModeEnabled is a
> preference, not a flag and so changes dynamically as the user changes it from
> chrome://settings. The use case I had in mind for this change was if we decide
> to make background mode the default operating mode for chrome (even without
any
> extensions with the "background" permission) then we could just set the
> --keep-alive-for-test flag to true (with a rename of course), in which case
the
> user should be able to turn it off via their preferences.

I see. I don't think that's how we'd enable background mode by default (by
somehow setting the keep-alive-for-test switch), but I'm OK with consolidating
the background mode logic in one place as you've done in this patch.

> 
> Also, I tried adding a unit test, but setting preferences via
> g_browser_process->local_state() from the unit test seems to do bizarre things
> (eg: local_state()->SetBoolean(kBackgroundModeEnabled, false) causes a
> notification that the preference changed to true). Do you know anything about
> that?

Hmmm. Doesn't sound familiar - can you upload the unit test as part of this
patch and I can take a look?

[koz (OOO until 15th September), 2012-08-15 00:29:40]

On 2012/08/14 21:09:43, Andrew T Wilson wrote:
> On 2012/08/14 05:31:55, koz wrote:
> > On 2012/08/13 21:20:35, Andrew T Wilson wrote:
> > > On 2012/08/13 21:13:06, Andrew T Wilson wrote:
> > > > Generally looks good, but I'm wondering if we should add a unit test to
> > verify
> > > > this new behavior?
> > > 
> > > And, actually, I'm wondering if maybe a simpler solution is just to fail a
> > > CHECK() if the user passes both --keep-alive-for-test=true and
> > > --kBackgroundModeEnabled=false since they are operating at cross purposes.
> > Just
> > > ignoring one over the other seems like a mistake (and if I were to make a
> > > change, I'd probably change it to give --keep-alive-for-test priority (so
it
> > > overrides any attempt to disable background mode).
> > 
> > Sorry, I think my commit message was misleading - kBackgroundModeEnabled is
a
> > preference, not a flag and so changes dynamically as the user changes it
from
> > chrome://settings. The use case I had in mind for this change was if we
decide
> > to make background mode the default operating mode for chrome (even without
> any
> > extensions with the "background" permission) then we could just set the
> > --keep-alive-for-test flag to true (with a rename of course), in which case
> the
> > user should be able to turn it off via their preferences.
> 
> I see. I don't think that's how we'd enable background mode by default (by
> somehow setting the keep-alive-for-test switch), but I'm OK with consolidating
> the background mode logic in one place as you've done in this patch.
> 
> > 
> > Also, I tried adding a unit test, but setting preferences via
> > g_browser_process->local_state() from the unit test seems to do bizarre
things
> > (eg: local_state()->SetBoolean(kBackgroundModeEnabled, false) causes a
> > notification that the preference changed to true). Do you know anything
about
> > that?
> 
> Hmmm. Doesn't sound familiar - can you upload the unit test as part of this
> patch and I can take a look?

Ah, silly mistake by me. I didn't realise that IsBackgrounModePrefEnabled() was
overridden in TestBackgroundModeManager. I've added the now-working tests :-)

[Andrew T Wilson (Slow), 2012-08-15 00:58:45]

lgtm

[commit-bot: I haz the power, 2012-08-15 01:06:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/10831254/7002

[commit-bot: I haz the power, 2012-08-15 02:42:51]

Try job failure for 10831254-7002 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-15 04:22:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/10831254/7002

[commit-bot: I haz the power, 2012-08-15 06:08:42]

Try job failure for 10831254-7002 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-15 06:29:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/10831254/7002

[commit-bot: I haz the power, 2012-08-15 08:53:19]

Change committed as 151674