[worklets] Split up InProcessWorkerMessagingProxy into a base+worker class.

[worklets] Split up InProcessWorkerMessagingProxy into a base+worker class.

This pulls out all of the lifetime logic for the WorkerThread into the base class ThreadedMessagingProxyBase.

This will allow us to re-use all of the lifetime logic (I've created a followup demo patch which does this https://codereview.chromium.org/2312493002/)

BUG=567358
Committed: https://crrev.com/89b9278e35bf785df739d89efb9587961a95f776
Cr-Commit-Position: refs/heads/master@{#417161}

[ikilpatrick, 2016-09-02 23:20:22]

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

[commit-bot: I haz the power, 2016-09-02 23:21:12]

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, 2016-09-03 00:57:34]

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

[commit-bot: I haz the power, 2016-09-03 00:57:35]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ikilpatrick, 2016-09-03 17:48:07]

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

[commit-bot: I haz the power, 2016-09-03 17:48:17]

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, 2016-09-03 18:59:00]

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

[commit-bot: I haz the power, 2016-09-03 18:59:00]

Dry run: This issue passed the CQ dry run.

[ikilpatrick, 2016-09-04 00:23:39]

Description was changed from

==========
split up the global scope proxy

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

to

==========
[worklets] Split up InProcessWorkerMessagingProxy into a base+worker class.

This pulls out all of the lifetime logic for the WorkerThread into the base
class ThreadedMessagingProxyBase.

This will allow us to re-use all of the lifetime logic (I've created a followup
demo patch which does this https://codereview.chromium.org/2312493002/)

BUG=567358
==========

[ikilpatrick, 2016-09-04 00:34:00]

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

[ikilpatrick, 2016-09-04 00:34:01]

This was a first pass of splitting this class up for use by the worklet
messaging code. Wasn't sure about renaming the pure virtual interface
(InProcessWorkerGlobalScopeProxy) but let me know.

https://codereview.chromium.org/2310673002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp
(right):

https://codereview.chromium.org/2310673002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp:150: {
This override only exists to perform the DCHECK below.

https://codereview.chromium.org/2310673002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/ThreadedGlobalScopeProxy.h (right):

https://codereview.chromium.org/2310673002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ThreadedGlobalScopeProxy.h:15: class
CORE_EXPORT ThreadedGlobalScopeProxy {
I wasn't sure what to do this with interface; we could have a set of interfaces
for each; or remove this entirely? Up to you.

[nhiroki, 2016-09-06 07:07:08]

https://codereview.chromium.org/2310673002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/ThreadedGlobalScopeProxy.h (right):

https://codereview.chromium.org/2310673002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ThreadedGlobalScopeProxy.h:15: class
CORE_EXPORT ThreadedGlobalScopeProxy {
On 2016/09/04 00:34:01, ikilpatrick wrote:
> I wasn't sure what to do this with interface; we could have a set of
interfaces
> for each; or remove this entirely? Up to you.

I think this (InProcessWorkerObjectProxy) is not useful/necessary. +1 to remove
this.

https://codereview.chromium.org/2310673002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.cpp
(left):

https://codereview.chromium.org/2310673002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.cpp:140:
std::unique_ptr<ExecutionContextTask> task =
createCrossThreadTask(&processMessageOnWorkerGlobalScope, message,
passed(std::move(channels)), crossThreadUnretained(&workerObjectProxy()));
Will Worklet support postMessage()? If it'll be supported, we could keep this
task creation in postOrQueueTaskToWorkerGlobalScope(). If it won't be supported,
we could entirely move postOrQueueTaskToWorkerGlobalScope() and fields
(m_unconfirmedMessageCount, m_queuedEarlyTasks) to
InProcessWorkerMessagingProxy.

https://codereview.chromium.org/2310673002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.cpp
(right):

https://codereview.chromium.org/2310673002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.cpp:63: void
ThreadedMessagingProxyBase::postOrQueueTaskToWorkerGlobalScope(std::unique_ptr<ExecutionContextTask>
task)
I'd prefer to keep 'Message(Event)' instead of 'Task' in order to clarify this
is different from postTaskToWorkerGlobalScope() and specialized for postMessage.

[ikilpatrick, 2016-09-06 18:57:00]

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

[commit-bot: I haz the power, 2016-09-06 18:57:27]

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

[ikilpatrick, 2016-09-06 18:59:50]

https://codereview.chromium.org/2310673002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/ThreadedGlobalScopeProxy.h (right):

https://codereview.chromium.org/2310673002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ThreadedGlobalScopeProxy.h:15: class
CORE_EXPORT ThreadedGlobalScopeProxy {
On 2016/09/06 07:07:08, nhiroki wrote:
> On 2016/09/04 00:34:01, ikilpatrick wrote:
> > I wasn't sure what to do this with interface; we could have a set of
> interfaces
> > for each; or remove this entirely? Up to you.
> 
> I think this (InProcessWorkerObjectProxy) is not useful/necessary. +1 to
remove
> this.

Sounds good. I sent out a separate patch for this removal+rename, and made this
change dependent.

https://codereview.chromium.org/2310673002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.cpp
(left):

https://codereview.chromium.org/2310673002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.cpp:140:
std::unique_ptr<ExecutionContextTask> task =
createCrossThreadTask(&processMessageOnWorkerGlobalScope, message,
passed(std::move(channels)), crossThreadUnretained(&workerObjectProxy()));
On 2016/09/06 07:07:08, nhiroki wrote:
> Will Worklet support postMessage()? If it'll be supported, we could keep this
> task creation in postOrQueueTaskToWorkerGlobalScope(). If it won't be
supported,
> we could entirely move postOrQueueTaskToWorkerGlobalScope() and fields
> (m_unconfirmedMessageCount, m_queuedEarlyTasks) to
> InProcessWorkerMessagingProxy.

Yeah I wasn't sure what to do here initially. My initial thoughts were to try
and contain all of the "lifecycle" methods in one class. But changed, and
actually looks pretty good.

Major thing is that worklets wont have a hasPendingActivity call now which I
think makes sense.

https://codereview.chromium.org/2310673002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.cpp
(right):

https://codereview.chromium.org/2310673002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.cpp:63: void
ThreadedMessagingProxyBase::postOrQueueTaskToWorkerGlobalScope(std::unique_ptr<ExecutionContextTask>
task)
On 2016/09/06 07:07:08, nhiroki wrote:
> I'd prefer to keep 'Message(Event)' instead of 'Task' in order to clarify this
> is different from postTaskToWorkerGlobalScope() and specialized for
postMessage.

Acknowledged.

[commit-bot: I haz the power, 2016-09-06 20:53:03]

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

[commit-bot: I haz the power, 2016-09-06 20:53:04]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2016-09-07 03:12:21]

LGTM :)

[haraken, 2016-09-07 04:11:13]

LGTM

[yhirano, 2016-09-07 04:46:38]

https://codereview.chromium.org/2310673002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp
(right):

https://codereview.chromium.org/2310673002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp:170:
for (auto& earlyTasks : m_queuedEarlyTasks)
This is not this CL's problem, but |earlyTasks| is a task so |task| or
|earlyTask| are more suitable.

https://codereview.chromium.org/2310673002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.h
(right):

https://codereview.chromium.org/2310673002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.h:27: virtual
void parentObjectDestroyed();
Is "parent object" defined? Can you write some comments?

[ikilpatrick, 2016-09-07 17:45:32]

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

[commit-bot: I haz the power, 2016-09-07 17:45:57]

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

[ikilpatrick, 2016-09-07 17:53:57]

https://codereview.chromium.org/2310673002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp
(right):

https://codereview.chromium.org/2310673002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp:170:
for (auto& earlyTasks : m_queuedEarlyTasks)
On 2016/09/07 04:46:37, yhirano wrote:
> This is not this CL's problem, but |earlyTasks| is a task so |task| or
> |earlyTask| are more suitable.

Changed to earlyTask :)

https://codereview.chromium.org/2310673002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.h
(right):

https://codereview.chromium.org/2310673002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.h:27: virtual
void parentObjectDestroyed();
On 2016/09/07 04:46:37, yhirano wrote:
> Is "parent object" defined? Can you write some comments?

Added comments in this file.

[commit-bot: I haz the power, 2016-09-07 19:46:55]

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

[commit-bot: I haz the power, 2016-09-07 19:46:57]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-09-08 01:53:23]

lgtm

[ikilpatrick, 2016-09-08 02:00:37]

The CQ bit was checked by ikilpatrick@chromium.org

[ikilpatrick, 2016-09-08 02:00:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
nhiroki@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2310673002/#ps180001
(title: "address comments.")

[commit-bot: I haz the power, 2016-09-08 02:01:00]

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, 2016-09-08 02:05:08]

Description was changed from

==========
[worklets] Split up InProcessWorkerMessagingProxy into a base+worker class.

This pulls out all of the lifetime logic for the WorkerThread into the base
class ThreadedMessagingProxyBase.

This will allow us to re-use all of the lifetime logic (I've created a followup
demo patch which does this https://codereview.chromium.org/2312493002/)

BUG=567358
==========

to

==========
[worklets] Split up InProcessWorkerMessagingProxy into a base+worker class.

This pulls out all of the lifetime logic for the WorkerThread into the base
class ThreadedMessagingProxyBase.

This will allow us to re-use all of the lifetime logic (I've created a followup
demo patch which does this https://codereview.chromium.org/2312493002/)

BUG=567358
==========

[commit-bot: I haz the power, 2016-09-08 02:05:09]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-08 02:07:02]

Description was changed from

==========
[worklets] Split up InProcessWorkerMessagingProxy into a base+worker class.

This pulls out all of the lifetime logic for the WorkerThread into the base
class ThreadedMessagingProxyBase.

This will allow us to re-use all of the lifetime logic (I've created a followup
demo patch which does this https://codereview.chromium.org/2312493002/)

BUG=567358
==========

to

==========
[worklets] Split up InProcessWorkerMessagingProxy into a base+worker class.

This pulls out all of the lifetime logic for the WorkerThread into the base
class ThreadedMessagingProxyBase.

This will allow us to re-use all of the lifetime logic (I've created a followup
demo patch which does this https://codereview.chromium.org/2312493002/)

BUG=567358

Committed: https://crrev.com/89b9278e35bf785df739d89efb9587961a95f776
Cr-Commit-Position: refs/heads/master@{#417161}
==========

[commit-bot: I haz the power, 2016-09-08 02:07:03]

Patchset 10 (id:??) landed as
https://crrev.com/89b9278e35bf785df739d89efb9587961a95f776
Cr-Commit-Position: refs/heads/master@{#417161}