Supporting media-related (e.g. onplay) IDL event attributes

[Philip Jägenstedt]

Hi Blinkers,

In <http://code.google.com/p/chromium/issues/detail?id=302279> I've
had a look at why onplay and friends aren't supported as IDL
properties in Blink, causing e.g. video.onplay = function() { ... }
does not work.

(The oncuechange property is also missing, even though that comes from
the TextTrack interface, not any Element, Document or Window.)

Two relevant WebKit commits are:

https://trac.webkit.org/changeset/44302
https://trac.webkit.org/changeset/44928

Fixing this in Blink is not hard, but before I go ahead I wanted to
ask if there's some history around these properties, since they are
currently commented out in Document.idl and Element.idl, but not in
Window.idl.

Fixing this means duplicating the lists in several IDL files and
header files, is that OK or do you want to move towards something like
the GlobalEventHandlers interface in
<http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#globaleventhandlers>?

Philip

[Adam Barth]

IMHO, GlobalEventHandlers is better than copy/pasting IDL.  Our code generator should be smart enough to handle that sort of thing these days.

Adam

[Philip Jägenstedt]

I've had a look at adding a GlobalEventHandlers.idl which could then
be brought into Document, Element and Window using a "Document
implements GlobalEventHandlers" statement or similar.

Most of the scaffolding for legacy onevent properties is on
EventTarget, but GlobalEventHandlers can't inherit from EventTarget
since most interfaces that inherit EventTarget shouldn't have these
properties. The second most obvious thing would be to give
GlobalEventHandlers a pointer to an EventTarget and forward
everything, but that would add an extra pointer to all Documents,
Elements and Windows, which would be pretty bad.

Are there more advanced features of WebIDL and/or the IDL generator
which would allow block of IDL to be shared between several
interfaces, interfaces which don't share any common ancestor
interfaces at a level where it's helpful? It would of course be nice
if the C++ bits could be shared as well, but sharing only the IDL
would still cause the build to fail when things get out of sync, which
would be an improvement.

By the way, the spec says that these properties should be on "HTML
elements", but its definition doesn't make it clear (to me) if that
implies Element or HTMLElement. Is Element where this stuff belongs?

Philip

[Nils Barth]

Philip Jägenstedt:

Are there more advanced features of WebIDL and/or the IDL generator

which would allow block of IDL to be shared between several
interfaces, interfaces which don't share any common ancestor
interfaces at a level where it's helpful?

There are implements statements; if you have interface X that you want to share between interfaces A, B, and C, you can have:

A implements X;

B implements X;

C implements X;

...and then they'll share X without inheriting from it.

This is similar to inheritance, but isn't formally the same – Web IDL is now single inheritance.

Perhaps this is what you're looking for?

[Philip Jägenstedt]

I mentioned "Document implements GlobalEventHandlers" in my previous
post, but I'm not sure if it can be made to work. Simply adding
GlobalEventHandlers.idl will require there to be a
GlobalEventHandlers.h for the V8 bindings to be generated, and that in
turn will require the properties declared in the IDL to actually be in
the header file. Here are the relevant bits of my experiment:

GlobalEventHandlers.idl:

[
NoInterfaceObject
] interface GlobalEventHandlers {
[NotEnumerable, PerWorldBindings] attribute EventHandler onplay;
};

GlobalEventHandlers.h:

class GlobalEventHandlers {
public:
DEFINE_ATTRIBUTE_EVENT_LISTENER(play);
};

(It doesn't compile.)

DEFINE_ATTRIBUTE_EVENT_LISTENER is from EventTarget.h, but it depends
on get/setAttributeEventListener from that same class. It doesn't
appear possible to just mix in the IDL using "Document implements
GlobalEventHandlers" without also having a C++ GlobalEventHandlers
that matches the interface.

Perhaps everything needed to handle legacy event attributes and
properties could be moved to a separate (internal) interface which
GlobalEventHandlers and every other interface requiring these could
use via "X implements InternalThingy" in IDL and plain inheritance in
C++? However, if making this work is really simple it seems like it
should already have happened...

Philip

[Adam Barth]

WindowTimers might be a good example to look at.  Notice that it takes the context object with the CPP type EventTarget*.  Given that you're only going to apply GlobalEventHandlers to objects that inherit from EventTarget, it's likely you can use a similar approach.  If you need more structure from your context object, there might be another base class that's appropriate, or we can invent a new base class.  You might even be able to use C++ overloading to handle different C++ types using different code in GlobalEventHandlers.h.

DEFINE_ATTRIBUTE_EVENT_LISTENER is from EventTarget.h, but it depends
on get/setAttributeEventListener from that same class. It doesn't
appear possible to just mix in the IDL using "Document implements
GlobalEventHandlers" without also having a C++ GlobalEventHandlers
that matches the interface.

Perhaps everything needed to handle legacy event attributes and
properties could be moved to a separate (internal) interface which
GlobalEventHandlers and every other interface requiring these could
use via "X implements InternalThingy" in IDL and plain inheritance in
C++? However, if making this work is really simple it seems like it
should already have happened...

I wouldn't make that assumption.  We only recently added support for "implements" to the IDL system.  Previously the only option was copy and pasting code.

Adam

[Philip Jägenstedt]

Thanks Adam, WindowTimers was indeed helpful. I haven't looked at the
source for the code generator, but it seems that when the
corresponding header file (DOMWindowTimers.h in this case) declares
the functions in a namespace and not in a class, the first argument
will be the context object, which indeed ought to be something that
inherits from EventTarget in all three cases. That was encouraging,
and it looked like I could share a lot of the IDL...

However, perhaps I should have started this by actually comparing the
existing Document.idl, Element.idl and Window.idl. One need not look
further than onabort before discovering that all three are different:

Document.idl:
[NotEnumerable] attribute EventHandler onabort;

Element.idl:
[NotEnumerable, PerWorldBindings] attribute EventHandler onabort;

Window.idl:
attribute EventHandler onabort;

At a glance, I found no attribute where the IDL was actually the same
for all three interfaces.

The spec doesn't say anything about this, so I suppose that it thinks
that the default visibility (whichever it is) should be used
everywhere. Here's a demo that shows that there are indeed a bunch of
enumerable onevent properties on Window but not on Document or
Element:

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/2552

In Gecko, a lot of onevent properties are actually visible on all
three interfaces.

Changing enumerability has an obvious compat risk, might someone know
why it's currently different? Would aligning with Gecko be a good idea
here, or is there a cost (other than the compat risk) to having these
properties enumerable on Document and Element?

Philip

[Adam Barth]

The code generator doesn't know whether the header file uses a class or a namespace.  It always calls a static function and passes the unwrapped receiver as the first parameter.

However, perhaps I should have started this by actually comparing the
existing Document.idl, Element.idl and Window.idl. One need not look
further than onabort before discovering that all three are different:

Document.idl:
[NotEnumerable] attribute EventHandler onabort;

Element.idl:
[NotEnumerable, PerWorldBindings] attribute EventHandler onabort;

Window.idl:
attribute EventHandler onabort;

At a glance, I found no attribute where the IDL was actually the same
for all three interfaces.

What does the spec say?  Is this property supposed to be enumerable in Window.idl, or is that a bug from our copy/pasta?

I'm pretty sure we can drop PerWorldBindings in this case.  That's really only needed on extremely hot APIs.  We could research when it was added to Element.idl to understand why it was put there.

In these sorts of refactoring projects, it's not uncommon to find a bunch of hairballs.  In SF, we have a Yak Shave statue for folks who clean up these sorts of issues as they work on the code.

http://en.wiktionary.org/wiki/yak_shaving

The spec doesn't say anything about this, so I suppose that it thinks
that the default visibility (whichever it is) should be used
everywhere. Here's a demo that shows that there are indeed a bunch of
enumerable onevent properties on Window but not on Document or
Element:

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/2552

In Gecko, a lot of onevent properties are actually visible on all
three interfaces.

If the spec doesn't have the NotEnumerable attribute, that means the property is enumerable.  If you research other user agents and find that they're consistent in not enumerating various properties, we should change the specs to match reality.

 

Changing enumerability has an obvious compat risk, might someone know
why it's currently different? Would aligning with Gecko be a good idea
here, or is there a cost (other than the compat risk) to having these
properties enumerable on Document and Element?

The ideal end state is for all browsers to behave the same way and for the specs to describe that behavior.  If Firefox is consistent with the spec, it's clear that we should move to match Firefox and the spec.  If browsers are consistent with each other but the spec says something different, we should probably change the spec.  If everyone is all over the map, then we should talk it out with folks in the standards world and come to some consensus.

Adam

[Philip Jägenstedt]

On Mon, Oct 7, 2013 at 10:01 PM, Adam Barth <aba...@chromium.org> wrote:
> On Mon, Oct 7, 2013 at 6:20 AM, Philip Jägenstedt <phi...@opera.com> wrote:
>>
>> However, perhaps I should have started this by actually comparing the
>> existing Document.idl, Element.idl and Window.idl. One need not look
>> further than onabort before discovering that all three are different:
>>
>> Document.idl:
>> [NotEnumerable] attribute EventHandler onabort;
>>
>> Element.idl:
>> [NotEnumerable, PerWorldBindings] attribute EventHandler onabort;
>>
>> Window.idl:
>> attribute EventHandler onabort;
>>
>> At a glance, I found no attribute where the IDL was actually the same
>> for all three interfaces.
>
>
> What does the spec say? Is this property supposed to be enumerable in
> Window.idl, or is that a bug from our copy/pasta?

The spec simply says "attribute EventHandler onabort;" I assume this
means the attribute should be enumerable, on all three implementing
interfaces.

Note also that the spec says "HTMLElement implements
GlobalEventHandlers" whereas Blink puts all of these properties on
Element. I wouldn't mind attempting this move, but if it turns out to
not be compatible with Web content, the spec(s) will have to change.

> I'm pretty sure we can drop PerWorldBindings in this case. That's really
> only needed on extremely hot APIs. We could research when it was added to
> Element.idl to understand why it was put there.

I'll investigate, it seems like a simple first step to reduce the
number of differences.

> In these sorts of refactoring projects, it's not uncommon to find a bunch of
> hairballs. In SF, we have a Yak Shave statue for folks who clean up these
> sorts of issues as they work on the code.
>
> http://en.wiktionary.org/wiki/yak_shaving
>
>> The spec doesn't say anything about this, so I suppose that it thinks
>> that the default visibility (whichever it is) should be used
>> everywhere. Here's a demo that shows that there are indeed a bunch of
>> enumerable onevent properties on Window but not on Document or
>> Element:
>>
>> http://software.hixie.ch/utilities/js/live-dom-viewer/saved/2552
>>
>> In Gecko, a lot of onevent properties are actually visible on all
>> three interfaces.
>
>
> If the spec doesn't have the NotEnumerable attribute, that means the
> property is enumerable. If you research other user agents and find that
> they're consistent in not enumerating various properties, we should change
> the specs to match reality.

I just tested Presto and it turns out we (Opera) enumerated these
properties on document and element but not on window, i.e. precisely
the opposite of what Blink does. I haven't tested IE.

To me, it seems simplest to just let the attributes be enumerable
everywhere, especially since Gecko seems able to get away with that.

I've created http://code.google.com/p/chromium/issues/detail?id=305112
to work towards getting in sync with the spec, which may very well
require changing the spec on some points.

Philip

[Philip Jägenstedt]

On Tue, Oct 8, 2013 at 1:54 PM, Philip Jägenstedt <phi...@opera.com> wrote:
> On Mon, Oct 7, 2013 at 10:01 PM, Adam Barth <aba...@chromium.org> wrote:
>> On Mon, Oct 7, 2013 at 6:20 AM, Philip Jägenstedt <phi...@opera.com> wrote:
>>>
>>> However, perhaps I should have started this by actually comparing the
>>> existing Document.idl, Element.idl and Window.idl. One need not look
>>> further than onabort before discovering that all three are different:
>>>
>>> Document.idl:
>>> [NotEnumerable] attribute EventHandler onabort;
>>>
>>> Element.idl:
>>> [NotEnumerable, PerWorldBindings] attribute EventHandler onabort;
>>>
>>> Window.idl:
>>> attribute EventHandler onabort;
>>>

>> I'm pretty sure we can drop PerWorldBindings in this case. That's really
>> only needed on extremely hot APIs. We could research when it was added to
>> Element.idl to understand why it was put there.
>
> I'll investigate, it seems like a simple first step to reduce the
> number of differences.

V8PerWorldBindings was added to Element.idl by Marja in r146259
<http://src.chromium.org/viewvc/blink?view=revision&revision=146259>.

It was added to Document.idl and Window.idl without fanfare in r154552
<http://src.chromium.org/viewvc/blink?view=revision&revision=154552>.

Assuming we can remove the NotEnumerable and PerWorldBindings extended
attributes, it looks like the lists could be reduced to
[ActivityLog=Setter] for those touched by r146259 and nothing special
at all for the rest.

Philip

[Adam Barth]

Sounds great.  Thanks!