Add management policy function allowing extensions to be disabled.

Add management policy function allowing extensions to be disabled.

This CL adds the MustRemainDisabled function to the ManagementPolicy::Provider
interface and calls it in appropriate places.

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

[asargent_no_longer_on_chrome, 2013-11-02 22:51:05]

miket: please review

Suggested review order: management_policy.h, management_policy.cc, then
everything else

[miket_OOO, 2013-11-04 23:18:10]

LGTM except for one must-fix and a few other elective areas for improvement.

https://codereview.chromium.org/54903011/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/54903011/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:898: if (extension &&
policy->MustRemainDisabled(extension, NULL, NULL))
I can't tell for sure whether this is always called at profile load time, but if
not, any chance you want an UMA metric to see whether anyone's repeatedly trying
to enable this in a surprising fashion?

https://codereview.chromium.org/54903011/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:909: 
WHA?!

https://codereview.chromium.org/54903011/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:1227: to_disable[extension->id()]
= disable_reason;
Check logic.

https://codereview.chromium.org/54903011/diff/1/chrome/browser/extensions/man...
File chrome/browser/extensions/management_policy.cc (right):

https://codereview.chromium.org/54903011/diff/1/chrome/browser/extensions/man...
chrome/browser/extensions/management_policy.cc:81: return true;
See any_of. http://www.cplusplus.com/reference/algorithm/ (not sure whether it
maps nicely to this case, but it seems like a candidate)

https://codereview.chromium.org/54903011/diff/1/chrome/browser/extensions/tes...
File chrome/browser/extensions/test_management_policy.cc (right):

https://codereview.chromium.org/54903011/diff/1/chrome/browser/extensions/tes...
chrome/browser/extensions/test_management_policy.cc:71: *reason =
disable_reason_;
Tiniest readability comment that you can ignore:

if (must_remain_disabled_) {
  if (foo)
     DoFoo();
  if (bar)
     DoBar();
}

To me this better expresses what you're trying to do; otherwise, the
reader/reviewer must twice ask whether the must_remain_disabled_ test is
correct. I suppose the computer does, too...

[asargent_no_longer_on_chrome, 2013-11-05 00:20:44]

Addressed all comments and sending to CQ.

https://codereview.chromium.org/54903011/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/54903011/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:898: if (extension &&
policy->MustRemainDisabled(extension, NULL, NULL))
On 2013/11/04 23:18:10, miket wrote:
> I can't tell for sure whether this is always called at profile load time, but
if
> not, any chance you want an UMA metric to see whether anyone's repeatedly
trying
> to enable this in a surprising fashion?

The InstalledLoader should catch cases during init, but this function can be
called from all kinds of places, and it would be interesting if we hit this case
very often, so a histogram is a good idea. 

done

https://codereview.chromium.org/54903011/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:909: 
On 2013/11/04 23:18:10, miket wrote:
> WHA?!
oops, fixed.

https://codereview.chromium.org/54903011/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:1227: to_disable[extension->id()]
= disable_reason;
On 2013/11/04 23:18:10, miket wrote:
> Check logic.

Good catch. The "!" was a typo.

Fixed.

https://codereview.chromium.org/54903011/diff/1/chrome/browser/extensions/man...
File chrome/browser/extensions/management_policy.cc (right):

https://codereview.chromium.org/54903011/diff/1/chrome/browser/extensions/man...
chrome/browser/extensions/management_policy.cc:81: return true;
On 2013/11/04 23:18:10, miket wrote:
> See any_of. http://www.cplusplus.com/reference/algorithm/ (not sure whether it
> maps nicely to this case, but it seems like a candidate)

I looked into it briefly, but it would be very awkward for this case because
any_of takes a UnaryPredicate function which takes only a single input (an
element of the collection). But in this case I need to call a method on each of
the objects kept in the collection, passing that method the 3 arguments passed
in to this method. I don't think our currying support in base/callback.h
supports binding a class method but leaving the object reference unbound while
binding further arguments to the method.

https://codereview.chromium.org/54903011/diff/1/chrome/browser/extensions/tes...
File chrome/browser/extensions/test_management_policy.cc (right):

https://codereview.chromium.org/54903011/diff/1/chrome/browser/extensions/tes...
chrome/browser/extensions/test_management_policy.cc:71: *reason =
disable_reason_;
On 2013/11/04 23:18:10, miket wrote:
> Tiniest readability comment that you can ignore:
> 
> if (must_remain_disabled_) {
>   if (foo)
>      DoFoo();
>   if (bar)
>      DoBar();
> }
> 
> To me this better expresses what you're trying to do; otherwise, the
> reader/reviewer must twice ask whether the must_remain_disabled_ test is
> correct. I suppose the computer does, too...

Done.

[commit-bot: I haz the power, 2013-11-05 00:44:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asargent@chromium.org/54903011/320001

[commit-bot: I haz the power, 2013-11-05 03:13:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asargent@chromium.org/54903011/320001

[commit-bot: I haz the power, 2013-11-05 04:46:53]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-05 05:39:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asargent@chromium.org/54903011/320001

[commit-bot: I haz the power, 2013-11-05 07:04:58]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-05 13:35:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asargent@chromium.org/54903011/320001

[commit-bot: I haz the power, 2013-11-05 14:15:38]

Change committed as 232976