Add more management policy checking after extension installed

Add more management policy checking after extension installed

This CL adds checking for MustRemainDisabled() from management policy after an extension is installed or updated, so that extensions supposed to be disabled will be disabled initially with proper disabled reason set.

This CL also assumes that all disabled extension comes with proper disabled reason, so there are additional changes to ensure this.

1) Another extension disabled reason DISABLE_EXTERNAL_EXTENSION is added for external extensions. These extensions will be disabled initially on windows for user prompting.
2) Two tests from extension_disabled_ui_browsertest.cc is removed since these two tests are meant for legacy disables with not disabled reason set in user pref, which should be rarely seen now. And these rare cases are handled by presuming it's disabled by user action as well.

BUG=None

Committed: https://crrev.com/47947f841e18fa93ff2ec76ab731a57965dc98ec
Cr-Commit-Position: refs/heads/master@{#304587}

[binjin, 2014-11-11 15:02:51]

binjin@chromium.org changed reviewers:
+ finnur@chromium.org, kalman@chromium.org

[binjin, 2014-11-11 15:02:52]

Finnur, Benjamin: PTAL at this CL

[binjin, 2014-11-11 17:03:53]

Patchset #2 (id:20001) has been deleted

[Finnur, 2014-11-11 22:58:03]

As a general comment, I've never liked the DISABLE_NONE enum value because it
looks, when you see it stand-alone, that it means "Disabled, but I'm not giving
a reason why", whereas the intended meaning is "This extension is not disabled".

It hasn't bothered me enough to go change it, but if we are sprinkling
DISABLED_NONE all over the codebase, maybe we should change it to something like
NOT_DISABLED.

WDYT, Ben?

https://codereview.chromium.org/714133002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/714133002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:2181: *disable_reason =
Extension::DISABLE_NONE;
It is a bit weird to set this only when this function returns false. Is that on
purpose?

https://codereview.chromium.org/714133002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/714133002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:526:
extensions::Extension::DisableReason initial_disable_reason,
I think |initial_state| and |initial_disable_reason| deserve documentation. For
example, if |initial_state| is ENABLED, what should |initial_disable_reason| be
set to?

https://codereview.chromium.org/714133002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:555: // applicable.
This comment is a bit confusing. How about:

|disable_reason| is ignored if NULL, but otherwise contains why |extension|
should not be enabled.

https://codereview.chromium.org/714133002/diff/40001/extensions/browser/exten...
File extensions/browser/extension_prefs.h (right):

https://codereview.chromium.org/714133002/diff/40001/extensions/browser/exten...
extensions/browser/extension_prefs.h:426: // to disable the extension initially,
if applicable.
This is also confusing to read. Can you revise?

[Finnur, 2014-11-11 23:01:20]

On 2014/11/11 22:58:03, Finnur wrote:
> As a general comment, I've never liked the DISABLE_NONE enum value because it
> looks, when you see it stand-alone, that it means "Disabled, but I'm not
giving
> a reason why", whereas the intended meaning is "This extension is not
disabled".
> 
> It hasn't bothered me enough to go change it, but if we are sprinkling
> DISABLED_NONE all over the codebase, maybe we should change it to something
like
> NOT_DISABLED.
> 
> WDYT, Ben?
> 

Or, if we sometimes disable extensions without giving a reason, then even
NO_DISABLE_REASON would be better.

[not at google - send to devlin, 2014-11-12 00:44:34]

On 2014/11/11 22:58:03, Finnur wrote:
> As a general comment, I've never liked the DISABLE_NONE enum value because it
> looks, when you see it stand-alone, that it means "Disabled, but I'm not
giving
> a reason why", whereas the intended meaning is "This extension is not
disabled".
> 
> It hasn't bothered me enough to go change it, but if we are sprinkling
> DISABLED_NONE all over the codebase, maybe we should change it to something
like
> NOT_DISABLED.
> 
> WDYT, Ben?

I had no idea we even had a not-disabled-disabled value :\ yes, it's super
confusing.

[not at google - send to devlin, 2014-11-12 00:54:19]

Had a cursory look, but my overwhelming reaction is that this is an awful lot of
plumbing. Could you highlight the functional change in here?

https://codereview.chromium.org/714133002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/714133002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:526:
extensions::Extension::DisableReason initial_disable_reason,
On 2014/11/11 22:58:03, Finnur wrote:
> I think |initial_state| and |initial_disable_reason| deserve documentation.
For
> example, if |initial_state| is ENABLED, what should |initial_disable_reason|
be
> set to?

This is pretty weird to look at (and looking at the files earlier, having to
pass in a "not disabled" flag every time you say "enabled", is very strange).

2 proposals:

- is it really that bad for whatever place needs to set a different disable
reason to add in a DISABLED state then later set the real disable state?

- perhaps look into replacing the initial state argument with the disable reason
flag? Still weird, but not quite as weird. The states are basically enabled,
disabled, and external-extension-uninstalled, the latter of which is a pretty
weird thing in the first place. I'd question why it needs to be passed in here
(is it still used?) and/or whether it should *really* be a disable reason.

[not at google - send to devlin, 2014-11-12 00:54:29]

(for me)

[binjin, 2014-11-12 16:33:18]

okay, I will follow the easy way then.

https://codereview.chromium.org/714133002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/714133002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:2181: *disable_reason =
Extension::DISABLE_NONE;
On 2014/11/11 22:58:03, Finnur wrote:
> It is a bit weird to set this only when this function returns false. Is that
on
> purpose?

Yes, the |disable_reason| will be ignored if true is returned.

https://codereview.chromium.org/714133002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/714133002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:526:
extensions::Extension::DisableReason initial_disable_reason,
On 2014/11/12 00:54:19, kalman wrote:
> On 2014/11/11 22:58:03, Finnur wrote:
> > I think |initial_state| and |initial_disable_reason| deserve documentation.
> For
> > example, if |initial_state| is ENABLED, what should |initial_disable_reason|
> be
> > set to?
> 
> This is pretty weird to look at (and looking at the files earlier, having to
> pass in a "not disabled" flag every time you say "enabled", is very strange).
> 
> 2 proposals:
> 
> - is it really that bad for whatever place needs to set a different disable
> reason to add in a DISABLED state then later set the real disable state?
> 
> - perhaps look into replacing the initial state argument with the disable
reason
> flag? Still weird, but not quite as weird. The states are basically enabled,
> disabled, and external-extension-uninstalled, the latter of which is a pretty
> weird thing in the first place. I'd question why it needs to be passed in here
> (is it still used?) and/or whether it should *really* be a disable reason.

Yes, I now followed your first proposal.

https://codereview.chromium.org/714133002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:555: // applicable.
On 2014/11/11 22:58:03, Finnur wrote:
> This comment is a bit confusing. How about:
> 
> |disable_reason| is ignored if NULL, but otherwise contains why |extension|
> should not be enabled.

Done.

[not at google - send to devlin, 2014-11-12 16:38:42]

https://codereview.chromium.org/714133002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/714133002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:526:
extensions::Extension::DisableReason initial_disable_reason,
On 2014/11/12 16:33:18, bjin wrote:
> On 2014/11/12 00:54:19, kalman wrote:
> > On 2014/11/11 22:58:03, Finnur wrote:
> > > I think |initial_state| and |initial_disable_reason| deserve
documentation.
> > For
> > > example, if |initial_state| is ENABLED, what should
|initial_disable_reason|
> > be
> > > set to?
> > 
> > This is pretty weird to look at (and looking at the files earlier, having to
> > pass in a "not disabled" flag every time you say "enabled", is very
strange).
> > 
> > 2 proposals:
> > 
> > - is it really that bad for whatever place needs to set a different disable
> > reason to add in a DISABLED state then later set the real disable state?
> > 
> > - perhaps look into replacing the initial state argument with the disable
> reason
> > flag? Still weird, but not quite as weird. The states are basically enabled,
> > disabled, and external-extension-uninstalled, the latter of which is a
pretty
> > weird thing in the first place. I'd question why it needs to be passed in
here
> > (is it still used?) and/or whether it should *really* be a disable reason.
> 
> Yes, I now followed your first proposal.

Cool! If you're going with this, it would be ideal to split this CL into 2
parts, firstly doing the parameter type change, then adding your feature.

[not at google - send to devlin, 2014-11-12 16:40:33]

https://codereview.chromium.org/714133002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/714133002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:526:
extensions::Extension::DisableReason initial_disable_reason,
On 2014/11/12 16:38:42, kalman wrote:
> On 2014/11/12 16:33:18, bjin wrote:
> > On 2014/11/12 00:54:19, kalman wrote:
> > > On 2014/11/11 22:58:03, Finnur wrote:
> > > > I think |initial_state| and |initial_disable_reason| deserve
> documentation.
> > > For
> > > > example, if |initial_state| is ENABLED, what should
> |initial_disable_reason|
> > > be
> > > > set to?
> > > 
> > > This is pretty weird to look at (and looking at the files earlier, having
to
> > > pass in a "not disabled" flag every time you say "enabled", is very
> strange).
> > > 
> > > 2 proposals:
> > > 
> > > - is it really that bad for whatever place needs to set a different
disable
> > > reason to add in a DISABLED state then later set the real disable state?
> > > 
> > > - perhaps look into replacing the initial state argument with the disable
> > reason
> > > flag? Still weird, but not quite as weird. The states are basically
enabled,
> > > disabled, and external-extension-uninstalled, the latter of which is a
> pretty
> > > weird thing in the first place. I'd question why it needs to be passed in
> here
> > > (is it still used?) and/or whether it should *really* be a disable reason.
> > 
> > Yes, I now followed your first proposal.
> 
> Cool! If you're going with this, it would be ideal to split this CL into 2
> parts, firstly doing the parameter type change, then adding your feature.

And perhaps a 3rd earlier to do the rename that Finnur suggested with the
DISABLED_NONE enum.

[binjin, 2014-11-12 16:50:49]

On 2014/11/12 16:38:42, kalman wrote:
> Cool! If you're going with this, it would be ideal to split this CL into 2
> parts, firstly doing the parameter type change, then adding your feature.

I mean I will just set the disable reasons separately, which is the easiest way,
since there are already code above that add disable reasons before setting the
disabled state.

please have a look at the latest patchset.

[not at google - send to devlin, 2014-11-12 18:02:15]

https://codereview.chromium.org/714133002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/714133002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:2166: // Extensions disabled by
managemeny policy should always be disabled, even
management

https://codereview.chromium.org/714133002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/714133002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:551: // (or upgraded) extension.
|disable_reason| will be ignored if NULL,
This comment is misleading, it should mention that disable_reason is an out
param. However, you could just return the disable reason from this method
instead. You may also want to rename the method to something more appropriate.

[binjin, 2014-11-12 18:39:02]

https://codereview.chromium.org/714133002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/714133002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:2166: // Extensions disabled by
managemeny policy should always be disabled, even
On 2014/11/12 18:02:15, kalman wrote:
> management

Done.

https://codereview.chromium.org/714133002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/714133002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:551: // (or upgraded) extension.
|disable_reason| will be ignored if NULL,
On 2014/11/12 18:02:15, kalman wrote:
> This comment is misleading, it should mention that disable_reason is an out
> param. However, you could just return the disable reason from this method
> instead. You may also want to rename the method to something more appropriate.

Done(editing the comments). Also this pointer is not supposed to be NULL, fixed
that in comments as well.

I'm not sure if it's a good idea to combine the boolean value and Disable
Reason, since DISABLE_NONE didn't imply enabling the extension.

[not at google - send to devlin, 2014-11-12 19:35:17]

https://codereview.chromium.org/714133002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/714133002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:551: // (or upgraded) extension.
|disable_reason| will be ignored if NULL,
> I'm not sure if it's a good idea to combine the boolean value and Disable
> Reason, since DISABLE_NONE didn't imply enabling the extension.

It looks to me to be precisely the reason that this returns a bool.

|initial_enable| is set to true if the external extension was uninstalled. You
could set it to DISABLE_NONE instead.

|initial_enable| is set to false if the extension has requirement errors. You
could set it to DISABLE_UNSUPPORTED_REQUIEMENT instead.

etc.

[binjin, 2014-11-13 12:03:31]

https://codereview.chromium.org/714133002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/714133002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:551: // (or upgraded) extension.
|disable_reason| will be ignored if NULL,
On 2014/11/12 19:35:17, kalman wrote:
> > I'm not sure if it's a good idea to combine the boolean value and Disable
> > Reason, since DISABLE_NONE didn't imply enabling the extension.
> 
> It looks to me to be precisely the reason that this returns a bool.
> 
> |initial_enable| is set to true if the external extension was uninstalled. You
> could set it to DISABLE_NONE instead.
> 
> |initial_enable| is set to false if the extension has requirement errors. You
> could set it to DISABLE_UNSUPPORTED_REQUIEMENT instead.
> 
> etc.

I'm sorry, I think I didn't get your point. Do you mean that we should return a
DisableReason and use a pointer like |initial_enable| as out params for setting
a bool?

[not at google - send to devlin, 2014-11-13 17:41:21]

https://codereview.chromium.org/714133002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/714133002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:551: // (or upgraded) extension.
|disable_reason| will be ignored if NULL,
On 2014/11/13 12:03:31, bjin wrote:
> On 2014/11/12 19:35:17, kalman wrote:
> > > I'm not sure if it's a good idea to combine the boolean value and Disable
> > > Reason, since DISABLE_NONE didn't imply enabling the extension.
> > 
> > It looks to me to be precisely the reason that this returns a bool.
> > 
> > |initial_enable| is set to true if the external extension was uninstalled.
You
> > could set it to DISABLE_NONE instead.
> > 
> > |initial_enable| is set to false if the extension has requirement errors.
You
> > could set it to DISABLE_UNSUPPORTED_REQUIEMENT instead.
> > 
> > etc.
> 
> I'm sorry, I think I didn't get your point. Do you mean that we should return
a
> DisableReason and use a pointer like |initial_enable| as out params for
setting
> a bool?

No I mean that you should change the return type of this function from a bool to
the DisableReason enum.

[binjin, 2014-11-13 19:44:38]

On 2014/11/13 17:41:21, kalman wrote:
> No I mean that you should change the return type of this function from a bool
to
> the DisableReason enum.

Both "uninstalled external extension" and "extension with requirement errors"
are not in scope of ExtensionService::ShouldEnableOnInstall(), they are instead
overriding return value of it in ExtensionService::OnExtensionInstalled().

I think it's not worth changing the return type of ShouldEnableOnInstall(),
since there are not direct relationship between disable reason and whether we
should enable an extension or not. Changing just introduce confusing in my
opinion.

For example, it looks legal to me that MustRemainDisabled() from
ManagementPolicy returns true but with DISABLE_NONE.

[not at google - send to devlin, 2014-11-13 20:23:27]

On 2014/11/13 19:44:38, bjin wrote:
> On 2014/11/13 17:41:21, kalman wrote:
> > No I mean that you should change the return type of this function from a
bool
> to
> > the DisableReason enum.
> 
> Both "uninstalled external extension" and "extension with requirement errors"
> are not in scope of ExtensionService::ShouldEnableOnInstall(), they are
instead
> overriding return value of it in ExtensionService::OnExtensionInstalled().
> 
> I think it's not worth changing the return type of ShouldEnableOnInstall(),
> since there are not direct relationship between disable reason and whether we
> should enable an extension or not. Changing just introduce confusing in my
> opinion.
> 
> For example, it looks legal to me that MustRemainDisabled() from
> ManagementPolicy returns true but with DISABLE_NONE.

I don't think ManagementPolicy::MustRemainDisabled is a good interface then.

Please change the return type.

[binjin, 2014-11-13 23:21:21]

On 2014/11/13 20:23:27, kalman wrote:
> I don't think ManagementPolicy::MustRemainDisabled is a good interface then.
> 
> Please change the return type.

Done. I also added a DCHECK for ManagementPolicy::MustRemainDisabled().

Also pay attension that I used DISABLE_REMOTE_INSTALL for disalbing an
unacknowledged external extensions.

[not at google - send to devlin, 2014-11-14 00:05:11]

lgtm after adding the enum + updating histograms.xml

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

https://codereview.chromium.org/714133002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:2189: return
Extension::DISABLE_REMOTE_INSTALL;
Thanks for doing this - add a new disable reason for this, though.
DISABLE_EXTERNAL_EXTENSION perhaps. You'll also need to update histograms.xml.

[binjin, 2014-11-14 12:28:25]

binjin@chromium.org changed reviewers:
+ asvitkine@chromium.org

[binjin, 2014-11-14 12:28:26]

@asvitkine, please have a look at tools/metrics/histograms/histograms.xml

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

https://codereview.chromium.org/714133002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:2189: return
Extension::DISABLE_REMOTE_INSTALL;
On 2014/11/14 00:05:11, kalman wrote:
> Thanks for doing this - add a new disable reason for this, though.
> DISABLE_EXTERNAL_EXTENSION perhaps. You'll also need to update histograms.xml.

Done.

Also pay attention that I made extra change to presume an extension disabled
without any reason specified is by user action. Such circumstance appears in
ExtensionDisabledGlobalErrorTest.UnknownReasonSamePermissions
 browser_tests.

[binjin, 2014-11-14 15:52:26]

On 2014/11/14 12:28:26, bjin wrote:
> @asvitkine, please have a look at tools/metrics/histograms/histograms.xml
> 
>
https://codereview.chromium.org/714133002/diff/120001/chrome/browser/extensio...
> File chrome/browser/extensions/extension_service.cc (right):
> 
>
https://codereview.chromium.org/714133002/diff/120001/chrome/browser/extensio...
> chrome/browser/extensions/extension_service.cc:2189: return
> Extension::DISABLE_REMOTE_INSTALL;
> On 2014/11/14 00:05:11, kalman wrote:
> > Thanks for doing this - add a new disable reason for this, though.
> > DISABLE_EXTERNAL_EXTENSION perhaps. You'll also need to update
histograms.xml.
> 
> Done.
> 
> Also pay attention that I made extra change to presume an extension disabled
> without any reason specified is by user action. Such circumstance appears in
> ExtensionDisabledGlobalErrorTest.UnknownReasonSamePermissions
>  browser_tests.

Ooops, it now fails the
ExtensionDisabledGlobalErrorTest.UnknownReasonHigherPermissions test.

The failure on UnknownReasonSamePermissions is due to this test disables
extensions without specified reason.

The failure on UnknownReasonHigherPermissions is due to condition check in
ExtensionService::AddExtension() are not happy with the USER_ACTION bit I
presumed when decide whether or not to throw a permission increased error.

I think I have either to fix these two tests or to revert changes back.

[not at google - send to devlin, 2014-11-14 17:20:43]

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

https://codereview.chromium.org/714133002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:2174: disable_reason =
static_cast<Extension::DisableReason>(
Damn, ok, this static_cast<> isn't actually right. Sorry for not noticing that.
It's possible that you will get undefined results if there are multiple disable
reasons, trying to cast it to a single one. It looks like this function actually
needs to return a bitmask.

Annoyingly, it looks like that will also require adding an AddDisableReasons
method to ExtensionPrefs which takes a bitmask (the interface on ExtensionPrefs
is pretty odd really).

[binjin, 2014-11-14 18:21:59]

The test failures are still not addressed.

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

https://codereview.chromium.org/714133002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:2174: disable_reason =
static_cast<Extension::DisableReason>(
On 2014/11/14 17:20:43, kalman wrote:
> Damn, ok, this static_cast<> isn't actually right. Sorry for not noticing
that.
> It's possible that you will get undefined results if there are multiple
disable
> reasons, trying to cast it to a single one. It looks like this function
actually
> needs to return a bitmask.
> 
> Annoyingly, it looks like that will also require adding an AddDisableReasons
> method to ExtensionPrefs which takes a bitmask (the interface on
ExtensionPrefs
> is pretty odd really).

Done.

[not at google - send to devlin, 2014-11-14 18:34:51]

lgtm

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

https://codereview.chromium.org/714133002/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1609: disable_reasons =
Extension::DISABLE_UNSUPPORTED_REQUIREMENT;
This should probably be &= not =

https://codereview.chromium.org/714133002/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1655: disable_reasons == 0 ?
Extension::ENABLED : Extension::DISABLED;
Use DISABLE_NONE not 0.

[binjin, 2014-11-14 18:38:26]

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

https://codereview.chromium.org/714133002/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1609: disable_reasons =
Extension::DISABLE_UNSUPPORTED_REQUIREMENT;
On 2014/11/14 18:34:51, kalman wrote:
> This should probably be &= not =

Do you mean |= ?

[not at google - send to devlin, 2014-11-14 18:39:34]

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

https://codereview.chromium.org/714133002/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1609: disable_reasons =
Extension::DISABLE_UNSUPPORTED_REQUIREMENT;
On 2014/11/14 18:38:26, bjin wrote:
> On 2014/11/14 18:34:51, kalman wrote:
> > This should probably be &= not =
> 
> Do you mean |= ?

Oops, yes.

[binjin, 2014-11-14 18:59:22]

I'm still unable to find proper way to handle the two failed tests, and I think
I'm going to remove these two tests.

These two tests are testing how extensions permission increase error are handled
when disabled reason is unknown. From the comments it seems that it only applies
to legacy versions which disables extensions without specified DisableReason.
Since we manually add USER_ACTION bit for such extensions now, it's handled well
now, except that these two tests are failing. 

@Benjamin: What do you think?

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

https://codereview.chromium.org/714133002/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1609: disable_reasons =
Extension::DISABLE_UNSUPPORTED_REQUIREMENT;
On 2014/11/14 18:39:34, kalman wrote:
> On 2014/11/14 18:38:26, bjin wrote:
> > On 2014/11/14 18:34:51, kalman wrote:
> > > This should probably be &= not =
> > 
> > Do you mean |= ?
> 
> Oops, yes.

Done.

https://codereview.chromium.org/714133002/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1655: disable_reasons == 0 ?
Extension::ENABLED : Extension::DISABLED;
On 2014/11/14 18:34:51, kalman wrote:
> Use DISABLE_NONE not 0.

Done.

[binjin, 2014-11-17 12:35:45]

binjin@chromium.org changed reviewers:
+ isherman@chromium.org, yoz@chromium.org

[binjin, 2014-11-17 12:35:46]

isherman, asvitkine: tools/metrics/histograms/histograms.xml

kalman: I updated CL description and extension_disabled_ui_browsertest.cc, PTAL

yoz: I'm removing two tests that you added two years ago, but I might miss
something, PTAL at changes to extension_disabled_ui_browsertest.cc

Thanks all

[not at google - send to devlin, 2014-11-17 17:08:47]

lgtm

[Ilya Sherman, 2014-11-17 22:22:17]

histograms.xml lgtm

[Yoyo Zhou, 2014-11-17 22:25:15]

LGTM, those tests were relevant about 18 versions of Chrome ago and shouldn't be
needed anymore.

[binjin, 2014-11-18 11:01:40]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2014-11-18 11:02:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/714133002/260001

[commit-bot: I haz the power, 2014-11-18 12:10:35]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2014-11-18 12:11:24]

Patchset 13 (id:??) landed as
https://crrev.com/47947f841e18fa93ff2ec76ab731a57965dc98ec
Cr-Commit-Position: refs/heads/master@{#304587}