Supervised Users always need custodian approval for permission increase

Supervised Users should always need custodian approval for permission increase.
Remove the field trial SupervisedUserExtensionPermissionIncrease

BUG=652235
Committed: https://crrev.com/51e05d0d56dec9ca587d39a50ae2e2f9275f6e86
Cr-Commit-Position: refs/heads/master@{#423497}

[mamir, 2016-10-05 18:27:41]

Description was changed from

==========
Supervised Users always need custodian approval for permission increase

BUG=652235
==========

to

==========
Supervised Users should always need custodian approval for permission increase.
Remove the field trial SupervisedUserExtensionPermissionIncrease

BUG=652235
==========

[mamir, 2016-10-05 18:27:41]

mamir@chromium.org changed reviewers:
+ treib@chromium.org

[Marc Treib, 2016-10-05 19:48:05]

lgtm

https://codereview.chromium.org/2396903002/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_disabled_ui.cc (right):

https://codereview.chromium.org/2396903002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_disabled_ui.cc:368: // button.
Now this whole "if" should never happen (you essentially removed a
DCHECK(false)), so just replace it all with
DCHECK(!extensions::util::IsExtensionSupervised(...));

[mamir, 2016-10-05 21:28:12]

mamir@chromium.org changed reviewers:
+ asargent@chromium.org

[mamir, 2016-10-05 21:28:12]

asargent@chromium.org: Please review changes here.

[mamir, 2016-10-05 21:32:03]

mamir@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Devlin, 2016-10-05 22:48:40]

On 2016/10/05 21:32:16, mamir wrote:

Am I needed?  (Looks like asargent@ was already here)

[asargent_no_longer_on_chrome, 2016-10-05 23:23:34]

lgtm with a few remarks

https://codereview.chromium.org/2396903002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_disabled_ui.cc (right):

https://codereview.chromium.org/2396903002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_disabled_ui.cc:340: // "cancel" button.
Can you clarify this a little bit? Do you mean that we should literally never
hit this code? If so should we have a CHECK here?

Or is it the act of returning an empty string which causes there to be no cancel
button on the dialog?

https://codereview.chromium.org/2396903002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/2396903002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1690: // supervised user
themselves can't re-enable the extension.
Should this comment be updated to reflect the fact that supervised users can't
re-enable the extension?

(The comment says "if" they can't, but from the CL description it sounds like
they are never allowed to re-enable?)

[mamir, 2016-10-06 09:09:23]

@Devlin: sorry, you aren't needed in this CL indeed.
@Antony: thank you.

https://codereview.chromium.org/2396903002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_disabled_ui.cc (right):

https://codereview.chromium.org/2396903002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_disabled_ui.cc:340: // "cancel" button.
On 2016/10/05 23:23:34, Antony Sargent wrote:
> Can you clarify this a little bit? Do you mean that we should literally never
> hit this code? If so should we have a CHECK here?
> 
> Or is it the act of returning an empty string which causes there to be no
cancel
> button on the dialog?

It's the latter.  We return an empty string which causes the cancel button not
to be shown in the dialog.

https://codereview.chromium.org/2396903002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/2396903002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1690: // supervised user
themselves can't re-enable the extension.
On 2016/10/05 23:23:34, Antony Sargent wrote:
> Should this comment be updated to reflect the fact that supervised users can't
> re-enable the extension?
> 
> (The comment says "if" they can't, but from the CL description it sounds like
> they are never allowed to re-enable?)

Done.

[mamir, 2016-10-06 09:09:31]

The CQ bit was checked by mamir@chromium.org

[mamir, 2016-10-06 09:09:31]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/2396903002/#ps40001
(title: "response to code review")

[commit-bot: I haz the power, 2016-10-06 09:09:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marc Treib, 2016-10-06 09:23:16]

https://codereview.chromium.org/2396903002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_util.cc (left):

https://codereview.chromium.org/2396903002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_util.cc:359: bool
NeedCustodianApprovalForPermissionIncrease(const Profile* profile) {
Ah, one more comment: Please remove this function also from the header!
(Can be in this CL or in a followup, whichever is easier)

[mamir, 2016-10-06 09:26:16]

The CQ bit was unchecked by mamir@chromium.org

[mamir, 2016-10-06 09:27:36]

The CQ bit was checked by mamir@chromium.org

[commit-bot: I haz the power, 2016-10-06 09:27:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mamir, 2016-10-06 09:30:07]

The CQ bit was unchecked by mamir@chromium.org

[mamir, 2016-10-06 09:32:55]

https://codereview.chromium.org/2396903002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_util.cc (left):

https://codereview.chromium.org/2396903002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_util.cc:359: bool
NeedCustodianApprovalForPermissionIncrease(const Profile* profile) {
On 2016/10/06 09:23:16, Marc Treib wrote:
> Ah, one more comment: Please remove this function also from the header!
> (Can be in this CL or in a followup, whichever is easier)

Done!

[mamir, 2016-10-06 09:33:00]

The CQ bit was checked by mamir@chromium.org

[mamir, 2016-10-06 09:33:00]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/2396903002/#ps60001
(title: "Remove deleted method from the header file.")

[commit-bot: I haz the power, 2016-10-06 09:33:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 10:55:20]

Description was changed from

==========
Supervised Users should always need custodian approval for permission increase.
Remove the field trial SupervisedUserExtensionPermissionIncrease

BUG=652235
==========

to

==========
Supervised Users should always need custodian approval for permission increase.
Remove the field trial SupervisedUserExtensionPermissionIncrease

BUG=652235
==========

[commit-bot: I haz the power, 2016-10-06 10:55:21]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-06 10:57:46]

Description was changed from

==========
Supervised Users should always need custodian approval for permission increase.
Remove the field trial SupervisedUserExtensionPermissionIncrease

BUG=652235
==========

to

==========
Supervised Users should always need custodian approval for permission increase.
Remove the field trial SupervisedUserExtensionPermissionIncrease

BUG=652235

Committed: https://crrev.com/51e05d0d56dec9ca587d39a50ae2e2f9275f6e86
Cr-Commit-Position: refs/heads/master@{#423497}
==========

[commit-bot: I haz the power, 2016-10-06 10:57:47]

Patchset 4 (id:??) landed as
https://crrev.com/51e05d0d56dec9ca587d39a50ae2e2f9275f6e86
Cr-Commit-Position: refs/heads/master@{#423497}