Don't block InstallableManager::GetData calls when waiting for a service worker.

Don't block InstallableManager::GetData calls when waiting for a service worker.

If the InstallableManager is processing a task that is waiting for the
site to register a service worker, all new tasks will block until the
first task is complete. If the site never registers a service worker,
all new calls to InstallableManager will never succeed, even if they are
not requesting a service worker check.

This is particularly important if installability checking on page load is
enabled. Trying to add a non-PWA with a manifest and icon to
homescreen will not use the manifest icon or details since the onload
check is hanging waiting for a service worker.

This CL addresses the bug by introducing a paused task queue to hold
waiting tasks. Existing tests are amended to explicitly check the paused
case.

BUG=737013
Review-Url: https://codereview.chromium.org/2963473003
Cr-Commit-Position: refs/heads/master@{#482564}
Committed: https://chromium.googlesource.com/chromium/src/+/ee088c9f11822f91a180217765a1f71261e24038

[dominickn, 2017-06-27 05:22:45]

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

[commit-bot: I haz the power, 2017-06-27 05:23:00]

Dry run: CQ is trying da patch.

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

[dominickn, 2017-06-27 05:23:05]

dominickn@chromium.org changed reviewers:
+ benwells@chromium.org

[dominickn, 2017-06-27 05:23:06]

PTAL, thanks!

[dominickn, 2017-06-27 05:23:43]

Description was changed from

==========
Don't block new InstallableManager::GetData calls when waiting for a service
worker.

If the InstallableManager is processing a task that is waiting for the
site to register a service worker, all new tasks will block until the
first task is complete. If the site never registers a service worker,
all new calls to InstallableManager will never succeed, even if they are
not requesting a service worker check.

This is particularly important if isntallability checking on load is
enabled, since trying to add a non-PWA with a manifest and icon to
homescreen will not use the manifest icon or details since the onload
check is hanging waiting for a service worker.

This CL addresses the bug by introducing a paused task queue to hold
waiting tasks. Existing tests are amended to explicitly check the paused
case.

BUG=737013
==========

to

==========
Don't block new InstallableManager::GetData calls when waiting for a service
worker.

If the InstallableManager is processing a task that is waiting for the
site to register a service worker, all new tasks will block until the
first task is complete. If the site never registers a service worker,
all new calls to InstallableManager will never succeed, even if they are
not requesting a service worker check.

This is particularly important if installability checking on page load is
enabled. Trying to add a non-PWA with a manifest and icon to
homescreen will not use the manifest icon or details since the onload
check is hanging waiting for a service worker.

This CL addresses the bug by introducing a paused task queue to hold
waiting tasks. Existing tests are amended to explicitly check the paused
case.

BUG=737013
==========

[dominickn, 2017-06-27 05:32:20]

Description was changed from

==========
Don't block new InstallableManager::GetData calls when waiting for a service
worker.

If the InstallableManager is processing a task that is waiting for the
site to register a service worker, all new tasks will block until the
first task is complete. If the site never registers a service worker,
all new calls to InstallableManager will never succeed, even if they are
not requesting a service worker check.

This is particularly important if installability checking on page load is
enabled. Trying to add a non-PWA with a manifest and icon to
homescreen will not use the manifest icon or details since the onload
check is hanging waiting for a service worker.

This CL addresses the bug by introducing a paused task queue to hold
waiting tasks. Existing tests are amended to explicitly check the paused
case.

BUG=737013
==========

to

==========
Don't block InstallableManager::GetData calls when waiting for a service worker.

If the InstallableManager is processing a task that is waiting for the
site to register a service worker, all new tasks will block until the
first task is complete. If the site never registers a service worker,
all new calls to InstallableManager will never succeed, even if they are
not requesting a service worker check.

This is particularly important if installability checking on page load is
enabled. Trying to add a non-PWA with a manifest and icon to
homescreen will not use the manifest icon or details since the onload
check is hanging waiting for a service worker.

This CL addresses the bug by introducing a paused task queue to hold
waiting tasks. Existing tests are amended to explicitly check the paused
case.

BUG=737013
==========

[commit-bot: I haz the power, 2017-06-27 06:27:48]

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

[commit-bot: I haz the power, 2017-06-27 06:27:49]

Dry run: This issue passed the CQ dry run.

[benwells, 2017-06-27 07:20:59]

lgtm

https://codereview.chromium.org/2963473003/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_manager_browsertest.cc (right):

https://codereview.chromium.org/2963473003/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager_browsertest.cc:721:
sw_run_loop.Run();
Unrelated to this change - do you need so many run loops? Can you just have the
one and reset it when required?

[dominickn, 2017-06-27 07:32:50]

https://codereview.chromium.org/2963473003/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_manager_browsertest.cc (right):

https://codereview.chromium.org/2963473003/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager_browsertest.cc:721:
sw_run_loop.Run();
On 2017/06/27 07:20:59, benwells wrote:
> Unrelated to this change - do you need so many run loops? Can you just have
the
> one and reset it when required?

base::RunLoop has no reset method, and it can only be run once (and its
QuitClosure can only be dispatched once).

[dominickn, 2017-06-27 07:32:55]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2017-06-27 07:33:02]

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-06-27 08:02:39]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1498548775430450, "parent_rev":
"40001e05a76c7b5423b810186f3ef4c33b657f13", "commit_rev":
"824f0c13d5680673d8685cbf5600a47474c54b52"}

[commit-bot: I haz the power, 2017-06-27 08:02:52]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1498548775430450, "parent_rev":
"6e938fbb79f93470263e9ca0db20a8a6fc6413d7", "commit_rev":
"ee088c9f11822f91a180217765a1f71261e24038"}

[commit-bot: I haz the power, 2017-06-27 08:03:05]

Description was changed from

==========
Don't block InstallableManager::GetData calls when waiting for a service worker.

If the InstallableManager is processing a task that is waiting for the
site to register a service worker, all new tasks will block until the
first task is complete. If the site never registers a service worker,
all new calls to InstallableManager will never succeed, even if they are
not requesting a service worker check.

This is particularly important if installability checking on page load is
enabled. Trying to add a non-PWA with a manifest and icon to
homescreen will not use the manifest icon or details since the onload
check is hanging waiting for a service worker.

This CL addresses the bug by introducing a paused task queue to hold
waiting tasks. Existing tests are amended to explicitly check the paused
case.

BUG=737013
==========

to

==========
Don't block InstallableManager::GetData calls when waiting for a service worker.

If the InstallableManager is processing a task that is waiting for the
site to register a service worker, all new tasks will block until the
first task is complete. If the site never registers a service worker,
all new calls to InstallableManager will never succeed, even if they are
not requesting a service worker check.

This is particularly important if installability checking on page load is
enabled. Trying to add a non-PWA with a manifest and icon to
homescreen will not use the manifest icon or details since the onload
check is hanging waiting for a service worker.

This CL addresses the bug by introducing a paused task queue to hold
waiting tasks. Existing tests are amended to explicitly check the paused
case.

BUG=737013

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

[commit-bot: I haz the power, 2017-06-27 08:03:07]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/ee088c9f11822f91a180217765a1...

[benwells, 2017-06-27 08:31:01]

https://codereview.chromium.org/2963473003/diff/1/chrome/browser/installable/...
File chrome/browser/installable/installable_manager_browsertest.cc (right):

https://codereview.chromium.org/2963473003/diff/1/chrome/browser/installable/...
chrome/browser/installable/installable_manager_browsertest.cc:721:
sw_run_loop.Run();
On 2017/06/27 07:32:49, dominickn wrote:
> On 2017/06/27 07:20:59, benwells wrote:
> > Unrelated to this change - do you need so many run loops? Can you just have
> the
> > one and reset it when required?
> 
> base::RunLoop has no reset method, and it can only be run once (and its
> QuitClosure can only be dispatched once).

Right, the pattern I've seen is to have a unique_ptr<> and call reset on that
with a new one after you've called Run and need to do it again.

[dominickn, 2017-06-27 23:51:01]

On 2017/06/27 08:31:01, benwells wrote:
>
https://codereview.chromium.org/2963473003/diff/1/chrome/browser/installable/...
> File chrome/browser/installable/installable_manager_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2963473003/diff/1/chrome/browser/installable/...
> chrome/browser/installable/installable_manager_browsertest.cc:721:
> sw_run_loop.Run();
> On 2017/06/27 07:32:49, dominickn wrote:
> > On 2017/06/27 07:20:59, benwells wrote:
> > > Unrelated to this change - do you need so many run loops? Can you just
have
> > the
> > > one and reset it when required?
> > 
> > base::RunLoop has no reset method, and it can only be run once (and its
> > QuitClosure can only be dispatched once).
> 
> Right, the pattern I've seen is to have a unique_ptr<> and call reset on that
> with a new one after you've called Run and need to do it again.

oic. I'll keep that in mind for the future. :)