Update AbstractWorker, Worker and SharedWorker to match the spec

Update AbstractWorker, Worker and SharedWorker to match the spec

Update AbstractWorker, Worker and SharedWorker to match the spec:
http://www.whatwg.org/specs/web-apps/current-work/multipage/workers.html#abstractworker
http://www.whatwg.org/specs/web-apps/current-work/multipage/workers.html#worker
http://www.whatwg.org/specs/web-apps/current-work/multipage/workers.html#sharedworker

Worker and SharedWorker no longer inherit from AbstractWorker. Web IDL 'implements'
statements are used instead. Worker and SharedWorker can now inherit from EventTarget
as per the specification.

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

[haraken, 2013-06-19 10:24:00]

https://codereview.chromium.org/17236004/diff/1/Source/core/workers/AbstractW...
File Source/core/workers/AbstractWorker.h (left):

https://codereview.chromium.org/17236004/diff/1/Source/core/workers/AbstractW...
Source/core/workers/AbstractWorker.h:49: class AbstractWorker : public
RefCounted<AbstractWorker>, public ScriptWrappable, public ActiveDOMObject,
public EventTarget {

Why did you move ScriptWrappable and ActiveDOMObject from AbstractWorker to
Worker and SharedWorker? It will be harmless but I'm curious about the
background.

https://codereview.chromium.org/17236004/diff/1/Source/core/workers/AbstractW...
File Source/core/workers/AbstractWorker.idl (left):

https://codereview.chromium.org/17236004/diff/1/Source/core/workers/AbstractW...
Source/core/workers/AbstractWorker.idl:46: [RaisesException] boolean
dispatchEvent(Event evt);

Just to confirm: Removing these doesn't change web-exposed behavior because
Worker and SharedWorker now inherit these methods from EventTarget. Right?

[do-not-use, 2013-06-19 10:39:09]

https://codereview.chromium.org/17236004/diff/1/Source/core/workers/AbstractW...
File Source/core/workers/AbstractWorker.h (left):

https://codereview.chromium.org/17236004/diff/1/Source/core/workers/AbstractW...
Source/core/workers/AbstractWorker.h:49: class AbstractWorker : public
RefCounted<AbstractWorker>, public ScriptWrappable, public ActiveDOMObject,
public EventTarget {
On 2013/06/19 10:24:00, haraken wrote:
> 
> Why did you move ScriptWrappable and ActiveDOMObject from AbstractWorker to
> Worker and SharedWorker? It will be harmless but I'm curious about the
> background.

It did not link if AbstractWorker would subclass ScriptWrappable in C++ because
of undefined
webCoreInitializeScriptWrappableForInterface(WebCore::AbstractWorker*). This is
because there is no longer any code generated for AbstractWorker.idl (No
JSAbstractWorker).

Regarding ActiveDOMObject, I was hitting assertions in the RefPtr unref without
the change (deletionHasBegun). I did not investigate yet why this happens but
since [ActiveDOMObject] was removed from AbstractWorker.idl, I figured this was
linked and it did fix the crashes.

https://codereview.chromium.org/17236004/diff/1/Source/core/workers/AbstractW...
File Source/core/workers/AbstractWorker.idl (left):

https://codereview.chromium.org/17236004/diff/1/Source/core/workers/AbstractW...
Source/core/workers/AbstractWorker.idl:46: [RaisesException] boolean
dispatchEvent(Event evt);
On 2013/06/19 10:24:00, haraken wrote:
> 
> Just to confirm: Removing these doesn't change web-exposed behavior because
> Worker and SharedWorker now inherit these methods from EventTarget. Right?

Right, they are now inherited from EventTarget. AFAIK, there is no WebExposed
change and it did not change layout tests' output either.

[haraken, 2013-06-19 10:44:05]

Makes sense. LGTM.

[do-not-use, 2013-06-19 10:52:08]

https://codereview.chromium.org/17236004/diff/1/Source/core/workers/AbstractW...
File Source/core/workers/AbstractWorker.h (left):

https://codereview.chromium.org/17236004/diff/1/Source/core/workers/AbstractW...
Source/core/workers/AbstractWorker.h:49: class AbstractWorker : public
RefCounted<AbstractWorker>, public ScriptWrappable, public ActiveDOMObject,
public EventTarget {
On 2013/06/19 10:39:09, Christophe Dumez wrote:
> On 2013/06/19 10:24:00, haraken wrote:
> > 
> > Why did you move ScriptWrappable and ActiveDOMObject from AbstractWorker to
> > Worker and SharedWorker? It will be harmless but I'm curious about the
> > background.
> 
> It did not link if AbstractWorker would subclass ScriptWrappable in C++
because
> of undefined
> webCoreInitializeScriptWrappableForInterface(WebCore::AbstractWorker*). This
is
> because there is no longer any code generated for AbstractWorker.idl (No
> JSAbstractWorker).
> 
> Regarding ActiveDOMObject, I was hitting assertions in the RefPtr unref
without
> the change (deletionHasBegun). I did not investigate yet why this happens but
> since [ActiveDOMObject] was removed from AbstractWorker.idl, I figured this
was
> linked and it did fix the crashes.

I'll try to move back ActiveDOMObject to AbstractWorker.h to make sure the move
is really needed.

[do-not-use, 2013-06-19 12:36:29]

On 2013/06/19 10:52:08, Christophe Dumez wrote:
>
https://codereview.chromium.org/17236004/diff/1/Source/core/workers/AbstractW...
> File Source/core/workers/AbstractWorker.h (left):
> 
>
https://codereview.chromium.org/17236004/diff/1/Source/core/workers/AbstractW...
> Source/core/workers/AbstractWorker.h:49: class AbstractWorker : public
> RefCounted<AbstractWorker>, public ScriptWrappable, public ActiveDOMObject,
> public EventTarget {
> On 2013/06/19 10:39:09, Christophe Dumez wrote:
> > On 2013/06/19 10:24:00, haraken wrote:
> > > 
> > > Why did you move ScriptWrappable and ActiveDOMObject from AbstractWorker
to
> > > Worker and SharedWorker? It will be harmless but I'm curious about the
> > > background.
> > 
> > It did not link if AbstractWorker would subclass ScriptWrappable in C++
> because
> > of undefined
> > webCoreInitializeScriptWrappableForInterface(WebCore::AbstractWorker*). This
> is
> > because there is no longer any code generated for AbstractWorker.idl (No
> > JSAbstractWorker).
> > 
> > Regarding ActiveDOMObject, I was hitting assertions in the RefPtr unref
> without
> > the change (deletionHasBegun). I did not investigate yet why this happens
but
> > since [ActiveDOMObject] was removed from AbstractWorker.idl, I figured this
> was
> > linked and it did fix the crashes.
> 
> I'll try to move back ActiveDOMObject to AbstractWorker.h to make sure the
move
> is really needed.

Ok, I moved back ActiveDOMObject to AbstractWorker.h to minimize the changes. It
turns out that it is not needed.

[haraken, 2013-06-19 13:00:38]

> Ok, I moved back ActiveDOMObject to AbstractWorker.h to minimize the changes.
It
> turns out that it is not needed.

Thanks. Then why do you have [ActiveDOMObject] not in AbstractWorker but in
Worker and SharedWorker?

What about ScriptWrappable? Do you need to move ScriptWrappable from
AbstractWorker to Worker and SharedWorker?

[do-not-use, 2013-06-19 13:05:20]

On 2013/06/19 13:00:38, haraken wrote:
> > Ok, I moved back ActiveDOMObject to AbstractWorker.h to minimize the
changes.
> It
> > turns out that it is not needed.
> 
> Thanks. Then why do you have [ActiveDOMObject] not in AbstractWorker but in
> Worker and SharedWorker?
> 
> What about ScriptWrappable? Do you need to move ScriptWrappable from
> AbstractWorker to Worker and SharedWorker?

Yes, as I explained earlier I do need to move ScriptWrappable:
"It did not link if AbstractWorker would subclass ScriptWrappable in C++ because
of undefined
webCoreInitializeScriptWrappableForInterface(WebCore::AbstractWorker*). This is
because there is no longer any code generated for AbstractWorker.idl (No
JSAbstractWorker)."

I also need to move [ActiveDOMObject] to Worker.idl and SharedWorker.idl because
we want the attribute to apply to these interfaces and they no longer inherit
AbstractWorker. Also having [ActiveDOMObject] on AbstractWorker because no
bindings code is generated for it.

[do-not-use, 2013-06-19 13:07:16]

On 2013/06/19 13:05:20, Christophe Dumez wrote:
> On 2013/06/19 13:00:38, haraken wrote:
> > > Ok, I moved back ActiveDOMObject to AbstractWorker.h to minimize the
> changes.
> > It
> > > turns out that it is not needed.
> > 
> > Thanks. Then why do you have [ActiveDOMObject] not in AbstractWorker but in
> > Worker and SharedWorker?
> > 
> > What about ScriptWrappable? Do you need to move ScriptWrappable from
> > AbstractWorker to Worker and SharedWorker?
> 
> Yes, as I explained earlier I do need to move ScriptWrappable:
> "It did not link if AbstractWorker would subclass ScriptWrappable in C++
because
> of undefined
> webCoreInitializeScriptWrappableForInterface(WebCore::AbstractWorker*). This
is
> because there is no longer any code generated for AbstractWorker.idl (No
> JSAbstractWorker)."
> 
> I also need to move [ActiveDOMObject] to Worker.idl and SharedWorker.idl
because
> we want the attribute to apply to these interfaces and they no longer inherit
> AbstractWorker. Also having [ActiveDOMObject] on AbstractWorker because no
> bindings code is generated for it.

I messed up my last sentence, sorry:
"Also having [ActiveDOMObject] on AbstractWorker would have no effect because no
> bindings code is generated for it and the attribute applies to the interface,
not the attributes."

[haraken, 2013-06-19 13:15:36]

Thanks for the clarification!

Regarding ScriptWrappable, I understood.

Regarding ActiveDOMObject, it's confusing that [ActiveDOMObject] is placed on
Worker and SharedWorker but ActiveDOMObject in C++ is inherited in
AbstractWorker.h.

> I also need to move [ActiveDOMObject] to Worker.idl and SharedWorker.idl
because
we want the attribute to apply to these interfaces and they no longer inherit
AbstractWorker.

Maybe we want to fix this behavior. If "A implements B" is found, A should
inherit IDL attributes defined in B. What do you think?

[do-not-use, 2013-06-19 13:23:49]

On 2013/06/19 13:15:36, haraken wrote:
> Thanks for the clarification!
> 
> Regarding ScriptWrappable, I understood.
> 
> Regarding ActiveDOMObject, it's confusing that [ActiveDOMObject] is placed on
> Worker and SharedWorker but ActiveDOMObject in C++ is inherited in
> AbstractWorker.h.
> 
> > I also need to move [ActiveDOMObject] to Worker.idl and SharedWorker.idl
> because
> we want the attribute to apply to these interfaces and they no longer inherit
> AbstractWorker.
> 
> Maybe we want to fix this behavior. If "A implements B" is found, A should
> inherit IDL attributes defined in B. What do you think?

I don't really like it. We don't do this for partial interfaces either. I
personally think this would be confusing as we already apply the extended
attributes of the supplemental interface to the attributes (which I think is the
correct behavior). I think supplemental interfaces should only be able to affect
the behavior of their extensions (attributes, operations), not the overall
behavior of the class they extend.

[arv (Not doing code reviews), 2013-06-19 13:25:28]

On 2013/06/19 13:15:36, haraken wrote:
> Maybe we want to fix this behavior. If "A implements B" is found, A should
> inherit IDL attributes defined in B. What do you think?

I don't think we want to do that. Or at least I don't think we need to block
this CL based on that.

LGTM

IDL extended interface attributes are more about implementation details and "A
implements B" is more about not having to repeat methods and property definition
in multiple places.

[haraken, 2013-06-19 13:33:03]

On 2013/06/19 13:25:28, arv wrote:
> On 2013/06/19 13:15:36, haraken wrote:
> > Maybe we want to fix this behavior. If "A implements B" is found, A should
> > inherit IDL attributes defined in B. What do you think?
> 
> I don't think we want to do that. Or at least I don't think we need to block
> this CL based on that.
> 
> LGTM
> 
> IDL extended interface attributes are more about implementation details and "A
> implements B" is more about not having to repeat methods and property
definition
> in multiple places.

Agreed. The current CL looks good. LGTM.

[commit-bot: I haz the power, 2013-06-19 13:51:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/17236004/7001

[commit-bot: I haz the power, 2013-06-19 14:30:38]

Change committed as 152723