Implement initial part of WebEmbeddedWorker

Implement initial part of WebEmbeddedWorker

This implements:
- Shadow page setup
- Initial script loading
- Some very basic wiring

WorkerThread / WorkerGlobalScope code is not yet wired.

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

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

PTL

[michaeln, 2013-11-21 02:00:40]

https://codereview.chromium.org/78233002/diff/1/Source/web/WebEmbeddedWorkerI...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/78233002/diff/1/Source/web/WebEmbeddedWorkerI...
Source/web/WebEmbeddedWorkerImpl.cpp:97: m_loading = false;
Once we've retrieved the results of the main script load, we no longer need the
m_scriptLoader.

https://codereview.chromium.org/78233002/diff/1/Source/web/WebEmbeddedWorkerI...
Source/web/WebEmbeddedWorkerImpl.cpp:109: 
What if the main script load involves a series of redirects, potentially to a
location on a different origin?

I guess thats a spec question as much as an impl question.

https://codereview.chromium.org/78233002/diff/1/Source/web/WebEmbeddedWorkerI...
Source/web/WebEmbeddedWorkerImpl.cpp:120:
m_workerStartData.contentSecurityPolicy,
ok... so this CSP header value... i think we should get that value out of the
script loader, WorkerScriptLoaderClient::didReceiveResponse, wdyt?

https://codereview.chromium.org/78233002/diff/1/Source/web/WebEmbeddedWorkerI...
Source/web/WebEmbeddedWorkerImpl.cpp:132: return;
How do we abort/cancel the main script load if it's not yet complete?

https://codereview.chromium.org/78233002/diff/1/Source/web/WebEmbeddedWorkerI...
File Source/web/WebEmbeddedWorkerImpl.h (right):

https://codereview.chromium.org/78233002/diff/1/Source/web/WebEmbeddedWorkerI...
Source/web/WebEmbeddedWorkerImpl.h:50: class WebEmbeddedWorkerImpl : public
WebEmbeddedWorker, public WebFrameClient, public
WebCore::WorkerScriptLoaderClient, public WebCore::WorkerLoaderProxy {
can you wrap this line so its easier to read what base classes this derives
from?

https://codereview.chromium.org/78233002/diff/1/Source/web/WebEmbeddedWorkerI...
Source/web/WebEmbeddedWorkerImpl.h:82: RefPtr<WebCore::WorkerScriptLoader>
m_scriptLoader;
m_mainScriptLoader might be more clear of a name

https://codereview.chromium.org/78233002/diff/1/public/web/WebServiceWorkerCo...
File public/web/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/78233002/diff/1/public/web/WebServiceWorkerCo...
public/web/WebServiceWorkerContextClient.h:44: // (and this should be called
first in the normal case) all methods of this
The contract for these callbacks is squishy. It might be nice to tighten that
up.

What if, in the usual case...
  contextStarted()
  contextClosed()  // why is this one needed btw?
  contextDestroyed()
... happen on the worker thread in that order.

And in the early termnination or failed to load the script case...
  contextFailedToStart()
... happens on the main thread and none of the others happen at all.

(nit: Also the verbs used are kind of mixed up, started but never stopped,
closed but never opened, destroyed but never created).

[kinuko, 2013-11-21 06:36:05]

Updated. I think it looks better.. PTAL?

https://codereview.chromium.org/78233002/diff/1/Source/web/WebEmbeddedWorkerI...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/78233002/diff/1/Source/web/WebEmbeddedWorkerI...
Source/web/WebEmbeddedWorkerImpl.cpp:97: m_loading = false;
On 2013/11/21 02:00:40, michaeln wrote:
> Once we've retrieved the results of the main script load, we no longer need
the
> m_scriptLoader.

Done.

https://codereview.chromium.org/78233002/diff/1/Source/web/WebEmbeddedWorkerI...
Source/web/WebEmbeddedWorkerImpl.cpp:109: 
On 2013/11/21 02:00:40, michaeln wrote:
> What if the main script load involves a series of redirects, potentially to a
> location on a different origin?
> 
> I guess thats a spec question as much as an impl question.

Currently we do deny cross-origin request, even via redirects, and this is same
as the current worker spec & behavior. I think we probably want to keep this in
SW too.
http://www.w3.org/TR/2009/WD-workers-20091029/#processing-model

https://codereview.chromium.org/78233002/diff/1/Source/web/WebEmbeddedWorkerI...
Source/web/WebEmbeddedWorkerImpl.cpp:120:
m_workerStartData.contentSecurityPolicy,
On 2013/11/21 02:00:40, michaeln wrote:
> ok... so this CSP header value... i think we should get that value out of the
> script loader, WorkerScriptLoaderClient::didReceiveResponse, wdyt?

Yup. Today I had time to investigate this a bit, and added code to get CSP info.
Also removed CSP headers from StartData (will update the chromium side patch
too).

https://codereview.chromium.org/78233002/diff/1/Source/web/WebEmbeddedWorkerI...
Source/web/WebEmbeddedWorkerImpl.cpp:132: return;
On 2013/11/21 02:00:40, michaeln wrote:
> How do we abort/cancel the main script load if it's not yet complete?

Added cancel().

https://codereview.chromium.org/78233002/diff/1/Source/web/WebEmbeddedWorkerI...
File Source/web/WebEmbeddedWorkerImpl.h (right):

https://codereview.chromium.org/78233002/diff/1/Source/web/WebEmbeddedWorkerI...
Source/web/WebEmbeddedWorkerImpl.h:50: class WebEmbeddedWorkerImpl : public
WebEmbeddedWorker, public WebFrameClient, public
WebCore::WorkerScriptLoaderClient, public WebCore::WorkerLoaderProxy {
On 2013/11/21 02:00:40, michaeln wrote:
> can you wrap this line so its easier to read what base classes this derives
> from?

Sure... done. (I'm not very happy with the new presubmit style check, which only
accepts rigid 4-space indent)

https://codereview.chromium.org/78233002/diff/1/Source/web/WebEmbeddedWorkerI...
Source/web/WebEmbeddedWorkerImpl.h:82: RefPtr<WebCore::WorkerScriptLoader>
m_scriptLoader;
On 2013/11/21 02:00:40, michaeln wrote:
> m_mainScriptLoader might be more clear of a name

Done.

https://codereview.chromium.org/78233002/diff/1/public/web/WebServiceWorkerCo...
File public/web/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/78233002/diff/1/public/web/WebServiceWorkerCo...
public/web/WebServiceWorkerContextClient.h:44: // (and this should be called
first in the normal case) all methods of this
On 2013/11/21 02:00:40, michaeln wrote:
> The contract for these callbacks is squishy. It might be nice to tighten that
> up.
> 
> What if, in the usual case...
>   contextStarted()
>   contextClosed()  // why is this one needed btw?
>   contextDestroyed()
> ... happen on the worker thread in that order.
> 
> And in the early termnination or failed to load the script case...
>   contextFailedToStart()
> ... happens on the main thread and none of the others happen at all.

Sounds good, done.
 
> (nit: Also the verbs used are kind of mixed up, started but never stopped,
> closed but never opened, destroyed but never created).

Yup, I had the same feeling. These come from existing WorkerGlobalScope /
WorkerReportingProxy verbs, we could reinvent them but I'm not too motivated to
do so for now.

Reg: close perhaps we may be able to live without this exposed at blink API...
removing it for now.  (SWGlobalScope inherits from WGS so app can call close(),
and there's a little period since close() is called and until the WGS is
destroyed. In SharedWorker case we let the browser know about it so that it
doesn't send more messages to the worker)

https://codereview.chromium.org/78233002/diff/140001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/78233002/diff/140001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:151: m_mainScriptLoader->cancel();
note: this will call didLoadScriptAndStartWorkerContext, where we call
workerContextFailedToStart.

[michaeln, 2013-11-21 22:05:35]

https://codereview.chromium.org/78233002/diff/140001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/78233002/diff/140001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:66: m_scriptLoader->setClient(0);
was wondering where the that setter was used

https://codereview.chromium.org/78233002/diff/140001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:69: virtual void
didReceiveResponse(unsigned long, const WebCore::ResourceResponse& response)
OVERRIDE keyword?

https://codereview.chromium.org/78233002/diff/140001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:142:
bind(&WebEmbeddedWorkerImpl::didLoadScriptAndStartWorkerContext, this));
i have to remind myself that despite looking like chromium-code, its not quite
the same... coke vs diet-coke :)

https://codereview.chromium.org/78233002/diff/140001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:196: void
WebEmbeddedWorkerImpl::didLoadScriptAndStartWorkerContext()
nit: method naming, its a combo of a notification that something is complete
mixed with a directive to start something else. Upon first reading the method
name i thought it meant... didLoadScript + didStartWorkerContext.

Since this is also used on the failure case, maybe rename to something that
doesn't sounds like it succeeded necessarily? If this were chromium code, i'd
suggest onScriptLoaderComplete or onScriptLoaderFinished or something along
those lines.

Or maybe have your loader class take separate inputs for didFail and didSucceed
callbacks?

https://codereview.chromium.org/78233002/diff/140001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:215: cspType =
ContentSecurityPolicy::Report;
I'm trying to figure out how the ContentSecurityPolicyResponseHeaders are
supposed to be interpretted and its not obvious The code you've written is using
a non-empty 'report' header as a flag that determines how to interpret the other
header value. Based on what ContentSecirutyPolicy.cpp contains, I don't think
that's right?

Here's how ContentSecirutyPolicy.cpp decodes this stuff...

    if (!headers.contentSecurityPolicy().isEmpty())
        didReceiveHeader(headers.contentSecurityPolicy(),
ContentSecurityPolicy::Enforce);
    if (!headers.contentSecurityPolicyReportOnly().isEmpty())
        didReceiveHeader(headers.contentSecurityPolicyReportOnly(),
ContentSecurityPolicy::Report);

https://codereview.chromium.org/78233002/diff/140001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:222: m_workerStartData.scriptURL,
If there is a same-origin redirect, i think we need to use the url of the final
location here. Relative urls will be computed based on the value passed in here.

https://codereview.chromium.org/78233002/diff/140001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.h (right):

https://codereview.chromium.org/78233002/diff/140001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.h:65: // WebCore::WorkerLoaderProxy overrides.
The fact that this class implements WorkerLoaderProxy feels like an
implementation detail. If you buy that, maybe move these overrides to the
private section.

https://codereview.chromium.org/78233002/diff/140001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.h:90: // 'shadow page' - created to proxy
loading requests from the worker.
This "shadow page" stuff is the technique we currently use for shared workers
and it's a good hack, we get a fully functional webcore "loader" for the bargain
basement price of the prepareShadowPageForLoader() method.

I don't know if nate &| adam can provide some pointers on a better way to cons
up a "loader". If not, I'm all for sticking with the good hack again.

[michaeln, 2013-11-21 22:14:37]

https://codereview.chromium.org/78233002/diff/1/Source/web/WebEmbeddedWorkerI...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/78233002/diff/1/Source/web/WebEmbeddedWorkerI...
Source/web/WebEmbeddedWorkerImpl.cpp:109: 
On 2013/11/21 06:36:05, kinuko wrote:
> On 2013/11/21 02:00:40, michaeln wrote:
> > What if the main script load involves a series of redirects, potentially to
a
> > location on a different origin?
> > 
> > I guess thats a spec question as much as an impl question.
> 
> Currently we do deny cross-origin request, even via redirects, and this is
same
> as the current worker spec & behavior. I think we probably want to keep this
in
> SW too.
> http://www.w3.org/TR/2009/WD-workers-20091029/#processing-model

That sgtm2, same-origin redirects only.

[kinuko, 2013-11-22 02:23:33]

Updated, removed some parts for a next patch

https://codereview.chromium.org/78233002/diff/140001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/78233002/diff/140001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:69: virtual void
didReceiveResponse(unsigned long, const WebCore::ResourceResponse& response)
On 2013/11/21 22:05:35, michaeln wrote:
> OVERRIDE keyword?

Done.

https://codereview.chromium.org/78233002/diff/140001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:196: void
WebEmbeddedWorkerImpl::didLoadScriptAndStartWorkerContext()
On 2013/11/21 22:05:35, michaeln wrote:
> nit: method naming, its a combo of a notification that something is complete
> mixed with a directive to start something else. Upon first reading the method
> name i thought it meant... didLoadScript + didStartWorkerContext.
> 
> Since this is also used on the failure case, maybe rename to something that
> doesn't sounds like it succeeded necessarily? If this were chromium code, i'd
> suggest onScriptLoaderComplete or onScriptLoaderFinished or something along
> those lines.
> 
> Or maybe have your loader class take separate inputs for didFail and
didSucceed
> callbacks?

Actually I suspected this name would be bad for the same reason, renamed to
onScriptLoaderFinished().

https://codereview.chromium.org/78233002/diff/140001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:215: cspType =
ContentSecurityPolicy::Report;
On 2013/11/21 22:05:35, michaeln wrote:
> I'm trying to figure out how the ContentSecurityPolicyResponseHeaders are
> supposed to be interpretted and its not obvious The code you've written is
using
> a non-empty 'report' header as a flag that determines how to interpret the
other
> header value. Based on what ContentSecirutyPolicy.cpp contains, I don't think
> that's right?
> 
> Here's how ContentSecirutyPolicy.cpp decodes this stuff...
> 
>     if (!headers.contentSecurityPolicy().isEmpty())
>         didReceiveHeader(headers.contentSecurityPolicy(),
> ContentSecurityPolicy::Enforce);
>     if (!headers.contentSecurityPolicyReportOnly().isEmpty())
>         didReceiveHeader(headers.contentSecurityPolicyReportOnly(),
> ContentSecurityPolicy::Report);
> 

I'm thinking about passing ContentSecurityPolicyResponseHeader to the
ThreadStartupData without interpreting it here.  Removed this part for now as we
can't do so yet

https://codereview.chromium.org/78233002/diff/140001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:222: m_workerStartData.scriptURL,
On 2013/11/21 22:05:35, michaeln wrote:
> If there is a same-origin redirect, i think we need to use the url of the
final
> location here. Relative urls will be computed based on the value passed in
here.

I was going to do so, but existing worker code is just using the given URL. I
need to investigate if that's the bug. Removed this WorkerThreadStartupData part
altogether too

https://codereview.chromium.org/78233002/diff/140001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.h (right):

https://codereview.chromium.org/78233002/diff/140001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.h:65: // WebCore::WorkerLoaderProxy overrides.
On 2013/11/21 22:05:35, michaeln wrote:
> The fact that this class implements WorkerLoaderProxy feels like an
> implementation detail. If you buy that, maybe move these overrides to the
> private section.

Not really determined yet, but removed this override for now (not yet used).

[kinuko, 2013-11-22 07:30:16]

While I was poking around CSP + Worker I learned that common usage is like
adding http meta header in the parent context's html file, like:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

This means that using didReceiveResponse() method for worker script doesn't do
what's expected in the common Worker usage.  This may need spec discussion how
we should do for ServiceWorker, but I guess if we ignore the CSP context
declared in the registering document users would be confused. WDYT?

(+mkwst for CSP discussion. Mike: we're adding a new worker type for
ServiceWorker and are confused how we should fill CSP info for this worker)

[kinuko, 2013-11-22 12:25:08]

Some more info from Mike: 

> So, normal workers will inherit the CSP from the document that spawned them
(the
> "protected resource" in the spec's parlance). Headers specified on the worker
> resources themselves are ignored, just as headers on scripts loaded into the
> protected resource are ignored.
> 
>  I don't think there's yet consensus regarding shared workers. Nor has the WG
> really considered ServiceWorkers yet. :/
> 
> That said:
> http://lists.w3.org/Archives/Public/public-webappsec/2013Nov/0008.html is
> proposing more or less what you have here. So things might change. :) If you
> have an opinion, I'd suggest hopping onto the mailing list and writing it
down.

So current CSP document says the owner document's CSP policy should be enforced
to regular workers, but not clear consensus for SharedWorkers yet and there's a
voice that it should be handled as a separated document (like our patch has been
doing).
I've removed the CSP code from the current patch for now.

[kinuko, 2013-11-22 12:26:28]

...and leaving that part for now (I'll file a bug on github), can you review the
patch again?  :)

[michaeln, 2013-11-22 19:56:19]

> So current CSP document says the owner document's CSP policy should be
enforced
> to regular workers, but not clear consensus for SharedWorkers yet and there's
a
> voice that it should be handled as a separated document (like our patch has
been
> doing).
> I've removed the CSP code from the current patch for now.

A dedicated worker inheriting the CSP of is parent page makes good sense, but
that doesn't work for a shared worker since multiple documents with different
policies can all reach the same shared worker. ServiceWorkers and SharedWorkers
establish a BrowsingContext that is distinct from any Document, DedicatedWorkers
are fundamentally different (more like a <script> that runs the the background).

> should be handled as a separated document

Absolutely, thats the only thing that makes sense. In the service worker case, i
think it gets more clear that the CSP really needs to come from the service
worker resource when you consider the update cycle.

As for <meta http-equiv> tags, serviceworkers don't have a DOM, headers have to
be set if you want to set CSP policies.

[michaeln, 2013-11-22 20:01:15]

https://codereview.chromium.org/78233002/diff/140001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/78233002/diff/140001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:222: m_workerStartData.scriptURL,
On 2013/11/22 02:23:33, kinuko wrote:
> On 2013/11/21 22:05:35, michaeln wrote:
> > If there is a same-origin redirect, i think we need to use the url of the
> final
> > location here. Relative urls will be computed based on the value passed in
> here.
> 
> I was going to do so, but existing worker code is just using the given URL. I
> need to investigate if that's the bug. Removed this WorkerThreadStartupData
part
> altogether too

If SharedWorkers get the baseurl wrong, that's no reason for ServiceWorkers to
get it wrong too :)

[michaeln, 2013-11-22 21:35:11]

lgtm

[commit-bot: I haz the power, 2013-11-23 00:22:10]

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

[commit-bot: I haz the power, 2013-11-23 00:22:15]

Failed to apply patch for Source/platform/network/ResourceRequest.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/platform/network/ResourceRequest.h
  Hunk #1 FAILED at 65.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/platform/network/ResourceRequest.h.rej

Patch:       Source/platform/network/ResourceRequest.h
Index: Source/platform/network/ResourceRequest.h
diff --git a/Source/platform/network/ResourceRequest.h
b/Source/platform/network/ResourceRequest.h
index
3372d4c3c04e6e7176459c18ae30ab4c268d69bc..b01449ba537190a316a024dec689dee34367946f
100644
--- a/Source/platform/network/ResourceRequest.h
+++ b/Source/platform/network/ResourceRequest.h
@@ -65,6 +65,7 @@ public:
         TargetIsFavicon,
         TargetIsXHR,
         TargetIsTextTrack,
+        TargetIsServiceWorker,
         TargetIsUnspecified,
     };

[kinuko, 2013-11-25 01:30:39]

abarth@: Could you do an owner review for public/web (minor changes only) and
Source/web?

[abarth-chromium, 2013-11-25 01:45:36]

I don't really feel qualified to review this CL...  Who is the right person to
review it?  I can rubber-stamp the CL to unblock you, but that doesn't solve the
longer term problem if having someone understand and own this mechanism...

I've added adamk to the reviewers line.  Maybe he'd be willing to take a look? 
Another possibility is that we could add kinuko to Source/web/OWNERS and have
her own this mechanism.

https://codereview.chromium.org/78233002/diff/540001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/78233002/diff/540001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:156: m_webView->setMainFrame(m_mainFrame);
Do shared workers do something like this?  If so, do we share this code with
that codepath?

https://codereview.chromium.org/78233002/diff/540001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.h (right):

https://codereview.chromium.org/78233002/diff/540001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.h:86: WebFrame* m_mainFrame;
What manages the lifetime of these raw pointers?  A raw pointer to a WebView is
kind of strange...

[kinuko, 2013-11-25 03:31:48]

Thanks, added some response + comments.

Let's see if adamk@ can review this (and related future changes) first...
adamk@, do you think you could review this?

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

https://codereview.chromium.org/78233002/diff/540001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/78233002/diff/540001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:156: m_webView->setMainFrame(m_mainFrame);
On 2013/11/25 01:45:37, abarth wrote:
> Do shared workers do something like this?  If so, do we share this code with
> that codepath?

Yes, mostly same. We should share this code eventually. I'm thinking about
extending this class to implement SharedWorker eventually, or will factor out a
common part as a base class. (I'm having difficult time to come up with a good
name for Shared + Service worker common code)
For now I left FIXME comment.

https://codereview.chromium.org/78233002/diff/540001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.h (right):

https://codereview.chromium.org/78233002/diff/540001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.h:86: WebFrame* m_mainFrame;
On 2013/11/25 01:45:37, abarth wrote:
> What manages the lifetime of these raw pointers?  A raw pointer to a WebView
is
> kind of strange...

I added comment about the ownership/lifetime semantics.
This class manages/owns them, and it calls WebView::close() and
WebFrame::close() in its dtor. Not sure that's the best way, but it looks this
is what SharedWorker does, and also most of test code does.

[kinuko, 2013-11-26 08:49:20]

adamk@: ping, would you be able to take a look?

Or I could try asking to add myself to OWNER and take ownership on these.

[adamk, 2013-11-26 18:54:34]

Sorry for the delay, this got lost in my inbox yesterday. I'd be happy to review
further changes in this series; I've been doing some minor cleanups in worker
code and would be interested in learning more about the brave new world of
shared-workers-in-renderer-processes.

This lgtm as a start, I have a few questions below. Also, I'm unfortunately not
a public/ OWNER so I can't stamp that part of the change.

https://codereview.chromium.org/78233002/diff/620001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/78233002/diff/620001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:172: m_loadingDocument =
webFrame->frame()->document();
Is this purposely retained after the m_mainScriptLoader is cleared?

https://codereview.chromium.org/78233002/diff/620001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.h (right):

https://codereview.chromium.org/78233002/diff/620001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.h:90: RefPtr<WebCore::ExecutionContext>
m_loadingDocument;
Seems odd that this isn't a WebCore::Document given the name "loadingDocument"

[abarth-chromium, 2013-11-26 19:40:38]

public/ <-- LGTM

[kinuko, 2013-11-27 00:22:21]

On 2013/11/26 18:54:34, adamk wrote:
> Sorry for the delay, this got lost in my inbox yesterday. I'd be happy to
review
> further changes in this series; I've been doing some minor cleanups in worker
> code and would be interested in learning more about the brave new world of
> shared-workers-in-renderer-processes.

That's great, thanks!

>
https://codereview.chromium.org/78233002/diff/620001/Source/web/WebEmbeddedWo...
> File Source/web/WebEmbeddedWorkerImpl.cpp (right):
> 
>
https://codereview.chromium.org/78233002/diff/620001/Source/web/WebEmbeddedWo...
> Source/web/WebEmbeddedWorkerImpl.cpp:172: m_loadingDocument =
> webFrame->frame()->document();
> Is this purposely retained after the m_mainScriptLoader is cleared?

Yes, this context will be also used for further resource loading from the worker
context, which is not yet written.

>
https://codereview.chromium.org/78233002/diff/620001/Source/web/WebEmbeddedWo...
> File Source/web/WebEmbeddedWorkerImpl.h (right):
> 
>
https://codereview.chromium.org/78233002/diff/620001/Source/web/WebEmbeddedWo...
> Source/web/WebEmbeddedWorkerImpl.h:90: RefPtr<WebCore::ExecutionContext>
> m_loadingDocument;
> Seems odd that this isn't a WebCore::Document given the name "loadingDocument"

Good point, will rename this to loadingContext.

[commit-bot: I haz the power, 2013-11-27 05:04:03]

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

[commit-bot: I haz the power, 2013-11-27 09:50:40]

Change committed as 162741