Switch guest profile from forced-incognito to UI mods like ChromeOS.

Switch guest profile from forced-incognito to UI mods like ChromeOS.

There have been issues with the way forced-incognito causes some things to behave and various menu items to appear.  We now follow the ChromeOS model (merging several instances of conditional ChromeOS code) of simply using the incognito profile when opening the window.

This change should fix some of the issues being seen in the field, simplify Guest profiles going forward, bring Desktop and ChromeOS experience closer together, and generally reduce Technical Debt from special-case scenarios.

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

[bcwhite, 2014-02-14 19:18:29]

yoz@chromium.org: Please review changes in 
chrome/browser/extensions/

noms@chromium.org: Please review changes in 
chrome/browser/profiles/

msw@chromium.org: Please review changes in 
chrome/browser/ui/

[noms (inactive), 2014-02-14 19:28:09]

https://codereview.chromium.org/164333007/diff/70001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/164333007/diff/70001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:1111: IncognitoModePrefs::ENABLED);
I think this is the default behaviour, so you don't need to set the pref
explicitly anymore.

https://codereview.chromium.org/164333007/diff/70001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:1112:
profile->GetPrefs()->SetBoolean(prefs::kShowBookmarkBar, false);
Is this still needed? At the moment you can bring it up with the accelerator, so
it's only hiding it, not disabling it.

https://codereview.chromium.org/164333007/diff/70001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:1126: return false;
If you move the ifdef to the top, you can just return profile->IsGuestSession(),
rather than returning twice.

[bcwhite, 2014-02-14 19:46:36]

https://codereview.chromium.org/164333007/diff/70001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/164333007/diff/70001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:1111: IncognitoModePrefs::ENABLED);
It has to be present to switch existing Guest profiles to the new state. 
Otherwise they will remain in forced-incognito.  I'll add a comment that it can
be removed later.

https://codereview.chromium.org/164333007/diff/70001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:1112:
profile->GetPrefs()->SetBoolean(prefs::kShowBookmarkBar, false);
On 2014/02/14 19:28:10, Monica Dinculescu wrote:
> Is this still needed? At the moment you can bring it up with the accelerator,
so
> it's only hiding it, not disabling it.

Done.

https://codereview.chromium.org/164333007/diff/70001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:1126: return false;
On 2014/02/14 19:28:10, Monica Dinculescu wrote:
> If you move the ifdef to the top, you can just return
profile->IsGuestSession(),
> rather than returning twice.

Could...  but I think the style of testing various conditions and returning is a
better choice here.

[Roger Tawa OOO till Jul 10th, 2014-02-14 20:15:31]

lgtm

I guess there are still changes needed in IsGuestSession() and
SwitchToGuestProfile()?

[Yoyo Zhou, 2014-02-14 20:45:50]

extensions LGTM

[bcwhite, 2014-02-14 21:18:01]

> I guess there are still changes needed in IsGuestSession() and
> SwitchToGuestProfile()?

No changes are necessary to either.  The former already included both the path
and command-line detection mechanisms.  The latter always opens windows as
incognito, just via a different mechanism.  There are (existing) CHECKs in the
code to ensure this.

[msw, 2014-02-15 00:30:51]

c/b/ui lgtm with nits.

https://codereview.chromium.org/164333007/diff/170002/chrome/browser/extensio...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://codereview.chromium.org/164333007/diff/170002/chrome/browser/extensio...
chrome/browser/extensions/chrome_extensions_browser_client.cc:126: if
((static_cast<Profile*>(context))->IsGuestSession() &&
nit: convert this if statement and following return to:
return !Profile::FromBrowserContext(context)->IsGuestSession() ||
context->IsOffTheRecord();

https://codereview.chromium.org/164333007/diff/170002/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/164333007/diff/170002/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1118: if (profile->IsGuestSession())
nit: if the order of checks between this and the CrOS block don't matter,
combine this with the final return statement to just:
return profile->IsGuestSession();

https://codereview.chromium.org/164333007/diff/170002/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/164333007/diff/170002/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:483: if
(browser_->profile()->IsGuestSession())
nit: just return !browser_->profile()->IsGuestSession();

https://codereview.chromium.org/164333007/diff/170002/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/164333007/diff/170002/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1578: Profile*
profile = Profile::FromWebUI(web_ui());
nit: inline this in the if statement below.

[bcwhite, 2014-02-18 15:19:36]

https://codereview.chromium.org/164333007/diff/170002/chrome/browser/extensio...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://codereview.chromium.org/164333007/diff/170002/chrome/browser/extensio...
chrome/browser/extensions/chrome_extensions_browser_client.cc:126: if
((static_cast<Profile*>(context))->IsGuestSession() &&
On 2014/02/15 00:30:52, msw wrote:
> nit: convert this if statement and following return to:
> return !Profile::FromBrowserContext(context)->IsGuestSession() ||
> context->IsOffTheRecord();

Done.

https://codereview.chromium.org/164333007/diff/170002/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/164333007/diff/170002/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1118: if (profile->IsGuestSession())
On 2014/02/15 00:30:52, msw wrote:
> nit: if the order of checks between this and the CrOS block don't matter,
> combine this with the final return statement to just:
> return profile->IsGuestSession();

Done.

https://codereview.chromium.org/164333007/diff/170002/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/164333007/diff/170002/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:483: if
(browser_->profile()->IsGuestSession())
On 2014/02/15 00:30:52, msw wrote:
> nit: just return !browser_->profile()->IsGuestSession();

Done.

https://codereview.chromium.org/164333007/diff/170002/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/164333007/diff/170002/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:1578: Profile*
profile = Profile::FromWebUI(web_ui());
On 2014/02/15 00:30:52, msw wrote:
> nit: inline this in the if statement below.

Done.

[noms (inactive), 2014-02-18 15:20:01]

profiles lgtm

[bcwhite, 2014-02-18 15:20:13]

The CQ bit was checked by bcwhite@chromium.org

[commit-bot: I haz the power, 2014-02-18 15:20:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bcwhite@chromium.org/164333007/210001

[commit-bot: I haz the power, 2014-02-18 18:20:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bcwhite@chromium.org/164333007/210001

[commit-bot: I haz the power, 2014-02-18 18:28:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bcwhite@chromium.org/164333007/210001

[commit-bot: I haz the power, 2014-02-18 19:07:15]

Change committed as 251803