Inherit EventTarget interface instead of duplicating its code

Inherit EventTarget interface instead of duplicating its code

Update most interfaces to stop using [EventTarget] IDL extended attribute
and inherit EventTarget instead. This avoids duplicating the EventTarget
API in each IDL and makes our IDL more standard.

Also remove [NoInterfaceObject] extended attribute from EventTarget to
facilitate testing and match the specification:
http://dom.spec.whatwg.org/#interface-eventtarget

Only 3 interfaces were not updated yet (Node, Window, Performance) because
they require special treatment at the moment.

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

[do-not-use, 2013-07-05 14:44:04]

https://codereview.chromium.org/18778002/diff/1/Source/core/css/FontLoader.h
File Source/core/css/FontLoader.h (right):

https://codereview.chromium.org/18778002/diff/1/Source/core/css/FontLoader.h#...
Source/core/css/FontLoader.h:50: class FontLoader : public EventTarget, public
RefCounted<FontLoader>, public ActiveDOMObject {
FYI, I'm changing the other because we hit the following assertion in the
bindings otherwise:
STDERR: ASSERTION FAILED:
static_cast<void*>(static_cast<EventTarget*>(impl.get())) ==
static_cast<void*>(impl.get())
STDERR: gen/webkit/bindings/V8MediaStream.cpp(470) : static
v8::Handle<v8::Object>
WebCore::V8MediaStream::createWrapper(WTF::PassRefPtr<WebCore::MediaStream>,
v8::Handle<v8::Object>, v8::Isolate*)

[do-not-use, 2013-07-05 14:47:44]

https://codereview.chromium.org/18778002/diff/1/Source/core/css/FontLoader.h
File Source/core/css/FontLoader.h (right):

https://codereview.chromium.org/18778002/diff/1/Source/core/css/FontLoader.h#...
Source/core/css/FontLoader.h:50: class FontLoader : public EventTarget, public
RefCounted<FontLoader>, public ActiveDOMObject {
On 2013/07/05 14:44:04, Christophe Dumez wrote:
> FYI, I'm changing the other because we hit the following assertion in the
> bindings otherwise:
> STDERR: ASSERTION FAILED:
> static_cast<void*>(static_cast<EventTarget*>(impl.get())) ==
> static_cast<void*>(impl.get())
> STDERR: gen/webkit/bindings/V8MediaStream.cpp(470) : static
> v8::Handle<v8::Object>
> WebCore::V8MediaStream::createWrapper(WTF::PassRefPtr<WebCore::MediaStream>,
> v8::Handle<v8::Object>, v8::Isolate*)

changing the *order* (not *other*).

[haraken, 2013-07-06 00:29:55]

This looks great!!

I guess this CL changes web-exposed behavior:

(before)
Object.getOwnPropertyDescriptor(XMLHttpRequest.prototype, "addEventListener");
// [Object]

(after)
Object.getOwnPropertyDescriptor(XMLHttpRequest.prototype, "addEventListener");
// undefined

Would you add a test about it?

https://codereview.chromium.org/18778002/diff/1/Source/core/css/FontLoader.h
File Source/core/css/FontLoader.h (right):

https://codereview.chromium.org/18778002/diff/1/Source/core/css/FontLoader.h#...
Source/core/css/FontLoader.h:50: class FontLoader : public EventTarget, public
RefCounted<FontLoader>, public ActiveDOMObject {

In your CL, the order swap doesn't matter.

The order sometimes matters for an object size. For example, in case of Node:

class TreeShared {
  uint32_t m_count;
};

class Node : public TreeShared<Node>, public EventTarget {
  uint32_t m_flags;
  Node* m_firstChild;
  ...;
};

By putting TreeShared<Node> at the first inheritance, we can pack the m_count
(32 bit) and m_flags (32 bit) into one 64 bit integer.

For objects where size matters, I think we already have ASSERTs about the object
size to guarantee that we don't regress the size. So I think your CL would be OK
as long as you don't hit any ASSERT.

[haraken, 2013-07-06 01:21:00]

https://codereview.chromium.org/18778002/diff/1/Source/core/css/FontLoader.h
File Source/core/css/FontLoader.h (right):

https://codereview.chromium.org/18778002/diff/1/Source/core/css/FontLoader.h#...
Source/core/css/FontLoader.h:50: class FontLoader : public EventTarget, public
RefCounted<FontLoader>, public ActiveDOMObject {

On the other hand, we might not want to change the inheritance order just for
the ASSERT. In other words, we might not want to introduce some dependency about
the inheritance order.

Wouldn't it be possible to check:

  static_cast<void*>(static_cast<EventTarget*>(impl.get())) ==
static_cast<void*>(impl.get())

without changing the inheritance order? (Sorry I have no idea since I'm not a
C++ expert:-)

[do-not-use, 2013-07-06 17:22:36]

On 2013/07/06 01:21:00, haraken wrote:
> https://codereview.chromium.org/18778002/diff/1/Source/core/css/FontLoader.h
> File Source/core/css/FontLoader.h (right):
> 
>
https://codereview.chromium.org/18778002/diff/1/Source/core/css/FontLoader.h#...
> Source/core/css/FontLoader.h:50: class FontLoader : public EventTarget, public
> RefCounted<FontLoader>, public ActiveDOMObject {
> 
> On the other hand, we might not want to change the inheritance order just for
> the ASSERT. In other words, we might not want to introduce some dependency
about
> the inheritance order.
> 
> Wouldn't it be possible to check:
> 
>   static_cast<void*>(static_cast<EventTarget*>(impl.get())) ==
> static_cast<void*>(impl.get())
> 
> without changing the inheritance order? (Sorry I have no idea since I'm not a
> C++ expert:-)

It seems this assertion was added by dominicc and reviewed by Adam Barth in:
https://trac.webkit.org/changeset/125251

The whole point of this assertion is to make sure that we extend EventTarget
first, which apparently is a requirement:
https://bugs.webkit.org/show_bug.cgi?id=88232#c10

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

I think it is fine to change the order here.

Can we add tests that ensures that we have EventTarget.prototype on the
prototype chain for these?

[dominicc (has gone to gerrit), 2013-07-07 00:14:28]

https://codereview.chromium.org/18778002/diff/1/Source/core/css/FontLoader.h
File Source/core/css/FontLoader.h (right):

https://codereview.chromium.org/18778002/diff/1/Source/core/css/FontLoader.h#...
Source/core/css/FontLoader.h:50: class FontLoader : public EventTarget, public
RefCounted<FontLoader>, public ActiveDOMObject {
On 2013/07/06 01:21:01, haraken wrote:
> Wouldn't it be possible to check:
> 
>   static_cast<void*>(static_cast<EventTarget*>(impl.get())) ==
> static_cast<void*>(impl.get())
> 
> without changing the inheritance order? (Sorry I have no idea since I'm not a
> C++ expert:-)

The issue is basically this one:
<http://stackoverflow.com/questions/3327631/casting-with-multiple-inheritance>

I think we should learn a trick from JSC here. Basically in V8 it would look
like this:

in V8EventTarget.h, in V8EventTarget (or any base type):

static inline void* toWrappedType(EventTarget* impl)
{
    return impl;
}

in V8Element.h, in V8Element (or any derived type):

static inline void* toWrappedType(Element* impl)
{
    return V8Node::toWrappedType(impl);
}

And similarly the other way:

static inline EventTarget* fromWrappedType(void* impl)
{
    return static_cast<EventTarget*>(impl);
}

static inline Element* fromWrappedType(void* impl)
{
    return static_cast<Element*>(V8Node::fromWrappedType(impl));
}

Then minimize static casting in the bindings. Most uses can be replaced with
these. For example the generated toNative uses fromWrappedType.

IIRC there might be places where the bindings store objects at different pointer
types. If they could be brought into a similar scheme that would be good.

[haraken, 2013-07-08 01:59:02]

> The resulting binary is ~450Kb smaller

BTW, I'm curious about where the diet comes from. Does just removing
addEventListener, removeEventListener and dispatchEvent from ObjectTemplate of
40 IDL interfaces decrease the binary size by 450 KB?

[do-not-use, 2013-07-08 06:16:20]

On 2013/07/08 01:59:02, haraken wrote:
> > The resulting binary is ~450Kb smaller
> 
> BTW, I'm curious about where the diet comes from. Does just removing
> addEventListener, removeEventListener and dispatchEvent from ObjectTemplate of
> 40 IDL interfaces decrease the binary size by 450 KB?

Yes, not duplicating those 3 methods' implementation in these 40 classes seems
to be the only impact. The binary is definitely 450 KB smaller though, I checked
it twice.

I will look into added a test case, as you suggested.

[do-not-use, 2013-07-08 07:00:43]

On 2013/07/08 06:16:20, Christophe Dumez wrote:
> On 2013/07/08 01:59:02, haraken wrote:
> > > The resulting binary is ~450Kb smaller
> > 
> > BTW, I'm curious about where the diet comes from. Does just removing
> > addEventListener, removeEventListener and dispatchEvent from ObjectTemplate
of
> > 40 IDL interfaces decrease the binary size by 450 KB?
> 
> Yes, not duplicating those 3 methods' implementation in these 40 classes seems
> to be the only impact. The binary is definitely 450 KB smaller though, I
checked
> it twice.

This is with Debug enabled, which may explain why the size difference is that
big.

[haraken, 2013-07-08 07:06:25]

> > Yes, not duplicating those 3 methods' implementation in these 40 classes
seems
> > to be the only impact. The binary is definitely 450 KB smaller though, I
> checked
> > it twice.
> 
> This is with Debug enabled, which may explain why the size difference is that
> big.

What about in a Release build?

It sounds strange to me that the size decreases so much even in Debug builds.
450 KB / 3 methods / 40 interfaces = 3.8 KB. This means that installing one DOM
method consumes 3.8 KB in a Debug build. This sounds too large to me.

(I'm not intending to block your CL by the binary size. I'm just intending to
say that there might be room for optimizing the binary size by looking into the
issue more in detail.)

[do-not-use, 2013-07-08 07:22:19]

On 2013/07/08 07:06:25, haraken wrote:
> > > Yes, not duplicating those 3 methods' implementation in these 40 classes
> seems
> > > to be the only impact. The binary is definitely 450 KB smaller though, I
> > checked
> > > it twice.
> > 
> > This is with Debug enabled, which may explain why the size difference is
that
> > big.
> 
> What about in a Release build?
> 
> It sounds strange to me that the size decreases so much even in Debug builds.
> 450 KB / 3 methods / 40 interfaces = 3.8 KB. This means that installing one
DOM
> method consumes 3.8 KB in a Debug build. This sounds too large to me.
> 
> (I'm not intending to block your CL by the binary size. I'm just intending to
> say that there might be room for optimizing the binary size by looking into
the
> issue more in detail.)

I added a layout test as suggested. I also exposed EventTarget to match the
specification and facilitate testing.

I will check the binary size difference in Release and report back soon.

[haraken, 2013-07-08 07:36:33]

The test looks good. What do you think about dominicc's opinion?

[do-not-use, 2013-07-08 07:43:10]

On 2013/07/08 07:36:33, haraken wrote:
> The test looks good. What do you think about dominicc's opinion?

I did not fully understand it yet. I understand that static_cast returns a
different pointer if casting to a parent class that is not the first one.
However, I haven't found yet why this is a problem and why the proposed solution
would help. I need more time to dig into the code.

[do-not-use, 2013-07-08 07:50:03]

On 2013/07/08 07:06:25, haraken wrote:
> > > Yes, not duplicating those 3 methods' implementation in these 40 classes
> seems
> > > to be the only impact. The binary is definitely 450 KB smaller though, I
> > checked
> > > it twice.
> > 
> > This is with Debug enabled, which may explain why the size difference is
that
> > big.
> 
> What about in a Release build?
> 
> It sounds strange to me that the size decreases so much even in Debug builds.
> 450 KB / 3 methods / 40 interfaces = 3.8 KB. This means that installing one
DOM
> method consumes 3.8 KB in a Debug build. This sounds too large to me.
> 
> (I'm not intending to block your CL by the binary size. I'm just intending to
> say that there might be room for optimizing the binary size by looking into
the
> issue more in detail.)

The binary is almost 90Kb smaller in release.

[haraken, 2013-07-08 07:52:45]

dominicc: Your point is that we can clean up the code by introducing
toWrappedType/fromWrappedType and not that we can avoid introducing the
inheritance order dependency by doing that, right? In other words, the current
CL looks good. Is my understanding correct?

[haraken, 2013-07-08 07:54:30]

Christophe: You might want to put the test to webexposed/.

[do-not-use, 2013-07-08 07:59:31]

On 2013/07/08 07:54:30, haraken wrote:
> Christophe: You might want to put the test to webexposed/.

Done. I also rebaselined 2 other tests whose output has changed due to exposing
EventTarget.

[haraken, 2013-07-08 08:00:21]

LGTM. Please wait for a comment from dominicc.

[dominicc (has gone to gerrit), 2013-07-08 08:27:36]

On 2013/07/08 07:52:45, haraken wrote:
> dominicc: Your point is that we can clean up the code by introducing
> toWrappedType/fromWrappedType and not that we can avoid introducing the
> inheritance order dependency by doing that, right? In other words, the current
> CL looks good. Is my understanding correct?

The current change looks correct.

Ultimately you need to handle Node, so you need to do toWrappedType/etc. at some
point. But it is fine to do it later if you're comfortable churning the
inheritance order now.

If you want to do this change but be conservative, you could check that
EventTarget did not move before any type that has virtual methods in the class
list. I did not notice any at a passing glance--I think you're good to go.

[haraken, 2013-07-08 08:33:33]

On 2013/07/08 08:27:36, dominicc wrote:
> On 2013/07/08 07:52:45, haraken wrote:
> > dominicc: Your point is that we can clean up the code by introducing
> > toWrappedType/fromWrappedType and not that we can avoid introducing the
> > inheritance order dependency by doing that, right? In other words, the
current
> > CL looks good. Is my understanding correct?
> 
> The current change looks correct.
> 
> Ultimately you need to handle Node, so you need to do toWrappedType/etc. at
some
> point. But it is fine to do it later if you're comfortable churning the
> inheritance order now.

ah, I got what you meant in comment #8. By using your technique, we can make our
change without changing the inheritance order. That sounds nicer.

christophe: Did you get it? :)

[do-not-use, 2013-07-08 09:41:00]

On 2013/07/08 08:33:33, haraken wrote:
> On 2013/07/08 08:27:36, dominicc wrote:
> > On 2013/07/08 07:52:45, haraken wrote:
> > > dominicc: Your point is that we can clean up the code by introducing
> > > toWrappedType/fromWrappedType and not that we can avoid introducing the
> > > inheritance order dependency by doing that, right? In other words, the
> current
> > > CL looks good. Is my understanding correct?
> > 
> > The current change looks correct.
> > 
> > Ultimately you need to handle Node, so you need to do toWrappedType/etc. at
> some
> > point. But it is fine to do it later if you're comfortable churning the
> > inheritance order now.
> 
> ah, I got what you meant in comment #8. By using your technique, we can make
our
> change without changing the inheritance order. That sounds nicer.
> 
> christophe: Did you get it? :)

I will give it a try and see how it goes. It would be nice indeed if it worked
without changing the inheritance order. Dominic is also right that we will need
this change sooner or later anyway due to Node.

[do-not-use, 2013-07-08 11:05:30]

On 2013/07/07 00:14:28, dominicc wrote:
> https://codereview.chromium.org/18778002/diff/1/Source/core/css/FontLoader.h
> File Source/core/css/FontLoader.h (right):
> 
>
https://codereview.chromium.org/18778002/diff/1/Source/core/css/FontLoader.h#...
> Source/core/css/FontLoader.h:50: class FontLoader : public EventTarget, public
> RefCounted<FontLoader>, public ActiveDOMObject {
> On 2013/07/06 01:21:01, haraken wrote:
> > Wouldn't it be possible to check:
> > 
> >   static_cast<void*>(static_cast<EventTarget*>(impl.get())) ==
> > static_cast<void*>(impl.get())
> > 
> > without changing the inheritance order? (Sorry I have no idea since I'm not
a
> > C++ expert:-)
> 
> The issue is basically this one:
> <http://stackoverflow.com/questions/3327631/casting-with-multiple-inheritance>
> 
> I think we should learn a trick from JSC here. Basically in V8 it would look
> like this:
> 
> in V8EventTarget.h, in V8EventTarget (or any base type):
> 
> static inline void* toWrappedType(EventTarget* impl)
> {
>     return impl;
> }
> 
> in V8Element.h, in V8Element (or any derived type):
> 
> static inline void* toWrappedType(Element* impl)
> {
>     return V8Node::toWrappedType(impl);
> }
> 
> And similarly the other way:
> 
> static inline EventTarget* fromWrappedType(void* impl)
> {
>     return static_cast<EventTarget*>(impl);
> }
> 
> static inline Element* fromWrappedType(void* impl)
> {
>     return static_cast<Element*>(V8Node::fromWrappedType(impl));
> }
> 
> Then minimize static casting in the bindings. Most uses can be replaced with
> these. For example the generated toNative uses fromWrappedType.
> 
> IIRC there might be places where the bindings store objects at different
pointer
> types. If they could be brought into a similar scheme that would be good.

Do I understand correctly that toWrappedType() would be used in
V8Element::createWrapper() for e.g.?
i.e.
- v8::Handle<v8::Object> wrapper = V8DOMWrapper::createWrapper(creationContext,
&info, impl.get(), isolate);
+ v8::Handle<v8::Object> wrapper = V8DOMWrapper::createWrapper(creationContext,
&info, toWrappedType(impl.get()), isolate);

[do-not-use, 2013-07-08 15:02:20]

I implemented Dominic's proposal so that we don't have to change the inheritance
order anymore. Dominic, could you confirm that this is what you had in mind?

Try bots are still running but it does not seem to cause regressions locally.

https://codereview.chromium.org/18778002/diff/47001/Source/modules/webaudio/M...
File Source/modules/webaudio/MediaStreamAudioDestinationNode.idl (right):

https://codereview.chromium.org/18778002/diff/47001/Source/modules/webaudio/M...
Source/modules/webaudio/MediaStreamAudioDestinationNode.idl:27: ] interface
MediaStreamAudioDestinationNode : AudioNode {
This change is here so that it compiles until this has landed:
https://codereview.chromium.org/18629003/

[arv (Not doing code reviews), 2013-07-08 15:51:45]

https://codereview.chromium.org/18778002/diff/47001/Source/bindings/scripts/C...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://codereview.chromium.org/18778002/diff/47001/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:757: $header{classPublic}->add("     
  return ${v8ParentClassName}::toWrappedType(impl);\n");
How about storing the return expression in a variable, then you do not need to
split the template.

https://codereview.chromium.org/18778002/diff/47001/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:769: $header{classPublic}->add("     
  return
static_cast<${nativeType}*>(${v8ParentClassName}::fromWrappedType(object));\n");
same here, extract the argument to a variable.

[do-not-use, 2013-07-08 16:07:15]

On 2013/07/08 15:51:45, arv wrote:
>
https://codereview.chromium.org/18778002/diff/47001/Source/bindings/scripts/C...
> File Source/bindings/scripts/CodeGeneratorV8.pm (right):
> 
>
https://codereview.chromium.org/18778002/diff/47001/Source/bindings/scripts/C...
> Source/bindings/scripts/CodeGeneratorV8.pm:757: $header{classPublic}->add("   
 
>   return ${v8ParentClassName}::toWrappedType(impl);\n");
> How about storing the return expression in a variable, then you do not need to
> split the template.
> 
>
https://codereview.chromium.org/18778002/diff/47001/Source/bindings/scripts/C...
> Source/bindings/scripts/CodeGeneratorV8.pm:769: $header{classPublic}->add("   
 
>   return
>
static_cast<${nativeType}*>(${v8ParentClassName}::fromWrappedType(object));\n");
> same here, extract the argument to a variable.

Good idea, done in Patch Set 9.

[haraken, 2013-07-08 23:59:21]

LGTM

toWrappedType/fromWrappedType sounds a bit unclear and confusing to me. It's not
related to a wrapper object nor a WrapperTypeInfo. Maybe
toInternalPointer/fromInternalPointer?

[do-not-use, 2013-07-09 06:32:24]

On 2013/07/08 23:59:21, haraken wrote:
> LGTM
> 
> toWrappedType/fromWrappedType sounds a bit unclear and confusing to me. It's
not
> related to a wrapper object nor a WrapperTypeInfo. Maybe
> toInternalPointer/fromInternalPointer?

Ok, I'll rename and reupload.

Does Dominic have any comment?

[do-not-use, 2013-07-10 06:37:13]

On 2013/07/09 06:32:24, Christophe Dumez wrote:
> On 2013/07/08 23:59:21, haraken wrote:
> > LGTM
> > 
> > toWrappedType/fromWrappedType sounds a bit unclear and confusing to me. It's
> not
> > related to a wrapper object nor a WrapperTypeInfo. Maybe
> > toInternalPointer/fromInternalPointer?
> 
> Ok, I'll rename and reupload.
> 
> Does Dominic have any comment?

Dominic seems to be busy. I will land this patch to unlock my work. We can
address Dominic's comments (if any) post-mortem.

[commit-bot: I haz the power, 2013-07-10 06:40:22]

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

[commit-bot: I haz the power, 2013-07-10 08:02:30]

Change committed as 153867