Add EmbeddedWorker interfaces in Blink API

Add EmbeddedWorker interfaces in Blink API

Rough design doc: https://docs.google.com/a/chromium.org/document/d/1HzjaXttZcRr6cE7oINMzvlVskbGpGnB_Tn37ayjST9Q/edit

Chromium-side patch: https://codereview.chromium.org/54573002/

BUG=313530
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162282

[kinuko, 2013-11-07 04:02:45]

I think this one's probably easier than chromium's one. Can you review?

[abarth-chromium, 2013-11-07 17:51:12]

https://codereview.chromium.org/61763004/diff/1/public/web/WebEmbeddedWorker.h
File public/web/WebEmbeddedWorker.h (right):

https://codereview.chromium.org/61763004/diff/1/public/web/WebEmbeddedWorker....
public/web/WebEmbeddedWorker.h:38: namespace WebKit {
WebKit -> blink

https://codereview.chromium.org/61763004/diff/1/public/web/WebEmbeddedWorker....
public/web/WebEmbeddedWorker.h:46: class WebEmbeddedWorker {
Is an EmbeddedWorker a SharedWorker or can it be another kind fo worker?

https://codereview.chromium.org/61763004/diff/1/public/web/WebEmbeddedWorker....
public/web/WebEmbeddedWorker.h:61: WebEmbeddedWorkerStartMode) { }
Should these be pure virtual?

Rather than adding all these parameters (which might grow over time), can we
pass in a struct containing these values?  That's easier to evolve over time.

https://codereview.chromium.org/61763004/diff/1/public/web/WebEmbeddedWorkerC...
File public/web/WebEmbeddedWorkerClient.h (right):

https://codereview.chromium.org/61763004/diff/1/public/web/WebEmbeddedWorkerC...
public/web/WebEmbeddedWorkerClient.h:36: namespace WebKit {
WebKit -> blink

[kinuko, 2013-11-08 12:17:23]

Thanks for reviewing,

https://codereview.chromium.org/61763004/diff/1/public/web/WebEmbeddedWorker.h
File public/web/WebEmbeddedWorker.h (right):

https://codereview.chromium.org/61763004/diff/1/public/web/WebEmbeddedWorker....
public/web/WebEmbeddedWorker.h:38: namespace WebKit {
On 2013/11/07 17:51:13, abarth wrote:
> WebKit -> blink

Done.

https://codereview.chromium.org/61763004/diff/1/public/web/WebEmbeddedWorker....
public/web/WebEmbeddedWorker.h:46: class WebEmbeddedWorker {
On 2013/11/07 17:51:13, abarth wrote:
> Is an EmbeddedWorker a SharedWorker or can it be another kind fo worker?

It's primarily for ServiceWorker. I'm thinking about refactoring SharedWorker to
follow the same/similar class structures but at a lower priority.

(Rough design doc is available here:
https://docs.google.com/a/chromium.org/document/d/1HzjaXttZcRr6cE7oINMzvlVskb...)

https://codereview.chromium.org/61763004/diff/1/public/web/WebEmbeddedWorker....
public/web/WebEmbeddedWorker.h:61: WebEmbeddedWorkerStartMode) { }
On 2013/11/07 17:51:13, abarth wrote:
> Should these be pure virtual?
> 
> Rather than adding all these parameters (which might grow over time), can we
> pass in a struct containing these values?  That's easier to evolve over time.

Done.

https://codereview.chromium.org/61763004/diff/1/public/web/WebEmbeddedWorkerC...
File public/web/WebEmbeddedWorkerClient.h (right):

https://codereview.chromium.org/61763004/diff/1/public/web/WebEmbeddedWorkerC...
public/web/WebEmbeddedWorkerClient.h:36: namespace WebKit {
On 2013/11/07 17:51:13, abarth wrote:
> WebKit -> blink

Done.

[abarth-chromium, 2013-11-08 18:03:50]

It doesn't look like you've uploaded a new version of your patch, but assuming
you've done everything in your last message, LGTM on the new version.

[alecflett, 2013-11-08 23:18:35]

https://codereview.chromium.org/61763004/diff/1/public/web/WebEmbeddedWorkerC...
File public/web/WebEmbeddedWorkerClient.h (right):

https://codereview.chromium.org/61763004/diff/1/public/web/WebEmbeddedWorkerC...
public/web/WebEmbeddedWorkerClient.h:45: class WebEmbeddedWorkerClient : public
WebWorkerPermissionClientProxy {
This may be a non-issue, but it seems odd for an "interface" like this to be
subclassed from this proxy class, rather than letting the implementation choose
to also implement it (i.e. via multiple inheritance)

If I'm totally off base and there are existing patterns like this let me know
too - I'm a little behind on some of the shared worker / permissions cleanup
that you've been doing.

[michaeln, 2013-11-12 22:49:59]

https://codereview.chromium.org/61763004/diff/100001/public/web/WebEmbeddedWo...
File public/web/WebEmbeddedWorkerStartData.h (right):

https://codereview.chromium.org/61763004/diff/100001/public/web/WebEmbeddedWo...
public/web/WebEmbeddedWorkerStartData.h:42:
WebEmbeddedWorkerStartModePauseOnStart
What is the pause mode about? Is this related to debugging somehow?

https://codereview.chromium.org/61763004/diff/100001/public/web/WebEmbeddedWo...
public/web/WebEmbeddedWorkerStartData.h:47: WebString scriptSource;
I think we should remove scriptSource from this set of stuff and instead declare
that a worker contexts first task is to load/decode its script thru its main
resource loader.

Unlike shared workers, a running page is not directly involved at the time an
embedded worker is spun up. Loading/decoding should be done on the blink side of
this api.

https://codereview.chromium.org/61763004/diff/100001/public/web/WebEmbeddedWo...
File public/web/WebEmbeddedWorkerStartMode.h (right):

https://codereview.chromium.org/61763004/diff/100001/public/web/WebEmbeddedWo...
public/web/WebEmbeddedWorkerStartMode.h:38:
WebEmbeddedWorkerStartModePauseOnStart
wait... dejavu... at least one of these (possibly both) is not needed ;)

https://codereview.chromium.org/61763004/diff/100001/public/web/WebServiceWor...
File public/web/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/61763004/diff/100001/public/web/WebServiceWor...
public/web/WebServiceWorkerContextClient.h:45: class
WebServiceWorkerContextClient : public WebWorkerPermissionClientProxy {
This derivation seems odd to me too, its not a generalization/specialization
really.

[kinuko, 2013-11-18 10:41:13]

Thanks, updated.

https://codereview.chromium.org/61763004/diff/100001/public/web/WebEmbeddedWo...
File public/web/WebEmbeddedWorkerStartData.h (right):

https://codereview.chromium.org/61763004/diff/100001/public/web/WebEmbeddedWo...
public/web/WebEmbeddedWorkerStartData.h:42:
WebEmbeddedWorkerStartModePauseOnStart
On 2013/11/12 22:50:00, michaeln wrote:
> What is the pause mode about? Is this related to debugging somehow?

Yes, it's for debugging via Inspector. (Basically trying to follow what's
available in shared worker)

https://codereview.chromium.org/61763004/diff/100001/public/web/WebEmbeddedWo...
public/web/WebEmbeddedWorkerStartData.h:47: WebString scriptSource;
On 2013/11/12 22:50:00, michaeln wrote:
> I think we should remove scriptSource from this set of stuff and instead
declare
> that a worker contexts first task is to load/decode its script thru its main
> resource loader.

Ok... done. I was assuming we'd want to do so (always loading from URL), but
wasn't sure which way we want to do in our early version for offline case so
just left both but with TODO comment (in chromium patch).
 
> Unlike shared workers, a running page is not directly involved at the time an
> embedded worker is spun up. Loading/decoding should be done on the blink side
of
> this api.

I was considering the possibility to supply or load it in the browser process,
not in the renderer peer.

https://codereview.chromium.org/61763004/diff/100001/public/web/WebEmbeddedWo...
File public/web/WebEmbeddedWorkerStartMode.h (right):

https://codereview.chromium.org/61763004/diff/100001/public/web/WebEmbeddedWo...
public/web/WebEmbeddedWorkerStartMode.h:38:
WebEmbeddedWorkerStartModePauseOnStart
On 2013/11/12 22:50:00, michaeln wrote:
> wait... dejavu... at least one of these (possibly both) is not needed ;)

The Cl's screwed up... removed this one ;)

https://codereview.chromium.org/61763004/diff/100001/public/web/WebServiceWor...
File public/web/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/61763004/diff/100001/public/web/WebServiceWor...
public/web/WebServiceWorkerContextClient.h:45: class
WebServiceWorkerContextClient : public WebWorkerPermissionClientProxy {
On 2013/11/12 22:50:00, michaeln wrote:
> This derivation seems odd to me too, its not a generalization/specialization
> really.

No. I wanted to make this class as a collection of client interfaces, like
WorkerPermissionClientProxy, WorkerReportingClientProxy and
WorkerServiceWorkerGlobalScopeClientProxy.

We can't simply let implementation inherit them by multiple inheritance because
these client instances are basically intended to be owned by WorkerGlobalScope.

For clearer interfaces I removed this inheritance anyway (which means we'll have
separate implementations for them in chromium side too).

[michaeln, 2013-11-18 19:48:15]

lgtm

I'm assuming the voodoo'ish startdata params are hoisted from the world of
sharedworkers?

https://codereview.chromium.org/61763004/diff/170001/public/web/WebEmbeddedWo...
File public/web/WebEmbeddedWorkerStartData.h (right):

https://codereview.chromium.org/61763004/diff/170001/public/web/WebEmbeddedWo...
public/web/WebEmbeddedWorkerStartData.h:48: WebString contentSecurityPolicy;
What kind of value goes here and where does it come from in the service-worker
case? Is an empty string a valid input?

https://codereview.chromium.org/61763004/diff/170001/public/web/WebEmbeddedWo...
public/web/WebEmbeddedWorkerStartData.h:49: WebContentSecurityPolicyType
contentSecurityPolicyType;
What do these values me... just curious?
    WebContentSecurityPolicyTypeReport,
    WebContentSecurityPolicyTypeEnforce,
(Oh... its security related, its not supposed to make sense ;)

[kinuko, 2013-11-19 07:32:29]

https://codereview.chromium.org/61763004/diff/170001/public/web/WebEmbeddedWo...
File public/web/WebEmbeddedWorkerStartData.h (right):

https://codereview.chromium.org/61763004/diff/170001/public/web/WebEmbeddedWo...
public/web/WebEmbeddedWorkerStartData.h:48: WebString contentSecurityPolicy;
On 2013/11/18 19:48:16, michaeln wrote:
> What kind of value goes here and where does it come from in the service-worker
> case? Is an empty string a valid input?

(As I wrote in the other review) it's for CSP, and the string's coming from
Content-security-policy HTTP header.  An empty string is a valid string, so
while we're still plumbing we could probably start with an empty string, but
eventually we'll probably need to send this from SWProvider.

[kinuko, 2013-11-19 07:33:04]

(I'm going to land this now, hopefully we can iterate on this)

[commit-bot: I haz the power, 2013-11-19 07:35:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/61763004/170001

[commit-bot: I haz the power, 2013-11-19 08:33:59]

Change committed as 162282