Actually start ServiceWorkerThread in EmbeddedWorker

Actually start ServiceWorkerThread in EmbeddedWorker

- Implement actual code to start ServiceWorkerThread and
  ServiceWorkerGlobalScope in WebEmbeddedWorkerImpl::onScriptLoaderFinished().
- Also adds ServiceWorkerGlobalScopeProxy class, which implements
  WebServiceWorkerContextProxy and WorkerLoaderProxy.
  This does mostly just relaying messages to/from WorkerGlobalScope
  and ServiceWorkerContextClient implementation in Chromium.

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

[kinuko, 2013-11-27 10:25:19]

This patch actually connects alec's WorkerGlobalScope patch and EmbeddedWorker
part. Can you review? (After holiday season finishes, this one can wait)

[kinuko, 2013-12-05 03:01:09]

adamk@: would you be able to review this?  This patch's actually starting a new
ServiceWorkerGlobalScope from the EmbeddedWorker.

[alecflett, 2013-12-05 04:21:19]

Are we at the point where we can start writing webkit_unittests against this?

This lgtm to me but adamk@ should obviously take a look.

https://codereview.chromium.org/91173002/diff/1/Source/web/ServiceWorkerGloba...
File Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/91173002/diff/1/Source/web/ServiceWorkerGloba...
Source/web/ServiceWorkerGlobalScopeProxy.h:54: // WorkerGlobalScope.
Can you outline the general purpose of this class too? It seems to basically be
a bridge between the WebKit API and the JS running in the ServiceWorker, which
is exposed through ServiceWorkerGlobalScope?

For example, Is this where the callbacks from onfetch/etc will dispatch out to
chrome?

[kinuko, 2013-12-05 07:36:55]

I'll check if we can start adding some tests for this.

https://codereview.chromium.org/91173002/diff/1/Source/web/ServiceWorkerGloba...
File Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/91173002/diff/1/Source/web/ServiceWorkerGloba...
Source/web/ServiceWorkerGlobalScopeProxy.h:54: // WorkerGlobalScope.
On 2013/12/05 04:21:19, alecflett wrote:
> Can you outline the general purpose of this class too? It seems to basically
be
> a bridge between the WebKit API and the JS running in the ServiceWorker, which
> is exposed through ServiceWorkerGlobalScope?
> 
> For example, Is this where the callbacks from onfetch/etc will dispatch out to
> chrome?

Yes, that's the current plan. I've added some overview comment here.

[adamk, 2013-12-05 22:50:12]

lgtm % style nits

Also interested in your thoughts about pointers vs references

https://codereview.chromium.org/91173002/diff/20001/Source/web/ServiceWorkerG...
File Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/91173002/diff/20001/Source/web/ServiceWorkerG...
Source/web/ServiceWorkerGlobalScopeProxy.cpp:94: ASSERT(m_client);
This ASSERT looks funny given that you're also assuming the other two arguments
are non-NULL too. This is what made me think that references might be the right
way to go here.

https://codereview.chromium.org/91173002/diff/20001/Source/web/ServiceWorkerG...
File Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/91173002/diff/20001/Source/web/ServiceWorkerG...
Source/web/ServiceWorkerGlobalScopeProxy.h:80: // These objects always outlive
this proxy.
Should they be references, then?

https://codereview.chromium.org/91173002/diff/20001/Source/web/WebEmbeddedWor...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/91173002/diff/20001/Source/web/WebEmbeddedWor...
Source/web/WebEmbeddedWorkerImpl.cpp:115: || !m_embeddedWorker->m_workerThread)
Nit: This linebreak looks funny in Blink

https://codereview.chromium.org/91173002/diff/20001/Source/web/WebEmbeddedWor...
Source/web/WebEmbeddedWorkerImpl.cpp:126: // Not owned, embedded worker owns
this.
Maybe should be a reference too?

I guess you're following the style around here (and in Chromium) by using
pointers; not clear if it's worth changing just a few of these, but I'd be
interested in your thoughts.

https://codereview.chromium.org/91173002/diff/20001/Source/web/WebEmbeddedWor...
Source/web/WebEmbeddedWorkerImpl.cpp:250: *m_loaderProxy.get(),
No need for .get() if you're using operator*.

https://codereview.chromium.org/91173002/diff/20001/Source/web/WebEmbeddedWor...
Source/web/WebEmbeddedWorkerImpl.cpp:251: *m_workerGlobalScopeProxy.get(),
Same here.

https://codereview.chromium.org/91173002/diff/20001/Source/web/WebEmbeddedWor...
Source/web/WebEmbeddedWorkerImpl.cpp:252: startupData.release());
More linebreaks...

[kinuko, 2013-12-09 12:24:20]

Thanks for review, updated the code.

Reg: ptr vs reference, I think I'm probably too accustomed to chromium style,
but using reference for non-null case totally makes sense (and we're in blink
here), so I followed (and will follow) your comment / the blink's style.

https://codereview.chromium.org/91173002/diff/20001/Source/web/ServiceWorkerG...
File Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/91173002/diff/20001/Source/web/ServiceWorkerG...
Source/web/ServiceWorkerGlobalScopeProxy.cpp:94: ASSERT(m_client);
On 2013/12/05 22:50:12, adamk wrote:
> This ASSERT looks funny given that you're also assuming the other two
arguments
> are non-NULL too. This is what made me think that references might be the
right
> way to go here.

Right... I think I wanted to have this assertion for m_client because this
pointer's given from outside blink (from chromium) while for the other two the
code paths are clearer.  Anyways, I changed other two to references to make the
assumption/contract more explicit.

https://codereview.chromium.org/91173002/diff/20001/Source/web/ServiceWorkerG...
File Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/91173002/diff/20001/Source/web/ServiceWorkerG...
Source/web/ServiceWorkerGlobalScopeProxy.h:80: // These objects always outlive
this proxy.
On 2013/12/05 22:50:12, adamk wrote:
> Should they be references, then?

Done.

https://codereview.chromium.org/91173002/diff/20001/Source/web/WebEmbeddedWor...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/91173002/diff/20001/Source/web/WebEmbeddedWor...
Source/web/WebEmbeddedWorkerImpl.cpp:115: || !m_embeddedWorker->m_workerThread)
On 2013/12/05 22:50:12, adamk wrote:
> Nit: This linebreak looks funny in Blink

Concatenated the lines.

https://codereview.chromium.org/91173002/diff/20001/Source/web/WebEmbeddedWor...
Source/web/WebEmbeddedWorkerImpl.cpp:126: // Not owned, embedded worker owns
this.
On 2013/12/05 22:50:12, adamk wrote:
> Maybe should be a reference too?
> 
> I guess you're following the style around here (and in Chromium) by using
> pointers; not clear if it's worth changing just a few of these, but I'd be
> interested in your thoughts.

Done.

https://codereview.chromium.org/91173002/diff/20001/Source/web/WebEmbeddedWor...
Source/web/WebEmbeddedWorkerImpl.cpp:250: *m_loaderProxy.get(),
On 2013/12/05 22:50:12, adamk wrote:
> No need for .get() if you're using operator*.

Done.

https://codereview.chromium.org/91173002/diff/20001/Source/web/WebEmbeddedWor...
Source/web/WebEmbeddedWorkerImpl.cpp:251: *m_workerGlobalScopeProxy.get(),
On 2013/12/05 22:50:12, adamk wrote:
> Same here.

Done.

https://codereview.chromium.org/91173002/diff/20001/Source/web/WebEmbeddedWor...
Source/web/WebEmbeddedWorkerImpl.cpp:252: startupData.release());
On 2013/12/05 22:50:12, adamk wrote:
> More linebreaks...

Too chrome'y? I packed these in one line.

[kinuko, 2013-12-09 12:33:56]

abarth@: could you do owner review for public/web change? (just adding one
forward decl)

[abarth-chromium, 2013-12-09 17:28:36]

public/web LGTM

[commit-bot: I haz the power, 2013-12-09 17:28:56]

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

[commit-bot: I haz the power, 2013-12-09 17:28:59]

Failed to apply patch for Source/web/WebEmbeddedWorkerImpl.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/web/WebEmbeddedWorkerImpl.h
  Hunk #1 FAILED at 35.
  Hunk #2 succeeded at 60 (offset -2 lines).
  Hunk #3 succeeded at 79 (offset -2 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
Source/web/WebEmbeddedWorkerImpl.h.rej

Patch:       Source/web/WebEmbeddedWorkerImpl.h
Index: Source/web/WebEmbeddedWorkerImpl.h
diff --git a/Source/web/WebEmbeddedWorkerImpl.h
b/Source/web/WebEmbeddedWorkerImpl.h
index
94dabc0ee9a29ade3ec28765731e815b1c25b174..038cd81865003bf19a5ab35a6a356df8401d039c
100644
--- a/Source/web/WebEmbeddedWorkerImpl.h
+++ b/Source/web/WebEmbeddedWorkerImpl.h
@@ -35,21 +35,21 @@
 #include "WebEmbeddedWorker.h"
 #include "WebEmbeddedWorkerStartData.h"
 #include "WebFrameClient.h"
-#include "core/frame/ContentSecurityPolicyResponseHeaders.h"
 
 namespace WebCore {
-class ContentSecurityPolicyResponseHeaders;
 class WorkerScriptLoader;
 class WorkerThread;
 }
 
 namespace blink {
 
+class ServiceWorkerGlobalScopeProxy;
 class WebView;
 
 class WebEmbeddedWorkerImpl :
     public WebEmbeddedWorker,
     public WebFrameClient {
+    WTF_MAKE_NONCOPYABLE(WebEmbeddedWorkerImpl);
 public:
     WebEmbeddedWorkerImpl(
         PassOwnPtr<WebServiceWorkerContextClient>,
@@ -62,6 +62,7 @@ public:
 
 private:
     class Loader;
+    class LoaderProxy;
 
     void prepareShadowPageForLoader();
     void onScriptLoaderFinished();
@@ -80,6 +81,8 @@ private:
     OwnPtr<Loader> m_mainScriptLoader;
 
     RefPtr<WebCore::WorkerThread> m_workerThread;
+    OwnPtr<LoaderProxy> m_loaderProxy;
+    OwnPtr<ServiceWorkerGlobalScopeProxy> m_workerGlobalScopeProxy;
 
     // 'shadow page' - created to proxy loading requests from the worker.
     // Both WebView and WebFrame objects are close()'ed (where they're

[commit-bot: I haz the power, 2013-12-10 03:32:49]

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

[commit-bot: I haz the power, 2013-12-10 05:07:44]

Change committed as 163495