compositor-worker: Introduce CompositorWorker.

compositor-worker: Introduce CompositorWorker.

Notable changes:

. Make Worker and WorkerMessagingProxy non-final, so that CompositorWorker
  and CompositorWorkerMessagingProxy can override them.
. Have CompositorWorkerMessagingProxy as the WorkerGlobalScopeProxy for the
  compositor worker-thread, so that it can create a CompositorWorkerThread
  instead of a DedicatedWorkerThread.
. Introduce a virtual Worker::createWorkerGlobalScopeProxy() so that
  CompositorWorker can create a CompositorWorkerMessagingProxy as the
  proxy.
. CompositorWorkerThread is introduced, but it doesn't do anything yet. In
  subsequent CLs, it will override functionalities related to managing
  v8::Isolate, thread etc.
. CompositorWorkerGlobalScope is also introduced.

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

[sadrul, 2015-03-20 00:43:08]

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

[sadrul, 2015-03-20 00:43:09]

Hi! This is mostly creating hooks in the current worker code so that
compositor-worker can override the right set of things. The more interesting
bits that actually implement the overrides will be in subsequent CLs.
(patchset#3 in this CL has some rough implementations of these, but I am
cleaning those up at the moment).

Please feel free to add additional reviewers as you see fit.

[kinuko, 2015-03-20 17:00:18]

(Will look into this tomorrow-ish or next week, sorry I didn't have time today)

[kinuko, 2015-03-24 02:27:19]

Looking good.  I think in general it'd be preferable to have a separate class
for CompositorWorker and CompositorWorkerThread, unless we will make
CompositorWorker inherit from Worker in IDL too.  I'm not sure if that's the
case--- is a CompositorWorker Worker? Do we have CompositorWorker IDL already?

[sadrul, 2015-03-24 02:48:36]

On 2015/03/24 02:27:19, kinuko wrote:
> Looking good.  I think in general it'd be preferable to have a separate class
> for CompositorWorker and CompositorWorkerThread, unless we will make
> CompositorWorker inherit from Worker in IDL too.  I'm not sure if that's the
> case--- is a CompositorWorker Worker? Do we have CompositorWorker IDL already?

This CL includes the separate CompositorWorker and CompositorWorkerThread
classes, and the CompositorWorker.idl file (CompositorWorker is indeed a
Worker). So I am not exactly sure what you mean :) Could you clarify a bit?

[kinuko, 2015-03-24 03:47:56]

On 2015/03/24 02:48:36, sadrul wrote:
> On 2015/03/24 02:27:19, kinuko wrote:
> > Looking good.  I think in general it'd be preferable to have a separate
class
> > for CompositorWorker and CompositorWorkerThread, unless we will make
> > CompositorWorker inherit from Worker in IDL too.  I'm not sure if that's the
> > case--- is a CompositorWorker Worker? Do we have CompositorWorker IDL
already?
> 
> This CL includes the separate CompositorWorker and CompositorWorkerThread
> classes, and the CompositorWorker.idl file (CompositorWorker is indeed a
> Worker). So I am not exactly sure what you mean :) Could you clarify a bit?

Sorry it wasn't clear, I wanted to ask:
1. Do we want to make CompositorWorker inherit from Worker? (For example
SharedWorker is a different beast and it doesn't inherit from Worker)
2. Do we need to make CompositorWorkerThread inherit from DedicatedWorkerThread
but not from WorkerThread? (For all types of workers usually we just create a
new class that inherit from WorkerThread)

[sadrul, 2015-03-24 04:33:54]

Ah, I see. Sorry I misunderstood.

On 2015/03/24 03:47:56, kinuko wrote:
> On 2015/03/24 02:48:36, sadrul wrote:
> > On 2015/03/24 02:27:19, kinuko wrote:
> > > Looking good.  I think in general it'd be preferable to have a separate
> class
> > > for CompositorWorker and CompositorWorkerThread, unless we will make
> > > CompositorWorker inherit from Worker in IDL too.  I'm not sure if that's
the
> > > case--- is a CompositorWorker Worker? Do we have CompositorWorker IDL
> already?
> > 
> > This CL includes the separate CompositorWorker and CompositorWorkerThread
> > classes, and the CompositorWorker.idl file (CompositorWorker is indeed a
> > Worker). So I am not exactly sure what you mean :) Could you clarify a bit?
> 
> Sorry it wasn't clear, I wanted to ask:
> 1. Do we want to make CompositorWorker inherit from Worker? (For example
> SharedWorker is a different beast and it doesn't inherit from Worker)

Inheriting from Worker allows us to not have to repeat code related to
postMessage/terminate etc. Having to implement these for CW wouldn't be that big
of a problem, but I don't see any downside to simply inheriting from Worker. Is
there anything in particular that concerns you about inheriting from Worker?

> 2. Do we need to make CompositorWorkerThread inherit from
DedicatedWorkerThread
> but not from WorkerThread? (For all types of workers usually we just create a
> new class that inherit from WorkerThread)

DedicatedWorkerThread implements createWorkerGlobalScope() and postInitialize().
CompositorWorkerThread is going to override createWorkerGlobalScope() (in
https://codereview.chromium.org/1026843003/). So the only remaining
implementation of DedicatedWT that CompositorWT inherits is the implementation
of postInitialize(). The implementation is small enough that we can easily do it
in CompositorWT too (although in this case too, I can't see any particular
downside to inheriting from DWT).

[kinuko, 2015-03-25 04:23:57]

On 2015/03/24 04:33:54, sadrul wrote:
> Ah, I see. Sorry I misunderstood.
> 
> On 2015/03/24 03:47:56, kinuko wrote:
> > On 2015/03/24 02:48:36, sadrul wrote:
> > Sorry it wasn't clear, I wanted to ask:
> > 1. Do we want to make CompositorWorker inherit from Worker? (For example
> > SharedWorker is a different beast and it doesn't inherit from Worker)
> 
> Inheriting from Worker allows us to not have to repeat code related to
> postMessage/terminate etc. Having to implement these for CW wouldn't be that
big
> of a problem, but I don't see any downside to simply inheriting from Worker.
Is
> there anything in particular that concerns you about inheriting from Worker?

I don't think we usually make inheritance relationships for the interface
objects that do not have actual relationships. Is the spec draft going to have
the IDL soonish?  It'll be really desirable to make it clear that if we want to
make it inherit from Worker or not. (As I wrote in the chat I don't think
removing code duplication is the right reason for inheritance)

> > 2. Do we need to make CompositorWorkerThread inherit from
> DedicatedWorkerThread
> > but not from WorkerThread? (For all types of workers usually we just create
a
> > new class that inherit from WorkerThread)
> 
> DedicatedWorkerThread implements createWorkerGlobalScope() and
postInitialize().
> CompositorWorkerThread is going to override createWorkerGlobalScope() (in
> https://codereview.chromium.org/1026843003/). So the only remaining
> implementation of DedicatedWT that CompositorWT inherits is the implementation
> of postInitialize(). The implementation is small enough that we can easily do
it
> in CompositorWT too (although in this case too, I can't see any particular
> downside to inheriting from DWT).

Is CompositorWorker going to use DedicatedWorkerGlobalScope for its worker
global scope, is that what your code mean?  If it's not clear to you yet either
I think starting from a set of new classes for CompositorWorker is a cleaner
way.

[kinuko, 2015-03-25 04:29:59]

I think my question is simple--- is CompositorWorker a type of DedicatedWorker
by its definition?  If it's not the case using inheritance could be confusing
and we should think twice if there's a better approach.

[kinuko, 2015-03-25 05:43:58]

(+tkent for API owner perspective review)

On 2015/03/25 04:29:59, kinuko wrote:
> I think my question is simple--- is CompositorWorker a type of DedicatedWorker
> by its definition?  If it's not the case using inheritance could be confusing
> and we should think twice if there's a better approach.

Ok, from the offline discussion it seems that's the case.

lgtm, but please consider speccing a draft IDL or something first, and have a
link to the spec draft in .idl and in the CL description so that reviewers can
tell that its inheritance relationship is from the spec or just for
implementation purpose. (I made the same comment in the design sketch doc.)

A few side notes: currently all existing workers have their own set of classes
for {Worker,WorkerGlobalScope,WorkerThread} and no one inherit from each other.
Having dependency to another spec/module could make the code and class
relationships more complex, so it'd be better to think twice if that's the
choice you really want to make for CompositorWorker.

[kinuko, 2015-03-25 05:48:45]

https://codereview.chromium.org/1018863002/diff/100001/Source/modules/composi...
File Source/modules/compositorworker/CompositorWorkerThread.h (right):

https://codereview.chromium.org/1018863002/diff/100001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerThread.h:12: class
CompositorWorkerThread final : public DedicatedWorkerThread {
If we're going to use the underlying thread differently from the existing
dedicated worker, I think it's cleaner to make this class just inherit from
WorkerThread but not from DedicatedWorkerThread. (It looks not much code is
resused in the new class either)

[tkent, 2015-03-25 05:55:09]

tkent@chromium.org changed reviewers:
+ tkent@chromium.org

[tkent, 2015-03-25 05:55:10]

https://codereview.chromium.org/1018863002/diff/100001/public/web/WebRuntimeF...
File public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/1018863002/diff/100001/public/web/WebRuntimeF...
public/web/WebRuntimeFeatures.h:57: BLINK_EXPORT static void
enableCompositorWorker(bool);
Why do you need public functions?

[sadrul, 2015-03-25 06:13:07]

https://codereview.chromium.org/1018863002/diff/100001/Source/modules/composi...
File Source/modules/compositorworker/CompositorWorkerThread.h (right):

https://codereview.chromium.org/1018863002/diff/100001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerThread.h:12: class
CompositorWorkerThread final : public DedicatedWorkerThread {
On 2015/03/25 05:48:45, kinuko wrote:
> If we're going to use the underlying thread differently from the existing
> dedicated worker, I think it's cleaner to make this class just inherit from
> WorkerThread but not from DedicatedWorkerThread. (It looks not much code is
> resused in the new class either)

WorkerMessagingProxy::createWorkerThread() expects a DedicatedWorkerThread to be
returned. So unfortunately this needs to be a DedicatedWorkerThread (for now).

https://codereview.chromium.org/1018863002/diff/100001/public/web/WebRuntimeF...
File public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/1018863002/diff/100001/public/web/WebRuntimeF...
public/web/WebRuntimeFeatures.h:57: BLINK_EXPORT static void
enableCompositorWorker(bool);
On 2015/03/25 05:55:10, tkent wrote:
> Why do you need public functions?
> 

My understanding is this would allow us to turn the feature on using a
command-line flag from chrome. Is there a better way for doing this?

[tkent, 2015-03-25 06:15:05]

https://codereview.chromium.org/1018863002/diff/100001/public/web/WebRuntimeF...
File public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/1018863002/diff/100001/public/web/WebRuntimeF...
public/web/WebRuntimeFeatures.h:57: BLINK_EXPORT static void
enableCompositorWorker(bool);
On 2015/03/25 06:13:07, sadrul wrote:
> On 2015/03/25 05:55:10, tkent wrote:
> > Why do you need public functions?
> > 
> 
> My understanding is this would allow us to turn the feature on using a
> command-line flag from chrome. Is there a better way for doing this?

Usually these functions are unnecessary for a feature with status=experimental
because users can enable it by
chrome://flags/#enable-experimental-web-platform-features

[sadrul, 2015-03-25 06:26:37]

https://codereview.chromium.org/1018863002/diff/100001/public/web/WebRuntimeF...
File public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/1018863002/diff/100001/public/web/WebRuntimeF...
public/web/WebRuntimeFeatures.h:57: BLINK_EXPORT static void
enableCompositorWorker(bool);
On 2015/03/25 06:15:05, tkent wrote:
> On 2015/03/25 06:13:07, sadrul wrote:
> > On 2015/03/25 05:55:10, tkent wrote:
> > > Why do you need public functions?
> > > 
> > 
> > My understanding is this would allow us to turn the feature on using a
> > command-line flag from chrome. Is there a better way for doing this?
> 
> Usually these functions are unnecessary for a feature with status=experimental
> because users can enable it by
> chrome://flags/#enable-experimental-web-platform-features

Ah, yes. That would be sufficient. Thanks. Removed these.

[tkent, 2015-03-25 07:37:55]

lgtm

https://codereview.chromium.org/1018863002/diff/100001/Source/modules/composi...
File Source/modules/compositorworker/CompositorWorkerThread.h (right):

https://codereview.chromium.org/1018863002/diff/100001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerThread.h:12: class
CompositorWorkerThread final : public DedicatedWorkerThread {
On 2015/03/25 06:13:07, sadrul wrote:
> On 2015/03/25 05:48:45, kinuko wrote:
> > If we're going to use the underlying thread differently from the existing
> > dedicated worker, I think it's cleaner to make this class just inherit from
> > WorkerThread but not from DedicatedWorkerThread. (It looks not much code is
> > resused in the new class either)
> 
> WorkerMessagingProxy::createWorkerThread() expects a DedicatedWorkerThread to
be
> returned. So unfortunately this needs to be a DedicatedWorkerThread (for now).

Probably we should introduce new WorkerThread class which is a base class of
DedicatedWorkerThread and CompositorWorkerThread.

[sadrul, 2015-03-25 20:53:51]

Patchset #8 (id:140001) has been deleted

[sadrul, 2015-03-26 01:27:48]

I have made some changes since last review (including pulling in
CompositorWorkerGlobalScope related changes in this CL). Mind reviewing it
again? Thanks!

https://codereview.chromium.org/1018863002/diff/100001/Source/modules/composi...
File Source/modules/compositorworker/CompositorWorkerThread.h (right):

https://codereview.chromium.org/1018863002/diff/100001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerThread.h:12: class
CompositorWorkerThread final : public DedicatedWorkerThread {
On 2015/03/25 07:37:55, tkent wrote:
> On 2015/03/25 06:13:07, sadrul wrote:
> > On 2015/03/25 05:48:45, kinuko wrote:
> > > If we're going to use the underlying thread differently from the existing
> > > dedicated worker, I think it's cleaner to make this class just inherit
from
> > > WorkerThread but not from DedicatedWorkerThread. (It looks not much code
is
> > > resused in the new class either)
> > 
> > WorkerMessagingProxy::createWorkerThread() expects a DedicatedWorkerThread
to
> be
> > returned. So unfortunately this needs to be a DedicatedWorkerThread (for
now).
> 
> Probably we should introduce new WorkerThread class which is a base class of
> DedicatedWorkerThread and CompositorWorkerThread.

Turns out, we can just use 'WorkerThread' (instead of DedicatedWorkerThread).
Done.

https://codereview.chromium.org/1018863002/diff/220001/Source/modules/composi...
File Source/modules/compositorworker/CompositorWorker.idl (right):

https://codereview.chromium.org/1018863002/diff/220001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorker.idl:12: ] interface
CompositorWorker : EventTarget {
CompositorWorker is no longer a Worker in the IDL.

For the implementation however, CompositorWorker class does still inherit from
the Worker class. I am planning to do some refactoring here, so that Worker
becomes an abstract class, with DedicatedWorker and CompositorWorker
implementing some of the different behaviours. Does this sound like a reasonable
approach?

[tkent, 2015-03-26 03:43:06]

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

[tkent, 2015-03-26 03:43:07]

+haraken for new custom binding code

[haraken, 2015-03-26 03:56:35]

bindings LGTM.

You don't need to do this in this CL, but you'll need to add 'CompositorWorker'
to EXPOSED_EXECUTION_CONTEXT_METHOD and EXPOSED_WORKERS in v8_utilities.py.

[sadrul, 2015-03-26 04:00:43]

On 2015/03/26 03:56:35, haraken wrote:
> bindings LGTM.
> 
> You don't need to do this in this CL, but you'll need to add
'CompositorWorker'
> to EXPOSED_EXECUTION_CONTEXT_METHOD and EXPOSED_WORKERS in v8_utilities.py.

I have made some changes in v8_utilities.py in this patchset
(https://codereview.chromium.org/1018863002/patch/220001/230010). Are there
additional places in that file I should add CompositorWorker?

[haraken, 2015-03-26 04:11:17]

On 2015/03/26 04:00:43, sadrul wrote:
> On 2015/03/26 03:56:35, haraken wrote:
> > bindings LGTM.
> > 
> > You don't need to do this in this CL, but you'll need to add
> 'CompositorWorker'
> > to EXPOSED_EXECUTION_CONTEXT_METHOD and EXPOSED_WORKERS in v8_utilities.py.
> 
> I have made some changes in v8_utilities.py in this patchset
> (https://codereview.chromium.org/1018863002/patch/220001/230010). Are there
> additional places in that file I should add CompositorWorker?

oh, sorry, I missed that. That looks enough :)

[tkent, 2015-03-26 04:22:10]

https://codereview.chromium.org/1018863002/diff/220001/Source/bindings/module...
File Source/bindings/modules/v8/custom/V8CompositorWorkerCustom.cpp (right):

https://codereview.chromium.org/1018863002/diff/220001/Source/bindings/module...
Source/bindings/modules/v8/custom/V8CompositorWorkerCustom.cpp:13: // FIXME:
This stub should be replaced by generated code.
nit: FIXME -> TODO(sadrul)

https://codereview.chromium.org/1018863002/diff/220001/Source/bindings/module...
File Source/bindings/modules/v8/custom/V8CompositorWorkerGlobalScopeCustom.cpp
(right):

https://codereview.chromium.org/1018863002/diff/220001/Source/bindings/module...
Source/bindings/modules/v8/custom/V8CompositorWorkerGlobalScopeCustom.cpp:13: //
FIXME: This stub should be replaced by generated code.
nit: FIXME -> TODO(sadrul)

https://codereview.chromium.org/1018863002/diff/220001/Source/modules/composi...
File Source/modules/compositorworker/CompositorWorkerGlobalScope.cpp (right):

https://codereview.chromium.org/1018863002/diff/220001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerGlobalScope.cpp:51:
DEFINE_TRACE(CompositorWorkerGlobalScope)
nit: You don't need this function because it just calls the parent class method.

https://codereview.chromium.org/1018863002/diff/220001/Source/modules/composi...
File Source/modules/compositorworker/CompositorWorkerThread.cpp (right):

https://codereview.chromium.org/1018863002/diff/220001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerThread.cpp:33: v8::Isolate*
CompositorWorkerThread::initializeIsolate()
Do you still need these override?

[sadrul, 2015-03-26 04:36:39]

https://codereview.chromium.org/1018863002/diff/220001/Source/bindings/module...
File Source/bindings/modules/v8/custom/V8CompositorWorkerCustom.cpp (right):

https://codereview.chromium.org/1018863002/diff/220001/Source/bindings/module...
Source/bindings/modules/v8/custom/V8CompositorWorkerCustom.cpp:13: // FIXME:
This stub should be replaced by generated code.
On 2015/03/26 04:22:10, tkent wrote:
> nit: FIXME -> TODO(sadrul)

Done.

https://codereview.chromium.org/1018863002/diff/220001/Source/bindings/module...
File Source/bindings/modules/v8/custom/V8CompositorWorkerGlobalScopeCustom.cpp
(right):

https://codereview.chromium.org/1018863002/diff/220001/Source/bindings/module...
Source/bindings/modules/v8/custom/V8CompositorWorkerGlobalScopeCustom.cpp:13: //
FIXME: This stub should be replaced by generated code.
On 2015/03/26 04:22:10, tkent wrote:
> nit: FIXME -> TODO(sadrul)

Done.

https://codereview.chromium.org/1018863002/diff/220001/Source/modules/composi...
File Source/modules/compositorworker/CompositorWorkerGlobalScope.cpp (right):

https://codereview.chromium.org/1018863002/diff/220001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerGlobalScope.cpp:51:
DEFINE_TRACE(CompositorWorkerGlobalScope)
On 2015/03/26 04:22:10, tkent wrote:
> nit: You don't need this function because it just calls the parent class
method.

Done.

https://codereview.chromium.org/1018863002/diff/220001/Source/modules/composi...
File Source/modules/compositorworker/CompositorWorkerThread.cpp (right):

https://codereview.chromium.org/1018863002/diff/220001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerThread.cpp:33: v8::Isolate*
CompositorWorkerThread::initializeIsolate()
On 2015/03/26 04:22:10, tkent wrote:
> Do you still need these override?

We still need to override these for CompositorWorkerThread, yes. But the current
implementation in this CL don't do anything. So I have removed these. In
subsequent CLs, these functions will be overridden with proper implementation.

[tkent, 2015-03-26 05:33:12]

https://codereview.chromium.org/1018863002/diff/220001/Source/modules/composi...
File Source/modules/compositorworker/CompositorWorkerThread.cpp (right):

https://codereview.chromium.org/1018863002/diff/220001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerThread.cpp:33: v8::Isolate*
CompositorWorkerThread::initializeIsolate()
On 2015/03/26 04:36:39, sadrul wrote:
> On 2015/03/26 04:22:10, tkent wrote:
> > Do you still need these override?
> 
> We still need to override these for CompositorWorkerThread, yes. But the
current
> implementation in this CL don't do anything. So I have removed these. In
> subsequent CLs, these functions will be overridden with proper implementation.

ok, it makes sense.  lgtm

[kinuko, 2015-03-26 07:57:44]

New patch lgtm, glad that now we have less dependencies between workers!

[sadrul, 2015-03-26 08:11:41]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2015-03-26 08:11:42]

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

[commit-bot: I haz the power, 2015-03-26 08:12:05]

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

[commit-bot: I haz the power, 2015-03-26 10:27:24]

Committed patchset #13 (id:260001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192592