Disable New Incognito Window command for guest profile.

Disable New Incognito Window command for guest profile.

This will disable the keyboard accelerator on all OSes and will disable
the File menu item on Mac.

BUG=413932, 103846
Committed: https://crrev.com/e01e6de4a23e8c9e53c497346d584e7f509e59f2
Cr-Commit-Position: refs/heads/master@{#297229}

[Mike Lerman, 2014-09-25 20:04:35]

mlerman@chromium.org changed reviewers:
+ msw@chromium.org

[Mike Lerman, 2014-09-25 20:04:36]

Hi Mike,

We had partially implemented that New Incognito Window would be available in
guest mode (this was done in the Hot Dog menu). We missed handling the keyboard
accelerator - this is done here.

Please review, Thanks!

Mike

[msw, 2014-09-25 21:20:06]

https://codereview.chromium.org/601283003/diff/1/chrome/browser/ui/browser_co...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/601283003/diff/1/chrome/browser/ui/browser_co...
chrome/browser/ui/browser_command_controller.cc:1032:
IncognitoModePrefs::GetAvailability(profile->GetPrefs());
It seems like this should return DISABLED for guest profiles...

https://codereview.chromium.org/601283003/diff/1/chrome/browser/ui/browser_co...
chrome/browser/ui/browser_command_controller.cc:1039: profile->GetPath() !=
ProfileManager::GetGuestProfilePath());
How and why is this condition different from the "guest_session =
profile->IsGuestSession();" immediately below?

[Mike Lerman, 2014-09-26 01:57:45]

https://codereview.chromium.org/601283003/diff/1/chrome/browser/ui/browser_co...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/601283003/diff/1/chrome/browser/ui/browser_co...
chrome/browser/ui/browser_command_controller.cc:1032:
IncognitoModePrefs::GetAvailability(profile->GetPrefs());
On 2014/09/25 21:20:06, msw wrote:
> It seems like this should return DISABLED for guest profiles...

The expectations around IncognitoModePrefs::GetAvailability() do not include
returning always for guest mode. There are times (such as tabs_api.cc:308) where
this method would prevent opening the incognito window. This would need to be
done carefully.

What I'd like to do is land a CL that addresses the current issue in time for
M39, and then land another CL that includes guest mode in the semantics of
GetAvailability() after branch point, so it has a full 6 weeks for any issues to
be reported. Thoughts?

https://codereview.chromium.org/601283003/diff/1/chrome/browser/ui/browser_co...
chrome/browser/ui/browser_command_controller.cc:1039: profile->GetPath() !=
ProfileManager::GetGuestProfilePath());
On 2014/09/25 21:20:06, msw wrote:
> How and why is this condition different from the "guest_session =
> profile->IsGuestSession();" immediately below?

They're the same. I'll use IsGuestSession().

[msw, 2014-09-26 03:45:25]

Can you find the original CL to disallow incognito for guest profiles?
(it might make sense to note that CL's issue in the BUG= as well)

https://codereview.chromium.org/601283003/diff/1/chrome/browser/ui/browser_co...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/601283003/diff/1/chrome/browser/ui/browser_co...
chrome/browser/ui/browser_command_controller.cc:1032:
IncognitoModePrefs::GetAvailability(profile->GetPrefs());
On 2014/09/26 01:57:45, Mike Lerman wrote:
> On 2014/09/25 21:20:06, msw wrote:
> > It seems like this should return DISABLED for guest profiles...
> 
> The expectations around IncognitoModePrefs::GetAvailability() do not include
> returning always for guest mode. There are times (such as tabs_api.cc:308)
where
> this method would prevent opening the incognito window. This would need to be
> done carefully.
> 
> What I'd like to do is land a CL that addresses the current issue in time for
> M39, and then land another CL that includes guest mode in the semantics of
> GetAvailability() after branch point, so it has a full 6 weeks for any issues
to
> be reported. Thoughts?

You should seek review from a profiles/guest expert, I'm just a c/b/ui/OWNERS.
It seems odd to me that we'd ad-hoc check for guest profiles in some places that
attempt to open incognito windows and not others, especially when we already
have a centralized function for checking incognito mode availability based on
profile information.

[Mike Lerman, 2014-09-26 14:15:50]

mlerman@chromium.org changed reviewers:
+ bauerb@chromium.org

[Mike Lerman, 2014-09-26 14:15:50]

Hi Mike,

The original CL is here: https://codereview.chromium.org/164333007. This was
implemented as part of general UI fixup to start implementing guest mode, and
the bug listed is the launch bug for guest mode.

My understanding of IncognitoModePrefs::GetAvailability is it's just reporting
the state of the preference, and is generally used in two ways: Both to
determine if an incognito browser can be opened now (always FORCED for guest)
and whether UI elements offering specifically an Incognito Window should be
enabled (always DISABLED for guest). This is why deciding how
profile->IsGuestSession() impacts the result is different in each calling part
of the code. We could optionally create two methods called
"CanCreateIncognitoBrowser" and "CreateIncognitoBrowserUIEnabled", that can
return one of ENABLED|DISABLED|FORCED, but that's open for discussion and
definitely won't make M39.

Re: owners, I'm the owner for Guest Mode (noms@ assigns guest-related profiles
items to me). I'm adding Bernard to this CL for his thoughts, as he seems to be
an owner of IncognitoPolicyPrefs. (Bernhard, I've worked with both you and Joao
around this, you tell me if he's a better person)

[Bernhard Bauer, 2014-09-26 15:32:33]

On 2014/09/26 14:15:50, Mike Lerman wrote:
> Hi Mike,
> 
> The original CL is here: https://codereview.chromium.org/164333007. This was
> implemented as part of general UI fixup to start implementing guest mode, and
> the bug listed is the launch bug for guest mode.
> 
> My understanding of IncognitoModePrefs::GetAvailability is it's just reporting
> the state of the preference, and is generally used in two ways: Both to
> determine if an incognito browser can be opened now (always FORCED for guest)
> and whether UI elements offering specifically an Incognito Window should be
> enabled (always DISABLED for guest). 

This behavior for guest mode seems strange. IncognitoModePrefs::GetAvailability
reports whether the user can open regular and incognito windows (AVAILABLE),
only regular windows (DISABLED), or only incognito windows (FORCED). Depending
on which one of these is allowed in guest mode, it should return exactly one of
the values. Using a different value in different contexts would be confusing.

> This is why deciding how
> profile->IsGuestSession() impacts the result is different in each calling part
> of the code. We could optionally create two methods called
> "CanCreateIncognitoBrowser" and "CreateIncognitoBrowserUIEnabled", that can
> return one of ENABLED|DISABLED|FORCED, but that's open for discussion and
> definitely won't make M39.

Are you saying you want to disable UI for incognito windows in guest mode, but
still allow incognito windows? Why exactly is that?

> Re: owners, I'm the owner for Guest Mode (noms@ assigns guest-related profiles
> items to me). I'm adding Bernard to this CL for his thoughts, as he seems to
be
> an owner of IncognitoPolicyPrefs. (Bernhard, I've worked with both you and
Joao
> around this, you tell me if he's a better person)

[Mike Lerman, 2014-09-26 15:46:33]

On 2014/09/26 15:32:33, Bernhard Bauer wrote:
> On 2014/09/26 14:15:50, Mike Lerman wrote:
> > Hi Mike,
> > 
> > The original CL is here: https://codereview.chromium.org/164333007. This was
> > implemented as part of general UI fixup to start implementing guest mode,
and
> > the bug listed is the launch bug for guest mode.
> > 
> > My understanding of IncognitoModePrefs::GetAvailability is it's just
reporting
> > the state of the preference, and is generally used in two ways: Both to
> > determine if an incognito browser can be opened now (always FORCED for
guest)
> > and whether UI elements offering specifically an Incognito Window should be
> > enabled (always DISABLED for guest). 
> 
> This behavior for guest mode seems strange.
IncognitoModePrefs::GetAvailability
> reports whether the user can open regular and incognito windows (AVAILABLE),
> only regular windows (DISABLED), or only incognito windows (FORCED). Depending
> on which one of these is allowed in guest mode, it should return exactly one
of
> the values. Using a different value in different contexts would be confusing.

This is why I suggest we leave GetAvailiability() to simply report the value of
the preference. Each context will incorporate the GuestMode state as applicable.

> 
> > This is why deciding how
> > profile->IsGuestSession() impacts the result is different in each calling
part
> > of the code. We could optionally create two methods called
> > "CanCreateIncognitoBrowser" and "CreateIncognitoBrowserUIEnabled", that can
> > return one of ENABLED|DISABLED|FORCED, but that's open for discussion and
> > definitely won't make M39.
> 
> Are you saying you want to disable UI for incognito windows in guest mode, but
> still allow incognito windows? Why exactly is that?

From a normal profile, the New Incognito Window opens a Window that's part of a
different Profile (e.g. the Profile's OffTheRecord profile), and people expect
that things like the cookie jar, etc. are separate. Since  guest mode doesn't
have an off-the-record profile, we don't want people getting having incorrect
expectations about what happens when they open a New Incognito Window. There are
also some UI niceness considerations (not having two options that do the same
thing, e.g. New Window and New Incognito Window).

Sample flow: I open a profile. Sign into gmail. Open a new incognito window. Go
to gmail.com - I'm not signed in. This is what users expect.
I open guest mode. Sign into gmail. Use the shortcut for a new incognito window.
Go to gmail.com... I'm signed in?!?

> 
> > Re: owners, I'm the owner for Guest Mode (noms@ assigns guest-related
profiles
> > items to me). I'm adding Bernard to this CL for his thoughts, as he seems to
> be
> > an owner of IncognitoPolicyPrefs. (Bernhard, I've worked with both you and
> Joao
> > around this, you tell me if he's a better person)

[Bernhard Bauer, 2014-09-26 16:00:53]

On 2014/09/26 15:46:33, Mike Lerman wrote:
> On 2014/09/26 15:32:33, Bernhard Bauer wrote:
> > On 2014/09/26 14:15:50, Mike Lerman wrote:
> > > Hi Mike,
> > > 
> > > The original CL is here: https://codereview.chromium.org/164333007. This
was
> > > implemented as part of general UI fixup to start implementing guest mode,
> and
> > > the bug listed is the launch bug for guest mode.
> > > 
> > > My understanding of IncognitoModePrefs::GetAvailability is it's just
> reporting
> > > the state of the preference, and is generally used in two ways: Both to
> > > determine if an incognito browser can be opened now (always FORCED for
> guest)
> > > and whether UI elements offering specifically an Incognito Window should
be
> > > enabled (always DISABLED for guest). 
> > 
> > This behavior for guest mode seems strange.
> IncognitoModePrefs::GetAvailability
> > reports whether the user can open regular and incognito windows (AVAILABLE),
> > only regular windows (DISABLED), or only incognito windows (FORCED).
Depending
> > on which one of these is allowed in guest mode, it should return exactly one
> of
> > the values. Using a different value in different contexts would be
confusing.
> 
> This is why I suggest we leave GetAvailiability() to simply report the value
of
> the preference. Each context will incorporate the GuestMode state as
applicable.
> 
> > 
> > > This is why deciding how
> > > profile->IsGuestSession() impacts the result is different in each calling
> part
> > > of the code. We could optionally create two methods called
> > > "CanCreateIncognitoBrowser" and "CreateIncognitoBrowserUIEnabled", that
can
> > > return one of ENABLED|DISABLED|FORCED, but that's open for discussion and
> > > definitely won't make M39.
> > 
> > Are you saying you want to disable UI for incognito windows in guest mode,
but
> > still allow incognito windows? Why exactly is that?
> 
> From a normal profile, the New Incognito Window opens a Window that's part of
a
> different Profile (e.g. the Profile's OffTheRecord profile), and people expect
> that things like the cookie jar, etc. are separate. Since  guest mode doesn't
> have an off-the-record profile, we don't want people getting having incorrect
> expectations about what happens when they open a New Incognito Window. 

OK, what this tells me is that incognito mode should simply be DISABLED, like
Mike suggested. What would be the problems with that?

[Mike Lerman, 2014-09-26 17:11:32]

On 2014/09/26 16:00:53, Bernhard Bauer wrote:
> On 2014/09/26 15:46:33, Mike Lerman wrote:
> > On 2014/09/26 15:32:33, Bernhard Bauer wrote:
> > > On 2014/09/26 14:15:50, Mike Lerman wrote:
> > > > Hi Mike,
> > > > 
> > > > The original CL is here: https://codereview.chromium.org/164333007. This
> was
> > > > implemented as part of general UI fixup to start implementing guest
mode,
> > and
> > > > the bug listed is the launch bug for guest mode.
> > > > 
> > > > My understanding of IncognitoModePrefs::GetAvailability is it's just
> > reporting
> > > > the state of the preference, and is generally used in two ways: Both to
> > > > determine if an incognito browser can be opened now (always FORCED for
> > guest)
> > > > and whether UI elements offering specifically an Incognito Window should
> be
> > > > enabled (always DISABLED for guest). 
> > > 
> > > This behavior for guest mode seems strange.
> > IncognitoModePrefs::GetAvailability
> > > reports whether the user can open regular and incognito windows
(AVAILABLE),
> > > only regular windows (DISABLED), or only incognito windows (FORCED).
> Depending
> > > on which one of these is allowed in guest mode, it should return exactly
one
> > of
> > > the values. Using a different value in different contexts would be
> confusing.
> > 
> > This is why I suggest we leave GetAvailiability() to simply report the value
> of
> > the preference. Each context will incorporate the GuestMode state as
> applicable.
> > 
> > > 
> > > > This is why deciding how
> > > > profile->IsGuestSession() impacts the result is different in each
calling
> > part
> > > > of the code. We could optionally create two methods called
> > > > "CanCreateIncognitoBrowser" and "CreateIncognitoBrowserUIEnabled", that
> can
> > > > return one of ENABLED|DISABLED|FORCED, but that's open for discussion
and
> > > > definitely won't make M39.
> > > 
> > > Are you saying you want to disable UI for incognito windows in guest mode,
> but
> > > still allow incognito windows? Why exactly is that?
> > 
> > From a normal profile, the New Incognito Window opens a Window that's part
of
> a
> > different Profile (e.g. the Profile's OffTheRecord profile), and people
expect
> > that things like the cookie jar, etc. are separate. Since  guest mode
doesn't
> > have an off-the-record profile, we don't want people getting having
incorrect
> > expectations about what happens when they open a New Incognito Window. 
> 
> OK, what this tells me is that incognito mode should simply be DISABLED, like
> Mike suggested. What would be the problems with that?

I'm not confident enough, for something I'd like to put into M39, that all calls
to GetAvailability() are intended to enable UI-Controls rather than prevent
opening the window at all. One example is
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...;
I'm not confident this wouldn't fail in guest mode if we change the semantics of
GetAvailability() to always return DISABLED for guest. There are 22 calls to
GetAvailability according to CodeSearch. What I'd prefer to do is land the patch
as is for M39, and then incorporate the change for GetAvailability() into ToT
after branch point (early next week) so the change has time to sit on Canary for
several weeks before it goes to Beta.

[Bernhard Bauer, 2014-09-27 11:19:37]

On 2014/09/26 17:11:32, Mike Lerman wrote:
> On 2014/09/26 16:00:53, Bernhard Bauer wrote:
> > On 2014/09/26 15:46:33, Mike Lerman wrote:
> > > On 2014/09/26 15:32:33, Bernhard Bauer wrote:
> > > > On 2014/09/26 14:15:50, Mike Lerman wrote:
> > > > > Hi Mike,
> > > > > 
> > > > > The original CL is here: https://codereview.chromium.org/164333007.
This
> > was
> > > > > implemented as part of general UI fixup to start implementing guest
> mode,
> > > and
> > > > > the bug listed is the launch bug for guest mode.
> > > > > 
> > > > > My understanding of IncognitoModePrefs::GetAvailability is it's just
> > > reporting
> > > > > the state of the preference, and is generally used in two ways: Both
to
> > > > > determine if an incognito browser can be opened now (always FORCED for
> > > guest)
> > > > > and whether UI elements offering specifically an Incognito Window
should
> > be
> > > > > enabled (always DISABLED for guest). 
> > > > 
> > > > This behavior for guest mode seems strange.
> > > IncognitoModePrefs::GetAvailability
> > > > reports whether the user can open regular and incognito windows
> (AVAILABLE),
> > > > only regular windows (DISABLED), or only incognito windows (FORCED).
> > Depending
> > > > on which one of these is allowed in guest mode, it should return exactly
> one
> > > of
> > > > the values. Using a different value in different contexts would be
> > confusing.
> > > 
> > > This is why I suggest we leave GetAvailiability() to simply report the
value
> > of
> > > the preference. Each context will incorporate the GuestMode state as
> > applicable.
> > > 
> > > > 
> > > > > This is why deciding how
> > > > > profile->IsGuestSession() impacts the result is different in each
> calling
> > > part
> > > > > of the code. We could optionally create two methods called
> > > > > "CanCreateIncognitoBrowser" and "CreateIncognitoBrowserUIEnabled",
that
> > can
> > > > > return one of ENABLED|DISABLED|FORCED, but that's open for discussion
> and
> > > > > definitely won't make M39.
> > > > 
> > > > Are you saying you want to disable UI for incognito windows in guest
mode,
> > but
> > > > still allow incognito windows? Why exactly is that?
> > > 
> > > From a normal profile, the New Incognito Window opens a Window that's part
> of
> > a
> > > different Profile (e.g. the Profile's OffTheRecord profile), and people
> expect
> > > that things like the cookie jar, etc. are separate. Since  guest mode
> doesn't
> > > have an off-the-record profile, we don't want people getting having
> incorrect
> > > expectations about what happens when they open a New Incognito Window. 
> > 
> > OK, what this tells me is that incognito mode should simply be DISABLED,
like
> > Mike suggested. What would be the problems with that?
> 
> I'm not confident enough, for something I'd like to put into M39, that all
calls
> to GetAvailability() are intended to enable UI-Controls rather than prevent
> opening the window at all. One example is
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...;
> I'm not confident this wouldn't fail in guest mode if we change the semantics
of
> GetAvailability() to always return DISABLED for guest. 

I can't imagine any case where we would want to do something different for the
UI than for the actual functionality. If incognito mode is disabled, it's
disabled. The UI should reflect that, because anything else would be really
confusing. In the tabs API case, I think failing is absolutely acceptable,
because incognito _is not available_ (also, do we even have extensions in guest
mode?). 

> There are 22 calls to
> GetAvailability according to CodeSearch. What I'd prefer to do is land the
patch
> as is for M39, and then incorporate the change for GetAvailability() into ToT
> after branch point (early next week) so the change has time to sit on Canary
for
> several weeks before it goes to Beta.

Okay, fair enough. Could you add a TODO to make the change to GetAvailability?

[Mike Lerman, 2014-09-29 17:32:37]

On 2014/09/27 11:19:37, Bernhard Bauer wrote:
> On 2014/09/26 17:11:32, Mike Lerman wrote:
> > On 2014/09/26 16:00:53, Bernhard Bauer wrote:
> > > On 2014/09/26 15:46:33, Mike Lerman wrote:
> > > > On 2014/09/26 15:32:33, Bernhard Bauer wrote:
> > > > > On 2014/09/26 14:15:50, Mike Lerman wrote:
> > > > > > Hi Mike,
> > > > > > 
> > > > > > The original CL is here: https://codereview.chromium.org/164333007.
> This
> > > was
> > > > > > implemented as part of general UI fixup to start implementing guest
> > mode,
> > > > and
> > > > > > the bug listed is the launch bug for guest mode.
> > > > > > 
> > > > > > My understanding of IncognitoModePrefs::GetAvailability is it's just
> > > > reporting
> > > > > > the state of the preference, and is generally used in two ways: Both
> to
> > > > > > determine if an incognito browser can be opened now (always FORCED
for
> > > > guest)
> > > > > > and whether UI elements offering specifically an Incognito Window
> should
> > > be
> > > > > > enabled (always DISABLED for guest). 
> > > > > 
> > > > > This behavior for guest mode seems strange.
> > > > IncognitoModePrefs::GetAvailability
> > > > > reports whether the user can open regular and incognito windows
> > (AVAILABLE),
> > > > > only regular windows (DISABLED), or only incognito windows (FORCED).
> > > Depending
> > > > > on which one of these is allowed in guest mode, it should return
exactly
> > one
> > > > of
> > > > > the values. Using a different value in different contexts would be
> > > confusing.
> > > > 
> > > > This is why I suggest we leave GetAvailiability() to simply report the
> value
> > > of
> > > > the preference. Each context will incorporate the GuestMode state as
> > > applicable.
> > > > 
> > > > > 
> > > > > > This is why deciding how
> > > > > > profile->IsGuestSession() impacts the result is different in each
> > calling
> > > > part
> > > > > > of the code. We could optionally create two methods called
> > > > > > "CanCreateIncognitoBrowser" and "CreateIncognitoBrowserUIEnabled",
> that
> > > can
> > > > > > return one of ENABLED|DISABLED|FORCED, but that's open for
discussion
> > and
> > > > > > definitely won't make M39.
> > > > > 
> > > > > Are you saying you want to disable UI for incognito windows in guest
> mode,
> > > but
> > > > > still allow incognito windows? Why exactly is that?
> > > > 
> > > > From a normal profile, the New Incognito Window opens a Window that's
part
> > of
> > > a
> > > > different Profile (e.g. the Profile's OffTheRecord profile), and people
> > expect
> > > > that things like the cookie jar, etc. are separate. Since  guest mode
> > doesn't
> > > > have an off-the-record profile, we don't want people getting having
> > incorrect
> > > > expectations about what happens when they open a New Incognito Window. 
> > > 
> > > OK, what this tells me is that incognito mode should simply be DISABLED,
> like
> > > Mike suggested. What would be the problems with that?
> > 
> > I'm not confident enough, for something I'd like to put into M39, that all
> calls
> > to GetAvailability() are intended to enable UI-Controls rather than prevent
> > opening the window at all. One example is
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...;
> > I'm not confident this wouldn't fail in guest mode if we change the
semantics
> of
> > GetAvailability() to always return DISABLED for guest. 
> 
> I can't imagine any case where we would want to do something different for the
> UI than for the actual functionality. If incognito mode is disabled, it's
> disabled. The UI should reflect that, because anything else would be really
> confusing. In the tabs API case, I think failing is absolutely acceptable,
> because incognito _is not available_ (also, do we even have extensions in
guest
> mode?). 
> 
> > There are 22 calls to
> > GetAvailability according to CodeSearch. What I'd prefer to do is land the
> patch
> > as is for M39, and then incorporate the change for GetAvailability() into
ToT
> > after branch point (early next week) so the change has time to sit on Canary
> for
> > several weeks before it goes to Beta.
> 
> Okay, fair enough. Could you add a TODO to make the change to GetAvailability?

Bernhard and I chatted offline.

TODO added.

[Bernhard Bauer, 2014-09-29 18:00:09]

lgtm

[msw, 2014-09-29 18:08:10]

rubber stamp lgtm

[Mike Lerman, 2014-09-29 18:20:13]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-09-29 18:21:45]

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

[commit-bot: I haz the power, 2014-09-29 19:26:54]

Committed patchset #3 (id:40001) as 88c80c8772bcbe4952ce6efd2cef1c12eae4eb40

[commit-bot: I haz the power, 2014-09-29 19:27:58]

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