Summary: | [WebIDL] Add support for [ImplementedAs] for EventHandler attributes | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||
Component: | Bindings | Assignee: | Chris Dumez <cdumez> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | cgarcia, cmarcelo, commit-queue, darin, esprehn+autocc, ggaren, kangil.han, kondapallykalyan, rniwa, sam | ||||
Priority: | P2 | ||||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Chris Dumez
2016-04-08 16:01:35 PDT
Created attachment 276049 [details]
Patch
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.
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? (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. I agree with Chris here. Since onshow is a replacement for ondisplay, it's more consistent for it to start with "on". 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. Maybe we shouldn’t use ImplementedAs for this. Attribute could be named EventType perhaps? (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. I think it’s strange to write "onshow" when there is no "onshow", just "show". 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. (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(). 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). (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. 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. 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. Comment on attachment 276049 [details] Patch Clearing flags on attachment: 276049 Committed r199316: <http://trac.webkit.org/changeset/199316> All reviewed patches have been landed. Closing bug. |