Event.h shouldn't need to know about every ifdef and feature that uses events
Created attachment 111742 [details] work in progress
Comment on attachment 111742 [details] work in progress View in context: https://bugs.webkit.org/attachment.cgi?id=111742&action=review > Source/WebCore/dom/ErrorEvent.cpp:88 > + return eventNames().interfaceForErrorEvent; I'm confused why you need an interface name which is separate from the event name.
(In reply to comment #2) > (From update of attachment 111742 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111742&action=review > > > Source/WebCore/dom/ErrorEvent.cpp:88 > > + return eventNames().interfaceForErrorEvent; > > I'm confused why you need an interface name which is separate from the event name. I originally rejected that idea because there wasn't a one-to-one mapping between event names and interface names, but I should check again to make sure there isn't a many-to-one relationship, which would also work.
Yeah, the DOM type of the event is different from the interface of the event. Consider, for example, the KeyboardEvent interface. JavaScript can call initKeyboardEvent and set the type to whatever it wants. That means keying the wrapper off the DOM type isn't safe.
Do we really still support CPP bindings?
> Do we really still support CPP bindings? Good question. I can break this patch down into smaller pieces and leave that question for a future patch, or we can go all cowboy and try to do the whole thing all at once.
Created attachment 111859 [details] Patch
Comment on attachment 111859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111859&action=review Looks good to me. I think you should consider changing interfaceName() to something more descriptive. Be it jsInterfaceName() or domInterfaceName(). I also think that it would make more sense to use eventNames().xmlHttpRequestEventDomInterfaceName instead of eventNames(). interfaceForXMLHttpRequestProgressEvent, despite the capitalization issues. We had to handle capitalization (manually) in the ElementFactory code too. > Source/WebCore/dom/Event.h:117 > + // These are needed for the CPP bindings. Do you want a FIXME: here that we can remove most of these?
Created attachment 111868 [details] Patch for landing
Attachment 111868 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/bindings/js/JSEventCustom.cpp:79: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 78 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r98044: <http://trac.webkit.org/changeset/98044>
Realized I haven't replied yet. > I think you should consider changing interfaceName() to something more descriptive. Be it jsInterfaceName() or domInterfaceName(). Yeah, I like domInterfaceName is better than jsInterfaceName because these interface names are used in Obj-C and other languages. I'll do this in a followup patch that removes more of the Boolean virtuals. > I also think that it would make more sense to use eventNames().xmlHttpRequestEventDomInterfaceName instead of eventNames(). interfaceForXMLHttpRequestProgressEvent, despite the capitalization issues. We had to handle capitalization (manually) in the ElementFactory code too. I didn't make this change because it seems like a bunch of extra complexity without much benefit. > > Source/WebCore/dom/Event.h:117 > > + // These are needed for the CPP bindings. > > Do you want a FIXME: here that we can remove most of these? That FIXME is on line 124.
It seems like this patch broke 75+ worker tests: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r98044%20(34066)/results.html
The blame list contains exactly one change set: http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Tests%29/builds/34066
Yep. https://bugs.webkit.org/show_bug.cgi?id=70576 is the bug.