Don't open the 'stats for nerds' in a non-incognito window when incognito is forced.

Don't open the 'stats for nerds' in a non-incognito window when incognito is forced.

Incognito mode may be always-on by configuration policy. When that is set then
the TaskManager shouldn't open a new non-incognito window.

The TaskManager doesn't have an associated Profile, so it uses the last used
Profile. This change introduces a getter for the last used allowed Profile,
which returns the OTR Profile if that's dictated by policy.

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

[Joao da Silva, 2013-05-29 12:58:50]

Please have a look:

@mirandac: profiles/
@yoshiki: task_manager/

Miranda, I'm not sure if this is an appropriate getter to add in ProfileManager.
I think that, in cases like this, it shouldn't be the TaskManager that checks
which Profile is appropriate to open the new window. Doing the checks behind the
scenes in FindOrCreateTabbedBrowser() or OpenNewWindow() is also not
appropriate, because some URLs should never open in incognito (e.g. settings
UI). So my best guess is adding it next to GetLastUsedProfile(), but I welcome
any advice you may have :-)

[Miranda Callahan, 2013-05-29 13:41:04]

On 2013/05/29 12:58:50, Joao da Silva wrote:
> Please have a look:
> 
> @mirandac: profiles/
> @yoshiki: task_manager/
> 
> Miranda, I'm not sure if this is an appropriate getter to add in
ProfileManager.
> I think that, in cases like this, it shouldn't be the TaskManager that checks
> which Profile is appropriate to open the new window. Doing the checks behind
the
> scenes in FindOrCreateTabbedBrowser() or OpenNewWindow() is also not
> appropriate, because some URLs should never open in incognito (e.g. settings
> UI). So my best guess is adding it next to GetLastUsedProfile(), but I welcome
> any advice you may have :-)

I actually think this check makes sense in ProfileManager, yes, but I have a few
other questions:

What do you think about doing this not as a separate function, but instead as a
BOOL parameter to GetLastUsedProfile? This would force future users to think
carefully about which version they want to use -- otherwise I'm afraid that this
function may be ignored or not used correctly by future coders. I know this
would be a bigger CL; but what do you think? 

If you do decide to keep the separate function instead, maybe add a comment to
GetLastUsedProfile pointing future devs to your new method if they're using it
to open browser windows. And perhaps tweak the name slightly, to
GetLastUsedProfileAllowedByPolicy -- so that the "policy" bit becomes clear?

Another question: Do you think any other of the many existing uses of
GetLastUsedProfile also need to be changed to this new policy-aware version?

[Joao da Silva, 2013-05-29 14:59:54]

> What do you think about doing this not as a separate function, but instead as
a
> BOOL parameter to GetLastUsedProfile? This would force future users to think
> carefully about which version they want to use -- otherwise I'm afraid that
this
> function may be ignored or not used correctly by future coders. I know this
> would be a bigger CL; but what do you think? 
> 
> If you do decide to keep the separate function instead, maybe add a comment to
> GetLastUsedProfile pointing future devs to your new method if they're using it
> to open browser windows. And perhaps tweak the name slightly, to
> GetLastUsedProfileAllowedByPolicy -- so that the "policy" bit becomes clear?
> 

Done both.

> Another question: Do you think any other of the many existing uses of
> GetLastUsedProfile also need to be changed to this new policy-aware version?

I found 18 callers of this outside ProfileManager and tests, and they aren't
used to open new windows except for the task_manager. That's why I'd rather do
this for now than change those 18 callers, as suggested above.

We also introduced a CHECK when new browser windows are created using a
disallowed profile type, so we're positive that we'll find any potential
offenders :-)

[Miranda Callahan, 2013-05-29 15:10:51]

On 2013/05/29 14:59:54, Joao da Silva wrote:
> > What do you think about doing this not as a separate function, but instead
as
> a
> > BOOL parameter to GetLastUsedProfile? This would force future users to think
> > carefully about which version they want to use -- otherwise I'm afraid that
> this
> > function may be ignored or not used correctly by future coders. I know this
> > would be a bigger CL; but what do you think? 
> > 
> > If you do decide to keep the separate function instead, maybe add a comment
to
> > GetLastUsedProfile pointing future devs to your new method if they're using
it
> > to open browser windows. And perhaps tweak the name slightly, to
> > GetLastUsedProfileAllowedByPolicy -- so that the "policy" bit becomes clear?
> > 
> 
> Done both.
> 
> > Another question: Do you think any other of the many existing uses of
> > GetLastUsedProfile also need to be changed to this new policy-aware version?
> 
> I found 18 callers of this outside ProfileManager and tests, and they aren't
> used to open new windows except for the task_manager. That's why I'd rather do
> this for now than change those 18 callers, as suggested above.
> 
> We also introduced a CHECK when new browser windows are created using a
> disallowed profile type, so we're positive that we'll find any potential
> offenders :-)

Ah, that's great that there's a CHECK -- super. :-) Thanks, and LGTM!

[yoshiki, 2013-05-29 16:08:42]

task_manager lgtm

[commit-bot: I haz the power, 2013-05-30 00:12:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/16072014/3002

[commit-bot: I haz the power, 2013-05-30 12:31:00]

Change committed as 203128