Add elevation to the managed mode navigation observer.

Add elevation to the managed mode navigation observer.

This changelist adds an elevation flag to the managed mode navigation observer
which can be used to save elevation for a certain WebContents. When navigation to a page which is not the Webstore, Extensions, Settings or History page, the elevation will be reset automatically.

BUG=222364
TEST=Manual


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

[Adrian Kuegel, 2013-03-20 13:50:31]

Bernhard, can you please review this changelist?

[Bernhard Bauer, 2013-03-20 15:08:30]

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right):

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:525: 
Is this newline necessary?

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_mode_navigation_observer.h (right):

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_mode_navigation_observer.h:47: void
SetElevated(bool is_elevated);
If this method only changes the flag, it's okay (and in fact more consistent
with the method above) to name it in hacker_style.

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_user_service.cc (right):

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_service.cc:119: // TODO(akuegel): This
should be deprecated.
Why don't you just go ahead and mark it as deprecated then? That still won't
keep anyone from using it, but I think we are the only users anyway.

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_service.cc:399: void
ManagedUserService::SetElevatedForWebContents(
I'm not convinced that adding this method is very useful over just setting the
flag directly on the ManagedModeNavigationObserver, but I'd need to see callers
to fully judge it

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_user_service.h (right):

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_service.h:46: bool IsElevated() const;
Aaand now this is getting confusing. How do per-profile elevation and per-tab
elevation interact? Do we even still want per-profile elevation anymore?

[Adrian Kuegel, 2013-03-20 15:51:16]

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right):

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:525: 
No, that was a leftover from copying the new method up. I will remove it.

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_mode_navigation_observer.h (right):

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_mode_navigation_observer.h:47: void
SetElevated(bool is_elevated);
On 2013/03/20 15:08:30, Bernhard Bauer wrote:
> If this method only changes the flag, it's okay (and in fact more consistent
> with the method above) to name it in hacker_style.

Done.

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_user_service.cc (right):

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_service.cc:119: // TODO(akuegel): This
should be deprecated.
On 2013/03/20 15:08:30, Bernhard Bauer wrote:
> Why don't you just go ahead and mark it as deprecated then? That still won't
> keep anyone from using it, but I think we are the only users anyway.

Done.

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_service.cc:399: void
ManagedUserService::SetElevatedForWebContents(
True, that would work, too. The only (very) small disadvantage is that I need
one more line for the include file at each call site, otherwise it should
basically result in the same number of lines. So I will remove this method.

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_user_service.h (right):

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_service.h:46: bool IsElevated() const;
No, we don't want that anymore, but I guess if I change all call sites at the
same time, the changelist will become quite big. So I would leave the method
here until all call sites use the new method.

[Bernhard Bauer, 2013-03-20 15:57:17]

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_user_service.cc (right):

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_service.cc:119: // TODO(akuegel): This
should be deprecated.
On 2013/03/20 15:51:16, Adrian Kuegel wrote:
> On 2013/03/20 15:08:30, Bernhard Bauer wrote:
> > Why don't you just go ahead and mark it as deprecated then? That still won't
> > keep anyone from using it, but I think we are the only users anyway.
> 
> Done.

Ideally, add "Use Foo() instead".

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_user_service.h (right):

https://codereview.chromium.org/12413028/diff/2001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_service.h:46: bool IsElevated() const;
On 2013/03/20 15:51:16, Adrian Kuegel wrote:
> No, we don't want that anymore, but I guess if I change all call sites at the
> same time, the changelist will become quite big. So I would leave the method
> here until all call sites use the new method.

OK, but please add a deprecation message in that case.

[Adrian Kuegel, 2013-03-20 16:18:35]

I added a deprecation comment for the IsElevated method, too. The SetElevated
method is probably still needed, otherwise one of the unit tests would not work
anymore, but we could rename it back to SetElevatedForTesting once all other
callers are changed.

[Bernhard Bauer, 2013-03-20 17:28:00]

On 2013/03/20 16:18:35, Adrian Kuegel wrote:
> I added a deprecation comment for the IsElevated method, too. The SetElevated
> method is probably still needed, otherwise one of the unit tests would not
work
> anymore, but we could rename it back to SetElevatedForTesting once all other
> callers are changed.

Which one is that? Couldn't we get the same behavior by not setting a
passphrase?

https://codereview.chromium.org/12413028/diff/16001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_service.cc (right):

https://codereview.chromium.org/12413028/diff/16001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.cc:103: // Deprecated. Use
IsElevatedForWebContents() instead.
In the header please, that way people are more likely to see it.

(Yes, I know that in this case, "people" probably means one of us anyway, but
consider it a learning exercise ;-) )

[Adrian Kuegel, 2013-03-21 09:40:52]

On 2013/03/20 17:28:00, Bernhard Bauer wrote:
> Which one is that? Couldn't we get the same behavior by not setting a
> passphrase?

It is the ManagedUserServiceExtensionTest.InstallContentPacks unit test.
Possibly we can get it working by not setting a passphrase when I add an
RequestAuthorization call to the unpacked installer. I probably have to do that,
anyway, although I haven't looked at it yet where it is usually used.

https://codereview.chromium.org/12413028/diff/16001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_service.cc (right):

https://codereview.chromium.org/12413028/diff/16001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.cc:103: // Deprecated. Use
IsElevatedForWebContents() instead.
On 2013/03/20 17:28:00, Bernhard Bauer wrote:
> In the header please, that way people are more likely to see it.
> 
> (Yes, I know that in this case, "people" probably means one of us anyway, but
> consider it a learning exercise ;-) )

Done.

[Adrian Kuegel, 2013-03-21 14:58:40]

I added the automatic reset of elevation. Can you please take a look at that?

[Bernhard Bauer, 2013-03-21 15:52:58]

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right):

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:370: const char
kChromeWebstoreURL[] = "chrome.google.com/webstore/";
1. Move constant declarations to the top of the file.
2. Const variables automatically get internal linkage, so an anonymous namespace
isn't necessary.
3. Isn't this declared as a constant somewhere?

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:381:
navigation_url.path();
Sorry, I don't understand this. I'm also thinking there must be a way to do this
that is less string-fiddly. Doesn't url_constants.h also declare just hostnames
for the chrome:// URLs? Could you just check those?

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:384: // the
special url occurs at position 0 as a substring of url.
base/string_util.h declares StartsWith() methods.

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:387:
url.find(chrome::kChromeUIHistoryFrameURL) == 0 ||
If we only call this method for main frames, why do we need to check for the
subframe URLs?

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:471: void
ManagedModeNavigationObserver::DidStartProvisionalLoadForFrame(
We can actually remove the whole method if we're not overriding it.

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:484: if
(!WillStayElevatedForURL(url))
Do we actually need both of these checks?

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_navigation_observer.h (right):

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.h:115: // Returns
whether the user would stay in elevated state if he visits this
Naming nit: "ShouldStayElevated"?

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_service.h (right):

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.h:98: bool IsPassphraseEmpty()
const;
Where do we need this method?

[Adrian Kuegel, 2013-03-21 16:30:04]

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right):

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:370: const char
kChromeWebstoreURL[] = "chrome.google.com/webstore/";
1. Done
2. Still some reviewers (for example Jochen) want to have it in an anonymous
namespace. But I can remove it if you want.
3. Yes, it is, and also in many places (usually with a more specific URL). In
one case it is the URL I need, but it is named GalleryBrowsePrefix (it is in the
file extension_constants.h). Should I use that one instead?

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:381:
navigation_url.path();
True, I can use the host. Thanks for the tip :)

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:384: // the
special url occurs at position 0 as a substring of url.
Ok, I used that instead. It is probably easier to read that way.

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:387:
url.find(chrome::kChromeUIHistoryFrameURL) == 0 ||
A user could enter the subframe URL manually. But I guess we don't need to
handle that, so I removed that now.

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:471: void
ManagedModeNavigationObserver::DidStartProvisionalLoadForFrame(
That is what I thought, too, but I thought maybe Sergiu still needs it for his
latest changes? Anyway, I removed it now.

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:484: if
(!WillStayElevatedForURL(url))
Yes. There are two cases: we do a real navigation, then
ProvisionalChangeToMainFrameURL is called before the interstitial is shown (so
we can still make sure to remove the elevation in time). The other case is: we
are on one of these pages with the uber frame, and navigate there (for example
to the help page). Then, only the other method is called, but this does not
bother us because we don't show an interstitial on chrome pages, anyway (still
we want to remove the elevation).

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_navigation_observer.h (right):

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.h:115: // Returns
whether the user would stay in elevated state if he visits this
On 2013/03/21 15:52:58, Bernhard Bauer wrote:
> Naming nit: "ShouldStayElevated"?

Done.

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_service.h (right):

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.h:98: bool IsPassphraseEmpty()
const;
In CanSkipPassphraseDialog. But I guess we can make it a private method. I
changed it like that.

[Bernhard Bauer, 2013-03-22 09:24:28]

LGTM w/ nits:

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right):

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:370: const char
kChromeWebstoreURL[] = "chrome.google.com/webstore/";
On 2013/03/21 16:30:04, Adrian Kuegel wrote:
> 1. Done
> 2. Still some reviewers (for example Jochen) want to have it in an anonymous
> namespace. But I can remove it if you want.

Meh. My take on this: if there is nothing else that warrants an anonymous
namespace, adding one is just unnecessary noise. If there already is one, it's
fine to add it inside.

> 3. Yes, it is, and also in many places (usually with a more specific URL). In
> one case it is the URL I need, but it is named GalleryBrowsePrefix (it is in
the
> file extension_constants.h). Should I use that one instead?

I'll have to see how the end result with the host checks look, but using an
already-existing constant would definitely be preferable.

https://codereview.chromium.org/12413028/diff/34001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right):

https://codereview.chromium.org/12413028/diff/34001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:387: // Check if
any of the special urls is a prefix of url.
Nit: it's "URL" (plural URLs) if you talk about an actual URL, and |url| if you
talk about the variable. Otherwise it reads a bit weird.

https://codereview.chromium.org/12413028/diff/34001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:389:
StartsWithASCII(url, chrome::kChromeUIExtensionsHost, false) ||
Nit: Might be nicer to align these?

[Adrian Kuegel, 2013-03-22 09:46:21]

Bernhard, do you want to take another look before I commit?

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right):

https://codereview.chromium.org/12413028/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:370: const char
kChromeWebstoreURL[] = "chrome.google.com/webstore/";
Ok, I am using this constant now.

https://codereview.chromium.org/12413028/diff/34001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right):

https://codereview.chromium.org/12413028/diff/34001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:387: // Check if
any of the special urls is a prefix of url.
On 2013/03/22 09:24:28, Bernhard Bauer wrote:
> Nit: it's "URL" (plural URLs) if you talk about an actual URL, and |url| if
you
> talk about the variable. Otherwise it reads a bit weird.

Done.

https://codereview.chromium.org/12413028/diff/34001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:389:
StartsWithASCII(url, chrome::kChromeUIExtensionsHost, false) ||
On 2013/03/22 09:24:28, Bernhard Bauer wrote:
> Nit: Might be nicer to align these?

Done.

[commit-bot: I haz the power, 2013-03-25 10:14:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akuegel@chromium.org/12413028/48001

[commit-bot: I haz the power, 2013-03-25 12:01:48]

Change committed as 190372