service worker: Plumb NavigationPreloadManager.enable/disable to browser-side

service worker: Plumb NavigationPreloadManager.enable/disable to browser-side

Now enable/disable are propagated to browser-side ServiceWorkerRegistration
and the promises are resolved renderer-side. But it doesn't yet
* write to disk
* propagate to ServiceWorkerVersion
* implement NavigationPreloadManager.getState

Tests will be added when getState is implemented.

BUG=649558
Committed: https://crrev.com/963669d666016ef3abf0102614d99779546e9e3b
Cr-Commit-Position: refs/heads/master@{#427175}

[falken, 2016-10-20 06:57:27]

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

[commit-bot: I haz the power, 2016-10-20 06:57:34]

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

[falken, 2016-10-20 07:07:43]

falken@chromium.org changed reviewers:
+ horo@chromium.org

[falken, 2016-10-20 07:07:43]

ptal

[horo, 2016-10-20 07:36:11]

lgtm

[falken, 2016-10-20 07:44:38]

falken@chromium.org changed reviewers:
+ nasko@chromium.org, rkaplow@chromium.org

[falken, 2016-10-20 07:44:39]

owners please review
nasko: bad_message.h and messages.h
rkaplow: histograms

[falken, 2016-10-20 07:44:57]

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

[commit-bot: I haz the power, 2016-10-20 07:45:10]

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, 2016-10-20 09:31:37]

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

[commit-bot: I haz the power, 2016-10-20 09:31:37]

Dry run: This issue passed the CQ dry run.

[rkaplow, 2016-10-20 14:10:19]

lgtm

histogram lg

[nasko, 2016-10-21 16:42:02]

bad_message usage looks good. IPC too, though I do have a question on improving
it a bit.

A side question - is this tested with PlzNavigate enabled? PlzNavigate was one
of the driving factors behind this functionality to begin with, so it will be
imperative we make it work.

https://codereview.chromium.org/2433273003/diff/20001/content/common/service_...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/2433273003/diff/20001/content/common/service_...
content/common/service_worker/service_worker_messages.h:493: base::string16 /*
message */)
Can we transfer the message as std::string and do the conversion in the renderer
process? It will save us memory, as most strings are going to be ASCII anyway
and converting to string16 will double the size.

[falken, 2016-10-24 05:14:42]

Thanks. I also added a more informative error message: this is useful because
uncaught promises don't come with a call stack when printed in DevTools.

Regarding PlzNavigate, thanks for the reminder. We'll start testing with it as
implementation continues.

https://codereview.chromium.org/2433273003/diff/20001/content/common/service_...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/2433273003/diff/20001/content/common/service_...
content/common/service_worker/service_worker_messages.h:493: base::string16 /*
message */)
On 2016/10/21 16:42:02, nasko (slow) wrote:
> Can we transfer the message as std::string and do the conversion in the
renderer
> process? It will save us memory, as most strings are going to be ASCII anyway
> and converting to string16 will double the size.

Yes this puzzled me recently too. For some reason all service worker messages
use string16. It might be required to support weird strings with unpaired
surrogates (see https://bugs.chromium.org/p/chromium/issues/detail?id=592011 and
https://codereview.chromium.org/1768063002/) but we shouldn't worry about
supporting those. If WebString::fromUtf8 results in a null string,
ServiceWorkerError::take will use the default message anyway without crashing.

Done for this patch, I'll update the other messages too if this makes sense.

[falken, 2016-10-24 05:15:23]

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

[commit-bot: I haz the power, 2016-10-24 05:15:35]

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, 2016-10-24 06:59:04]

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

[commit-bot: I haz the power, 2016-10-24 06:59:05]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-10-24 15:54:49]

> Regarding PlzNavigate, thanks for the reminder. We'll start 
> testing with it as implementation continues.

Thanks! It will be key to ensure SW and PlzNavigate work well together ahead of
time, as it is a tricky area.

LGTM

https://codereview.chromium.org/2433273003/diff/20001/content/common/service_...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/2433273003/diff/20001/content/common/service_...
content/common/service_worker/service_worker_messages.h:493: base::string16 /*
message */)
On 2016/10/24 05:14:42, falken (ooo Oct 21) wrote:
> On 2016/10/21 16:42:02, nasko (slow) wrote:
> > Can we transfer the message as std::string and do the conversion in the
> renderer
> > process? It will save us memory, as most strings are going to be ASCII
anyway
> > and converting to string16 will double the size.
> 
> Yes this puzzled me recently too. For some reason all service worker messages
> use string16. It might be required to support weird strings with unpaired
> surrogates (see https://bugs.chromium.org/p/chromium/issues/detail?id=592011
and
> https://codereview.chromium.org/1768063002/) but we shouldn't worry about
> supporting those. If WebString::fromUtf8 results in a null string,
> ServiceWorkerError::take will use the default message anyway without crashing.

Thanks for the background info!
 
> Done for this patch, I'll update the other messages too if this makes sense.

Yes, it makes sense. The less data we shuffle through process boundaries, the
more performant we will be and less memory usage. I'll be happy to review those
patches :).

[falken, 2016-10-24 22:43:46]

Thank you.

[falken, 2016-10-24 22:44:08]

The CQ bit was checked by falken@chromium.org

[falken, 2016-10-24 22:44:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, horo@chromium.org
Link to the patchset: https://codereview.chromium.org/2433273003/#ps60001
(title: "comments")

[commit-bot: I haz the power, 2016-10-24 22:45:07]

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, 2016-10-24 23:05:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-24 23:10:51]

Description was changed from

==========
service worker: Plumb NavigationPreloadManager.enable/disable to browser-side

Now enable/disable are propagated to browser-side ServiceWorkerRegistration
and the promises are resolved renderer-side. But it doesn't yet
* write to disk
* propagate to ServiceWorkerVersion
* implement NavigationPreloadManager.getState

Tests will be added when getState is implemented.

BUG=649558
==========

to

==========
service worker: Plumb NavigationPreloadManager.enable/disable to browser-side

Now enable/disable are propagated to browser-side ServiceWorkerRegistration
and the promises are resolved renderer-side. But it doesn't yet
* write to disk
* propagate to ServiceWorkerVersion
* implement NavigationPreloadManager.getState

Tests will be added when getState is implemented.

BUG=649558

Committed: https://crrev.com/963669d666016ef3abf0102614d99779546e9e3b
Cr-Commit-Position: refs/heads/master@{#427175}
==========

[commit-bot: I haz the power, 2016-10-24 23:10:52]

Patchset 4 (id:??) landed as
https://crrev.com/963669d666016ef3abf0102614d99779546e9e3b
Cr-Commit-Position: refs/heads/master@{#427175}