Simplify ExtensionInstallChecker into a single-use class

Simplify ExtensionInstallChecker into a single-use class

ExtensionInstallChecker does asynchronous checks, and its set of checks
can be started more than once. This functionality isn't being used and
makes the class more complicated. Remove
ExtensionInstallChecker::ResetResults().

It's also unclear why the users of this class (CrxInstaller and
UnpackedInstaller) are intentionally storing their Profile and Extension
pointers in ExtensionInstallChecker class instead of remembering them
locally, so update that and remove
ExtensionInstallChecker::set_extension().

Lastly, allocate the ExtensionInstallChecker dynamically since we don't
always use it.

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

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

[michaelpg, 2017-03-15 02:20:43]

The CQ bit was checked by michaelpg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 02:21:33]

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

[michaelpg, 2017-03-15 02:22:11]

Description was changed from

==========
Simplify ExtensionInstallChecker into a single-use class

ExtensionInstallChecker does asynchronous checks, and its set of checks
can be started more than once. This functionality isn't being used and
makes the class more complicated. Remove
ExtensionInstallChecker::ResetResults().

It's also unclear why the users of this class (CrxInstaller and
UnpackedInstaller) are intentionally storing their Profile and Extension
pointers in ExtensionInstallChecker class instead of remembering them
locally, so update that and remove
ExtensionInstallChecker::set_extension().

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

to

==========
Simplify ExtensionInstallChecker into a single-use class

ExtensionInstallChecker does asynchronous checks, and its set of checks
can be started more than once. This functionality isn't being used and
makes the class more complicated. Remove
ExtensionInstallChecker::ResetResults().

It's also unclear why the users of this class (CrxInstaller and
UnpackedInstaller) are intentionally storing their Profile and Extension
pointers in ExtensionInstallChecker class instead of remembering them
locally, so update that and remove
ExtensionInstallChecker::set_extension().

Lastly, allocate the ExtensionInstallChecker dynamically since we don't
always use it.

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

[michaelpg, 2017-03-15 02:23:36]

WDYT?

[commit-bot: I haz the power, 2017-03-15 02:26:20]

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

[commit-bot: I haz the power, 2017-03-15 02:26:21]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Devlin, 2017-03-16 19:29:41]

Overall, I like it!  Yay for less complexity.

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/crx_installer.h (right):

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/crx_installer.h:448:
std::unique_ptr<ExtensionInstallChecker> install_checker_;
optional: Truthfully, given the (lack of) complexity of ExtensionInstallChecker
to initialize, it's probably just as cheaper or cheaper overall to avoid the
heap allocation this requires.  But... this isn't performance sensitive code. 
So whichever you prefer.

If you opt for this route, you can remove the include and replace it with a
forward declaration.

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/crx_installer_browsertest.cc (right):

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/crx_installer_browsertest.cc:287: ASSERT_EQ(nullptr,
crx_installer->install_checker_.get());
This is testing an implementation detail of the CrxInstaller, rather than
behavior.  If we ever decide to instantiate install_checker_ when there are no
errors, this will fail.  Additionally, this wouldn't catch the case of an
installation being triggered, the install checker running, and finding no
errors.

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

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_checker.cc:52: enabled_checks_ =
enabled_checks;
With this class becoming single use, we can move a lot of these to the ctor.

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_checker.cc:60: if (!is_running())
not your code, but we're here. :)

these is_running() calls are weird and kind of useless.  I think it's clearer to
just have
if (enabled & FOO) {
  CheckFoo();
}
if (enabled & BAR) {
  CheckBar();
}

I don't think the early-returns here are buying us much, if anything.

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_checker.cc:162:
callback_copy.Run(failed_mask);
sorry for the drive-by:
base::ResetAndReturn(&callback_).Run(failed_mask);

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_install_checker.h (right):

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_checker.h:45: explicit
ExtensionInstallChecker(Profile* profile,
This no longer needs explicit.

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_checker.h:49: // Start the set of
checks. |enabled_checks| is a bitmask of CheckTypes to
nit: since we're updating this anyway, function comments should be descriptive,
not imperative.

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_checker.h:55: // This function
should be invoked at most once.
Let's also put something about single use in the class comments.

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

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_checker_unittest.cc:248: for
(ExtensionInstallCheckerForTest* checker : checkers) {
optional nit: just inline checkers

[michaelpg, 2017-03-17 04:25:23]

ptal.

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/crx_installer.h (right):

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/crx_installer.h:448:
std::unique_ptr<ExtensionInstallChecker> install_checker_;
On 2017/03/16 19:29:40, Devlin wrote:
> optional: Truthfully, given the (lack of) complexity of
ExtensionInstallChecker
> to initialize, it's probably just as cheaper or cheaper overall to avoid the
> heap allocation this requires.  But... this isn't performance sensitive code. 
> So whichever you prefer.

I checked when I was considering using base::Optional (to avoid the heap
allocation). As a member, it'd add about 100 bytes to the class size. Not a ton,
but not trivial.

I like the pointer (or Optional) for the semantics of "this doesn't always
exist".

> 
> If you opt for this route, you can remove the include and replace it with a
> forward declaration.

Done.

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/crx_installer_browsertest.cc (right):

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/crx_installer_browsertest.cc:287: ASSERT_EQ(nullptr,
crx_installer->install_checker_.get());
On 2017/03/16 19:29:40, Devlin wrote:
> This is testing an implementation detail of the CrxInstaller, rather than
> behavior.  If we ever decide to instantiate install_checker_ when there are no
> errors, this will fail.  Additionally, this wouldn't catch the case of an
> installation being triggered, the install checker running, and finding no
> errors.

This turns out to be the only user of WaitForCrxInstallerDone(), so I've
modified that to ensure the installer finishes *successfully*. So I feel OK
removing this check, which also lets us unfriend CrxInstaller!

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

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_checker.cc:52: enabled_checks_ =
enabled_checks;
On 2017/03/16 19:29:40, Devlin wrote:
> With this class becoming single use, we can move a lot of these to the ctor.

Done.

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_checker.cc:60: if (!is_running())
On 2017/03/16 19:29:40, Devlin wrote:
> not your code, but we're here. :)
> 
> these is_running() calls are weird and kind of useless.  I think it's clearer
to
> just have
> if (enabled & FOO) {
>   CheckFoo();
> }
> if (enabled & BAR) {
>   CheckBar();
> }
> 
> I don't think the early-returns here are buying us much, if anything.

this particular early return triggers if the management policy check fails and
fail_fast = true. Changing that would be a change in behavior for this class,
which I'd rather not combine with a refactoring. (Also don't know if I agree
with that change.)

The early return after CheckRequirements() is useless, because that check
happens async (with the current permutation of RequirementsChecker and
WebGlChecker implementations used by Chrome), *but* the next CL or two will move
this logic into a PreloadCheckGroup class anyway, so I'd rather keep as-is for
now.

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_checker.cc:162:
callback_copy.Run(failed_mask);
On 2017/03/16 19:29:40, Devlin wrote:
> sorry for the drive-by:
> base::ResetAndReturn(&callback_).Run(failed_mask);

Awesome! Done and suggested documentation in
https://codereview.chromium.org/2755073002

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_install_checker.h (right):

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_checker.h:45: explicit
ExtensionInstallChecker(Profile* profile,
On 2017/03/16 19:29:40, Devlin wrote:
> This no longer needs explicit.

Made PG-rated.

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_checker.h:49: // Start the set of
checks. |enabled_checks| is a bitmask of CheckTypes to
On 2017/03/16 19:29:41, Devlin wrote:
> nit: since we're updating this anyway, function comments should be
descriptive,
> not imperative.

Done.

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_checker.h:55: // This function
should be invoked at most once.
On 2017/03/16 19:29:40, Devlin wrote:
> Let's also put something about single use in the class comments.

Done.

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

https://codereview.chromium.org/2751013002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_checker_unittest.cc:248: for
(ExtensionInstallCheckerForTest* checker : checkers) {
On 2017/03/16 19:29:41, Devlin wrote:
> optional nit: just inline checkers

IMO the polymorphism is a bit more obvious being explicit like this

Also, seems like that would require a static_cast to EICFT* for async_checker
:-\

I made the range-declaration "auto*" tho.

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

https://codereview.chromium.org/2751013002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker_unittest.cc:309:
/*fail_fast=*/true);
for some reason the test functions and constructors had these two arguments in
different orders, and APPARENTLY we don't get compiler warnings when using a
bool as an enum or using an enum as a bool, so I had some fun debugging why the
tests were suddenly crashing after my change... /facepalm

[michaelpg, 2017-03-17 23:15:23]

The CQ bit was checked by michaelpg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 23:16:24]

Dry run: 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-03-17 23:22:06]

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

[commit-bot: I haz the power, 2017-03-17 23:22:07]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Devlin, 2017-03-20 15:36:46]

lgtm

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

https://codereview.chromium.org/2751013002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker_unittest.cc:309:
/*fail_fast=*/true);
On 2017/03/17 04:25:23, michaelpg wrote:
> for some reason the test functions and constructors had these two arguments in
> different orders, and APPARENTLY we don't get compiler warnings when using a
> bool as an enum or using an enum as a bool, so I had some fun debugging why
the
> tests were suddenly crashing after my change... /facepalm

Yeah, I think enum -> bool can be implicit.  bool -> enum I don't think normally
works, but the ctor accepts an int.  An alternative might be a std::bitset<>,
but it doesn't appear widely used in chromium, and I'm not familiar enough with
it to enthusiastically encourage it over a normal int.

https://codereview.chromium.org/2751013002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_browsertest.h (right):

https://codereview.chromium.org/2751013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_browsertest.h:268: bool
WaitForCrxInstallerDone() {
nit: WaitForCrxInstallerSuccess() to reflect this change?

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

https://codereview.chromium.org/2751013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:28: running_checks_(0),
We could combine running_checks_ and enabled_checks_ into e.g.
remaining_checks_.

https://codereview.chromium.org/2751013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:46: NOTREACHED();
Unrelated to this CL, but usually we only want to use a NOTREACHED(); return;
pattern when it's possible to happen through e.g. untrusted code (like IPCs,
weird JS, etc).  This seems like we can just move it to a CHECK/DCHECK.  We
shouldn't do that in this CL, but can you add a TODO? (Ditto for above)

https://codereview.chromium.org/2751013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:103: if (!is_running())
Actually, I think we can make this (D)CHECK is_running().  MaybeInvokeCallback()
invalidates weak ptrs if fail_fast_ is true and we fail, so the callback won't
be invoked.

https://codereview.chromium.org/2751013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:124: if (!is_running())
ditto

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

https://codereview.chromium.org/2751013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.h:60: Profile* profile()
const { return profile_; }
I think we can remove this?

https://codereview.chromium.org/2751013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.h:62: const
scoped_refptr<const Extension>& extension() { return extension_; }
and maybe this?

[michaelpg, 2017-03-22 00:17:12]

https://codereview.chromium.org/2751013002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_browsertest.h (right):

https://codereview.chromium.org/2751013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_browsertest.h:268: bool
WaitForCrxInstallerDone() {
On 2017/03/20 15:36:46, Devlin wrote:
> nit: WaitForCrxInstallerSuccess() to reflect this change?

well, it still waits for it to be done, not for it to succeed.

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

https://codereview.chromium.org/2751013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:28: running_checks_(0),
On 2017/03/20 15:36:46, Devlin wrote:
> We could combine running_checks_ and enabled_checks_ into e.g.
> remaining_checks_.

Added a TODO. running_checks_ has a use currently (detects if Start() is called
more than once). We can probably simplify that into a bool or maybe remove the
NOTREACHED altogether, but i'll save that for a possible future CL.

https://codereview.chromium.org/2751013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:46: NOTREACHED();
On 2017/03/20 15:36:46, Devlin wrote:
> Unrelated to this CL, but usually we only want to use a NOTREACHED(); return;
> pattern when it's possible to happen through e.g. untrusted code (like IPCs,
> weird JS, etc).  This seems like we can just move it to a CHECK/DCHECK.  We
> shouldn't do that in this CL, but can you add a TODO? (Ditto for above)

Done.

https://codereview.chromium.org/2751013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:103: if (!is_running())
On 2017/03/20 15:36:46, Devlin wrote:
> Actually, I think we can make this (D)CHECK is_running(). 
MaybeInvokeCallback()
> invalidates weak ptrs if fail_fast_ is true and we fail, so the callback won't
> be invoked.

Done.

https://codereview.chromium.org/2751013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:124: if (!is_running())
On 2017/03/20 15:36:46, Devlin wrote:
> ditto

Done.

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

https://codereview.chromium.org/2751013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.h:60: Profile* profile()
const { return profile_; }
On 2017/03/20 15:36:46, Devlin wrote:
> I think we can remove this?

Done.

https://codereview.chromium.org/2751013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.h:62: const
scoped_refptr<const Extension>& extension() { return extension_; }
On 2017/03/20 15:36:46, Devlin wrote:
> and maybe this?

Done.

[Devlin, 2017-03-22 15:37:13]

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

https://codereview.chromium.org/2751013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:28: running_checks_(0),
On 2017/03/22 00:17:12, michaelpg wrote:
> On 2017/03/20 15:36:46, Devlin wrote:
> > We could combine running_checks_ and enabled_checks_ into e.g.
> > remaining_checks_.
> 
> Added a TODO. running_checks_ has a use currently (detects if Start() is
called
> more than once). We can probably simplify that into a bool or maybe remove the
> NOTREACHED altogether, but i'll save that for a possible future CL.

TODO sounds fine, but I don't see one added - am I just missing it?

[michaelpg, 2017-03-22 20:44:54]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2017-03-22 20:44:54]

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/2751013002/#ps120001
(title: "todo")

[commit-bot: I haz the power, 2017-03-22 20:45:25]

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

[michaelpg, 2017-03-22 20:46:10]

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

https://codereview.chromium.org/2751013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_checker.cc:28: running_checks_(0),
On 2017/03/22 15:37:13, Devlin wrote:
> On 2017/03/22 00:17:12, michaelpg wrote:
> > On 2017/03/20 15:36:46, Devlin wrote:
> > > We could combine running_checks_ and enabled_checks_ into e.g.
> > > remaining_checks_.
> > 
> > Added a TODO. running_checks_ has a use currently (detects if Start() is
> called
> > more than once). We can probably simplify that into a bool or maybe remove
the
> > NOTREACHED altogether, but i'll save that for a possible future CL.
> 
> TODO sounds fine, but I don't see one added - am I just missing it?

Weird. Uploaded again and it's there (in the header)

[commit-bot: I haz the power, 2017-03-22 21:44:21]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490215494420210,
"parent_rev": "b8ea03baf58d48221c6e64bba6a5f7dd355b0a02", "commit_rev":
"b24adc5bcbfde7fd67c6b23c3b0721610c77f63b"}

[commit-bot: I haz the power, 2017-03-22 21:45:49]

Description was changed from

==========
Simplify ExtensionInstallChecker into a single-use class

ExtensionInstallChecker does asynchronous checks, and its set of checks
can be started more than once. This functionality isn't being used and
makes the class more complicated. Remove
ExtensionInstallChecker::ResetResults().

It's also unclear why the users of this class (CrxInstaller and
UnpackedInstaller) are intentionally storing their Profile and Extension
pointers in ExtensionInstallChecker class instead of remembering them
locally, so update that and remove
ExtensionInstallChecker::set_extension().

Lastly, allocate the ExtensionInstallChecker dynamically since we don't
always use it.

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

to

==========
Simplify ExtensionInstallChecker into a single-use class

ExtensionInstallChecker does asynchronous checks, and its set of checks
can be started more than once. This functionality isn't being used and
makes the class more complicated. Remove
ExtensionInstallChecker::ResetResults().

It's also unclear why the users of this class (CrxInstaller and
UnpackedInstaller) are intentionally storing their Profile and Extension
pointers in ExtensionInstallChecker class instead of remembering them
locally, so update that and remove
ExtensionInstallChecker::set_extension().

Lastly, allocate the ExtensionInstallChecker dynamically since we don't
always use it.

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

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

[commit-bot: I haz the power, 2017-03-22 21:45:51]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/b24adc5bcbfde7fd67c6b23c3b07...

[dgozman, 2017-03-22 23:33:38]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2769813004/ by dgozman@chromium.org.

The reason for reverting is: Speculative revert for
ServiceWorkerTest.ServiceWorkerSuspensionOnExtensionUnload failure on the
following bot:
https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%....