Add "Keep chrome running in background" preference.

Add "Keep chrome running in background" preference.

BUG=76828
TEST=Install background app, disable "Keep chrome running in background", see status tray icon disappear.

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

[Andrew T Wilson (Slow), 2011-05-11 18:31:50]

Please take a look - you can ignore the advanced_options* files if you like -
I'm going to dig up a DOM UI reviewer for those, but feel free to review them
too if you feel inclined.

[Rick Campbell, 2011-05-11 20:33:01]

LGTM with some wording nits.

http://codereview.chromium.org/6954001/diff/5001/chrome/app/chromium_strings.grd
File chrome/app/chromium_strings.grd (right):

http://codereview.chromium.org/6954001/diff/5001/chrome/app/chromium_strings....
chrome/app/chromium_strings.grd:339: Continue running background apps when
Chromium is closed
I think that you should replace "Continue running background apps when Chromium
is closed" with "Keep Chromium running in the background after the last window
is closed."

This phrasing revives points from older discussions, specifically (a) Chrome is
running, not just those apps and (b) "Chromium is closed" is not as
clear/correct as "last window closed."

http://codereview.chromium.org/6954001/diff/5001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6954001/diff/5001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:227: <message
name="IDS_KEEP_CHROME_RUNNING_IN_BACKGROUND" desc="The checkbox in the status
tray context menu that controls whether chrome keeps running in the background
after the last window is closed">
Should probably be "Chromium" rather than "chrome"

http://codereview.chromium.org/6954001/diff/5001/chrome/app/google_chrome_str...
File chrome/app/google_chrome_strings.grd (right):

http://codereview.chromium.org/6954001/diff/5001/chrome/app/google_chrome_str...
chrome/app/google_chrome_strings.grd:346: Continue running background apps when
Google Chrome is closed
Perhaps "Continue runnin Chromium in the background after the last window is
closed."

[Andrew T Wilson (Slow), 2011-05-11 23:00:48]

Please take another look - I've updated the unit tests.

Evan, can you take a look at the preferences changes? I've already gotten Glen's
approval from the UX team, although this stuff will go through UI review again.

http://codereview.chromium.org/6954001/diff/5001/chrome/app/chromium_strings.grd
File chrome/app/chromium_strings.grd (right):

http://codereview.chromium.org/6954001/diff/5001/chrome/app/chromium_strings....
chrome/app/chromium_strings.grd:339: Continue running background apps when
Chromium is closed
My concern with the alternate wording is it sounds like checking the box would
keep chrome running in the background regardless of the presence of background
apps.

In any case, the strings I've added are basically verbatim from Jeff Chang's
email, and they'll go through UI review once I've landed this.

http://codereview.chromium.org/6954001/diff/5001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6954001/diff/5001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:227: <message
name="IDS_KEEP_CHROME_RUNNING_IN_BACKGROUND" desc="The checkbox in the status
tray context menu that controls whether chrome keeps running in the background
after the last window is closed">
Good point. I've broken this out into separate chrome/chromium strings.

[Rick Campbell, 2011-05-12 05:42:42]

LGTM if one comment is fixed.  I suggest one new DCHECK as well.

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/background_mo...
File chrome/browser/background_mode_manager.cc (right):

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:236: // If background mode should be
enabled, but isn't, turn it on.
Consider adding DCHECK(IsBackgroundModePrefEnabled())

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/background_mo...
File chrome/browser/background_mode_manager_unittest.cc (right):

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/background_mo...
chrome/browser/background_mode_manager_unittest.cc:92: // Re-enable background
mode.
Reverse the sense of this comment to match the code that follows.

[Evan Stade, 2011-05-12 18:36:31]

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/resources/opt...
File chrome/browser/resources/options/advanced_options.html (right):

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/advanced_options.html:219: <span
i18n-content="backgroundModeCheckbox"></span>
why can't you use pref= here?

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/resources/opt...
File chrome/browser/resources/options/advanced_options.js (right):

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/advanced_options.js:116: if (!cr.isChromeOS &&
!cr.isMac) {
can you make this a check for the existence of backgroundModeCheckbox

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/advanced_options_handler.cc (right):

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/advanced_options_handler.cc:436: std::string
metric = (enabled ? "Options_BackgroundMode_Enable" :
no parens

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/advanced_options_handler.cc:437:
"Options_BackgroundMode_Disable");
indent to same horizontal position as Options_BackgroundMode_Enable

[Andrew T Wilson (Slow), 2011-05-12 23:34:30]

Please take another look.

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/background_mo...
File chrome/browser/background_mode_manager.cc (right):

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/background_mo...
chrome/browser/background_mode_manager.cc:236: // If background mode should be
enabled, but isn't, turn it on.
On 2011/05/12 05:42:42, Rick Campbell wrote:
> Consider adding DCHECK(IsBackgroundModePrefEnabled())

Done.

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/background_mo...
File chrome/browser/background_mode_manager_unittest.cc (right):

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/background_mo...
chrome/browser/background_mode_manager_unittest.cc:92: // Re-enable background
mode.
On 2011/05/12 05:42:42, Rick Campbell wrote:
> Reverse the sense of this comment to match the code that follows.
Actually, I needed to reverse the sense of the code :) This test is supposed to
re-enable background mode and confirm that nothing happens since there are no
background apps installed. Good catch!

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/resources/opt...
File chrome/browser/resources/options/advanced_options.html (right):

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/advanced_options.html:219: <span
i18n-content="backgroundModeCheckbox"></span>
On 2011/05/12 18:36:32, Evan Stade wrote:
> why can't you use pref= here?
We're using local_state and pref="" assumes that we're using the profile prefs,
I believe.

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/resources/opt...
File chrome/browser/resources/options/advanced_options.js (right):

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/advanced_options.js:116: if (!cr.isChromeOS &&
!cr.isMac) {
On 2011/05/12 18:36:32, Evan Stade wrote:
> can you make this a check for the existence of backgroundModeCheckbox

Done.

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/advanced_options_handler.cc (right):

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/advanced_options_handler.cc:436: std::string
metric = (enabled ? "Options_BackgroundMode_Enable" :
On 2011/05/12 18:36:32, Evan Stade wrote:
> no parens

Done.

http://codereview.chromium.org/6954001/diff/9001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/advanced_options_handler.cc:437:
"Options_BackgroundMode_Disable");
On 2011/05/12 18:36:32, Evan Stade wrote:
> indent to same horizontal position as Options_BackgroundMode_Enable

Done.

[Evan Stade, 2011-05-13 20:33:24]

lgtm

[non.chromium.rickcam, 2011-05-13 21:23:41]

LGTM as well