Worker: Move ParentFrameTaskRunners from WorkerReportingProxy to ObjectProxy

Worker: Move ParentFrameTaskRunners from WorkerReportingProxy to ObjectProxy

WorkerReportingProxy::getParentFrameTaskRunners() was introduced by [1] in order
to provide a way to post a task from a worker thread to the main thread. This is
still useful, but would be misplaced as a review comment[2].

This CL removes WorkerReportingProxy::getParentFrameTaskRunners() and instead
each ObjectProxy and WorkerThread has its own ParentFrameTaskRunners instance if
necessary. MessagingProxy on the main thread creates ParentFrameTaskRunners and
then passes it to ObjectProxy and WorkerThread on a worker thread separately.

[1] https://codereview.chromium.org/2513413003/
[2] https://codereview.chromium.org/2513413003/#msg40

BUG=667310
Review-Url: https://codereview.chromium.org/2539443004
Cr-Commit-Position: refs/heads/master@{#445673}
Committed: https://chromium.googlesource.com/chromium/src/+/22d251e18ae5a0b5f21a31a4875156c36b884f03

[nhiroki, 2016-11-29 06:48:29]

Description was changed from

==========
Worker: Remove WorkerReportingProxy::getParentFrameTaskRunners()

WorkerReportingProxy::getParentFrameTaskRunners() was introduced by [1] in order
to provide a way to post a task from a worker thread to the main thread. This is
still useful, but its layering is not good as a review comment[2].

This CL removes WorkerReportingProxy::getParentFrameTaskRunners() and instead
each WorkerObjectProxy has its own ParentFrameTaskRunners instance if necessary.
In addition, this makes WorkerThread directly use the default task runner
instead of ParentFrameTaskRunners. This should be legitimate because

BUG=667310
==========

to

==========
Worker: Move ParentFrameTaskRunners from WorkerReportingProxy to ObjectProxy

WorkerReportingProxy::getParentFrameTaskRunners() was introduced by [1] in order
to provide a way to post a task from a worker thread to the main thread. This is
still useful, but would be misplaced as a review comment[2].

This CL removes WorkerReportingProxy::getParentFrameTaskRunners() and instead
each WorkerObjectProxy has its own ParentFrameTaskRunners instance if necessary.
In addition, this makes WorkerThread directly use the default task runner
instead of ParentFrameTaskRunners. This should be legitimate because
WorkerThread uses it only for sending a thread control message that is not
associated with any document's frame to the current thread. Also, it's a delayed
task and we don't take care of the task order with other tasks.

[1] https://codereview.chromium.org/2513413003/
[2] https://codereview.chromium.org/2513413003/#msg40

BUG=667310
==========

[nhiroki, 2016-11-29 06:52:13]

Description was changed from

==========
Worker: Move ParentFrameTaskRunners from WorkerReportingProxy to ObjectProxy

WorkerReportingProxy::getParentFrameTaskRunners() was introduced by [1] in order
to provide a way to post a task from a worker thread to the main thread. This is
still useful, but would be misplaced as a review comment[2].

This CL removes WorkerReportingProxy::getParentFrameTaskRunners() and instead
each WorkerObjectProxy has its own ParentFrameTaskRunners instance if necessary.
In addition, this makes WorkerThread directly use the default task runner
instead of ParentFrameTaskRunners. This should be legitimate because
WorkerThread uses it only for sending a thread control message that is not
associated with any document's frame to the current thread. Also, it's a delayed
task and we don't take care of the task order with other tasks.

[1] https://codereview.chromium.org/2513413003/
[2] https://codereview.chromium.org/2513413003/#msg40

BUG=667310
==========

to

==========
Worker: Move ParentFrameTaskRunners from WorkerReportingProxy to ObjectProxy

WorkerReportingProxy::getParentFrameTaskRunners() was introduced by [1] in order
to provide a way to post a task from a worker thread to the main thread. This is
still useful, but would be misplaced as a review comment[2].

This CL removes WorkerReportingProxy::getParentFrameTaskRunners() and instead
each WorkerObjectProxy has its own ParentFrameTaskRunners instance if necessary.
In addition, this makes WorkerThread directly use the default task runner
instead of ParentFrameTaskRunners. This should be legitimate because
WorkerThread uses it only for sending a thread control message that is not
associated with any document's frame to the current thread. Also, it's a delayed
task and we don't take care of the task order among this and other tasks.

[1] https://codereview.chromium.org/2513413003/
[2] https://codereview.chromium.org/2513413003/#msg40

BUG=667310
==========

[nhiroki, 2016-11-29 06:55:13]

Patchset #1 (id:1) has been deleted

[nhiroki, 2016-11-29 06:55:21]

Description was changed from

==========
Worker: Move ParentFrameTaskRunners from WorkerReportingProxy to ObjectProxy

WorkerReportingProxy::getParentFrameTaskRunners() was introduced by [1] in order
to provide a way to post a task from a worker thread to the main thread. This is
still useful, but would be misplaced as a review comment[2].

This CL removes WorkerReportingProxy::getParentFrameTaskRunners() and instead
each WorkerObjectProxy has its own ParentFrameTaskRunners instance if necessary.
In addition, this makes WorkerThread directly use the default task runner
instead of ParentFrameTaskRunners. This should be legitimate because
WorkerThread uses it only for sending a thread control message that is not
associated with any document's frame to the current thread. Also, it's a delayed
task and we don't take care of the task order among this and other tasks.

[1] https://codereview.chromium.org/2513413003/
[2] https://codereview.chromium.org/2513413003/#msg40

BUG=667310
==========

to

==========
Worker: Move ParentFrameTaskRunners from WorkerReportingProxy to ObjectProxy

WorkerReportingProxy::getParentFrameTaskRunners() was introduced by [1] in order
to provide a way to post a task from a worker thread to the main thread. This is
still useful, but would be misplaced as a review comment[2].

This CL removes WorkerReportingProxy::getParentFrameTaskRunners() and instead
each WorkerObjectProxy has its own ParentFrameTaskRunners instance if necessary.
In addition, this makes WorkerThread directly use the default task runner
instead of ParentFrameTaskRunners. This should be legitimate because
WorkerThread uses it only for sending a thread control message that is not
associated with any document's frame to the current thread. Also, it's a delayed
task and we don't take care of the task order among this and other tasks.

[1] https://codereview.chromium.org/2513413003/
[2] https://codereview.chromium.org/2513413003/#msg40

BUG=667310
==========

[nhiroki, 2016-11-29 07:01:30]

Description was changed from

==========
Worker: Move ParentFrameTaskRunners from WorkerReportingProxy to ObjectProxy

WorkerReportingProxy::getParentFrameTaskRunners() was introduced by [1] in order
to provide a way to post a task from a worker thread to the main thread. This is
still useful, but would be misplaced as a review comment[2].

This CL removes WorkerReportingProxy::getParentFrameTaskRunners() and instead
each WorkerObjectProxy has its own ParentFrameTaskRunners instance if necessary.
In addition, this makes WorkerThread directly use the default task runner
instead of ParentFrameTaskRunners. This should be legitimate because
WorkerThread uses it only for sending a thread control message that is not
associated with any document's frame to the current thread. Also, it's a delayed
task and we don't take care of the task order among this and other tasks.

[1] https://codereview.chromium.org/2513413003/
[2] https://codereview.chromium.org/2513413003/#msg40

BUG=667310
==========

to

==========
Worker: Move ParentFrameTaskRunners from WorkerReportingProxy to ObjectProxy

WorkerReportingProxy::getParentFrameTaskRunners() was introduced by [1] in order
to provide a way to post a task from a worker thread to the main thread. This is
still useful, but would be misplaced as a review comment[2].

This CL removes WorkerReportingProxy::getParentFrameTaskRunners() and instead
each WorkerObjectProxy has its own ParentFrameTaskRunners instance if necessary.
In addition, this makes WorkerThread directly use the default task runner
instead of ParentFrameTaskRunners to simplify the codebase. This should be
legitimate because WorkerThread uses it only for sending a thread control
message that is not associated with any document's frame to the current thread.
Also, it's a delayed task and we don't take care of the task order among this
and other tasks.

[1] https://codereview.chromium.org/2513413003/
[2] https://codereview.chromium.org/2513413003/#msg40

BUG=667310
==========

[nhiroki, 2016-11-29 07:03:11]

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

[nhiroki, 2016-11-29 07:03:26]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2016-11-29 07:03:30]

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

[nhiroki, 2016-11-29 07:03:43]

PTAL, thanks!

[commit-bot: I haz the power, 2016-11-29 07:04:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nhiroki, 2016-11-29 07:04:35]

nhiroki@chromium.org changed reviewers:
+ haraken@chromium.org

[nhiroki, 2016-11-29 07:04:35]

+haraken for changes in WorkerThread.cpp.

[nhiroki, 2016-11-29 07:08:27]

https://codereview.chromium.org/2539443004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (left):

https://codereview.chromium.org/2539443004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:433:
m_parentFrameTaskRunners = ParentFrameTaskRunners::create(nullptr);
Just note: We might need to take back this removal when we replacing
postInspectorTask() in
ServiceWorkerGlobalScopeProxy::postMessageToPageInspector() with
ParentFrameTaskRunners().

[commit-bot: I haz the power, 2016-11-29 08:52:31]

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

[commit-bot: I haz the power, 2016-11-29 08:52:32]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-11-29 12:52:52]

Thanks for the follow-up.

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

https://codereview.chromium.org/2539443004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:357: // essentially we
don't have to take care of the task order.
delayed task doesn't necessarily mean it doesn't take care of the task order,
right?

I'm not fully sure if we want to make this change.  Instead we could 1) pass a
relevant task runner (e.g. internal task runner of the parent frame in
InProcessWorker cases) to the WebThread constructor, or 2) keep the
getParentFrameTaskRunners at WorkerReportingProxy but adjust the
WorkerReportingProxy's role to repurpose.  I think we might be able to do 1) if
not 2)?

[nhiroki, 2016-11-30 14:51:12]

Sorry, I couldn't have time to work on this today. I'll update this CL / reply
to you day after tomorrow (because I'll be OOO on Dec 1).

[nhiroki, 2016-12-05 08:50:32]

On 2016/11/30 14:51:12, nhiroki wrote:
> Sorry, I couldn't have time to work on this today. I'll update this CL / reply
> to you day after tomorrow (because I'll be OOO on Dec 1).

This CL is conflicting other ongoing CLs. I'll resume this after they are
landed.

[nhiroki, 2017-01-20 08:37:34]

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

[commit-bot: I haz the power, 2017-01-20 08:37:46]

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-01-20 10:53:02]

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

[commit-bot: I haz the power, 2017-01-20 10:53:03]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2017-01-24 01:42:05]

Description was changed from

==========
Worker: Move ParentFrameTaskRunners from WorkerReportingProxy to ObjectProxy

WorkerReportingProxy::getParentFrameTaskRunners() was introduced by [1] in order
to provide a way to post a task from a worker thread to the main thread. This is
still useful, but would be misplaced as a review comment[2].

This CL removes WorkerReportingProxy::getParentFrameTaskRunners() and instead
each WorkerObjectProxy has its own ParentFrameTaskRunners instance if necessary.
In addition, this makes WorkerThread directly use the default task runner
instead of ParentFrameTaskRunners to simplify the codebase. This should be
legitimate because WorkerThread uses it only for sending a thread control
message that is not associated with any document's frame to the current thread.
Also, it's a delayed task and we don't take care of the task order among this
and other tasks.

[1] https://codereview.chromium.org/2513413003/
[2] https://codereview.chromium.org/2513413003/#msg40

BUG=667310
==========

to

==========
Worker: Move ParentFrameTaskRunners from WorkerReportingProxy to ObjectProxy

WorkerReportingProxy::getParentFrameTaskRunners() was introduced by [1] in order
to provide a way to post a task from a worker thread to the main thread. This is
still useful, but would be misplaced as a review comment[2].

This CL removes WorkerReportingProxy::getParentFrameTaskRunners() and instead
each WorkerObjectProxy and WorkerThread has its own ParentFrameTaskRunners
instance if necessary. MessagingProxy on the main thread creates
ParentFrameTaskRunners and then passes it to ObjectProxy and WorkerThread
separately.

[1] https://codereview.chromium.org/2513413003/
[2] https://codereview.chromium.org/2513413003/#msg40

BUG=667310
==========

[nhiroki, 2017-01-24 01:46:09]

Description was changed from

==========
Worker: Move ParentFrameTaskRunners from WorkerReportingProxy to ObjectProxy

WorkerReportingProxy::getParentFrameTaskRunners() was introduced by [1] in order
to provide a way to post a task from a worker thread to the main thread. This is
still useful, but would be misplaced as a review comment[2].

This CL removes WorkerReportingProxy::getParentFrameTaskRunners() and instead
each WorkerObjectProxy and WorkerThread has its own ParentFrameTaskRunners
instance if necessary. MessagingProxy on the main thread creates
ParentFrameTaskRunners and then passes it to ObjectProxy and WorkerThread
separately.

[1] https://codereview.chromium.org/2513413003/
[2] https://codereview.chromium.org/2513413003/#msg40

BUG=667310
==========

to

==========
Worker: Move ParentFrameTaskRunners from WorkerReportingProxy to ObjectProxy

WorkerReportingProxy::getParentFrameTaskRunners() was introduced by [1] in order
to provide a way to post a task from a worker thread to the main thread. This is
still useful, but would be misplaced as a review comment[2].

This CL removes WorkerReportingProxy::getParentFrameTaskRunners() and instead
each WorkerObjectProxy and WorkerThread has its own ParentFrameTaskRunners
instance if necessary. MessagingProxy on the main thread creates
ParentFrameTaskRunners and then passes it to ObjectProxy and WorkerThread on
a worker thread separately.

[1] https://codereview.chromium.org/2513413003/
[2] https://codereview.chromium.org/2513413003/#msg40

BUG=667310
==========

[nhiroki, 2017-01-24 01:48:12]

Hi kinuko-san and haraken-san, can you take another look at this?

I remade the entire patch and updated the CL description. The difference from
the original CL is that WorkerThread also has its own ParentFrameTaskRunners so
that it doesn't have to use the default task runner.

Thanks.

[nhiroki, 2017-01-24 01:48:47]

Description was changed from

==========
Worker: Move ParentFrameTaskRunners from WorkerReportingProxy to ObjectProxy

WorkerReportingProxy::getParentFrameTaskRunners() was introduced by [1] in order
to provide a way to post a task from a worker thread to the main thread. This is
still useful, but would be misplaced as a review comment[2].

This CL removes WorkerReportingProxy::getParentFrameTaskRunners() and instead
each WorkerObjectProxy and WorkerThread has its own ParentFrameTaskRunners
instance if necessary. MessagingProxy on the main thread creates
ParentFrameTaskRunners and then passes it to ObjectProxy and WorkerThread on
a worker thread separately.

[1] https://codereview.chromium.org/2513413003/
[2] https://codereview.chromium.org/2513413003/#msg40

BUG=667310
==========

to

==========
Worker: Move ParentFrameTaskRunners from WorkerReportingProxy to ObjectProxy

WorkerReportingProxy::getParentFrameTaskRunners() was introduced by [1] in order
to provide a way to post a task from a worker thread to the main thread. This is
still useful, but would be misplaced as a review comment[2].

This CL removes WorkerReportingProxy::getParentFrameTaskRunners() and instead
each ObjectProxy and WorkerThread has its own ParentFrameTaskRunners instance if
necessary. MessagingProxy on the main thread creates ParentFrameTaskRunners and
then passes it to ObjectProxy and WorkerThread on a worker thread separately.

[1] https://codereview.chromium.org/2513413003/
[2] https://codereview.chromium.org/2513413003/#msg40

BUG=667310
==========

[kinuko, 2017-01-24 02:02:35]

lgtm

[haraken, 2017-01-24 02:09:29]

LGTM

[nhiroki, 2017-01-24 02:10:35]

Thank you :)

[nhiroki, 2017-01-24 02:10:42]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2017-01-24 02:11:26]

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-01-24 04:08:43]

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

[commit-bot: I haz the power, 2017-01-24 04:08:46]

Failed to apply patch for third_party/WebKit/Source/core/workers/WorkerThread.h:
While running git apply --index -p1;
  error: patch failed: third_party/WebKit/Source/core/workers/WorkerThread.h:30
  error: third_party/WebKit/Source/core/workers/WorkerThread.h: patch does not
apply

Patch:       third_party/WebKit/Source/core/workers/WorkerThread.h
Index: third_party/WebKit/Source/core/workers/WorkerThread.h
diff --git a/third_party/WebKit/Source/core/workers/WorkerThread.h
b/third_party/WebKit/Source/core/workers/WorkerThread.h
index
d3af4f3ac85540a7c3938ee202f0001beee50472..8688e3de9e540a001256c65e79db54c6a1ba4b20
100644
--- a/third_party/WebKit/Source/core/workers/WorkerThread.h
+++ b/third_party/WebKit/Source/core/workers/WorkerThread.h
@@ -30,6 +30,7 @@
 #include "core/CoreExport.h"
 #include "core/dom/ExecutionContextTask.h"
 #include "core/frame/csp/ContentSecurityPolicy.h"
+#include "core/workers/ParentFrameTaskRunners.h"
 #include "core/workers/WorkerLoaderProxy.h"
 #include "core/workers/WorkerThreadLifecycleObserver.h"
 #include "platform/LifecycleNotifier.h"
@@ -174,8 +175,14 @@ class CORE_EXPORT WorkerThread : public
WebThread::TaskObserver {
 
   void waitForShutdownForTesting() { m_shutdownEvent->wait(); }
 
+  ParentFrameTaskRunners* getParentFrameTaskRunners() const {
+    return m_parentFrameTaskRunners.get();
+  }
+
  protected:
-  WorkerThread(PassRefPtr<WorkerLoaderProxy>, WorkerReportingProxy&);
+  WorkerThread(PassRefPtr<WorkerLoaderProxy>,
+               WorkerReportingProxy&,
+               ParentFrameTaskRunners*);
 
   // Factory method for creating a new worker context for the thread.
   // Called on the worker thread.
@@ -290,6 +297,7 @@ class CORE_EXPORT WorkerThread : public
WebThread::TaskObserver {
 
   RefPtr<WorkerLoaderProxy> m_workerLoaderProxy;
   WorkerReportingProxy& m_workerReportingProxy;
+  CrossThreadPersistent<ParentFrameTaskRunners> m_parentFrameTaskRunners;
 
   // This lock protects |m_globalScope|, |m_requestedToTerminate|,
   // |m_threadState|, |m_runningDebuggerTask| and |m_exitCode|.

[nhiroki, 2017-01-24 04:53:38]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2017-01-24 04:53:39]

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

[commit-bot: I haz the power, 2017-01-24 04:53:55]

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-01-24 07:15:56]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485233618545710,
"parent_rev": "53c571c9fc446c851e59da61ac36277b666b1705", "commit_rev":
"22d251e18ae5a0b5f21a31a4875156c36b884f03"}

[commit-bot: I haz the power, 2017-01-24 07:16:30]

Description was changed from

==========
Worker: Move ParentFrameTaskRunners from WorkerReportingProxy to ObjectProxy

WorkerReportingProxy::getParentFrameTaskRunners() was introduced by [1] in order
to provide a way to post a task from a worker thread to the main thread. This is
still useful, but would be misplaced as a review comment[2].

This CL removes WorkerReportingProxy::getParentFrameTaskRunners() and instead
each ObjectProxy and WorkerThread has its own ParentFrameTaskRunners instance if
necessary. MessagingProxy on the main thread creates ParentFrameTaskRunners and
then passes it to ObjectProxy and WorkerThread on a worker thread separately.

[1] https://codereview.chromium.org/2513413003/
[2] https://codereview.chromium.org/2513413003/#msg40

BUG=667310
==========

to

==========
Worker: Move ParentFrameTaskRunners from WorkerReportingProxy to ObjectProxy

WorkerReportingProxy::getParentFrameTaskRunners() was introduced by [1] in order
to provide a way to post a task from a worker thread to the main thread. This is
still useful, but would be misplaced as a review comment[2].

This CL removes WorkerReportingProxy::getParentFrameTaskRunners() and instead
each ObjectProxy and WorkerThread has its own ParentFrameTaskRunners instance if
necessary. MessagingProxy on the main thread creates ParentFrameTaskRunners and
then passes it to ObjectProxy and WorkerThread on a worker thread separately.

[1] https://codereview.chromium.org/2513413003/
[2] https://codereview.chromium.org/2513413003/#msg40

BUG=667310

Review-Url: https://codereview.chromium.org/2539443004
Cr-Commit-Position: refs/heads/master@{#445673}
Committed:
https://chromium.googlesource.com/chromium/src/+/22d251e18ae5a0b5f21a31a48751...
==========

[commit-bot: I haz the power, 2017-01-24 07:16:32]

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