Remove WorkerScriptLoaderClient and inheritances

Remove WorkerScriptLoaderClient and inheritances

This patch modifies WorkerScriptLoader not to use client
callback class, but to use Closures on asynchronous load.

Also, integrate some distributed functionalities from
WebEmbeddedWorkerImpl::Loader and
WebSharedWorkerImpl::Loader to WorkerScriptLoader so to remove
these similar loader classes.

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

[Takashi Toyoshima, 2015-06-18 12:32:14]

toyoshim@chromium.org changed reviewers:
+ kinuko@chromium.org

[Takashi Toyoshima, 2015-06-18 12:33:04]

not urgent since I won't have much time tomorrow and will do another
investigation on thread termination.

[Takashi Toyoshima, 2015-06-19 05:24:46]

https://codereview.chromium.org/1190133002/diff/70001/Source/core/workers/Wor...
File Source/core/workers/WorkerScriptLoader.cpp (left):

https://codereview.chromium.org/1190133002/diff/70001/Source/core/workers/Wor...
Source/core/workers/WorkerScriptLoader.cpp:95: // During create, callbacks may
happen which remove the last reference to this object.
Note:
This code was introduced by https://bugs.webkit.org/show_bug.cgi?id=62469, and
added test fast/workers/worker-crash-with-invalid-location.html works without
this workaround.

But, my understandings on ThreadableLoader isn't perfect at all.

[Takashi Toyoshima, 2015-06-23 11:55:48]

toyoshim@chromium.org changed reviewers:
+ mkwst@chromium.org

[Takashi Toyoshima, 2015-06-23 12:00:46]

Mike West, PTAL.

Since this change touches codes that could affect CSP propagation, it would be
nice if Mike can review.

See following comments for Kinuko. If we always create an empty CSP at the
WorkerScriptLoader's ctor, it breaks a test, worker-blob-inherits-csp.html as I
did in PS5.

https://codereview.chromium.org/1190133002/diff/90001/Source/core/workers/Wor...
File Source/core/workers/WorkerScriptLoader.cpp (right):

https://codereview.chromium.org/1190133002/diff/90001/Source/core/workers/Wor...
Source/core/workers/WorkerScriptLoader.cpp:205: // Should not create an empty
ContentSecurityPolicy object for other cases
Kinuko: this comment describes the point I talked offline today.

[Mike West, 2015-06-24 07:18:34]

The CSP bits LGTM % nits.

https://codereview.chromium.org/1190133002/diff/90001/Source/core/workers/InP...
File Source/core/workers/InProcessWorkerBase.h (right):

https://codereview.chromium.org/1190133002/diff/90001/Source/core/workers/InP...
Source/core/workers/InProcessWorkerBase.h:28: // creates them.
Nit: There's no 80-column limit in Blink, so there's no need to change this
comment.

https://codereview.chromium.org/1190133002/diff/90001/Source/core/workers/Wor...
File Source/core/workers/WorkerScriptLoader.cpp (right):

https://codereview.chromium.org/1190133002/diff/90001/Source/core/workers/Wor...
Source/core/workers/WorkerScriptLoader.cpp:205: // Should not create an empty
ContentSecurityPolicy object for other cases
On 2015/06/23 at 12:00:46, Takashi Toyoshima (chromium) wrote:
> Kinuko: this comment describes the point I talked offline today.

I'd rephrase this comment a bit. Something like: "Per
http://www.w3.org/TR/CSP2/#processing-model-workers, if the Worker's URL is not
a GUID, then it grabs its CSP from the response headers directly. Otherwise, the
Worker inherits the policy from the parent document (which is implemented in
WorkerMessagingProxy)."

[kinuko, 2015-06-24 07:27:55]

Looking good.

https://codereview.chromium.org/1190133002/diff/70001/Source/core/workers/Wor...
File Source/core/workers/WorkerScriptLoader.cpp (left):

https://codereview.chromium.org/1190133002/diff/70001/Source/core/workers/Wor...
Source/core/workers/WorkerScriptLoader.cpp:95: // During create, callbacks may
happen which remove the last reference to this object.
On 2015/06/19 05:24:46, Takashi Toyoshima (chromium) wrote:
> Note:
> This code was introduced by https://bugs.webkit.org/show_bug.cgi?id=62469, and
> added test fast/workers/worker-crash-with-invalid-location.html works without
> this workaround.
> 
> But, my understandings on ThreadableLoader isn't perfect at all.

This looks to be ok, but can you make sure that ThreadableLoader::create()
doesn't have a code path that could synchronously call any of given callbacks if
you haven't?

https://codereview.chromium.org/1190133002/diff/90001/Source/core/workers/Wor...
File Source/core/workers/WorkerScriptLoader.cpp (right):

https://codereview.chromium.org/1190133002/diff/90001/Source/core/workers/Wor...
Source/core/workers/WorkerScriptLoader.cpp:55:
WorkerScriptLoader::~WorkerScriptLoader() = default;
nit: it's now allowed and ok, but in this case it doesn't look too useful to use
this

https://codereview.chromium.org/1190133002/diff/90001/Source/core/workers/Wor...
Source/core/workers/WorkerScriptLoader.cpp:205: // Should not create an empty
ContentSecurityPolicy object for other cases
Ok, I found the relevant part:

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

https://codereview.chromium.org/1190133002/diff/90001/Source/web/WebSharedWor...
File Source/web/WebSharedWorkerImpl.cpp (right):

https://codereview.chromium.org/1190133002/diff/90001/Source/web/WebSharedWor...
Source/web/WebSharedWorkerImpl.cpp:363: contentSecurityPolicy->headers(),
nit: probably we could allow passing nullptr here (when contentSecurityPolicy is
null) and handle null-case in WorkerThreadStartupData's ctor.

[Takashi Toyoshima, 2015-06-24 08:53:45]

Thanks!

https://codereview.chromium.org/1190133002/diff/90001/Source/core/workers/InP...
File Source/core/workers/InProcessWorkerBase.h (right):

https://codereview.chromium.org/1190133002/diff/90001/Source/core/workers/InP...
Source/core/workers/InProcessWorkerBase.h:28: // creates them.
Yeah, you are right. But sometime top directory owner suggests me to wrap
comments in 80-columns. That must not be mandatory, I think. But I also prefer
80-column comments if it does not make things worse.

https://codereview.chromium.org/1190133002/diff/90001/Source/core/workers/Wor...
File Source/core/workers/WorkerScriptLoader.cpp (right):

https://codereview.chromium.org/1190133002/diff/90001/Source/core/workers/Wor...
Source/core/workers/WorkerScriptLoader.cpp:55:
WorkerScriptLoader::~WorkerScriptLoader() = default;
ok, revert this part.

https://codereview.chromium.org/1190133002/diff/90001/Source/core/workers/Wor...
Source/core/workers/WorkerScriptLoader.cpp:205: // Should not create an empty
ContentSecurityPolicy object for other cases
Thanks, Mike.
I'll replace this comment with your suggestion.
But, I still want to keep following part since it's a little tricky.
"m_contentSecurityPolicy should be left as nullptr to inherit the policy"

https://codereview.chromium.org/1190133002/diff/90001/Source/web/WebSharedWor...
File Source/web/WebSharedWorkerImpl.cpp (right):

https://codereview.chromium.org/1190133002/diff/90001/Source/web/WebSharedWor...
Source/web/WebSharedWorkerImpl.cpp:363: contentSecurityPolicy->headers(),
On 2015/06/24 07:27:54, kinuko wrote:
> nit: probably we could allow passing nullptr here (when contentSecurityPolicy
is
> null) and handle null-case in WorkerThreadStartupData's ctor.

Done.

[Takashi Toyoshima, 2015-06-24 08:54:42]

ok, let me see the try results, then ptal if it's fine.

[Takashi Toyoshima, 2015-06-24 10:41:55]

test looks fine :)

[kinuko, 2015-06-24 14:10:14]

lgtm

[Takashi Toyoshima, 2015-06-25 00:38:21]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2015-06-25 00:38:21]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1190133002/#ps110001
(title: "review #6 and #7")

[commit-bot: I haz the power, 2015-06-25 00:38:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1190133002/110001

[commit-bot: I haz the power, 2015-06-25 00:42:37]

Committed patchset #7 (id:110001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197787