Worker: Remove WorkerLoaderProxy for clean-up

Worker: Remove WorkerLoaderProxy for clean-up

For clean-up, this CL removes WorkerLoaderProxy and makes worker components
directly use ThreadableLoadingContext instead.

Before this CL WorkerLoaderProxy is used for conveying ThreadableLoadingContext
between main and worker threads, but it's no longer necessary: we can directly
pass ThreadableLoadingContext among the threads. One of the key features of
WorkerLoaderProxy was to detect worker termination on the main thread
(DetatchProvider()), but we already have a more proper way to do it
(WorkerThreadLifecycleObserver) and it's already used for loading.

BUG=694914
Review-Url: https://codereview.chromium.org/2840473002
Cr-Commit-Position: refs/heads/master@{#475308}
Committed: https://chromium.googlesource.com/chromium/src/+/3e327942abfd83502aa17cc59acefa6e30c91d0e

[nhiroki, 2017-04-24 03:59:17]

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

[commit-bot: I haz the power, 2017-04-24 03:59:25]

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, 2017-04-24 05:22:36]

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

[commit-bot: I haz the power, 2017-04-24 05:22:37]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2017-04-24 05:38:29]

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

[commit-bot: I haz the power, 2017-04-24 05:38:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nhiroki, 2017-04-24 06:03:36]

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

[commit-bot: I haz the power, 2017-04-24 06:03:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nhiroki, 2017-04-24 06:25:18]

Description was changed from

==========
Worker: Merge WorkerLoaderProxyProvider into WorkerLoaderProxy for cleanup

BUG=
==========

to

==========
Worker: Directly pass ThreadableLoadingContext instead of wrapping it with
WorkerLoaderProxy

Before this CL WorkerLoaderProxy is used for conveying ThreadableLoadingContext
between main and worker threads, but this is not necessary: we can directly pass
ThreadableLoadingContext among the threads.

This CL makes worker components directly use ThreadableLoadingContext and
removes WorkerLoaderProxy for cleanup.

BUG=
==========

[nhiroki, 2017-04-24 06:28:18]

Description was changed from

==========
Worker: Directly pass ThreadableLoadingContext instead of wrapping it with
WorkerLoaderProxy

Before this CL WorkerLoaderProxy is used for conveying ThreadableLoadingContext
between main and worker threads, but this is not necessary: we can directly pass
ThreadableLoadingContext among the threads.

This CL makes worker components directly use ThreadableLoadingContext and
removes WorkerLoaderProxy for cleanup.

BUG=
==========

to

==========
Worker: Directly pass ThreadableLoadingContext instead of wrapping it with
WorkerLoaderProxy

Before this CL WorkerLoaderProxy is used for conveying ThreadableLoadingContext
between main and worker threads, but this is not necessary: we can directly pass
ThreadableLoadingContext among the threads.

This CL makes worker components directly use ThreadableLoadingContext and
removes WorkerLoaderProxy for cleanup.

BUG=
==========

[nhiroki, 2017-04-24 06:42:39]

Description was changed from

==========
Worker: Directly pass ThreadableLoadingContext instead of wrapping it with
WorkerLoaderProxy

Before this CL WorkerLoaderProxy is used for conveying ThreadableLoadingContext
between main and worker threads, but this is not necessary: we can directly pass
ThreadableLoadingContext among the threads.

This CL makes worker components directly use ThreadableLoadingContext and
removes WorkerLoaderProxy for cleanup.

BUG=
==========

to

==========
Worker: Directly pass ThreadableLoadingContext instead of wrapping it with
WorkerLoaderProxy

For clean-up, this CL removes WorkerLoaderProxy and makes worker components
directly use ThreadableLoadingContext instead.

Before this CL WorkerLoaderProxy is used for conveying ThreadableLoadingContext
between main and worker threads, but it's no longer necessary: we can directly
pass ThreadableLoadingContext among the threads. One of the important features
of WorkerLoaderProxy is a mechanism to detect worker termination on the main
thread (DetatchProvider()), but we already have a proper way to do it
(WorkerThreadLifecycleObserver) and it works.

BUG=694914
==========

[nhiroki, 2017-04-24 06:43:21]

Description was changed from

==========
Worker: Directly pass ThreadableLoadingContext instead of wrapping it with
WorkerLoaderProxy

For clean-up, this CL removes WorkerLoaderProxy and makes worker components
directly use ThreadableLoadingContext instead.

Before this CL WorkerLoaderProxy is used for conveying ThreadableLoadingContext
between main and worker threads, but it's no longer necessary: we can directly
pass ThreadableLoadingContext among the threads. One of the important features
of WorkerLoaderProxy is a mechanism to detect worker termination on the main
thread (DetatchProvider()), but we already have a proper way to do it
(WorkerThreadLifecycleObserver) and it works.

BUG=694914
==========

to

==========
Worker: Removes WorkerLoaderProxy for clean-up

For clean-up, this CL removes WorkerLoaderProxy and makes worker components
directly use ThreadableLoadingContext instead.

Before this CL WorkerLoaderProxy is used for conveying ThreadableLoadingContext
between main and worker threads, but it's no longer necessary: we can directly
pass ThreadableLoadingContext among the threads. One of the important features
of WorkerLoaderProxy is a mechanism to detect worker termination on the main
thread (DetatchProvider()), but we already have a proper way to do it
(WorkerThreadLifecycleObserver) and it works.

BUG=694914
==========

[nhiroki, 2017-04-24 06:43:27]

Description was changed from

==========
Worker: Removes WorkerLoaderProxy for clean-up

For clean-up, this CL removes WorkerLoaderProxy and makes worker components
directly use ThreadableLoadingContext instead.

Before this CL WorkerLoaderProxy is used for conveying ThreadableLoadingContext
between main and worker threads, but it's no longer necessary: we can directly
pass ThreadableLoadingContext among the threads. One of the important features
of WorkerLoaderProxy is a mechanism to detect worker termination on the main
thread (DetatchProvider()), but we already have a proper way to do it
(WorkerThreadLifecycleObserver) and it works.

BUG=694914
==========

to

==========
Worker: Removes WorkerLoaderProxy for clean-up

For clean-up, this CL removes WorkerLoaderProxy and makes worker components
directly use ThreadableLoadingContext instead.

Before this CL WorkerLoaderProxy is used for conveying ThreadableLoadingContext
between main and worker threads, but it's no longer necessary: we can directly
pass ThreadableLoadingContext among the threads. One of the important features
of WorkerLoaderProxy is a mechanism to detect worker termination on the main
thread (DetatchProvider()), but we already have a proper way to do it
(WorkerThreadLifecycleObserver) and it works.

BUG=694914
==========

[nhiroki, 2017-04-24 06:45:07]

Description was changed from

==========
Worker: Removes WorkerLoaderProxy for clean-up

For clean-up, this CL removes WorkerLoaderProxy and makes worker components
directly use ThreadableLoadingContext instead.

Before this CL WorkerLoaderProxy is used for conveying ThreadableLoadingContext
between main and worker threads, but it's no longer necessary: we can directly
pass ThreadableLoadingContext among the threads. One of the important features
of WorkerLoaderProxy is a mechanism to detect worker termination on the main
thread (DetatchProvider()), but we already have a proper way to do it
(WorkerThreadLifecycleObserver) and it works.

BUG=694914
==========

to

==========
Worker: Removes WorkerLoaderProxy for clean-up

For clean-up, this CL removes WorkerLoaderProxy and makes worker components
directly use ThreadableLoadingContext instead.

Before this CL WorkerLoaderProxy is used for conveying ThreadableLoadingContext
between main and worker threads, but it's no longer necessary: we can directly
pass ThreadableLoadingContext among the threads. One of the key features of
WorkerLoaderProxy was to detect worker termination on the main thread
(DetatchProvider()), but we already have a more proper way to do it
(WorkerThreadLifecycleObserver) and it's already used for loading.

BUG=694914
==========

[nhiroki, 2017-04-24 06:45:52]

Patchset #1 (id:1) has been deleted

[nhiroki, 2017-04-24 06:46:03]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2017-04-24 06:51:54]

nhiroki@chromium.org changed reviewers:
+ kinuko@chromium.org, yhirano@chromium.org

[nhiroki, 2017-04-24 06:51:55]

PTAL, thanks!

[nhiroki, 2017-04-24 06:52:06]

Description was changed from

==========
Worker: Removes WorkerLoaderProxy for clean-up

For clean-up, this CL removes WorkerLoaderProxy and makes worker components
directly use ThreadableLoadingContext instead.

Before this CL WorkerLoaderProxy is used for conveying ThreadableLoadingContext
between main and worker threads, but it's no longer necessary: we can directly
pass ThreadableLoadingContext among the threads. One of the key features of
WorkerLoaderProxy was to detect worker termination on the main thread
(DetatchProvider()), but we already have a more proper way to do it
(WorkerThreadLifecycleObserver) and it's already used for loading.

BUG=694914
==========

to

==========
Worker: Remove WorkerLoaderProxy for clean-up

For clean-up, this CL removes WorkerLoaderProxy and makes worker components
directly use ThreadableLoadingContext instead.

Before this CL WorkerLoaderProxy is used for conveying ThreadableLoadingContext
between main and worker threads, but it's no longer necessary: we can directly
pass ThreadableLoadingContext among the threads. One of the key features of
WorkerLoaderProxy was to detect worker termination on the main thread
(DetatchProvider()), but we already have a more proper way to do it
(WorkerThreadLifecycleObserver) and it's already used for loading.

BUG=694914
==========

[commit-bot: I haz the power, 2017-04-24 07:30:08]

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

[commit-bot: I haz the power, 2017-04-24 07:30:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kinuko, 2017-04-24 09:30:16]

lgtm

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp (right):

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:246:
ThreadableLoadingContext::Create(GetDocument()), *reporting_proxy_));
nit: makeunique (while you're there)

[yhirano, 2017-04-24 09:47:10]

Can ThreadableLoadingContext::GetTaskRunner() be called from arbitrary thread?
TaskRunnerHelper::Get(_, Document*) calls Document::GetFrame and the function is
not generally thread safe.

[kinuko, 2017-04-25 01:44:26]

All the methods of threadable loading context should be called 'only from the
thread where the loading context is bound to (e.g. on the main thread).' is the
intention and what's documented in the class header.  If we're not following the
pattern we should fix.

[yhirano, 2017-04-25 02:17:57]

On 2017/04/25 01:44:26, kinuko wrote:
> All the methods of threadable loading context should be called 'only from the
> thread where the loading context is bound to (e.g. on the main thread).' is
the
> intention and what's documented in the class header.  If we're not following
the
> pattern we should fix.

lgtm

Then adding one another DCHECK in ThreadableLoadingContext::GetTaskRunner will
be helpful.

[yhirano, 2017-04-25 02:17:57]

On 2017/04/25 01:44:26, kinuko wrote:
> All the methods of threadable loading context should be called 'only from the
> thread where the loading context is bound to (e.g. on the main thread).' is
the
> intention and what's documented in the class header.  If we're not following
the
> pattern we should fix.

lgtm

Then adding one another DCHECK in ThreadableLoadingContext::GetTaskRunner will
be helpful.

[kinuko, 2017-04-25 02:26:47]

On 2017/04/25 02:17:57, yhirano wrote:
> On 2017/04/25 01:44:26, kinuko wrote:
> > All the methods of threadable loading context should be called 'only from
the
> > thread where the loading context is bound to (e.g. on the main thread).' is
> the
> > intention and what's documented in the class header.  If we're not following
> the
> > pattern we should fix.
> 
> lgtm
> 
> Then adding one another DCHECK in ThreadableLoadingContext::GetTaskRunner will
> be helpful.

+1

[nhiroki, 2017-04-25 06:13:42]

Thank you for your comments. Unfortunately, this patch is now blocked by perf
regression issue (https://crbug.com/714874) caused by the global-scope-scheduler
that this patch depends on. I'll update and land this once the issue is
resolved.

[haraken, 2017-04-25 16:53:42]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org, keishi@chromium.org

[haraken, 2017-04-25 16:53:44]

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.h:289: // Set on the main
thread and accessed on the worker thread.

Set => Created

BTW it is unsafe to pass around a raw pointer of ThreadableLoadingContext
because it lets the worker thread access the heap object on the main thread
without going through CrossThreadPersistent. The rule is that if you want to
access a cross-thread heap object, you must use CrossThreadPersistent. Raw
pointers are not allowed.

So you must use CrossThreadPersistent on all the stack variables. However, I
understand that it will mess up the code base.

+keishi: Do you have any idea on this?

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerThread.cpp
(right):

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerThread.cpp:45: new
ServiceWorkerThread(loading_context, worker_reporting_proxy));

MakeUnique

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioWorkletThread.cpp (right):

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioWorkletThread.cpp:31: new
AudioWorkletThread(loading_context, worker_reporting_proxy));

MakeUnique

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.h
(right):

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.h:184:
CrossThreadPersistent<ThreadableLoadingContext> loading_context_;

Add a comment and mention that this is set on the main thread and accessed by a
worker thread.

[keishi, 2017-04-25 23:42:02]

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.h:289: // Set on the main
thread and accessed on the worker thread.
On 2017/04/25 16:53:43, haraken wrote:
> 
> Set => Created
> 
> BTW it is unsafe to pass around a raw pointer of ThreadableLoadingContext
> because it lets the worker thread access the heap object on the main thread
> without going through CrossThreadPersistent. The rule is that if you want to
> access a cross-thread heap object, you must use CrossThreadPersistent. Raw
> pointers are not allowed.
> 
> So you must use CrossThreadPersistent on all the stack variables. However, I
> understand that it will mess up the code base.
> 
> +keishi: Do you have any idea on this?

The GetLoadingContext() and loading_context_.Clear() run on the same thread so
unless that order reverses we should be safe.

[kinuko, 2017-04-26 02:57:18]

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.h:289: // Set on the main
thread and accessed on the worker thread.
On 2017/04/25 23:42:02, keishi wrote:
> On 2017/04/25 16:53:43, haraken wrote:
> > 
> > Set => Created
> > 
> > BTW it is unsafe to pass around a raw pointer of ThreadableLoadingContext
> > because it lets the worker thread access the heap object on the main thread
> > without going through CrossThreadPersistent. The rule is that if you want to
> > access a cross-thread heap object, you must use CrossThreadPersistent. Raw
> > pointers are not allowed.
> > 
> > So you must use CrossThreadPersistent on all the stack variables. However, I
> > understand that it will mess up the code base.
> > 
> > +keishi: Do you have any idea on this?
> 
> The GetLoadingContext() and loading_context_.Clear() run on the same thread so
> unless that order reverses we should be safe.

It'd be probably also good to rephrase this comment, this is created on the main
thread, passed to the worker thread but should kept being accessed only on the
main thread.

[haraken, 2017-04-26 03:04:59]

Chatted with keishi@ offline. The conclusion is that it will be safe to access
the ThreadableLoadingContext with a raw pointer as long as it is guaranteed to
be kept alive in some way (CrossThreadPersistent in this case).

LGTM.


On 2017/04/26 02:57:18, kinuko wrote:
>
https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/workers/WorkerThread.h (right):
> 
>
https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/workers/WorkerThread.h:289: // Set on the main
> thread and accessed on the worker thread.
> On 2017/04/25 23:42:02, keishi wrote:
> > On 2017/04/25 16:53:43, haraken wrote:
> > > 
> > > Set => Created
> > > 
> > > BTW it is unsafe to pass around a raw pointer of ThreadableLoadingContext
> > > because it lets the worker thread access the heap object on the main
thread
> > > without going through CrossThreadPersistent. The rule is that if you want
to
> > > access a cross-thread heap object, you must use CrossThreadPersistent. Raw
> > > pointers are not allowed.
> > > 
> > > So you must use CrossThreadPersistent on all the stack variables. However,
I
> > > understand that it will mess up the code base.
> > > 
> > > +keishi: Do you have any idea on this?
> > 
> > The GetLoadingContext() and loading_context_.Clear() run on the same thread
so
> > unless that order reverses we should be safe.
> 
> It'd be probably also good to rephrase this comment, this is created on the
main
> thread, passed to the worker thread but should kept being accessed only on the
> main thread.

It is accessed by the worker thread, isn't it? GetLoadingContext() is called by
a worker thread and the worker thread obtains the raw pointer.

[kinuko, 2017-04-26 03:21:51]

On 2017/04/26 03:04:59, haraken wrote:
> Chatted with keishi@ offline. The conclusion is that it will be safe to access
> the ThreadableLoadingContext with a raw pointer as long as it is guaranteed to
> be kept alive in some way (CrossThreadPersistent in this case).
> 
> LGTM.
> 
> 
> On 2017/04/26 02:57:18, kinuko wrote:
> >
>
https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/workers/WorkerThread.h (right):
> > 
> >
>
https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/workers/WorkerThread.h:289: // Set on the
main
> > thread and accessed on the worker thread.
> > On 2017/04/25 23:42:02, keishi wrote:
> > > On 2017/04/25 16:53:43, haraken wrote:
> > > > 
> > > > Set => Created
> > > > 
> > > > BTW it is unsafe to pass around a raw pointer of
ThreadableLoadingContext
> > > > because it lets the worker thread access the heap object on the main
> thread
> > > > without going through CrossThreadPersistent. The rule is that if you
want
> to
> > > > access a cross-thread heap object, you must use CrossThreadPersistent.
Raw
> > > > pointers are not allowed.
> > > > 
> > > > So you must use CrossThreadPersistent on all the stack variables.
However,
> I
> > > > understand that it will mess up the code base.
> > > > 
> > > > +keishi: Do you have any idea on this?
> > > 
> > > The GetLoadingContext() and loading_context_.Clear() run on the same
thread
> so
> > > unless that order reverses we should be safe.
> > 
> > It'd be probably also good to rephrase this comment, this is created on the
> main
> > thread, passed to the worker thread but should kept being accessed only on
the
> > main thread.
> 
> It is accessed by the worker thread, isn't it? GetLoadingContext() is called
by
> a worker thread and the worker thread obtains the raw pointer.

It's passed to the worker thread but deref'ing is supposed to happen only on the
main thread (so its usage is like passing weak ptr to another thread for future
derefs on the original thread).

[nhiroki, 2017-05-29 03:43:40]

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

[commit-bot: I haz the power, 2017-05-29 03:43:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nhiroki, 2017-05-29 04:16:40]

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

[commit-bot: I haz the power, 2017-05-29 04:17:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nhiroki, 2017-05-29 04:20:24]

Sorry for putting this on hold for a while. The blocker issue was resolved. I
addressed your review comments and will push this into the CQ.

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp (right):

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:246:
ThreadableLoadingContext::Create(GetDocument()), *reporting_proxy_));
On 2017/04/24 09:30:16, kinuko wrote:
> nit: makeunique (while you're there)

Done.

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.h:289: // Set on the main
thread and accessed on the worker thread.
On 2017/04/26 02:57:18, kinuko wrote:
> On 2017/04/25 23:42:02, keishi wrote:
> > On 2017/04/25 16:53:43, haraken wrote:
> > > 
> > > Set => Created
> > > 
> > > BTW it is unsafe to pass around a raw pointer of ThreadableLoadingContext
> > > because it lets the worker thread access the heap object on the main
thread
> > > without going through CrossThreadPersistent. The rule is that if you want
to
> > > access a cross-thread heap object, you must use CrossThreadPersistent. Raw
> > > pointers are not allowed.
> > > 
> > > So you must use CrossThreadPersistent on all the stack variables. However,
I
> > > understand that it will mess up the code base.
> > > 
> > > +keishi: Do you have any idea on this?
> > 
> > The GetLoadingContext() and loading_context_.Clear() run on the same thread
so
> > unless that order reverses we should be safe.
> 
> It'd be probably also good to rephrase this comment, this is created on the
main
> thread, passed to the worker thread but should kept being accessed only on the
> main thread.

Done.

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerThread.cpp
(right):

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerThread.cpp:45: new
ServiceWorkerThread(loading_context, worker_reporting_proxy));
On 2017/04/25 16:53:43, haraken wrote:
> 
> MakeUnique

Done.

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioWorkletThread.cpp (right):

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioWorkletThread.cpp:31: new
AudioWorkletThread(loading_context, worker_reporting_proxy));
On 2017/04/25 16:53:43, haraken wrote:
> 
> MakeUnique

Let me keep this as is for now because (1) MakeUnique cannot access the private
ctor, (2) providing both Create() and the public ctor look duplicate, and (3)
removing Create() in favor of the public ctor could change the behavior of
TRACE_EVENT in Create() (I'd like to avoid such change in this big CL as much as
possible).

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.h
(right):

https://codereview.chromium.org/2840473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.h:184:
CrossThreadPersistent<ThreadableLoadingContext> loading_context_;
On 2017/04/25 16:53:43, haraken wrote:
> 
> Add a comment and mention that this is set on the main thread and accessed by
a
> worker thread.

Removed this field because it was unused.

[nhiroki, 2017-05-29 04:20:43]

On 2017/04/25 02:26:47, kinuko wrote:
> On 2017/04/25 02:17:57, yhirano wrote:
> > On 2017/04/25 01:44:26, kinuko wrote:
> > > All the methods of threadable loading context should be called 'only from
> the
> > > thread where the loading context is bound to (e.g. on the main thread).'
is
> > the
> > > intention and what's documented in the class header.  If we're not
following
> > the
> > > pattern we should fix.
> > 
> > lgtm
> > 
> > Then adding one another DCHECK in ThreadableLoadingContext::GetTaskRunner
will
> > be helpful.
> 
> +1

Done.

[nhiroki, 2017-05-29 04:33:05]

The CQ bit was unchecked by nhiroki@chromium.org

[nhiroki, 2017-05-29 04:33:15]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2017-05-29 04:33:16]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
haraken@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2840473002/#ps100001
(title: "clean up")

[commit-bot: I haz the power, 2017-05-29 04:33:29]

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, 2017-05-29 08:09:20]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496032395788650,
"parent_rev": "28307e4e66a5ed3bef0c6dde62995a0fd8625799", "commit_rev":
"3e327942abfd83502aa17cc59acefa6e30c91d0e"}

[commit-bot: I haz the power, 2017-05-29 08:09:35]

Description was changed from

==========
Worker: Remove WorkerLoaderProxy for clean-up

For clean-up, this CL removes WorkerLoaderProxy and makes worker components
directly use ThreadableLoadingContext instead.

Before this CL WorkerLoaderProxy is used for conveying ThreadableLoadingContext
between main and worker threads, but it's no longer necessary: we can directly
pass ThreadableLoadingContext among the threads. One of the key features of
WorkerLoaderProxy was to detect worker termination on the main thread
(DetatchProvider()), but we already have a more proper way to do it
(WorkerThreadLifecycleObserver) and it's already used for loading.

BUG=694914
==========

to

==========
Worker: Remove WorkerLoaderProxy for clean-up

For clean-up, this CL removes WorkerLoaderProxy and makes worker components
directly use ThreadableLoadingContext instead.

Before this CL WorkerLoaderProxy is used for conveying ThreadableLoadingContext
between main and worker threads, but it's no longer necessary: we can directly
pass ThreadableLoadingContext among the threads. One of the key features of
WorkerLoaderProxy was to detect worker termination on the main thread
(DetatchProvider()), but we already have a more proper way to do it
(WorkerThreadLifecycleObserver) and it's already used for loading.

BUG=694914

Review-Url: https://codereview.chromium.org/2840473002
Cr-Commit-Position: refs/heads/master@{#475308}
Committed:
https://chromium.googlesource.com/chromium/src/+/3e327942abfd83502aa17cc59ace...
==========

[commit-bot: I haz the power, 2017-05-29 08:09:38]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/3e327942abfd83502aa17cc59ace...