Replace ExtensionInstallChecker with generic PreloadCheckGroup

Replace ExtensionInstallChecker with generic PreloadCheckGroup

Use the PreloadCheck interface for a PreloadCheckGroup check that runs a
set of checks in parallel. Use this directly in CrxInstaller and
UnpackedInstaller; remove ExtensionInstallChecker, along with its mildly
arcane interface and its smattering of tests (the individual checks and
the Installers are already tested individually).

This generic "group" check can be provided with Chrome-specific checks,
but doesn't have to depend on Chrome itself, so it could be used in
non-Chrome code, e.g. //apps and //extensions.

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

Review-Url: https://codereview.chromium.org/2783143005
Cr-Commit-Position: refs/heads/master@{#464533}
Committed: https://chromium.googlesource.com/chromium/src/+/6a4874fd53dccf53b7e5dc6f31275245dd4573bc

[michaelpg, 2017-03-31 21:28:27]

Description was changed from

==========
Replace ExtensionInstallChecker with generic PreloadCheckGroup

Use the PreloadCheck interface for a PreloadCheckGroup check that runs a set of
checks in parallel. Use this directly in CrxInstaller and UnpackedInstaller and
remove ExtensionInstallChecker, along with its mildly arcane interface and
smattering of tests (the individual checks, and the Installers, are already
tested individually).

An advantage of this generic "group" check is that it can be provided
Chrome-specific checks, but doesn't have to depend on Chrome itself, so it
could be used in non-Chrome code, e.g. //apps and //extensions.

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

to

==========
Replace ExtensionInstallChecker with generic PreloadCheckGroup

Use the PreloadCheck interface for a PreloadCheckGroup check that runs a set of
checks in parallel. Use this directly in CrxInstaller and UnpackedInstaller and
remove ExtensionInstallChecker, along with its mildly arcane interface and
smattering of tests (the individual checks, and the Installers, are already
tested individually).

An advantage of this generic "group" check is that it can be provided
Chrome-specific checks, but doesn't have to depend on Chrome itself, so it
could be used in non-Chrome code, e.g. //apps and //extensions.

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

[michaelpg, 2017-03-31 21:29:59]

Description was changed from

==========
Replace ExtensionInstallChecker with generic PreloadCheckGroup

Use the PreloadCheck interface for a PreloadCheckGroup check that runs a set of
checks in parallel. Use this directly in CrxInstaller and UnpackedInstaller and
remove ExtensionInstallChecker, along with its mildly arcane interface and
smattering of tests (the individual checks, and the Installers, are already
tested individually).

An advantage of this generic "group" check is that it can be provided
Chrome-specific checks, but doesn't have to depend on Chrome itself, so it
could be used in non-Chrome code, e.g. //apps and //extensions.

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

to

==========
Replace ExtensionInstallChecker with generic PreloadCheckGroup

Use the PreloadCheck interface for a PreloadCheckGroup check that runs a
set of checks in parallel. Use this directly in CrxInstaller and
UnpackedInstaller; remove ExtensionInstallChecker, along with its mildly
arcane interface and its smattering of tests (the individual checks and
the Installers are already tested individually).

This generic "group" check can be provided with Chrome-specific checks,
but doesn't have to depend on Chrome itself, so it could be used in
non-Chrome code, e.g. //apps and //extensions.

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

[michaelpg, 2017-03-31 21:30:25]

PTAL!

[Devlin, 2017-04-04 01:37:26]

Nice little cleanup!

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

https://codereview.chromium.org/2783143005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:539: if (!errors.empty()) {
this block hurts my eyes a bit.  Maybe
if (errors.empty()) {
  ConfirmInstall();
  return;
}

WDYT?  Worth the duplicated ConfirmInstall() at the end?

https://codereview.chromium.org/2783143005/diff/20001/extensions/browser/prel...
File extensions/browser/preload_check_group.cc (right):

https://codereview.chromium.org/2783143005/diff/20001/extensions/browser/prel...
extensions/browser/preload_check_group.cc:26: DCHECK(running_checks_ == 0);
nit: DCHECK_EQ(0, running_checks_)

https://codereview.chromium.org/2783143005/diff/20001/extensions/browser/prel...
File extensions/browser/preload_check_group.h (right):

https://codereview.chromium.org/2783143005/diff/20001/extensions/browser/prel...
extensions/browser/preload_check_group.h:46: // Checks to run. Not owned.
Any particular reason to not own these?

[michaelpg, 2017-04-04 21:41:29]

PTAL. only the .cc files changed, the rest was a rebase.

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

https://codereview.chromium.org/2783143005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:539: if (!errors.empty()) {
On 2017/04/04 01:37:26, Devlin wrote:
> this block hurts my eyes a bit.  Maybe
> if (errors.empty()) {
>   ConfirmInstall();
>   return;
> }
> 
> WDYT?  Worth the duplicated ConfirmInstall() at the end?

Done. I also fiddled with the order of nested ifs below. (It does change when
|install_flags_| is updated for the failure case, but it's ignored in that case
anyway, which matches the other checks too.)

https://codereview.chromium.org/2783143005/diff/20001/extensions/browser/prel...
File extensions/browser/preload_check_group.cc (right):

https://codereview.chromium.org/2783143005/diff/20001/extensions/browser/prel...
extensions/browser/preload_check_group.cc:26: DCHECK(running_checks_ == 0);
On 2017/04/04 01:37:26, Devlin wrote:
> nit: DCHECK_EQ(0, running_checks_)

Done.

https://codereview.chromium.org/2783143005/diff/20001/extensions/browser/prel...
File extensions/browser/preload_check_group.h (right):

https://codereview.chromium.org/2783143005/diff/20001/extensions/browser/prel...
extensions/browser/preload_check_group.h:46: // Checks to run. Not owned.
On 2017/04/04 01:37:26, Devlin wrote:
> Any particular reason to not own these?

TL;DR: unless you have a preference one way or another, I'll leave as-is and
maybe change it later depending on what use cases come up.

Users of PreloadCheckGroup may want to retain pointers to their individual
checks (e.g., CrxInstaller using its policy_check_ to get the policy-specific
error message). To own the checks here, PreloadCheckGroup would have to
explicitly guarantee that smart pointers moved into AddCheck() are not destroyed
until this class is destroyed. Users would have to ensure raw pointers to
PreloadChecks are not used after the PreloadCheckGroup is destroyed. Just seems
to require too much knowledge of the PreloadCheckGroup implementation.

OTOH, retaining a raw pointer after a function call ends is a bit sketchy. But
the owner of this class probably also owns the object pointed to, so it doesn't
seem like an anti-pattern to me. Style guide doesn't explicitly disallow it:

> If the function does not modify `t`'s ownership, declare the param as `T*`.
The caller
> is expected to ensure `t` stays alive as long as necessary, generally through
the
> duration of the call.

But I guess passing ownership to the Group would make things easier for users
who want to just create a check and pass it in, and not have to hold on to it.

[Devlin, 2017-04-06 02:03:53]

lgtm

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

https://codereview.chromium.org/2783143005/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:545: if
(!requirements_check_->GetErrorMessage().empty()) {
I know I mentioned before that it's weird to sometimes consult
GetErrorMessage(), other times consult |errors| in a different CL... do you have
any ideas in the pipeline to make it a bit less unusual?  (Not something that
needs to be done for this CL, but I do think it would be nice to have something
more clear).

https://codereview.chromium.org/2783143005/diff/60001/extensions/browser/prel...
File extensions/browser/preload_check_test_util.cc (right):

https://codereview.chromium.org/2783143005/diff/60001/extensions/browser/prel...
extensions/browser/preload_check_test_util.cc:39: WaitForComplete();
Just a note - currently this is incompatible with sync checks.  We could make it
sync/async agnostic if we always instantiated the run loop before the call to
Run() and always called Quit() in OnCheckComplete.  RunLoop is designed to be
able to accept Quit() being called before Run() (in which case the loop quits
immediately).

https://codereview.chromium.org/2783143005/diff/60001/extensions/browser/prel...
File extensions/browser/preload_check_test_util.h (right):

https://codereview.chromium.org/2783143005/diff/60001/extensions/browser/prel...
extensions/browser/preload_check_test_util.h:60: explicit PreloadCheckStub(const
Errors& errors = Errors());
nit: I'd prefer we don't use default arguments on non-POD data.  Let's keep
either a SetErrors() method, or pass in an empty array in cases of no error.

[michaelpg, 2017-04-06 21:55:13]

Patchset #5 (id:80001) has been deleted

[michaelpg, 2017-04-06 22:31:17]

Devlin, PTAL at these changes and let me know if it's still good. Thanks!

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

https://codereview.chromium.org/2783143005/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:545: if
(!requirements_check_->GetErrorMessage().empty()) {
On 2017/04/06 02:03:53, Devlin wrote:
> I know I mentioned before that it's weird to sometimes consult
> GetErrorMessage(), other times consult |errors| in a different CL... do you
have
> any ideas in the pipeline to make it a bit less unusual?  (Not something that
> needs to be done for this CL, but I do think it would be nice to have
something
> more clear).

:-\ Both pieces of info are useful. We could add a standard HadSomeError()
method (adding more redundancy), or we could explicitly check for every error
type (there are 3 requirements errors we'd check for). I see why it's weird, but
I haven't thought of anything clearly better.

The utility of PreloadCheck[Group] is outsourcing the boilerplate sync/async
check/check all code. At the end of the day we still have to allow for examining
each error and taking action individually.

https://codereview.chromium.org/2783143005/diff/60001/extensions/browser/prel...
File extensions/browser/preload_check_test_util.cc (right):

https://codereview.chromium.org/2783143005/diff/60001/extensions/browser/prel...
extensions/browser/preload_check_test_util.cc:39: WaitForComplete();
On 2017/04/06 02:03:53, Devlin wrote:
> Just a note - currently this is incompatible with sync checks.  We could make
it
> sync/async agnostic if we always instantiated the run loop before the call to
> Run() and always called Quit() in OnCheckComplete.  RunLoop is designed to be
> able to accept Quit() being called before Run() (in which case the loop quits
> immediately).

RunUntilComplete requires the check to be async, thus ASSERT_FALSE(called_).

But, that's kind of subtle. I'll use your suggestion; a caller can still decide
when to assert the check *doesn't* start.

https://codereview.chromium.org/2783143005/diff/60001/extensions/browser/prel...
File extensions/browser/preload_check_test_util.h (right):

https://codereview.chromium.org/2783143005/diff/60001/extensions/browser/prel...
extensions/browser/preload_check_test_util.h:60: explicit PreloadCheckStub(const
Errors& errors = Errors());
On 2017/04/06 02:03:53, Devlin wrote:
> nit: I'd prefer we don't use default arguments on non-POD data.  Let's keep
> either a SetErrors() method, or pass in an empty array in cases of no error.

Done.

[michaelpg, 2017-04-06 22:32:17]

Description was changed from

==========
Replace ExtensionInstallChecker with generic PreloadCheckGroup

Use the PreloadCheck interface for a PreloadCheckGroup check that runs a
set of checks in parallel. Use this directly in CrxInstaller and
UnpackedInstaller; remove ExtensionInstallChecker, along with its mildly
arcane interface and its smattering of tests (the individual checks and
the Installers are already tested individually).

This generic "group" check can be provided with Chrome-specific checks,
but doesn't have to depend on Chrome itself, so it could be used in
non-Chrome code, e.g. //apps and //extensions.

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

to

==========
Replace ExtensionInstallChecker with generic PreloadCheckGroup

Use the PreloadCheck interface for a PreloadCheckGroup check that runs a
set of checks in parallel. Use this directly in CrxInstaller and
UnpackedInstaller; remove ExtensionInstallChecker, along with its mildly
arcane interface and its smattering of tests (the individual checks and
the Installers are already tested individually).

This generic "group" check can be provided with Chrome-specific checks,
but doesn't have to depend on Chrome itself, so it could be used in
non-Chrome code, e.g. //apps and //extensions.

BUG=679971
R=rdevlin.cronin@chromium.org
TBR=bauerb@chromium.org # for chrome/browser/content_settings include
==========

[michaelpg, 2017-04-06 22:33:27]

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

[michaelpg, 2017-04-06 22:33:28]

piman: PTAL for extension/browser/DEPS include rule for gpu/config.

[piman, 2017-04-06 22:57:17]

On 2017/04/06 22:33:28, michaelpg wrote:
> piman: PTAL for extension/browser/DEPS include rule for gpu/config.

I don't see that file in latest PS. Forgot to git add?

[michaelpg, 2017-04-06 23:33:00]

On 2017/04/06 22:57:17, piman wrote:
> On 2017/04/06 22:33:28, michaelpg wrote:
> > piman: PTAL for extension/browser/DEPS include rule for gpu/config.
> 
> I don't see that file in latest PS. Forgot to git add?

:-\ Sorry, must've still had the other CL in my head.

[michaelpg, 2017-04-06 23:33:08]

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

[michaelpg, 2017-04-06 23:33:34]

Description was changed from

==========
Replace ExtensionInstallChecker with generic PreloadCheckGroup

Use the PreloadCheck interface for a PreloadCheckGroup check that runs a
set of checks in parallel. Use this directly in CrxInstaller and
UnpackedInstaller; remove ExtensionInstallChecker, along with its mildly
arcane interface and its smattering of tests (the individual checks and
the Installers are already tested individually).

This generic "group" check can be provided with Chrome-specific checks,
but doesn't have to depend on Chrome itself, so it could be used in
non-Chrome code, e.g. //apps and //extensions.

BUG=679971
R=rdevlin.cronin@chromium.org
TBR=bauerb@chromium.org # for chrome/browser/content_settings include
==========

to

==========
Replace ExtensionInstallChecker with generic PreloadCheckGroup

Use the PreloadCheck interface for a PreloadCheckGroup check that runs a
set of checks in parallel. Use this directly in CrxInstaller and
UnpackedInstaller; remove ExtensionInstallChecker, along with its mildly
arcane interface and its smattering of tests (the individual checks and
the Installers are already tested individually).

This generic "group" check can be provided with Chrome-specific checks,
but doesn't have to depend on Chrome itself, so it could be used in
non-Chrome code, e.g. //apps and //extensions.

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

[Devlin, 2017-04-07 14:33:57]

s lgtm; errors vs GetErrorMessage() can be resolved async and in followups.

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

https://codereview.chromium.org/2783143005/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:545: if
(!requirements_check_->GetErrorMessage().empty()) {
On 2017/04/06 22:31:16, michaelpg wrote:
> On 2017/04/06 02:03:53, Devlin wrote:
> > I know I mentioned before that it's weird to sometimes consult
> > GetErrorMessage(), other times consult |errors| in a different CL... do you
> have
> > any ideas in the pipeline to make it a bit less unusual?  (Not something
that
> > needs to be done for this CL, but I do think it would be nice to have
> something
> > more clear).
> 
> :-\ Both pieces of info are useful. We could add a standard HadSomeError()
> method (adding more redundancy), or we could explicitly check for every error
> type (there are 3 requirements errors we'd check for). I see why it's weird,
but
> I haven't thought of anything clearly better.
> 
> The utility of PreloadCheck[Group] is outsourcing the boilerplate sync/async
> check/check all code. At the end of the day we still have to allow for
examining
> each error and taking action individually.

One thought I had is that we could make an Error struct
PreloadCheck::Error {
  ErrorCode code;
  base::string16 message;
}

That way, we still have all the utility have having code + error, and we don't
have the weirdness of sometimes checking errors.contains(foo) and sometimes
checking PreloadCheck::GetErrorMessage().

A few drawbacks:
- This doesn't work when we coalesce multiple errors from a preload check into a
single error message, like we do in RequirementsChecker.  Instead, that would be
the responsibility of the callback (or a utility method called by the callback).
- errors.count(SOME_ERROR) doesn't work, unless we have an operator==(ErrorCode)
on Error, or we make an ErrorSet class.

Overall, this might lead to overarchitecting.  For the time being, I'm fine with
this, but if/when we add more of these checks, it will get increasingly weird. 
Do you have an idea how many of these checks we'll have?

[michaelpg, 2017-04-07 22:33:49]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2017-04-07 22:34:15]

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

[michaelpg, 2017-04-07 22:47:26]

On 2017/04/07 14:33:57, Devlin wrote:
> s lgtm; errors vs GetErrorMessage() can be resolved async and in followups.
> 
>
https://codereview.chromium.org/2783143005/diff/60001/chrome/browser/extensio...
> File chrome/browser/extensions/crx_installer.cc (right):
> 
>
https://codereview.chromium.org/2783143005/diff/60001/chrome/browser/extensio...
> chrome/browser/extensions/crx_installer.cc:545: if
> (!requirements_check_->GetErrorMessage().empty()) {
> On 2017/04/06 22:31:16, michaelpg wrote:
> > On 2017/04/06 02:03:53, Devlin wrote:
> > > I know I mentioned before that it's weird to sometimes consult
> > > GetErrorMessage(), other times consult |errors| in a different CL... do
you
> > have
> > > any ideas in the pipeline to make it a bit less unusual?  (Not something
> that
> > > needs to be done for this CL, but I do think it would be nice to have
> > something
> > > more clear).
> > 
> > :-\ Both pieces of info are useful. We could add a standard HadSomeError()
> > method (adding more redundancy), or we could explicitly check for every
error
> > type (there are 3 requirements errors we'd check for). I see why it's weird,
> but
> > I haven't thought of anything clearly better.
> > 
> > The utility of PreloadCheck[Group] is outsourcing the boilerplate sync/async
> > check/check all code. At the end of the day we still have to allow for
> examining
> > each error and taking action individually.
> 
> One thought I had is that we could make an Error struct
> PreloadCheck::Error {
>   ErrorCode code;
>   base::string16 message;
> }
> 
> That way, we still have all the utility have having code + error, and we don't
> have the weirdness of sometimes checking errors.contains(foo) and sometimes
> checking PreloadCheck::GetErrorMessage().
> 
> A few drawbacks:
> - This doesn't work when we coalesce multiple errors from a preload check into
a
> single error message, like we do in RequirementsChecker.  Instead, that would
be
> the responsibility of the callback (or a utility method called by the
callback).
> - errors.count(SOME_ERROR) doesn't work, unless we have an
operator==(ErrorCode)
> on Error, or we make an ErrorSet class.
> 
> Overall, this might lead to overarchitecting.  For the time being, I'm fine
with
> this, but if/when we add more of these checks, it will get increasingly weird.

> Do you have an idea how many of these checks we'll have?

Hopefully a number proportionate to how useful and simple the class is.
AKA "we'll see" :-)

[commit-bot: I haz the power, 2017-04-07 23:05:35]

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

[commit-bot: I haz the power, 2017-04-07 23:05:36]

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

[michaelpg, 2017-04-12 04:46:52]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2017-04-12 04:46:52]

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

[commit-bot: I haz the power, 2017-04-12 04:47:10]

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-12 05:23:17]

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

[commit-bot: I haz the power, 2017-04-12 05:23:19]

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

[michaelpg, 2017-04-13 19:39:00]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2017-04-13 19:39:00]

The CQ bit was unchecked by michaelpg@chromium.org

[michaelpg, 2017-04-13 19:39:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2783143005/#ps160001
(title: "fix debug build")

[michaelpg, 2017-04-13 19:39:01]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2017-04-13 19:39:35]

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-13 20:41:46]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1492112341456690,
"parent_rev": "a78996284e826673ba36113865a623743c7ef048", "commit_rev":
"6a4874fd53dccf53b7e5dc6f31275245dd4573bc"}

[commit-bot: I haz the power, 2017-04-13 20:42:44]

Description was changed from

==========
Replace ExtensionInstallChecker with generic PreloadCheckGroup

Use the PreloadCheck interface for a PreloadCheckGroup check that runs a
set of checks in parallel. Use this directly in CrxInstaller and
UnpackedInstaller; remove ExtensionInstallChecker, along with its mildly
arcane interface and its smattering of tests (the individual checks and
the Installers are already tested individually).

This generic "group" check can be provided with Chrome-specific checks,
but doesn't have to depend on Chrome itself, so it could be used in
non-Chrome code, e.g. //apps and //extensions.

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

to

==========
Replace ExtensionInstallChecker with generic PreloadCheckGroup

Use the PreloadCheck interface for a PreloadCheckGroup check that runs a
set of checks in parallel. Use this directly in CrxInstaller and
UnpackedInstaller; remove ExtensionInstallChecker, along with its mildly
arcane interface and its smattering of tests (the individual checks and
the Installers are already tested individually).

This generic "group" check can be provided with Chrome-specific checks,
but doesn't have to depend on Chrome itself, so it could be used in
non-Chrome code, e.g. //apps and //extensions.

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

Review-Url: https://codereview.chromium.org/2783143005
Cr-Commit-Position: refs/heads/master@{#464533}
Committed:
https://chromium.googlesource.com/chromium/src/+/6a4874fd53dccf53b7e5dc6f3127...
==========

[commit-bot: I haz the power, 2017-04-13 20:42:46]

Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/6a4874fd53dccf53b7e5dc6f3127...