Bug 156421 – [WebIDL] Add support for [ImplementedAs] for EventHandler attributes

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

Add support for [ImplementedAs] for EventHandler attributes so we can get rid of some ugly name hardcoding in the bindings generator.

[Chris Dumez]

Created attachment 276049 [details]
Patch

[WebKit Commit Bot]

Attachment 276049 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1502:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2747:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 276049 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276049&action=review

> Source/WebCore/Modules/notifications/Notification.idl:49
> +    [ImplementedAs=onshow] attribute EventHandler ondisplay;

Why include "on" in the ImplementedAs name? Can’t this just be [ImplementedAs=show] instead?

[Chris Dumez]

(In reply to comment #3)
> Comment on attachment 276049 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276049&action=review
> 
> > Source/WebCore/Modules/notifications/Notification.idl:49
> > +    [ImplementedAs=onshow] attribute EventHandler ondisplay;
> 
> Why include "on" in the ImplementedAs name? Can’t this just be
> [ImplementedAs=show] instead?

While this is shorter, I would personally find it a bit confusing. EventHandlers always start with "on" prefix. [ImplementedAs] is used to provide the alternative name for the attribute. Here the alternative name for the attribute is 'onshow' for the attribute 'ondisplay'. 'show' would be the name of the Event we deduce from the name of the event handler attribute.

I would prefer landing the patch as is unless you feel strongly about this.

[Ryosuke Niwa]

I agree with Chris here. Since onshow is a replacement for ondisplay, it's more consistent for it to start with "on".

[Darin Adler]

I don’t agree with you guys.

There is no "onshow" attribute nor an "onshow" event type. So I don’t see why we would ever write "onshow".

The attribute is named "ondisplay" and the event type it sets up a handler for is "display". The new attribute is telling the code generator that the "ondisplay" attribute instead should set up an event handler for the event type "show".

There’s no "onshow" involved at all.

[Darin Adler]

Maybe we shouldn’t use ImplementedAs for this.

[Darin Adler]

Attribute could be named EventType perhaps?

[Chris Dumez]

(In reply to comment #7)
> Maybe we shouldn’t use ImplementedAs for this.

I did not want to add yet another WebKit-specific IDL extended attribute just for this. I thought that ImplementedAs suited pretty well for this.

[Darin Adler]

I think it’s strange to write "onshow" when there is no "onshow", just "show".

[Chris Dumez]

ImplementedAs as nothing to do with IDL really. It merely tells that we are using a different name for this on the native side.

In general, when we use ImplementedAs on an attribute, it means that the implementation Setter/Getter uses a different naming, which is not based on the attribute name in the IDL.

[ImplementedAs=bar] attribute DOMString foo;

We deduce that we need to call bar() / setBar() on native side.

Then for EventHandler attributes, there is no corresponding getter / setter. The native getter / setter is generic and relies on an event name that is based on the IDL attribute name.

[ImplementedAs=onbar] attribute DOMString onfoo;

We deduce that we need to call eventHandlerAttribute(element, eventNames().barEvent) / setEventHandlerAttribute(*state, jsElement, element, eventNames().barEvent, value) on native side.

I personally think it does make sense.

[Chris Dumez]

(In reply to comment #10)
> I think it’s strange to write "onshow" when there is no "onshow", just
> "show".

[ImplementedAs=bar] attribute DOMString foo;

There is not 'bar' either. We just deduce that we need to call getters/setters based on a different name 'bar', which ends up resolving to bar() / setBar().

[Chris Dumez]

I suppose we could update the bindings generator so that it would allow both 'bar' and 'onbar' in [ImplementedAs] in the case of EventHandlers (i.e. drop the 'on' prefix to deduce the event name, if there is such prefix).

[Chris Dumez]

(In reply to comment #10)
> I think it’s strange to write "onshow" when there is no "onshow", just
> "show".

"show" would be the name of the event fired on JS side. But what the bindings generator needs to generate is actually eventNames().showEvent. The bindings generator is not really aware of anything named "show" AFAICT.

[Darin Adler]

OK, but eventNames().showEvent is just the string "show", and is defined to be that. We use eventNames() for efficiency, there’s no layer of abstraction there.

[Darin Adler]

I can live with this, since you feel strongly about it.

I have long been frustrated and felt confused when people talk about things like "the 'onload' event", which does not exist. I seem to remember that I added support for EventHandler and I had this very thing in mind and tried hard to not conflate the names of the event handler attributes with the names of the event they are intended for.

[WebKit Commit Bot]

Comment on attachment 276049 [details]
Patch

Clearing flags on attachment: 276049

Committed r199316: <http://trac.webkit.org/changeset/199316>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.