Supervised users: if history recording is off, allow incognito and deleting history

Supervised users: if history recording is off, allow incognito and deleting history

Some related cleanup:
- History UI: Properly distinguish profile->IsSupervised vs. prefs::kAllowDeletingBrowserHistory vs groupByDomain
- "New incognito window" in hotdog menu: Honor IncognitoModePrefs

BUG=232316
Committed: https://crrev.com/6f959801c7ae3a9ff745220638032a03bbb57047
Cr-Commit-Position: refs/heads/master@{#303627}

[Marc Treib, 2014-11-06 18:17:33]

treib@chromium.org changed reviewers:
+ bauerb@chromium.org, dbeam@chromium.org, msw@chromium.org, rsesek@chromium.org

[Marc Treib, 2014-11-06 18:17:34]

rsesek@chromium.org: Please review changes in app_controller_mac.mm

msw@chromium.org: Please review changes in wrench_menu_model.cc

bauerb@chromium.org: Please review changes in c/b/supervised_user/*

dbeam@chromium.org: Please review changes in history.js and history_ui.cc

[Robert Sesek, 2014-11-06 19:06:41]

app_controller_mac LGTM

https://codereview.chromium.org/697693004/diff/20001/chrome/browser/app_contr...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/697693004/diff/20001/chrome/browser/app_contr...
chrome/browser/app_controller_mac.mm:1471: if (!profile ||
IncognitoModePrefs::GetAvailability(profile->GetPrefs()) !=
nit: would be a little better to break after || and put the second condition all
one line

[msw, 2014-11-06 19:08:03]

On 2014/11/06 18:17:34, Marc Treib wrote:
> mailto:rsesek@chromium.org: Please review changes in app_controller_mac.mm
> mailto:msw@chromium.org: Please review changes in wrench_menu_model.cc
> mailto:bauerb@chromium.org: Please review changes in c/b/supervised_user/*
> mailto:dbeam@chromium.org: Please review changes in history.js and
history_ui.cc

It would make more sense to have one initial reviewer take a look at the entire
CL (rather than pinging 4 people) when it's this small. disparate OWNERS can be
pinged after an initial review, or TBR'ed for very small/mechanical changes.

https://codereview.chromium.org/697693004/diff/20001/chrome/browser/ui/toolba...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/697693004/diff/20001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/wrench_menu_model.cc:524: return
IncognitoModePrefs::GetAvailability(browser_->profile()->GetPrefs()) !=
I thought there was some older discussion to making this the canonical way to
check for incognito availability, so that guest session profiles would return
false for that, and you could avoid the check above.

[Dan Beam, 2014-11-06 19:30:06]

c/b/resources and c/b/ui/webui lgtm

https://codereview.chromium.org/697693004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/history/history.js (right):

https://codereview.chromium.org/697693004/diff/20001/chrome/browser/resources...
chrome/browser/resources/history/history.js:1938: // Only show the controls if
the command line switch is activated.
// or the user is supervised.

https://codereview.chromium.org/697693004/diff/20001/chrome/browser/resources...
chrome/browser/resources/history/history.js:1945: if
(!loadTimeData.getBoolean('allowDeletingHistory')) {
nit: no curlies

[Bernhard Bauer, 2014-11-06 22:59:37]

LGTM w/ a nit:

https://codereview.chromium.org/697693004/diff/20001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_constants.h (right):

https://codereview.chromium.org/697693004/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_constants.h:17: extern const char
kAllowDeletingBrowserHistory[];
Keeps these sorted please.

[Marc Treib, 2014-11-07 10:16:42]

msw: Thanks for the hint, I'll do that next time. And sorry for any troubles
this time.

https://codereview.chromium.org/697693004/diff/20001/chrome/browser/app_contr...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/697693004/diff/20001/chrome/browser/app_contr...
chrome/browser/app_controller_mac.mm:1471: if (!profile ||
IncognitoModePrefs::GetAvailability(profile->GetPrefs()) !=
On 2014/11/06 19:06:41, Robert Sesek wrote:
> nit: would be a little better to break after || and put the second condition
all
> one line

Break after || done. The condition still doesn't fit on one line, though.

https://codereview.chromium.org/697693004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/history/history.js (right):

https://codereview.chromium.org/697693004/diff/20001/chrome/browser/resources...
chrome/browser/resources/history/history.js:1938: // Only show the controls if
the command line switch is activated.
On 2014/11/06 19:30:06, Dan Beam wrote:
> // or the user is supervised.

Done.

https://codereview.chromium.org/697693004/diff/20001/chrome/browser/resources...
chrome/browser/resources/history/history.js:1945: if
(!loadTimeData.getBoolean('allowDeletingHistory')) {
On 2014/11/06 19:30:06, Dan Beam wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/697693004/diff/20001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_constants.h (right):

https://codereview.chromium.org/697693004/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_constants.h:17: extern const char
kAllowDeletingBrowserHistory[];
On 2014/11/06 22:59:36, Bernhard Bauer wrote:
> Keeps these sorted please.

Done.

https://codereview.chromium.org/697693004/diff/20001/chrome/browser/ui/toolba...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/697693004/diff/20001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/wrench_menu_model.cc:524: return
IncognitoModePrefs::GetAvailability(browser_->profile()->GetPrefs()) !=
On 2014/11/06 19:08:03, msw wrote:
> I thought there was some older discussion to making this the canonical way to
> check for incognito availability, so that guest session profiles would return
> false for that, and you could avoid the check above.

Right now, this returns ENABLED for guest profiles; see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...
and
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
According to that TODO, making IncognitoModePrefs work correctly for guests
seems to be ongoing work.

[Bernhard Bauer, 2014-11-07 11:06:18]

https://codereview.chromium.org/697693004/diff/60001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_pref_store.cc (right):

https://codereview.chromium.org/697693004/diff/60001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_pref_store.cc:47:
supervised_users::kAllowDeletingBrowserHistory,
It might make sense to also keep these alphabetized.

[Marc Treib, 2014-11-07 13:23:01]

https://codereview.chromium.org/697693004/diff/60001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_pref_store.cc (right):

https://codereview.chromium.org/697693004/diff/60001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_pref_store.cc:47:
supervised_users::kAllowDeletingBrowserHistory,
On 2014/11/07 11:06:18, Bernhard Bauer wrote:
> It might make sense to also keep these alphabetized.

Done.

[msw, 2014-11-11 08:15:48]

wrench_menu_model.cc lgtm

[Marc Treib, 2014-11-11 11:26:54]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2014-11-11 11:27:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/697693004/100001

[commit-bot: I haz the power, 2014-11-11 12:21:32]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-11 12:23:03]

Patchset 6 (id:??) landed as
https://crrev.com/6f959801c7ae3a9ff745220638032a03bbb57047
Cr-Commit-Position: refs/heads/master@{#303627}