Add minimum version to extension management

Add minimum version to extension management

This CL adds 'minimum_version' setting to ExtensionManagement. Allows policy to specify the minimum version required for the specified extension to be enabled.

This CL adds another Extension::DisableReason DISABLE_UPDATE_REQUIRED_BY_POLICY, and using it to track extensions being disabled by minimum version settings, and re-enable them if necessary.

BUG=177351
Committed: https://crrev.com/8e3d01889d55c01618e575d0b8271a24a50cb5c2
Cr-Commit-Position: refs/heads/master@{#306834}

[binjin, 2014-11-10 18:26:30]

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

[binjin, 2014-11-10 18:26:31]

Hello Joao,

This CL is still a WIP, but I think it's now okay for an initial code review.
Both unit tests and browser tests are added. But it fails the browser tests
since MustRemainDisabled() is not called on extension installed, See inline
place holder comments for details.

I will add webui changes in another CL, since some refactoring is needed for
that.

Bin

[Joao da Silva, 2014-11-11 12:04:24]

I have limited availability to review this during this week. Can you find
another reviewer? Maybe Julian can make a first pass.

[binjin, 2014-11-11 12:26:23]

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

[binjin, 2014-11-11 12:26:24]

+Julian

[pastarmovj, 2014-11-11 12:50:55]

mostly nits and a couple of suggestions for the unit tests.

However I can not judge the goodness of the extensions internal code but in
general the logic looks ok to me.

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:124: // Returns true if
|extension| meet the minimum required version set for it.
s/meet/meets/

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:125: // If there are no such
requirement set for it, returns true as well.
s/are/is/

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_management_internal.cc (right):

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_management_internal.cc:138: // We accept a
general version string here. Note that count of components in
nit: Some articles in the second sentence. 

Note that the count of the components in a version string of extensions is
limited to 4

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_management_unittest.cc (right):

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_management_unittest.cc:180: return
extension_management_->CheckMinimumVersion(extension.get(), nullptr);
I would check here that the second parameter is set or not set depending on the
conditions you have put. Also of course check that it doesn't crash if the
parameter is null like your code does now.

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

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1140: // Find all extensions
(including the ones got disabled just now) disabled
nit: ...ones _that_ got...

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1640: // from enterprise policy,
and now satisfies it now and there are not other
nit: remove the first "now". Also s/not/no/

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1682: // WIP(binjin): need to
figure out a way to check MustRemainDisabled here and
nit: AFAIK WIP is not officially used in Chromium replace this with TODO.

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/policy/po...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/policy/po...
chrome/browser/policy/policy_browsertest.cc:1995: // Provide a new version
(1.0.0.1) which are expect to be auto updated to
s/are expect/is expected/

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/policy/po...
chrome/browser/policy/policy_browsertest.cc:1996: // via update URL in manifest
of older version.
nit: _the_ manifest and _the_ older version

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/policy/po...
chrome/browser/policy/policy_browsertest.cc:2029: // Uninsatll the extension.
nit: Uninst_a_ll

[binjin, 2014-11-11 19:10:38]

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:124: // Returns true if
|extension| meet the minimum required version set for it.
On 2014/11/11 12:50:54, pastarmovj wrote:
> s/meet/meets/

Done.

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:125: // If there are no such
requirement set for it, returns true as well.
On 2014/11/11 12:50:54, pastarmovj wrote:
> s/are/is/

Done.

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_management_internal.cc (right):

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_management_internal.cc:138: // We accept a
general version string here. Note that count of components in
On 2014/11/11 12:50:54, pastarmovj wrote:
> nit: Some articles in the second sentence. 
> 
> Note that the count of the components in a version string of extensions is
> limited to 4

Done.

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_management_unittest.cc (right):

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_management_unittest.cc:180: return
extension_management_->CheckMinimumVersion(extension.get(), nullptr);
On 2014/11/11 12:50:54, pastarmovj wrote:
> I would check here that the second parameter is set or not set depending on
the
> conditions you have put. Also of course check that it doesn't crash if the
> parameter is null like your code does now.

Done.

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

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1140: // Find all extensions
(including the ones got disabled just now) disabled
On 2014/11/11 12:50:55, pastarmovj wrote:
> nit: ...ones _that_ got...

Done.

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1640: // from enterprise policy,
and now satisfies it now and there are not other
On 2014/11/11 12:50:55, pastarmovj wrote:
> nit: remove the first "now". Also s/not/no/

Done.

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1682: // WIP(binjin): need to
figure out a way to check MustRemainDisabled here and
On 2014/11/11 12:50:55, pastarmovj wrote:
> nit: AFAIK WIP is not officially used in Chromium replace this with TODO.

It's a mark for myself, reminding that there are unhandled issues in this CL.
it's now addressed by https://codereview.chromium.org/714133002/

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/policy/po...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/policy/po...
chrome/browser/policy/policy_browsertest.cc:1995: // Provide a new version
(1.0.0.1) which are expect to be auto updated to
On 2014/11/11 12:50:55, pastarmovj wrote:
> s/are expect/is expected/

Done.

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/policy/po...
chrome/browser/policy/policy_browsertest.cc:1996: // via update URL in manifest
of older version.
On 2014/11/11 12:50:55, pastarmovj wrote:
> nit: _the_ manifest and _the_ older version

Done.

https://codereview.chromium.org/706623004/diff/60001/chrome/browser/policy/po...
chrome/browser/policy/policy_browsertest.cc:2029: // Uninsatll the extension.
On 2014/11/11 12:50:55, pastarmovj wrote:
> nit: Uninst_a_ll

Done.

[binjin, 2014-11-17 14:48:02]

binjin@chromium.org changed reviewers:
- pastarmovj@chromium.org

[binjin, 2014-11-17 14:48:03]

Joao, please take a look

[Joao da Silva, 2014-11-17 16:17:42]

Looks good overall. Really good test coverage.

https://codereview.chromium.org/706623004/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_constants.cc (right):

https://codereview.chromium.org/706623004/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_constants.cc:27: const char
kMinimumVersion[] = "minimum_version";
Suggestion: rename this to "minimum_version_required", which is more informative
when found out of context.

https://codereview.chromium.org/706623004/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_internal.cc (right):

https://codereview.chromium.org/706623004/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.cc:141: LOG(WARNING) <<
kMalformedPreferenceWarning;
In this case, the administrator won't see any problem reported in about:policy
and won't detect that there is a problem with this policy at all.

WDYT of setting |minimum_version| to 0.0.0.0 in this case?

Another idea: can you file a bug to get errors out of the parsing of this
policy? Basically this function could take an "std::string* errors" parameter,
and instead of logging we would append the errors to that string. Then we could
still "return true" in general but show any parsing errors in about:policy. That
would be for another CL though.

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

https://codereview.chromium.org/706623004/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1130: // automaticall
automaticallyy.
"automatically" (only once)

https://codereview.chromium.org/706623004/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1135: if
(extensions::ExtensionManagementFactory::GetForBrowserContext(profile())
Get this outside of the loop and put in a local variable

https://codereview.chromium.org/706623004/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1160: for (size_t i = 0; i <
to_enable.size(); ++i)
for (const std::string& id : to_enable) { ... }

Does that work? It would be more consistent with the other loops.

https://codereview.chromium.org/706623004/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1671: }
What if the extension had more than one disable_reason? We should clear the
DISABLE_UPDATE_REQUIRED_BY_POLICY flag in that case.

https://codereview.chromium.org/706623004/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/standard_management_policy_provider.h (right):

https://codereview.chromium.org/706623004/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/standard_management_policy_provider.h:8: #include
<string>
#include "base/strings/string16.h"

[binjin, 2014-11-18 13:25:55]

https://codereview.chromium.org/706623004/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_constants.cc (right):

https://codereview.chromium.org/706623004/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_constants.cc:27: const char
kMinimumVersion[] = "minimum_version";
On 2014/11/17 16:17:41, Joao da Silva wrote:
> Suggestion: rename this to "minimum_version_required", which is more
informative
> when found out of context.

Done.

https://codereview.chromium.org/706623004/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_internal.cc (right):

https://codereview.chromium.org/706623004/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.cc:141: LOG(WARNING) <<
kMalformedPreferenceWarning;
On 2014/11/17 16:17:41, Joao da Silva wrote:
> In this case, the administrator won't see any problem reported in about:policy
> and won't detect that there is a problem with this policy at all.
> 
> WDYT of setting |minimum_version| to 0.0.0.0 in this case?
> 
> Another idea: can you file a bug to get errors out of the parsing of this
> policy? Basically this function could take an "std::string* errors" parameter,
> and instead of logging we would append the errors to that string. Then we
could
> still "return true" in general but show any parsing errors in about:policy.
That
> would be for another CL though.

It's possible to enforce the exact requirement with regex (up to four digits
from 0 to 65535 without leading zero and joined with dot), but I'm just not sure
if it's a good idea to add such unreadable regex to policy template. Another
reason for accepting a general string is that I'm not sure if the extension
format is persistent or not.

These parsing error are actually not expected since we already do verbose
checking in policyhandler (with help of schema). But yes, I left some cases
unresolved.

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

https://codereview.chromium.org/706623004/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1130: // automaticall
automaticallyy.
On 2014/11/17 16:17:41, Joao da Silva wrote:
> "automatically" (only once)

Done.

https://codereview.chromium.org/706623004/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1135: if
(extensions::ExtensionManagementFactory::GetForBrowserContext(profile())
On 2014/11/17 16:17:41, Joao da Silva wrote:
> Get this outside of the loop and put in a local variable

Done.

https://codereview.chromium.org/706623004/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1160: for (size_t i = 0; i <
to_enable.size(); ++i)
On 2014/11/17 16:17:41, Joao da Silva wrote:
> for (const std::string& id : to_enable) { ... }
> 
> Does that work? It would be more consistent with the other loops.

Done. Actually I intended to keep consistent with to_unload routines.

https://codereview.chromium.org/706623004/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1671: }
On 2014/11/17 16:17:41, Joao da Silva wrote:
> What if the extension had more than one disable_reason? We should clear the
> DISABLE_UPDATE_REQUIRED_BY_POLICY flag in that case.

Done. Also made similar change to code above(unsupported requirement).

https://codereview.chromium.org/706623004/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/standard_management_policy_provider.h (right):

https://codereview.chromium.org/706623004/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/standard_management_policy_provider.h:8: #include
<string>
On 2014/11/17 16:17:41, Joao da Silva wrote:
> #include "base/strings/string16.h"

Done.

[Joao da Silva, 2014-11-18 14:18:57]

lgtm after fixing the remaining comments

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

https://codereview.chromium.org/706623004/diff/280001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1162: for (const std::string& id
: to_enable)
Actually, we can also use "const auto&" here. I think it's fine as is.

https://codereview.chromium.org/706623004/diff/280001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1665: id,
Extension::DISABLE_UNSUPPORTED_REQUIREMENT);
|disable_reasons| needs to be updated after this call (also below)

https://codereview.chromium.org/706623004/diff/280001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1680: id,
Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY);
Update |disable_reasons| to remove DISABLE_UPDATE_REQUIRED_BY_POLICY, or just
get it again after removing this reason.

Note that |disable_reasons| will be checked again below.

[binjin, 2014-11-18 14:40:08]

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

[binjin, 2014-11-18 14:40:09]

finnur:
chrome/browser/extensions/*
chrome/browser/resources/extensions/*
chrome/browser/ui/webui/extensions/*
extensions/*

isherman:
tools/metrics/histograms/histograms.xml

Thanks

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

https://codereview.chromium.org/706623004/diff/280001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1162: for (const std::string& id
: to_enable)
On 2014/11/18 14:18:56, Joao da Silva wrote:
> Actually, we can also use "const auto&" here. I think it's fine as is.

Acknowledged.

https://codereview.chromium.org/706623004/diff/280001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1665: id,
Extension::DISABLE_UNSUPPORTED_REQUIREMENT);
On 2014/11/18 14:18:56, Joao da Silva wrote:
> |disable_reasons| needs to be updated after this call (also below)

Done.

https://codereview.chromium.org/706623004/diff/280001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1680: id,
Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY);
On 2014/11/18 14:18:56, Joao da Silva wrote:
> Update |disable_reasons| to remove DISABLE_UPDATE_REQUIRED_BY_POLICY, or just
> get it again after removing this reason.
> 
> Note that |disable_reasons| will be checked again below.

Done.

[Ilya Sherman, 2014-11-18 21:38:15]

histograms.xml lgtm

[Finnur, 2014-11-19 10:16:26]

https://codereview.chromium.org/706623004/diff/320001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5313: +        (This extension is outdated
and disabled by enterprise policy. Please try updating it to a newer version.)
Why parenthesis? The other ExtensionDisabled reasons don't use parenthesis.
Also, I'm not sure it is correct to ask them to update it. Extensions
auto-update and to update manually you'd have to uninstall (which may not be
possible in an Enterprise setting) and find a newer version.

Shouldn't we say instead: It will become enabled automatically when a newer
version is available. ??

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:221: bool
ExtensionManagement::CheckMinimumVersion(
s/CheckMinimumVersion/IsAboveMinimumVersion/ ?

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:221: bool
ExtensionManagement::CheckMinimumVersion(
I started wondering here if minimum_version is respected outside enterprise
settings. Is it? And if so, is that desirable?

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:228: bool met_requirement =
extension->version()->CompareTo(
nit: s/met_requirement/meets_requirement/

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:127: // minimum required
version as string.
s/set it to the minimum required version as string/the minimum required version
is returned/

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_internal.cc (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.cc:38: }
Can you initialize installation_mode here?

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_test_util.cc (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_test_util.cc:196:
pref_->SetString(make_path(id, schema::kMinimumVersionRequired), version);
What happens if blank string is passed in? Actually, why not just let
SetMinimumVersionRequired take a blank |version| string and have that equate to
a nullptr? Then you don't even need the Unset function.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_unittest.cc (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_unittest.cc:177: bool
CheckMinimumVersion(const std::string& id, const std::string& version) {
s/CheckMinimumVersion/IsAboveMinimumVersion ?

It would be good to document the params here. At a glance it would seem you are
checking if extension with |id| has |version| as the minimum version, but that's
not the case.

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

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:856: disable_reason !=
Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY &&
This doesn't look quite right. Disable reasons use a bitmask, so there could be
more than one. You need to replace != with &, or use HasDisableReason from
prefs.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1115: std::vector<std::string>
to_enable;
It seems a bit weird to let the system disable extensions that have only the
DISABLE_UPDATE_REQUIRED_BY_POLICY flag set, and then re-enable them if they've
already passed the min version requirement. Is that necessary?

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1141: if
(extension_prefs_->GetDisableReasons(extension->id()) ==
I would comment this line with:
// Is DISABLE_UPDATE_REQUIRED_BY_POLICY the *only* reason?

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1143: // We need to enable those
disabled only due to minimum version
nit: Indentation is wrong here.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1145:
to_enable.push_back(extension->id());
You've already established here that this is an extension that no longer fails
the min version check -- and that that was the only thing holding it back. Can
we instead remove the id from to_disable so it doesn't get disabled?

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1146:
extension_prefs_->ClearDisableReasons(extension->id());
Delete line 1146, remove the else clause and just always call
RemoveDisableReason for DISABLE_UPDATE_REQUIRED_BY_POLICY (between lines 1151
and 1152).

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1168: // update.
nit: This comment will probably need to be updated if you follow my
recommendations above.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1174:
params.ids.push_back(extension->id());
Is this code *required* to get a disabled extension past the min version or will
that happen anyway at some point? If it is *not* required, then I would not
check *unless* DISABLE_UPDATE_REQUIRED_BY_POLICY is the *only* thing holding it
back (given that we already know it won't become enabled after updating because
more things are holding it back).

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1667: }
Both RemoveDisableReason and the bitmask removal of
DISABLE_UNSUPPORTED_REQUIREMENT will result in disable_reasons becoming
DISABLE_NONE -- assuming DISABLE_UNSUPPORTED_REQUIREMENT was the only thing
remaining. Therefore, I don't think we need to explicitly check here (in two
places now) if DISABLE_UNSUPPORTED_REQUIREMENT was the only thing remaining. We
can just do:

// Unsupported requirements overrides the management policy.
if (install_flags & extensions::kInstallFlagHasRequirementErrors) {
  disable_reasons |= Extension::DISABLE_UNSUPPORTED_REQUIREMENT;
} else {
  // Otherwise just remove the corresponding disable reason.
  extension_prefs_->RemoveDisableReason(
      id, Extension::DISABLE_UNSUPPORTED_REQUIREMENT);
  disable_reasons &= ~Extension::DISABLE_UNSUPPORTED_REQUIREMENT;
}

Right? Similar logic below.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/resource...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_list.js:180:
extension.updateRequiredByPolicy) {
nit: Lets do three lines here, instead of two.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_list.js:431: // an force installed
extension might be disabled due to being outdated
s/an/a/
s/force installed/force-installed/

Is there a message somewhere saying "this extension is required by your admin"?
If so, should we hide that, while this message is visible?

https://codereview.chromium.org/706623004/diff/320001/extensions/common/exten...
File extensions/common/extension.h (right):

https://codereview.chromium.org/706623004/diff/320001/extensions/common/exten...
extensions/common/extension.h:107: DISABLE_UPDATE_REQUIRED_BY_POLICY = 1 << 14,
Maybe comment:
// Doesn't meet minimum version requirement.

https://codereview.chromium.org/706623004/diff/320001/extensions/extensions_s...
File extensions/extensions_strings.grd (right):

https://codereview.chromium.org/706623004/diff/320001/extensions/extensions_s...
extensions/extensions_strings.grd:290: The administrator of this machine
requires <ph name="EXTENSION_NAME">$1<ex>Google Talk</ex></ph> to have a minimum
version of <ph name="EXTENSION_VERSION">$2<ex>1.1.0</ex></ph>. It cannot be
enabled until being updated to the minimum version or higher.
How about: It cannot be enabled until it has updated to that version (or
higher).
?

[binjin, 2014-11-19 13:36:11]

https://codereview.chromium.org/706623004/diff/320001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5313: +        (This extension is outdated
and disabled by enterprise policy. Please try updating it to a newer version.)
On 2014/11/19 10:16:25, Finnur wrote:
> Why parenthesis? The other ExtensionDisabled reasons don't use parenthesis.
> Also, I'm not sure it is correct to ask them to update it. Extensions
> auto-update and to update manually you'd have to uninstall (which may not be
> possible in an Enterprise setting) and find a newer version.
> 
> Shouldn't we say instead: It will become enabled automatically when a newer
> version is available. ??

Removed parenthesis and modified guide to user. The reason I used parenthesis is
that message for force-installed extension used it.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:221: bool
ExtensionManagement::CheckMinimumVersion(
On 2014/11/19 10:16:25, Finnur wrote:
> s/CheckMinimumVersion/IsAboveMinimumVersion/ ?

Done. "above" is not strictly correct, it should actually be "above or equals
to". Renamed to "CheckMinimumVersionRequirement"

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:221: bool
ExtensionManagement::CheckMinimumVersion(
On 2014/11/19 10:16:25, Finnur wrote:
> I started wondering here if minimum_version is respected outside enterprise
> settings. Is it? And if so, is that desirable?

I'm not sure if I understand correctly, but the minimum_version requirement is
an enterprise only settings enforced on particular extension to have a minimum
version to be enabled.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:228: bool met_requirement =
extension->version()->CompareTo(
On 2014/11/19 10:16:25, Finnur wrote:
> nit: s/met_requirement/meets_requirement/

Done.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:127: // minimum required
version as string.
On 2014/11/19 10:16:25, Finnur wrote:
> s/set it to the minimum required version as string/the minimum required
version
> is returned/

Done.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_internal.cc (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.cc:38: }
On 2014/11/19 10:16:25, Finnur wrote:
> Can you initialize installation_mode here?

In a destructor? I assume you mean Parse(), the purpose of Parse() and
constructor is different, Parse() are meant for parsing only and the caller had
to inherit settings from default settings before calling Parse(). That is, if
the installation_mode is not specified, we will use inherited installation_mode
settings.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_test_util.cc (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_test_util.cc:196:
pref_->SetString(make_path(id, schema::kMinimumVersionRequired), version);
On 2014/11/19 10:16:25, Finnur wrote:
> What happens if blank string is passed in? Actually, why not just let
> SetMinimumVersionRequired take a blank |version| string and have that equate
to
> a nullptr? Then you don't even need the Unset function.

By "blank string", do you mean an empty string, or a NULL pointer? I don't
think empty string works here. These are helper functions for tests, and
constructing scoped StringValue to pass is not very convenience.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_unittest.cc (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_unittest.cc:177: bool
CheckMinimumVersion(const std::string& id, const std::string& version) {
On 2014/11/19 10:16:25, Finnur wrote:
> s/CheckMinimumVersion/IsAboveMinimumVersion ?
> 
> It would be good to document the params here. At a glance it would seem you
are
> checking if extension with |id| has |version| as the minimum version, but
that's
> not the case.

Done. See comments in extension_management.h.

Also added comments stating these are just wrappers forfunctions from
ExtensionManagement

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

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:856: disable_reason !=
Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY &&
On 2014/11/19 10:16:26, Finnur wrote:
> This doesn't look quite right. Disable reasons use a bitmask, so there could
be
> more than one. You need to replace != with &, or use HasDisableReason from
> prefs.

I think it's desired, the Extension::DisableReason is an enum stating a single
disable reason. For places that we actually need a bitmask, an int is explictly
used.

Also if there are multiple reasons, we don't want to block that since there are
other reasons to disable the extensions.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1115: std::vector<std::string>
to_enable;
On 2014/11/19 10:16:26, Finnur wrote:
> It seems a bit weird to let the system disable extensions that have only the
> DISABLE_UPDATE_REQUIRED_BY_POLICY flag set, and then re-enable them if they've
> already passed the min version requirement. Is that necessary?

|to_enable| and |to_disable| are mutually exclusive, we collected two lists of
extensions then do corresponding operations. And finally we call updater with
certain extensions.

That is, we might disable certain extensins, but won't enable it immediately,
but we call updater to check a newer version if possible.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1141: if
(extension_prefs_->GetDisableReasons(extension->id()) ==
On 2014/11/19 10:16:26, Finnur wrote:
> I would comment this line with:
> // Is DISABLE_UPDATE_REQUIRED_BY_POLICY the *only* reason?

Done.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1143: // We need to enable those
disabled only due to minimum version
On 2014/11/19 10:16:26, Finnur wrote:
> nit: Indentation is wrong here.
Oops, Done.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1145:
to_enable.push_back(extension->id());
On 2014/11/19 10:16:26, Finnur wrote:
> You've already established here that this is an extension that no longer fails
> the min version check -- and that that was the only thing holding it back. Can
> we instead remove the id from to_disable so it doesn't get disabled?

|to_enable| and |to_disable| are mutually exclusive, see comments above.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1146:
extension_prefs_->ClearDisableReasons(extension->id());
On 2014/11/19 10:16:26, Finnur wrote:
> Delete line 1146, remove the else clause and just always call
> RemoveDisableReason for DISABLE_UPDATE_REQUIRED_BY_POLICY (between lines 1151
> and 1152).

Done.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1168: // update.
On 2014/11/19 10:16:26, Finnur wrote:
> nit: This comment will probably need to be updated if you follow my
> recommendations above.

Acknowledged.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1174:
params.ids.push_back(extension->id());
On 2014/11/19 10:16:25, Finnur wrote:
> Is this code *required* to get a disabled extension past the min version or
will
> that happen anyway at some point? If it is *not* required, then I would not
> check *unless* DISABLE_UPDATE_REQUIRED_BY_POLICY is the *only* thing holding
it
> back (given that we already know it won't become enabled after updating
because
> more things are holding it back).

Done.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1667: }
On 2014/11/19 10:16:26, Finnur wrote:
> Both RemoveDisableReason and the bitmask removal of
> DISABLE_UNSUPPORTED_REQUIREMENT will result in disable_reasons becoming
> DISABLE_NONE -- assuming DISABLE_UNSUPPORTED_REQUIREMENT was the only thing
> remaining. Therefore, I don't think we need to explicitly check here (in two
> places now) if DISABLE_UNSUPPORTED_REQUIREMENT was the only thing remaining.
We
> can just do:
> 
> // Unsupported requirements overrides the management policy.
> if (install_flags & extensions::kInstallFlagHasRequirementErrors) {
>   disable_reasons |= Extension::DISABLE_UNSUPPORTED_REQUIREMENT;
> } else {
>   // Otherwise just remove the corresponding disable reason.
>   extension_prefs_->RemoveDisableReason(
>       id, Extension::DISABLE_UNSUPPORTED_REQUIREMENT);
>   disable_reasons &= ~Extension::DISABLE_UNSUPPORTED_REQUIREMENT;
> }
> 
> Right? Similar logic below.

Done.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/resource...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_list.js:180:
extension.updateRequiredByPolicy) {
On 2014/11/19 10:16:26, Finnur wrote:
> nit: Lets do three lines here, instead of two.

Done.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_list.js:431: // an force installed
extension might be disabled due to being outdated
On 2014/11/19 10:16:26, Finnur wrote:
> s/an/a/
> s/force installed/force-installed/
> 
> Is there a message somewhere saying "this extension is required by your
admin"?
> If so, should we hide that, while this message is visible?

Done. I prefer to show both message so that user are aware of the conflict.
Maybe we can tell additional details in this case?

https://codereview.chromium.org/706623004/diff/320001/extensions/common/exten...
File extensions/common/extension.h (right):

https://codereview.chromium.org/706623004/diff/320001/extensions/common/exten...
extensions/common/extension.h:107: DISABLE_UPDATE_REQUIRED_BY_POLICY = 1 << 14,
On 2014/11/19 10:16:26, Finnur wrote:
> Maybe comment:
> // Doesn't meet minimum version requirement.

Done.

https://codereview.chromium.org/706623004/diff/320001/extensions/extensions_s...
File extensions/extensions_strings.grd (right):

https://codereview.chromium.org/706623004/diff/320001/extensions/extensions_s...
extensions/extensions_strings.grd:290: The administrator of this machine
requires <ph name="EXTENSION_NAME">$1<ex>Google Talk</ex></ph> to have a minimum
version of <ph name="EXTENSION_VERSION">$2<ex>1.1.0</ex></ph>. It cannot be
enabled until being updated to the minimum version or higher.
On 2014/11/19 10:16:26, Finnur wrote:
> How about: It cannot be enabled until it has updated to that version (or
> higher).
> ?

Done.

[Finnur, 2014-11-19 15:47:39]

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:221: bool
ExtensionManagement::CheckMinimumVersion(
Adding 'Requirement' suffix feels redundant here. I'd lose it.

> is an enterprise only settings enforced on particular extension to 
> have a minimum version to be enabled.

I was asking what happens if you add this string in the manifest to a
non-managed extension.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_unittest.cc (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_unittest.cc:177: bool
CheckMinimumVersion(const std::string& id, const std::string& version) {
I think you missed the point on the comment. You mention in the comment that
this function is a wrapper around CheckMinimumVersion in extension_management,
yet the wrapper has a different type for |version| and uses it in a different
way than the function it wraps. That is what should be clarified with a comment,
that |version| is the version the constructed extension will be

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

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:856: disable_reason !=
Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY &&
Ah, right! Sorry.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1115: std::vector<std::string>
to_enable;
Hmmm, that's totally non-obvious due to the MustRemainDisabled call. Does that
function return TRUE for extensions failing this new minimum version check? Is
that expected?

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/resource...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_list.js:431: // an force installed
extension might be disabled due to being outdated
You got the wrong An->a conversion there (I was referring to line 431, not 430).

As for the two messages, this is what we would show, right?

"The administrator of this machine requires FOO to be installed. It cannot be
removed or modified."
"The administrator of this machine requires FOO to have a minimum version of
BAR. It cannot be enabled until it has updated to that version (or higher)."

I think there's a lot of redundancy there and you could hide the first message
and change the second message to say:

"The administrator of this machine requires FOO to have a minimum version of
BAR. It will remain disabled until it has updated to that version (or higher)."

That basically covers the gist of the other two messages (admin required + can't
be modified by user + min version).

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

https://codereview.chromium.org/706623004/diff/340001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1655: // Satisfies now, remove
the corresponding disable reason instead.
"Satisfies now" is weird.

But more importantly, I think there's a bug in the code now.  It is now removing
the disable reason _only_ if DISABLE_UNSUPPORTED_REQUIREMENT was the sole reason
it was disabled. Compare my else clause to your else if. I think you need to
convert this to a plain else, right?

[binjin, 2014-11-19 17:21:42]

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:221: bool
ExtensionManagement::CheckMinimumVersion(
On 2014/11/19 15:47:38, Finnur wrote:
> Adding 'Requirement' suffix feels redundant here. I'd lose it.
> 
> > is an enterprise only settings enforced on particular extension to 
> > have a minimum version to be enabled.
> 
> I was asking what happens if you add this string in the manifest to a
> non-managed extension.

This is not added to manifest of extension, it's part of a dictionary controlled
directly by policy.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_unittest.cc (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_unittest.cc:177: bool
CheckMinimumVersion(const std::string& id, const std::string& version) {
On 2014/11/19 15:47:38, Finnur wrote:
> I think you missed the point on the comment. You mention in the comment that
> this function is a wrapper around CheckMinimumVersion in extension_management,
> yet the wrapper has a different type for |version| and uses it in a different
> way than the function it wraps. That is what should be clarified with a
comment,
> that |version| is the version the constructed extension will be

Oh, I see. Added comments for every wrapper.

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

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1115: std::vector<std::string>
to_enable;
On 2014/11/19 15:47:38, Finnur wrote:
> Hmmm, that's totally non-obvious due to the MustRemainDisabled call. Does that
> function return TRUE for extensions failing this new minimum version check? Is
> that expected?

Yes, we use MustRemainDisabled() for that purpose. We prepare these three lists,
and after that we do the operation. Thus the two enumeration loops below is kind
of independent.

The |to_enable| list are selected from |registry_->disabled_extensions()|, and
|to_disable| list are selected from |registry_->enabled_extensions()|, that's
the reason they will be mutally exclusive.

Added few more comments on this.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/resource...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_list.js:431: // an force installed
extension might be disabled due to being outdated
On 2014/11/19 15:47:39, Finnur wrote:
> You got the wrong An->a conversion there (I was referring to line 431, not
430).

Oops. Fixed.

> 
> As for the two messages, this is what we would show, right?
> 
> "The administrator of this machine requires FOO to be installed. It cannot be
> removed or modified."
> "The administrator of this machine requires FOO to have a minimum version of
> BAR. It cannot be enabled until it has updated to that version (or higher)."
> 
> I think there's a lot of redundancy there and you could hide the first message
> and change the second message to say:
> 
> "The administrator of this machine requires FOO to have a minimum version of
> BAR. It will remain disabled until it has updated to that version (or
higher)."
> 
> That basically covers the gist of the other two messages (admin required +
can't
> be modified by user + min version).

Done.

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

https://codereview.chromium.org/706623004/diff/340001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1655: // Satisfies now, remove
the corresponding disable reason instead.
On 2014/11/19 15:47:39, Finnur wrote:
> "Satisfies now" is weird.
> 
> But more importantly, I think there's a bug in the code now.  It is now
removing
> the disable reason _only_ if DISABLE_UNSUPPORTED_REQUIREMENT was the sole
reason
> it was disabled. Compare my else clause to your else if. I think you need to
> convert this to a plain else, right?

Ooops, yes.

[Finnur, 2014-11-19 20:50:18]

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:221: bool
ExtensionManagement::CheckMinimumVersion(
OK, fine. If this is only set-able from the dictionary controlled by the policy,
then there's nothing further to discuss.

But I still think you should drop the Requirement suffix from the function name.
:)

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

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1115: std::vector<std::string>
to_enable;
But... that's my point. Isn't it a bug that MustRemainDisabled returns TRUE for
an extension that fails the min_version check? Will that lead to an extension
getting enabled when it should be disabled?

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

https://codereview.chromium.org/706623004/diff/340001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1655: // Satisfies now, remove
the corresponding disable reason instead.
I'm sorry, but I'm totally lost now.

How did we go from "yes, Finnur's right and we need to convert an 'else if' into
an 'else'" to this solution?

Can you walk me through what is happening? Specifically, why do we _only_ remove
DISABLE_UNSUPPORTED_REQUIREMENT (lines 1656-8) when it is the _sole_ disable
reason left (line 1653-4)? That doesn't seem right.

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

https://codereview.chromium.org/706623004/diff/360001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1169:
extensions::ExtensionUpdater::CheckParams params;
Nit: params is a bit general. To be consistent with the other vars it would make
sense name this: to_recheck (or to_check_again). :)

https://codereview.chromium.org/706623004/diff/360001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1174:
params.ids.push_back(extension->id());
Nit: Comment this line with:
// The minimum version check is the only thing holding this 
// extension back, so check if it can be updated to fix that.
...  or something to that effect.

[binjin, 2014-11-19 21:31:41]

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

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1115: std::vector<std::string>
to_enable;
On 2014/11/19 20:50:18, Finnur wrote:
> But... that's my point. Isn't it a bug that MustRemainDisabled returns TRUE
for
> an extension that fails the min_version check? Will that lead to an extension
> getting enabled when it should be disabled?

I'm confused. MustRemainDisaabled() will add an extension to |to_disable| list,
but we won't enable extensions from |to_disable|. We only tell updater to check
immediately if there is a newer version for it or not. After fetching and
installed the crx, OnExtensionInstalled() will enable it much later if the new
version meets the minimum version requirement.

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

https://codereview.chromium.org/706623004/diff/340001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1655: // Satisfies now, remove
the corresponding disable reason instead.
On 2014/11/19 20:50:18, Finnur wrote:
> I'm sorry, but I'm totally lost now.
> 
> How did we go from "yes, Finnur's right and we need to convert an 'else if'
into
> an 'else'" to this solution?
> 
> Can you walk me through what is happening? Specifically, why do we _only_
remove
> DISABLE_UNSUPPORTED_REQUIREMENT (lines 1656-8) when it is the _sole_ disable
> reason left (line 1653-4)? That doesn't seem right.

I'm confused as well at this point. I think my solution is exactly "convert an
'else if' into an 'else'".

The behavior is slightly different from the original code but I think it's still
correct.

For ExtensionPrefs, "ClearDisableReasons(id);" clears all bits, and
"RemoveDisableReason(id, DISABLE_REASON);"  clear a single bits, they are
logically same.

For 'disable_reasons', the original code "disable_reasons =
Extension::DISABLE_NONE;" is overriding the previous result, but the modified
code have slightly different behavior. "disable_reasons &=
~Extension::DISABLE_UNSUPPORTED_REQUIREMENT;" just discard the reason we are
sure not holding now. Both behavior works I think.

[Finnur, 2014-11-20 12:25:35]

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

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1115: std::vector<std::string>
to_enable;
Sorry, this confusion is, I think, on my part and is a combination of me being
jetlagged and trying to rush through the review at the last minute to give you
feedback before I leave for the day.

I think you are saying that MustRemainDisabled will return TRUE for extensions
with this new DISABLE_UPDATE_REQUIRED_BY_POLICY, in which case I don't have a
concern. If it does not let me know.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1162: for (const std::string& id
: to_enable)
I would comment this |for| loop with something like:
// No extension is getting re-enabled here after disabling/unloading
// because to_enable is mutually exclusive to to_disable + to_unload.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1168: // update.
Suggest for easier reading:
// Find all extensions disabled due to minimum version requirement from
// policy (including the ones that got disabled just now), and check for
// update.

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

https://codereview.chromium.org/706623004/diff/340001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1655: // Satisfies now, remove
the corresponding disable reason instead.
Again, I think the confusion is probably my fault. I notice now that I did
attribute some weirdness in the code to your CL, when in fact it was part of the
original code. I'll make another comment elsewhere, I think that would be easier
to comprehend, rather than continue this thread.

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

https://codereview.chromium.org/706623004/diff/360001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1169:
extensions::ExtensionUpdater::CheckParams params;
What about these two (line 1174 also)?

https://codereview.chromium.org/706623004/diff/360001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1671: }
So, I agree with you, that this looks like what we should be doing. If we detect
that the faulty condition no longer applies, we can remove these disable
reasons. 

However, can you track down the author of the original check (lines 1607-1617 on
the left) and ask why DISABLE_UNSUPPORTED_REQUIREMENT was not being removed
unless it was the *last* disable reason? Just to make sure we're not introducing
a bug here? I suspect the answer is "because it won't be enabled anyway, unless
it is the last one", but we should make sure.

[binjin, 2014-11-20 13:11:54]

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:221: bool
ExtensionManagement::CheckMinimumVersion(
On 2014/11/19 20:50:18, Finnur wrote:
> OK, fine. If this is only set-able from the dictionary controlled by the
policy,
> then there's nothing further to discuss.
> 
> But I still think you should drop the Requirement suffix from the function
name.
> :)

Done (dropped suffix).

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

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1162: for (const std::string& id
: to_enable)
On 2014/11/20 12:25:34, Finnur wrote:
> I would comment this |for| loop with something like:
> // No extension is getting re-enabled here after disabling/unloading
> // because to_enable is mutually exclusive to to_disable + to_unload.

Done.

https://codereview.chromium.org/706623004/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1168: // update.
On 2014/11/20 12:25:34, Finnur wrote:
> Suggest for easier reading:
> // Find all extensions disabled due to minimum version requirement from
> // policy (including the ones that got disabled just now), and check for
> // update.
> 

Done.

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

https://codereview.chromium.org/706623004/diff/360001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1169:
extensions::ExtensionUpdater::CheckParams params;
On 2014/11/20 12:25:35, Finnur wrote:
> What about these two (line 1174 also)?

Sorry, I yesterday was also trying to make quick reply for things I got
confused. And thus ignored other nits.

https://codereview.chromium.org/706623004/diff/360001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1169:
extensions::ExtensionUpdater::CheckParams params;
On 2014/11/20 12:25:35, Finnur wrote:
> What about these two (line 1174 also)?

I yesterday was also trying to make quick response for things I got confused. I
didn't make any changes.

https://codereview.chromium.org/706623004/diff/360001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1174:
params.ids.push_back(extension->id());
On 2014/11/19 20:50:18, Finnur wrote:
> Nit: Comment this line with:
> // The minimum version check is the only thing holding this 
> // extension back, so check if it can be updated to fix that.
> ...  or something to that effect.

Done.

https://codereview.chromium.org/706623004/diff/360001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1671: }
On 2014/11/20 12:25:35, Finnur wrote:
> So, I agree with you, that this looks like what we should be doing. If we
detect
> that the faulty condition no longer applies, we can remove these disable
> reasons. 
> 
> However, can you track down the author of the original check (lines 1607-1617
on
> the left) and ask why DISABLE_UNSUPPORTED_REQUIREMENT was not being removed
> unless it was the *last* disable reason? Just to make sure we're not
introducing
> a bug here? I suspect the answer is "because it won't be enabled anyway,
unless
> it is the last one", but we should make sure.

Acknowledged.

[binjin, 2014-11-20 13:17:17]

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

https://codereview.chromium.org/706623004/diff/360001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1671: }
On 2014/11/20 12:25:35, Finnur wrote:
> So, I agree with you, that this looks like what we should be doing. If we
detect
> that the faulty condition no longer applies, we can remove these disable
> reasons. 
> 
> However, can you track down the author of the original check (lines 1607-1617
on
> the left) and ask why DISABLE_UNSUPPORTED_REQUIREMENT was not being removed
> unless it was the *last* disable reason? Just to make sure we're not
introducing
> a bug here? I suspect the answer is "because it won't be enabled anyway,
unless
> it is the last one", but we should make sure.

@eaugusti, sorry for bothering, you are the original author of code removing
Extension::DISABLE_UNSUPPORTED_REQUIREMENT here. See
https://codereview.chromium.org/10908184/diff/1/chrome/browser/extensions/ext...

Could you clarify the reason we only remove this as the LAST disable reason? The
current modified behavior is that we will remove this disable reason even if
there are other reasons. And only re-enable it if there are no other disable
reasons.

[binjin, 2014-11-25 11:26:57]

Finnur, could you please have a look at my fixes addressing your comments?

[Finnur, 2014-11-27 13:38:40]

I believe they do. LGTM.

[binjin, 2014-11-27 13:42:18]

The CQ bit was checked by binjin@chromium.org

[binjin, 2014-11-27 13:42:24]

The CQ bit was unchecked by binjin@chromium.org

[binjin, 2014-11-27 13:42:55]

The CQ bit was unchecked by binjin@chromium.org

[binjin, 2014-11-27 13:46:42]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2014-11-27 13:47:01]

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

[binjin, 2014-11-27 14:49:58]

The CQ bit was unchecked by binjin@chromium.org

[binjin, 2014-12-01 13:44:45]

Patchset #22 (id:420001) has been deleted

[binjin, 2014-12-01 14:54:59]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2014-12-01 14:55:49]

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

[binjin, 2014-12-01 14:59:12]

The CQ bit was unchecked by binjin@chromium.org

[binjin, 2014-12-03 12:18:46]

Patchset #23 (id:460001) has been deleted

[binjin, 2014-12-03 12:21:45]

Joao, could you have a look at latest patchset, for a fix to the flaky browser
test?

[Joao da Silva, 2014-12-03 12:25:45]

https://codereview.chromium.org/706623004/diff/500001/chrome/browser/policy/p...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/706623004/diff/500001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2012:
interceptor.AddRequestServicedCallback(base::Bind(
This must be prepared before the policy is modified, otherwise it's going to be
racy.

Try this instead:

ASSERT_EQ(2u, interceptor.GetPendingSize());
base::RunLoop service_request_run_loop;
interceptor.AddRequestServicedCallback(service_request_run_loop.QuitClosure());
{
  // change the policy
}
service_request_run_loop.Run();

Note also that you can pass the QuitClosure directly as the callback, no need to
bind another PostTask.

[binjin, 2014-12-03 12:40:00]

https://codereview.chromium.org/706623004/diff/500001/chrome/browser/policy/p...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/706623004/diff/500001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2012:
interceptor.AddRequestServicedCallback(base::Bind(
On 2014/12/03 12:25:45, Joao da Silva wrote:
> This must be prepared before the policy is modified, otherwise it's going to
be
> racy.
> 
> Try this instead:
> 
> ASSERT_EQ(2u, interceptor.GetPendingSize());
> base::RunLoop service_request_run_loop;
>
interceptor.AddRequestServicedCallback(service_request_run_loop.QuitClosure());
> {
>   // change the policy
> }
> service_request_run_loop.Run();
> 
> Note also that you can pass the QuitClosure directly as the callback, no need
to
> bind another PostTask.

Done.

[Joao da Silva, 2014-12-03 12:42:50]

lgtm, thanks!

[binjin, 2014-12-03 12:58:48]

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

[binjin, 2014-12-03 12:58:49]

Bartosz, I need another owner approval for policy code, since Joao is not an
OWNER now. Please have a look at 
chrome/browser/policy/policy_browsertest.cc and
components/policy/resources/policy_templates.json

[bartfab (slow), 2014-12-04 13:53:01]

chrome/browser/policy/policy_browsertest.cc and
components/policy/resources/policy_templates.json LGTM.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:18: #include
"base/message_loop/message_loop_proxy.h"
Nit: You do not need this include. You never dereference the MessageLoopProxy
type.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:1965: // Verifies that extensions
with version older than minimum version required by
Nit: s/than/than the/

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:1989:
ASSERT_TRUE(InstallExtension(kGoodV1CrxName));
Nit: s/ASSERT/EXPECT/

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:1990:
ASSERT_TRUE(registry->enabled_extensions().Contains(kGoodCrxId));
Nit: s/ASSERT/EXPECT/

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2000:
ASSERT_TRUE(registry->enabled_extensions().Contains(kGoodCrxId));
Nit: s/ASSERT/EXPECT/

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2004: ASSERT_EQ(2u,
interceptor.GetPendingSize());
Nit: s/ASSERT/EXPECT/

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2013: ASSERT_EQ(1u,
interceptor.GetPendingSize());
Nit: s/ASSERT/EXPECT/

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2015:
ASSERT_TRUE(registry->disabled_extensions().Contains(kGoodCrxId));
Nit: s/ASSERT/EXPECT/

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2020: // via update URL in the
manifest of the older version.
Nit: s/via/via the/

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2021: ASSERT_EQ(1u,
interceptor.GetPendingSize());
Nit: s/ASSERT/EXPECT/

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2029: ASSERT_EQ(0u,
interceptor.GetPendingSize());
Nit: s/ASSERT/EXPECT/

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2066:
ASSERT_TRUE(InstallExtension(kGoodV1CrxName));
Nit: s/ASSERT/EXPECT/

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2067:
ASSERT_TRUE(registry->disabled_extensions().Contains(kGoodCrxId));
Nit: s/ASSERT/EXPECT/

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2074: ASSERT_EQ(1u,
interceptor.GetPendingSize());
Nit: s/ASSERT/EXPECT/

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2080: // Set a higher minimum
version, just intend to trigger an policy update.
Nit: s/an/a/

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2088: ASSERT_EQ(0u,
interceptor.GetPendingSize());
Nit: s/ASSERT/EXPECT/

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2097: // Remove the minimum version
requirement, it should be re-enabled.
Nit: s/, it/. The extension/

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2109: // Verifies that force
installed missing the minimum version requirement is
Nit: s/force installed missing the/a force-installed extension which does not
meet a subsequently set/

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2118: base::FilePath path =
Nit: const.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2120: GURL
url(URLRequestMockHTTPJob::GetMockUrl(path));
Nit: const.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2122: // Set policy to force install
the extension, it should be installed and
Nit: s/force /force-/

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2127:
ASSERT_FALSE(registry->enabled_extensions().Contains(kGoodCrxId));
Nit: s/ASSERT/EXPECT/

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2136:
ASSERT_TRUE(registry->enabled_extensions().Contains(kGoodCrxId));
Nit: s/ASSERT/EXPECT/

https://codereview.chromium.org/706623004/diff/520001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/706623004/diff/520001/components/policy/resou...
components/policy/resources/policy_templates.json:1899:
"minimum_version_required": maps to a version string. The format of the version
string is the same as the one used in extension manifest, as described at <ph
name="LINK_TO_EXTENSION_DOC4">https://developer.chrome.com/apps/manifest/version</ph>.
Extension with version older than the specified minimum version will be
disabled. This applies to force-installed extensions as well.
Nit: s/Extension with/An extension with a/

[binjin, 2014-12-04 15:43:16]

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:18: #include
"base/message_loop/message_loop_proxy.h"
On 2014/12/04 13:53:01, bartfab wrote:
> Nit: You do not need this include. You never dereference the MessageLoopProxy
> type.

Done. It's needed in previous patchset and I forget to remove this.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:1965: // Verifies that extensions
with version older than minimum version required by
On 2014/12/04 13:53:00, bartfab wrote:
> Nit: s/than/than the/

Done.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:1989:
ASSERT_TRUE(InstallExtension(kGoodV1CrxName));
On 2014/12/04 13:53:01, bartfab wrote:
> Nit: s/ASSERT/EXPECT/

Done.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:1990:
ASSERT_TRUE(registry->enabled_extensions().Contains(kGoodCrxId));
On 2014/12/04 13:53:00, bartfab wrote:
> Nit: s/ASSERT/EXPECT/

Done.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2000:
ASSERT_TRUE(registry->enabled_extensions().Contains(kGoodCrxId));
On 2014/12/04 13:53:00, bartfab wrote:
> Nit: s/ASSERT/EXPECT/

Done.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2004: ASSERT_EQ(2u,
interceptor.GetPendingSize());
On 2014/12/04 13:53:01, bartfab wrote:
> Nit: s/ASSERT/EXPECT/

Done.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2013: ASSERT_EQ(1u,
interceptor.GetPendingSize());
On 2014/12/04 13:53:00, bartfab wrote:
> Nit: s/ASSERT/EXPECT/

Done.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2015:
ASSERT_TRUE(registry->disabled_extensions().Contains(kGoodCrxId));
On 2014/12/04 13:53:01, bartfab wrote:
> Nit: s/ASSERT/EXPECT/

Done.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2020: // via update URL in the
manifest of the older version.
On 2014/12/04 13:53:01, bartfab wrote:
> Nit: s/via/via the/

Done.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2021: ASSERT_EQ(1u,
interceptor.GetPendingSize());
On 2014/12/04 13:53:00, bartfab wrote:
> Nit: s/ASSERT/EXPECT/

Done.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2029: ASSERT_EQ(0u,
interceptor.GetPendingSize());
On 2014/12/04 13:53:00, bartfab wrote:
> Nit: s/ASSERT/EXPECT/

Done.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2066:
ASSERT_TRUE(InstallExtension(kGoodV1CrxName));
On 2014/12/04 13:53:01, bartfab wrote:
> Nit: s/ASSERT/EXPECT/

Done.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2067:
ASSERT_TRUE(registry->disabled_extensions().Contains(kGoodCrxId));
On 2014/12/04 13:53:00, bartfab wrote:
> Nit: s/ASSERT/EXPECT/

Done.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2074: ASSERT_EQ(1u,
interceptor.GetPendingSize());
On 2014/12/04 13:53:00, bartfab wrote:
> Nit: s/ASSERT/EXPECT/

Done.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2080: // Set a higher minimum
version, just intend to trigger an policy update.
On 2014/12/04 13:53:00, bartfab wrote:
> Nit: s/an/a/

Done.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2088: ASSERT_EQ(0u,
interceptor.GetPendingSize());
On 2014/12/04 13:53:00, bartfab wrote:
> Nit: s/ASSERT/EXPECT/

Done.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2097: // Remove the minimum version
requirement, it should be re-enabled.
On 2014/12/04 13:53:00, bartfab wrote:
> Nit: s/, it/. The extension/

Done.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2118: base::FilePath path =
On 2014/12/04 13:53:00, bartfab wrote:
> Nit: const.

Done.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2120: GURL
url(URLRequestMockHTTPJob::GetMockUrl(path));
On 2014/12/04 13:53:00, bartfab wrote:
> Nit: const.

Done.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2122: // Set policy to force install
the extension, it should be installed and
On 2014/12/04 13:53:00, bartfab wrote:
> Nit: s/force /force-/

Done.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2127:
ASSERT_FALSE(registry->enabled_extensions().Contains(kGoodCrxId));
On 2014/12/04 13:53:00, bartfab wrote:
> Nit: s/ASSERT/EXPECT/

Done.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2127:
ASSERT_FALSE(registry->enabled_extensions().Contains(kGoodCrxId));
On 2014/12/04 13:53:00, bartfab wrote:
> Nit: s/ASSERT/EXPECT/

Done.

https://codereview.chromium.org/706623004/diff/520001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2136:
ASSERT_TRUE(registry->enabled_extensions().Contains(kGoodCrxId));
On 2014/12/04 13:53:00, bartfab wrote:
> Nit: s/ASSERT/EXPECT/

Done.

https://codereview.chromium.org/706623004/diff/520001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/706623004/diff/520001/components/policy/resou...
components/policy/resources/policy_templates.json:1899:
"minimum_version_required": maps to a version string. The format of the version
string is the same as the one used in extension manifest, as described at <ph
name="LINK_TO_EXTENSION_DOC4">https://developer.chrome.com/apps/manifest/version</ph>.
Extension with version older than the specified minimum version will be
disabled. This applies to force-installed extensions as well.
On 2014/12/04 13:53:01, bartfab wrote:
> Nit: s/Extension with/An extension with a/

Done.

[binjin, 2014-12-04 16:05:05]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2014-12-04 16:05:39]

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

[commit-bot: I haz the power, 2014-12-04 16:44:40]

Committed patchset #26 (id:540001)

[commit-bot: I haz the power, 2014-12-04 16:45:21]

Patchset 26 (id:??) landed as
https://crrev.com/8e3d01889d55c01618e575d0b8271a24a50cb5c2
Cr-Commit-Position: refs/heads/master@{#306834}