Initial Managed Mode extension API, supporting querying the setting and a stub for enabling the mod…

Initial Managed Mode extension API, supporting querying the setting and a stub for enabling the mode.

BUG=115447, 115448
TEST=covered by automated tests (browser_tests --gtest_filter=ExtensionApiTest.ManagedMode*)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125218

[Pam (message me for reviews), 2012-03-01 14:07:27]

Bernhard - please review, thanks
Aaron - please review extensions/* as desired for owner approval

The profile_manager.cc changes are copied from Bernhard's CL, included here
because this CL depends on them too. I'll remove them once he submits his patch.

I'm having trouble with the extension doc generator, but didn't want to block
review for that. I'll make sure the experimental.managedMode.html file is right
before submitting this patch.

To pre-reply to a potential comment, I think it's important that this API have
its own permission and message. My concern is that otherwise an extension could
silently use "managed mode is enabled" as a proxy for "the user is naive; let's
take advantage of them".

Thanks,
- Pam

[Bernhard Bauer, 2012-03-01 16:32:14]

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_managed_mode_api.cc (right):

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_managed_mode_api.cc:13: #include
"chrome/browser/prefs/pref_service.h"
Nit: Remove one empty line here, but add empty lines between the namespace
declaration? :-)

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_managed_mode_api.cc:30: if
(!ValidateBrowserPref(kManagedModeEnabledKey, &browser_pref))
What does it mean when this check fails? The key is a constant, so it should be
fine, and the ExtensionFunctionDispatcher (I think) should already check whether
we have the permission (we only have to do it manually for other preference
functions, because they're used by ChromeSettings with different permissions).

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_managed_mode_api.cc:33: bool enabled =
local_state->GetBoolean(browser_pref.c_str());
Hm, thinking about it, we could directly use the pref key here, no?

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_managed_mode_api.cc:39: std::string
level_of_control = enabled ? keys::kNotControllable
I'm not sure if this makes anything clearer to the extension :-D The mechanism
for setting local state prefs works differently than for user prefs in any case
(as there is no controlling extension in that sense).

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_managed_mode_api.h (right):

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_managed_mode_api.h:17: virtual
~GetManagedModeFunction();
I don't think you need to declare the destructor again.

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_preference_api.h (right):

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.h:73: virtual
~PreferenceFunction();
Nit: new line?

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.h:78: // modify that pref.
Can you add a comment that it sets an error if the extension doesn't have the
API permission?

http://codereview.chromium.org/9566007/diff/7001/chrome/common/extensions/api...
File chrome/common/extensions/api/experimental.managedMode.json (right):

http://codereview.chromium.org/9566007/diff/7001/chrome/common/extensions/api...
chrome/common/extensions/api/experimental.managedMode.json:38: "description":
"This callback function is similar to that used by a $ref:ChromeSetting, except
that the incognitoSpecific parameter will never be present and the possible
values of levelOfControl differ.",
The "$ref" in here will not be expanded. You could use an <a> to link to the
documentation though.

http://codereview.chromium.org/9566007/diff/7001/chrome/common/extensions/api...
chrome/common/extensions/api/experimental.managedMode.json:38: "description":
"This callback function is similar to that used by a $ref:ChromeSetting, except
that the incognitoSpecific parameter will never be present and the possible
values of levelOfControl differ.",
Nit: Use some sort of markup for |incognitoSpecific| and |levelOfControl|?

http://codereview.chromium.org/9566007/diff/7001/chrome/common/extensions/api...
chrome/common/extensions/api/experimental.managedMode.json:47: "type": "any"
The type can be boolean.

http://codereview.chromium.org/9566007/diff/7001/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/managedMode/manifest.json (right):

http://codereview.chromium.org/9566007/diff/7001/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/managedMode/manifest.json:8: "page":
"test.html"
I think you can directly reference a script here, and the background page will
be automatically generated.

[Pam (message me for reviews), 2012-03-01 20:47:34]

All comments fixed, except as noted below. Please take another look.

Thanks,
- Pam

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_managed_mode_api.cc (right):

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_managed_mode_api.cc:39: std::string
level_of_control = enabled ? keys::kNotControllable
On 2012/03/01 16:32:14, Bernhard Bauer wrote:
> I'm not sure if this makes anything clearer to the extension :-D 

My goal was to keep the get() method as close to the one from ChromeSettings as
makes sense. It's conceivable we could have a policy that prohibits managed
mode, in which case "not_controllable" would become meaningful and distinct from
"enabled". I felt it was worth keeping levelOfControl around just in case it
became useful. That requires putting *something* into it, and this was the best
match.

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_managed_mode_api.h (right):

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_managed_mode_api.h:17: virtual
~GetManagedModeFunction();
On 2012/03/01 16:32:14, Bernhard Bauer wrote:
> I don't think you need to declare the destructor again.

Sorry, I don't understand this comment. "Again"? Where is it already declared?

http://codereview.chromium.org/9566007/diff/7001/chrome/common/extensions/api...
File chrome/common/extensions/api/experimental.managedMode.json (right):

http://codereview.chromium.org/9566007/diff/7001/chrome/common/extensions/api...
chrome/common/extensions/api/experimental.managedMode.json:38: "description":
"This callback function is similar to that used by a $ref:ChromeSetting, except
that the incognitoSpecific parameter will never be present and the possible
values of levelOfControl differ.",
On 2012/03/01 16:32:14, Bernhard Bauer wrote:
> The "$ref" in here will not be expanded. You could use an <a> to link to the
> documentation though.

OK... How come it works in windows.json? ($ref:Tab)

This is of course one of the things I will check once i get the docs generating
properly. :P

http://codereview.chromium.org/9566007/diff/7001/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/managedMode/manifest.json (right):

http://codereview.chromium.org/9566007/diff/7001/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/managedMode/manifest.json:8: "page":
"test.html"
On 2012/03/01 16:32:14, Bernhard Bauer wrote:
> I think you can directly reference a script here, and the background page will
> be automatically generated.

You mean just put "test.js"? I tried it, but it didn't work (the browser_test
timed out).

[Bernhard Bauer, 2012-03-02 09:23:58]

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_managed_mode_api.cc (right):

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_managed_mode_api.cc:39: std::string
level_of_control = enabled ? keys::kNotControllable
On 2012/03/01 20:47:34, Pam wrote:
> On 2012/03/01 16:32:14, Bernhard Bauer wrote:
> > I'm not sure if this makes anything clearer to the extension :-D 
> 
> My goal was to keep the get() method as close to the one from ChromeSettings
as
> makes sense. It's conceivable we could have a policy that prohibits managed
> mode, in which case "not_controllable" would become meaningful and distinct
from
> "enabled". I felt it was worth keeping levelOfControl around just in case it
> became useful. That requires putting *something* into it, and this was the
best
> match. 

OK. We still might to think about what controllable etc. mean here though.

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_managed_mode_api.h (right):

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_managed_mode_api.h:17: virtual
~GetManagedModeFunction();
On 2012/03/01 20:47:34, Pam wrote:
> On 2012/03/01 16:32:14, Bernhard Bauer wrote:
> > I don't think you need to declare the destructor again.
> 
> Sorry, I don't understand this comment. "Again"? Where is it already declared?

In the base class. IIUC, this is only necessary if you define additional
non-trivial members in the subclass, as that will cause a non-trivial destructor
to be synthesized by the compiler if you don't declare one yourself.

http://codereview.chromium.org/9566007/diff/7001/chrome/common/extensions/api...
File chrome/common/extensions/api/experimental.managedMode.json (right):

http://codereview.chromium.org/9566007/diff/7001/chrome/common/extensions/api...
chrome/common/extensions/api/experimental.managedMode.json:38: "description":
"This callback function is similar to that used by a $ref:ChromeSetting, except
that the incognitoSpecific parameter will never be present and the possible
values of levelOfControl differ.",
On 2012/03/01 20:47:34, Pam wrote:
> On 2012/03/01 16:32:14, Bernhard Bauer wrote:
> > The "$ref" in here will not be expanded. You could use an <a> to link to the
> > documentation though.
> 
> OK... How come it works in windows.json? ($ref:Tab)

Huh, it works. Whaddyaknow ;-)

> This is of course one of the things I will check once i get the docs
generating
> properly. :P

http://codereview.chromium.org/9566007/diff/7001/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/managedMode/manifest.json (right):

http://codereview.chromium.org/9566007/diff/7001/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/managedMode/manifest.json:8: "page":
"test.html"
On 2012/03/01 20:47:34, Pam wrote:
> On 2012/03/01 16:32:14, Bernhard Bauer wrote:
> > I think you can directly reference a script here, and the background page
will
> > be automatically generated.
> 
> You mean just put "test.js"? I tried it, but it didn't work (the browser_test
> timed out).

Using "scripts": ["test.js"] should work.

http://codereview.chromium.org/9566007/diff/6030/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_preference_api.h (right):

http://codereview.chromium.org/9566007/diff/6030/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.h:79: // modify that pref.
Sets error_ if the extension doesn't have the needed
Tiny nit: |error_|

[Pam (message me for reviews), 2012-03-02 12:39:35]

Finally got the document generator working. Woot!

All comments addressed, except the one about levelOfControl (not sure what you
want to do there) and the one below. Please take another look.

Thanks,
- Pam

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_managed_mode_api.h (right):

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_managed_mode_api.h:17: virtual
~GetManagedModeFunction();
> IIUC, this is only necessary if you define additional
> non-trivial members in the subclass, as that will cause a non-trivial
destructor
> to be synthesized by the compiler if you don't declare one yourself.

I believe that's correct, but unless you feel strongly, I prefer to include it.
It protects against leaks in case someone makes the subclass more complicated
and doesn't notice the lack of a virtual ctor. And it shouldn't cost much; we're
already paying the space penalty because of the virtual RunImpl().

WDYT?

[Bernhard Bauer, 2012-03-02 13:31:09]

On 2012/03/02 12:39:35, Pam wrote:
> Finally got the document generator working. Woot!
> 
> All comments addressed, except the one about levelOfControl (not sure what you
> want to do there)

What I meant there is that it isn't clear to me whether the level of control
should be kNotControllable or kControlledByThisExtension if the extension was
the one that activated managed mode. It's true that if managed mode is enabled,
the extension couldn't disable it, but it couldn't either if managed mode is
disabled.

I guess what I want to say is that level of control doesn't mean whether an
extension could *change* the value, but where the value comes from, relative to
its own position in the pref stack.

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_managed_mode_api.h (right):

http://codereview.chromium.org/9566007/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_managed_mode_api.h:17: virtual
~GetManagedModeFunction();
On 2012/03/02 12:39:36, Pam wrote:
> > IIUC, this is only necessary if you define additional
> > non-trivial members in the subclass, as that will cause a non-trivial
> destructor
> > to be synthesized by the compiler if you don't declare one yourself.
> 
> I believe that's correct, but unless you feel strongly, I prefer to include
it.
> It protects against leaks in case someone makes the subclass more complicated
> and doesn't notice the lack of a virtual ctor. And it shouldn't cost much;
we're
> already paying the space penalty because of the virtual RunImpl().
> 
> WDYT?

I don't feel strongly about it. Putting on my smartass hat, I just want to point
out that even if someone would add members to the subclass, it wouldn't cause a
leak, because the compiler would still synthesize a destructor.

[Bernhard Bauer, 2012-03-02 13:31:22]

Oh, but LGTM otherwise.

[Pam (message me for reviews), 2012-03-02 15:38:20]

Thanks. Following our discussion, I've removed levelOfControl until we have any
other way for managed mode to be controlled, and reworded all instances of
"enable" to "start" or "enter" to avoid confusion.

Ready for a final look, and for owner approval.

- Pam

[Bernhard Bauer, 2012-03-02 16:44:56]

LGTM still holds.

[Pam (message me for reviews), 2012-03-05 09:51:34]

Finnur, would you mind taking a look for owners approval?

- Pam

[Finnur, 2012-03-05 10:45:01]

LGTM, with nits.

I'm assuming here the discussion on whether to add this API to the extension
system has already taken place and received favorable votes? :)

http://codereview.chromium.org/9566007/diff/15040/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/9566007/diff/15040/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:340: bool
PreferenceFunction::ValidateBrowserPref(std::string extension_pref_key,
const std::string& ?

http://codereview.chromium.org/9566007/diff/15040/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_preference_api_constants.h (right):

http://codereview.chromium.org/9566007/diff/15040/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api_constants.h:17: extern const
char kValue[];
Are these just regular keys like the ones above? If so, maybe have them in one
block, sorted alphabetically?

http://codereview.chromium.org/9566007/diff/15040/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api_constants.h:20: extern const
char kNotControllable[];
This should be last in this section.

http://codereview.chromium.org/9566007/diff/15040/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/managedMode/test.js (right):

http://codereview.chromium.org/9566007/diff/15040/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/managedMode/test.js:16: 
nit: remove extra line break?

http://codereview.chromium.org/9566007/diff/15040/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/managedMode/test.js:25: function
enterSucceeds() {
Wording nit: enterSucceeds -> enteringSucceeds  ?

http://codereview.chromium.org/9566007/diff/15040/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/managedMode/test.js:27: 'Managed mode
should be able to be entered.'));
Wording nit: 'Should be able to enter managed mode' ?

http://codereview.chromium.org/9566007/diff/15040/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/managedMode/test.js:35: 'Managed mode
should be able to be entered.'));
Ditto?
Also, indentation (and on line 37).

http://codereview.chromium.org/9566007/diff/15040/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/managedMode/test.js:37: 'Managed mode
should be able to be entered twice.'));
Wording nit: 'Should be able to enter managed mode twice' ?

[commit-bot: I haz the power, 2012-03-05 14:58:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pam@chromium.org/9566007/23001

[commit-bot: I haz the power, 2012-03-05 14:59:02]

Presubmit check for 9566007-23001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: chrome/browser/profiles/profile_manager.cc

Presubmit checks took 2.2s to calculate.

[Pam (message me for reviews), 2012-03-05 15:16:02]

Sailesh, would you please take a look at the profile_manager.cc change and give
owners approval if it's acceptable?

Thanks,
- Pam

[Pam (message me for reviews), 2012-03-05 15:16:38]

Sailesh, would you please take a look at the profile_manager.cc change and give
owners approval if it's acceptable?

Thanks,
- Pam

[sail, 2012-03-05 18:41:29]

http://codereview.chromium.org/9566007/diff/23001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/9566007/diff/23001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:688:
DCHECK(!local_state->GetBoolean(prefs::kInManagedMode));
I don't think sprinkling these DCHECKs here is very helpful and it makes the
code harder to read.

http://codereview.chromium.org/9566007/diff/23001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:850:
DCHECK(!local_state->GetBoolean(prefs::kInManagedMode));
same as above

[Pam (message me for reviews), 2012-03-05 19:14:50]

>
http://codereview.chromium.org/9566007/diff/23001/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_manager.cc:688:
> DCHECK(!local_state->GetBoolean(prefs::kInManagedMode));
> I don't think sprinkling these DCHECKs here is very helpful and it makes the
> code harder to read.

Fair enough. I'll talk with Bernhard, who made this file's changes (we're both
working with the same pref, and my change got finished first). If he doesn't
have a strong case for keeping them, I'll take them out.

In the meantime, to save a full 24-hour review round-trip, are you willing to
LGTM it either as it is, or with the understanding that I'll take out the
DCHECKs when I get in tomorrow morning?

- Pam

[sail, 2012-03-05 19:20:16]

On 2012/03/05 19:14:50, Pam wrote:
> >
>
http://codereview.chromium.org/9566007/diff/23001/chrome/browser/profiles/pro...
> > chrome/browser/profiles/profile_manager.cc:688:
> > DCHECK(!local_state->GetBoolean(prefs::kInManagedMode));
> > I don't think sprinkling these DCHECKs here is very helpful and it makes the
> > code harder to read.
> 
> Fair enough. I'll talk with Bernhard, who made this file's changes (we're both
> working with the same pref, and my change got finished first). If he doesn't
> have a strong case for keeping them, I'll take them out.
> 
> In the meantime, to save a full 24-hour review round-trip, are you willing to
> LGTM it either as it is, or with the understanding that I'll take out the
> DCHECKs when I get in tomorrow morning?
> 
> - Pam

Sounds good!

[sail, 2012-03-05 19:20:36]

profile/* LGTM!

[Pam (message me for reviews), 2012-03-06 14:28:13]

Thanks, Sailesh. I took both DCHECKs out. Berhnard has been discussing changes
to them over in his patch, and he'll restore the lines that were agreed on there
(i.e., in a followup checkin after this one).

- Pam

[commit-bot: I haz the power, 2012-03-06 14:28:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pam@chromium.org/9566007/25001

[commit-bot: I haz the power, 2012-03-06 16:04:29]

Can't apply patch for file chrome/chrome_browser.gypi.
While running patch -p0 --forward --force;
patching file chrome/chrome_browser.gypi
Hunk #1 FAILED at 1193.
1 out of 1 hunk FAILED -- saving rejects to file chrome/chrome_browser.gypi.rej

[commit-bot: I haz the power, 2012-03-06 18:51:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pam@chromium.org/9566007/26026

[commit-bot: I haz the power, 2012-03-06 21:00:52]

Change committed as 125218