Move ChromeRequirementsChecker to //extensions as a PreloadCheck

Move ChromeRequirementsChecker to //extensions as a PreloadCheck

Remove the "Chrome" from ChromeRequirementsChecker. Make it implement
PreloadCheck and simplify its async logic a bit. Transform
RequirementsCheckerBrowserTest to a unit test.

This lets AppShell and other modules use RequirementsChecker. (This
wasn't possible until GPUFeatureChecker was moved to //content in
http://crrev.com/2666243002.)

BUG=679971
R=rdevlin.cronin@chromium.org
TBR=bauerb@chromium.org

Review-Url: https://codereview.chromium.org/2783813002
Cr-Commit-Position: refs/heads/master@{#462607}
Committed: https://chromium.googlesource.com/chromium/src/+/a8ea037e3ee7b0f25d694801bd4accb8f22b46c4

[michaelpg, 2017-03-29 01:29:18]

Description was changed from

==========
Move ChromeRequirementsChecker to //extensions as a PreloadCheck

Remove the "Chrome" from ChromeRequirementsChecker. Make it implement
PreloadCheck and simplify its async logic a bit. Transform
RequirementsCheckerBrowserTest to a unit test.

BUG=679971
R=rdevlin.cronin@chromium.org
==========

to

==========
Move ChromeRequirementsChecker to //extensions as a PreloadCheck

Remove the "Chrome" from ChromeRequirementsChecker. Make it implement
PreloadCheck and simplify its async logic a bit. Transform
RequirementsCheckerBrowserTest to a unit test.

This lets AppShell and other modules use RequirementsChecker. (This
wasn't possible until GPUFeatureChecker was moved to //content in
http://crrev.com/2666243002.)

BUG=679971
R=rdevlin.cronin@chromium.org
==========

[michaelpg, 2017-03-29 01:30:03]

michaelpg@chromium.org changed reviewers:
+ piman@chromium.org

[michaelpg, 2017-03-29 01:35:26]

Devlin, PTAL.

piman: for gpu/config in extensions/browser/DEPS.

[piman, 2017-03-29 17:38:36]

lgtm

[michaelpg, 2017-03-30 02:36:09]

https://codereview.chromium.org/2783813002/diff/60001/extensions/browser/requ...
File extensions/browser/requirements_checker_unittest.cc (right):

https://codereview.chromium.org/2783813002/diff/60001/extensions/browser/requ...
extensions/browser/requirements_checker_unittest.cc:81:
ASSERT_TRUE(requirements_handler_.Parse(extension_.get(), &handler_error));
hmm... so loading/parsing a manifest this way DCHECKs in a couple different
ways. i'll figure out why tomorrow

[Devlin, 2017-03-30 17:10:55]

https://codereview.chromium.org/2783813002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_checker.cc (right):

https://codereview.chromium.org/2783813002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:108:
base::UTF16ToUTF8(requirements_check_->GetErrorMessage());
This back and forth between conversions is a shame.  Can we just store it as a
base::string16 on this object?

https://codereview.chromium.org/2783813002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:148: if
(requirements_error_message_.size())
nit: I think we have a slight preference to prefer !empty to size.

It's also a little weird to me that we rely on different indicators here for
whether there's an error - checking the code for blacklist error and the message
for policy and requirements errors.  It'd be cleaner to see if we can coalesce
those, but not a big deal right now (and can be saved for later cls).

https://codereview.chromium.org/2783813002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_checker_unittest.cc (right):

https://codereview.chromium.org/2783813002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker_unittest.cc:98: std::string
message) {
const &

https://codereview.chromium.org/2783813002/diff/60001/extensions/browser/requ...
File extensions/browser/requirements_checker_unittest.cc (right):

https://codereview.chromium.org/2783813002/diff/60001/extensions/browser/requ...
extensions/browser/requirements_checker_unittest.cc:101: base::ListValue*
features_list;
nit: initialize to nullptr

https://codereview.chromium.org/2783813002/diff/60001/extensions/browser/requ...
extensions/browser/requirements_checker_unittest.cc:111: CHECK(!called);
ASSERT_FALSE?

https://codereview.chromium.org/2783813002/diff/60001/extensions/browser/requ...
extensions/browser/requirements_checker_unittest.cc:115: "{\n"
this would be prettier with raw string literals. :)

Though, FYI, https://codereview.chromium.org/2756793003/ is coming at some
point, and this will be quite a bit shorter then.

https://codereview.chromium.org/2783813002/diff/60001/extensions/browser/requ...
extensions/browser/requirements_checker_unittest.cc:175: if
(!MightSupportWebGL()) {
Won't the blacklist still take effect, even if the check won't fail immediately?

[michaelpg, 2017-03-30 22:49:01]

Devlin, PTAL. A bit of churn to move plugins_handler, but it'd be hard to land
some permutation of {requirements checker, requirements test change, plugins
handler move} in separate CLs.

https://codereview.chromium.org/2783813002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_checker.cc (right):

https://codereview.chromium.org/2783813002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:108:
base::UTF16ToUTF8(requirements_check_->GetErrorMessage());
On 2017/03/30 17:10:55, Devlin wrote:
> This back and forth between conversions is a shame.  Can we just store it as a
> base::string16 on this object?

Done.

https://codereview.chromium.org/2783813002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:148: if
(requirements_error_message_.size())
On 2017/03/30 17:10:55, Devlin wrote:
> nit: I think we have a slight preference to prefer !empty to size.

Done. It's not slight -- apparently it's about 35:1, in /chrome and /extensions
:-O

> 
> It's also a little weird to me that we rely on different indicators here for
> whether there's an error - checking the code for blacklist error and the
message
> for policy and requirements errors.  It'd be cleaner to see if we can coalesce
> those, but not a big deal right now (and can be saved for later cls).

Agreed. I'm not sure how to fix that, since requirements errors could be any of
a number of codes and there isn't not a pretty way of writing "if foo in {...}".
Maybe a helper function? Stay tuned for the next CL.

https://codereview.chromium.org/2783813002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_checker_unittest.cc (right):

https://codereview.chromium.org/2783813002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker_unittest.cc:98: std::string
message) {
On 2017/03/30 17:10:55, Devlin wrote:
> const &

Done.

https://codereview.chromium.org/2783813002/diff/60001/extensions/browser/requ...
File extensions/browser/requirements_checker_unittest.cc (right):

https://codereview.chromium.org/2783813002/diff/60001/extensions/browser/requ...
extensions/browser/requirements_checker_unittest.cc:81:
ASSERT_TRUE(requirements_handler_.Parse(extension_.get(), &handler_error));
On 2017/03/30 02:36:09, michaelpg wrote:
> hmm... so loading/parsing a manifest this way DCHECKs in a couple different
> ways. i'll figure out why tomorrow

Okay, ManifestHandler::Parse can't be called here because the extension has
already "finished" parsing its manifest. We can't register the
RequirementsHandler ourselves; the TestExtensionsClient calls
RegisterCommonManifestHandlers() at startup, then finalizes the registry.

I think the simplest solution is to move RequirementsHandler::Register into
common_manifest_handlers.cc since it doesn't require Chrome. It *does* require
PluginsHandler, though, so that one would also have to be moved to //extensions;
that seems fine, too,

https://codereview.chromium.org/2783813002/diff/60001/extensions/browser/requ...
extensions/browser/requirements_checker_unittest.cc:101: base::ListValue*
features_list;
On 2017/03/30 17:10:55, Devlin wrote:
> nit: initialize to nullptr

Done.

https://codereview.chromium.org/2783813002/diff/60001/extensions/browser/requ...
extensions/browser/requirements_checker_unittest.cc:111: CHECK(!called);
On 2017/03/30 17:10:55, Devlin wrote:
> ASSERT_FALSE?

Done. (This was mostly moved from the browser_test)

https://codereview.chromium.org/2783813002/diff/60001/extensions/browser/requ...
extensions/browser/requirements_checker_unittest.cc:115: "{\n"
On 2017/03/30 17:10:55, Devlin wrote:
> this would be prettier with raw string literals. :)
> 
> Though, FYI, https://codereview.chromium.org/2756793003/ is coming at some
> point, and this will be quite a bit shorter then.

Done.

https://codereview.chromium.org/2783813002/diff/60001/extensions/browser/requ...
extensions/browser/requirements_checker_unittest.cc:175: if
(!MightSupportWebGL()) {
On 2017/03/30 17:10:55, Devlin wrote:
> Won't the blacklist still take effect, even if the check won't fail
immediately?

The blacklist is only set in the following test. The WebGL check may even
succeed synchronously, so we only require it if we're sure it will fail
immediately.

https://codereview.chromium.org/2783813002/diff/80001/extensions/common/manif...
File extensions/common/manifest_handlers/plugins_handler.h (right):

https://codereview.chromium.org/2783813002/diff/80001/extensions/common/manif...
extensions/common/manifest_handlers/plugins_handler.h:25: bool is_public;      
// False if only this extension can load this plugin.
git cl format ¯\_(ツ)_/¯

[michaelpg, 2017-03-31 21:20:06]

Patchset #8 (id:140001) has been deleted

[Devlin, 2017-04-04 01:27:43]

lgtm

https://codereview.chromium.org/2783813002/diff/120001/extensions/browser/req...
File extensions/browser/requirements_checker.cc (right):

https://codereview.chromium.org/2783813002/diff/120001/extensions/browser/req...
extensions/browser/requirements_checker.cc:78: void
RequirementsChecker::OnGetWebGLAvailability(bool available) {
OnGot...?  OnDetermined...? OnChecked...?  Something past-tense.

[michaelpg, 2017-04-04 21:42:49]

Description was changed from

==========
Move ChromeRequirementsChecker to //extensions as a PreloadCheck

Remove the "Chrome" from ChromeRequirementsChecker. Make it implement
PreloadCheck and simplify its async logic a bit. Transform
RequirementsCheckerBrowserTest to a unit test.

This lets AppShell and other modules use RequirementsChecker. (This
wasn't possible until GPUFeatureChecker was moved to //content in
http://crrev.com/2666243002.)

BUG=679971
R=rdevlin.cronin@chromium.org
==========

to

==========
Move ChromeRequirementsChecker to //extensions as a PreloadCheck

Remove the "Chrome" from ChromeRequirementsChecker. Make it implement
PreloadCheck and simplify its async logic a bit. Transform
RequirementsCheckerBrowserTest to a unit test.

This lets AppShell and other modules use RequirementsChecker. (This
wasn't possible until GPUFeatureChecker was moved to //content in
http://crrev.com/2666243002.)

BUG=679971
R=rdevlin.cronin@chromium.org
TBR=bauerb@chromium.org
==========

[michaelpg, 2017-04-04 21:45:40]

https://codereview.chromium.org/2783813002/diff/120001/extensions/browser/req...
File extensions/browser/requirements_checker.cc (right):

https://codereview.chromium.org/2783813002/diff/120001/extensions/browser/req...
extensions/browser/requirements_checker.cc:78: void
RequirementsChecker::OnGetWebGLAvailability(bool available) {
On 2017/04/04 01:27:43, Devlin wrote:
> OnGot...?  OnDetermined...? OnChecked...?  Something past-tense.

Made it less event-y and more imperativey.

[michaelpg, 2017-04-04 21:52:26]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2017-04-04 21:52:26]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2783813002/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2017-04-04 21:52:56]

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

[commit-bot: I haz the power, 2017-04-04 22:46:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 22:46:01]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[michaelpg, 2017-04-04 23:46:23]

Hrmm... failing DisableExtensionsExceptBrowserTest.DisableExtensionsExceptFla
g because of an assumption in ExtensionService that extensions are always
installed asynchronously -- even if the checks could complete synchronously.

[michaelpg, 2017-04-06 18:54:32]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2017-04-06 18:54:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2783813002/#ps240001
(title: "rebase?")

[commit-bot: I haz the power, 2017-04-06 18:55:59]

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

[commit-bot: I haz the power, 2017-04-06 20:41:45]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1491504872893120,
"parent_rev": "64fa0be168f6c59bceeef35c5bce6b9cf85c1ef8", "commit_rev":
"a8ea037e3ee7b0f25d694801bd4accb8f22b46c4"}

[commit-bot: I haz the power, 2017-04-06 20:43:05]

Description was changed from

==========
Move ChromeRequirementsChecker to //extensions as a PreloadCheck

Remove the "Chrome" from ChromeRequirementsChecker. Make it implement
PreloadCheck and simplify its async logic a bit. Transform
RequirementsCheckerBrowserTest to a unit test.

This lets AppShell and other modules use RequirementsChecker. (This
wasn't possible until GPUFeatureChecker was moved to //content in
http://crrev.com/2666243002.)

BUG=679971
R=rdevlin.cronin@chromium.org
TBR=bauerb@chromium.org
==========

to

==========
Move ChromeRequirementsChecker to //extensions as a PreloadCheck

Remove the "Chrome" from ChromeRequirementsChecker. Make it implement
PreloadCheck and simplify its async logic a bit. Transform
RequirementsCheckerBrowserTest to a unit test.

This lets AppShell and other modules use RequirementsChecker. (This
wasn't possible until GPUFeatureChecker was moved to //content in
http://crrev.com/2666243002.)

BUG=679971
R=rdevlin.cronin@chromium.org
TBR=bauerb@chromium.org

Review-Url: https://codereview.chromium.org/2783813002
Cr-Commit-Position: refs/heads/master@{#462607}
Committed:
https://chromium.googlesource.com/chromium/src/+/a8ea037e3ee7b0f25d694801bd4a...
==========

[commit-bot: I haz the power, 2017-04-06 20:43:07]

Committed patchset #12 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/a8ea037e3ee7b0f25d694801bd4a...