Start in elevated mode when creating a new managed user profile.

Start in elevated mode when creating a new managed user profile.

BUG=222364
TEST=browser_tests


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

[Adrian Kuegel, 2013-04-08 09:54:13]

Bernhard, can you please review this changelist?

[Bernhard Bauer, 2013-04-08 12:26:09]

https://chromiumcodereview.appspot.com/13778004/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/managed_user_settings_handler.cc (right):

https://chromiumcodereview.appspot.com/13778004/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/managed_user_settings_handler.cc:58:
passphrase_empty)) {
Why are we checking for an empty passphrase when kResetLocalPassphrase is set?

https://chromiumcodereview.appspot.com/13778004/diff/1/chrome/common/pref_nam...
File chrome/common/pref_names.cc (right):

https://chromiumcodereview.appspot.com/13778004/diff/1/chrome/common/pref_nam...
chrome/common/pref_names.cc:41: const char kManagedModeHasSeenSetupDialog[] =
"managed_mode.setup_seen";
For least surprise, it would be good if the string value could be inferred from
looking at the constant, i.e. either call the constant kManagedModeSetupSeen or
set the value to "managed_mode.has_seen_setup_dialog".

https://chromiumcodereview.appspot.com/13778004/diff/1/chrome/common/pref_nam...
File chrome/common/pref_names.h (right):

https://chromiumcodereview.appspot.com/13778004/diff/1/chrome/common/pref_nam...
chrome/common/pref_names.h:25: extern const char
kManagedModeHasSeenSetupDialog[];
I'm not very happy using "managed mode" in code (we have managed profiles, but
managed mode is deprecated), but I guess it would be better to change these at
once.

[Adrian Kuegel, 2013-04-08 14:31:04]

I uploaded a new patch. It turns out that the didClosePage method in the
javascript does what we want, it is called whenever the page is closed by
clicking ok, the user navigated to another page, or the tab is closed. So I used
that now to call the SaveMetrics method, which also resets the
ManagedModeHasSeenSetupDialog pref.
Also I rewrote the browser tests for the settings page, so that it checks that
the page is unlocked the first time, but locked the second time it is opened. I
removed the second test because it would be mostly duplicate code, and it was
more meaningful at the time when having no passphrase would unlock the page
automatically.

https://chromiumcodereview.appspot.com/13778004/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/managed_user_settings_handler.cc (right):

https://chromiumcodereview.appspot.com/13778004/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/managed_user_settings_handler.cc:58:
passphrase_empty)) {
This method is called whenever the settings frame is reloaded, so if I don't
check for the passphrase to be empty, it could happen that a new passphrase was
already set, but still the settings page is opened in authenticated state the
next time during the same browser run.

https://chromiumcodereview.appspot.com/13778004/diff/1/chrome/common/pref_nam...
File chrome/common/pref_names.cc (right):

https://chromiumcodereview.appspot.com/13778004/diff/1/chrome/common/pref_nam...
chrome/common/pref_names.cc:41: const char kManagedModeHasSeenSetupDialog[] =
"managed_mode.setup_seen";
On 2013/04/08 12:26:09, Bernhard Bauer wrote:
> For least surprise, it would be good if the string value could be inferred
from
> looking at the constant, i.e. either call the constant kManagedModeSetupSeen
or
> set the value to "managed_mode.has_seen_setup_dialog".

Done.

https://chromiumcodereview.appspot.com/13778004/diff/1/chrome/common/pref_nam...
File chrome/common/pref_names.h (right):

https://chromiumcodereview.appspot.com/13778004/diff/1/chrome/common/pref_nam...
chrome/common/pref_names.h:25: extern const char
kManagedModeHasSeenSetupDialog[];
What prefix would you suggest instead? ManagedUser?
But I guess you are right, in order to have it consistent it should all be
changed, which should probably belong to a separate changelist.

[Bernhard Bauer, 2013-04-09 11:41:22]

On 2013/04/08 14:31:04, Adrian Kuegel wrote:
> I uploaded a new patch. It turns out that the didClosePage method in the
> javascript does what we want, it is called whenever the page is closed by
> clicking ok, the user navigated to another page, or the tab is closed. So I
used
> that now to call the SaveMetrics method, which also resets the
> ManagedModeHasSeenSetupDialog pref.

Wait, what happens for example if the process is killed? I guess we could say
we're fine with that, but it still seems weird to trigger this from Javascript.

https://codereview.chromium.org/13778004/diff/1/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/13778004/diff/1/chrome/common/pref_names.h#ne...
chrome/common/pref_names.h:25: extern const char
kManagedModeHasSeenSetupDialog[];
On 2013/04/08 14:31:04, Adrian Kuegel wrote:
> What prefix would you suggest instead? ManagedUser?
> But I guess you are right, in order to have it consistent it should all be
> changed, which should probably belong to a separate changelist.

Yes, ManagedUser (managed_user.X in the actual string) would probably be best,
but as the preferences get grouped by the dot-prefix anyway, it makes more sense
to do that separately.

[Adrian Kuegel, 2013-04-09 12:34:45]

On 2013/04/09 11:41:22, Bernhard Bauer wrote:
> On 2013/04/08 14:31:04, Adrian Kuegel wrote:
> > I uploaded a new patch. It turns out that the didClosePage method in the
> > javascript does what we want, it is called whenever the page is closed by
> > clicking ok, the user navigated to another page, or the tab is closed. So I
> used
> > that now to call the SaveMetrics method, which also resets the
> > ManagedModeHasSeenSetupDialog pref.
> 
> Wait, what happens for example if the process is killed? I guess we could say
> we're fine with that, but it still seems weird to trigger this from
Javascript.

This method seems to be called even if the browser is killed. Not sure if we can
rely on that, though, I just tested it manually.
Anyway, we need to trigger this from Javascript as far as I know, since the
handler is not called automatically when a overlay page is closed. The question
is, if we need to call it also from the Destructor of the handler. If yes, I
would have to prevent that the metrics are saved twice.

[Bernhard Bauer, 2013-04-09 13:28:36]

On 2013/04/09 12:34:45, Adrian Kuegel wrote:
> On 2013/04/09 11:41:22, Bernhard Bauer wrote:
> > On 2013/04/08 14:31:04, Adrian Kuegel wrote:
> > > I uploaded a new patch. It turns out that the didClosePage method in the
> > > javascript does what we want, it is called whenever the page is closed by
> > > clicking ok, the user navigated to another page, or the tab is closed. So
I
> > used
> > > that now to call the SaveMetrics method, which also resets the
> > > ManagedModeHasSeenSetupDialog pref.
> > 
> > Wait, what happens for example if the process is killed? I guess we could
say
> > we're fine with that, but it still seems weird to trigger this from
> Javascript.
> 
> This method seems to be called even if the browser is killed. Not sure if we
can
> rely on that, though, I just tested it manually.
> Anyway, we need to trigger this from Javascript as far as I know, since the
> handler is not called automatically when a overlay page is closed. The
question
> is, if we need to call it also from the Destructor of the handler. If yes, I
> would have to prevent that the metrics are saved twice.

Well, can't you *just* call it from the destructor?

[Adrian Kuegel, 2013-04-09 13:38:12]

On 2013/04/09 13:28:36, Bernhard Bauer wrote:
> Well, can't you *just* call it from the destructor?

This may work for resetting the preference, but not for SaveMetrics, right? I
need to think it through if there is a case where resetting the preference in
the destructor is not desirable. For example, can we be sure that the destructor
is called before a new instance of the WebUIHandler is generated? Imagine this
case: the custodian closed the managed user settings dialog, but kept the
settings page open. Now the managed user opens a new tab and opens the settings
page. I think he would see the settings page unlocked, but I will check it now.

[Adrian Kuegel, 2013-04-09 14:12:47]

I just tried it, and it is really as I expected: the destructor is not called
when closing the managed user settings page, it is only called when you navigate
away from the settings frame (or close the tab, etc.). So we cannot use the
destructor for that purpose to reset the preference (and also not for calling
SaveMetrics).

[Adrian Kuegel, 2013-04-10 09:04:59]

As we discussed offline, I use now a boolean flag in the ManagedUserService to
store if it is the first run for the profile. With this approach, the
ManagedUserSettingsBrowsertest doesn't have to be changed, but I modified the
Startup test to check for elevation.
I kept the preference item in order to make the UMA stats work correctly.

[Bernhard Bauer, 2013-04-10 09:30:01]

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

https://codereview.chromium.org/13778004/diff/18001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:274:
managed_user_service_->startup_elevation()) {
I would rather do this when we know that the settings page is opened, i.e.
somewhere in the WebUI handler. Otherwise it might happen that we elevate some
unrelated WebContents.

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

https://codereview.chromium.org/13778004/diff/18001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.h:107: void
AddElevationForURL(const GURL& url);
These method declarations are not unnecessary (and not implemented :-D ).

https://codereview.chromium.org/13778004/diff/18001/chrome/browser/ui/startup...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/13778004/diff/18001/chrome/browser/ui/startup...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:936: LOG(INFO) <<
is_first_run_;
Remove logging.

https://codereview.chromium.org/13778004/diff/18001/chrome/browser/ui/startup...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:943:
profile_)->set_startup_elevation(true);
This looks kinda ugly. Can you extract the ManagedUserService to a local
variable?

[Adrian Kuegel, 2013-04-10 09:47:01]

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

https://codereview.chromium.org/13778004/diff/18001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:274:
managed_user_service_->startup_elevation()) {
On 2013/04/10 09:30:03, Bernhard Bauer wrote:
> I would rather do this when we know that the settings page is opened, i.e.
> somewhere in the WebUI handler. Otherwise it might happen that we elevate some
> unrelated WebContents.

Done.

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

https://codereview.chromium.org/13778004/diff/18001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.h:107: void
AddElevationForURL(const GURL& url);
I had already implemented them, but forgot to remove them when we discussed that
having a flag is better. I removed them now.

https://codereview.chromium.org/13778004/diff/18001/chrome/browser/ui/startup...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/13778004/diff/18001/chrome/browser/ui/startup...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:936: LOG(INFO) <<
is_first_run_;
On 2013/04/10 09:30:03, Bernhard Bauer wrote:
> Remove logging.

Done.

https://codereview.chromium.org/13778004/diff/18001/chrome/browser/ui/startup...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:943:
profile_)->set_startup_elevation(true);
On 2013/04/10 09:30:03, Bernhard Bauer wrote:
> This looks kinda ugly. Can you extract the ManagedUserService to a local
> variable?

Done.

[Adrian Kuegel, 2013-04-10 11:52:38]

I realized that I don't need the user preference anymore for the UMA, I can do
it with another boolean flag instead. I also fixed the BrowserTest so that it
waits for the page to be loaded. Can you please take another look?

[Bernhard Bauer, 2013-04-10 12:17:12]

https://codereview.chromium.org/13778004/diff/47001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/managed_user_settings_handler.cc (right):

https://codereview.chromium.org/13778004/diff/47001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/managed_user_settings_handler.cc:140:
has_seen_settings_dialog_ = false;
Why do you set this to false again?

[Adrian Kuegel, 2013-04-10 12:22:42]

https://codereview.chromium.org/13778004/diff/47001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/managed_user_settings_handler.cc (right):

https://codereview.chromium.org/13778004/diff/47001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/managed_user_settings_handler.cc:140:
has_seen_settings_dialog_ = false;
Oh, the bug is in fact in the constructor. I should set it to true there. And I
even wrote it in the documentation how it should work, yet I made the bug :-(
I fixed it now.

[Bernhard Bauer, 2013-04-10 12:26:17]

https://codereview.chromium.org/13778004/diff/52001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/managed_user_settings_handler.cc (right):

https://codereview.chromium.org/13778004/diff/52001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/managed_user_settings_handler.cc:140:
has_seen_settings_dialog_ = false;
Hmm... I'm wondering if it would make more sense to initialize the value to
false, then set if to true in an else branch here. That way, we won't have a
true -> false -> true transition.

[Adrian Kuegel, 2013-04-10 12:34:55]

https://codereview.chromium.org/13778004/diff/52001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/managed_user_settings_handler.cc (right):

https://codereview.chromium.org/13778004/diff/52001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/managed_user_settings_handler.cc:140:
has_seen_settings_dialog_ = false;
Yes, SGTM. I changed it like that.

[Bernhard Bauer, 2013-04-10 12:48:16]

LGTM!

[Adrian Kuegel, 2013-04-10 12:52:14]

James, can you please give me approval for the files in
chrome/browser/resources/options/* and chrome/browser/ui/
* ?

[James Hawkins, 2013-04-10 16:16:15]

lgtm

https://codereview.chromium.org/13778004/diff/55003/chrome/browser/ui/startup...
File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right):

https://codereview.chromium.org/13778004/diff/55003/chrome/browser/ui/startup...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:941: // And the
managed user should be in elevated state.
// Managed user should...

[commit-bot: I haz the power, 2013-04-10 16:27:40]

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

[commit-bot: I haz the power, 2013-04-10 19:50:22]

Change committed as 193433