Bug 70483

Summary: Event.h shouldn't need to know about every ifdef and feature that uses events
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, japhet, rniwa, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
work in progress
none
Patch
none
Patch for landing abarth: commit-queue+

Description Adam Barth 2011-10-20 01:49:10 PDT 
Event.h shouldn't need to know about every ifdef and feature that uses events
Comment 1 Adam Barth 2011-10-20 01:50:30 PDT 
Created attachment 111742 [details]
work in progress
Comment 2 Eric Seidel (no email) 2011-10-20 02:03:23 PDT 
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.
Comment 3 Adam Barth 2011-10-20 10:20:23 PDT 
(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.
Comment 4 Adam Barth 2011-10-20 10:25:03 PDT 
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.
Comment 5 Sam Weinig 2011-10-20 13:04:45 PDT 
Do we really still support CPP bindings?
Comment 6 Adam Barth 2011-10-20 13:07:00 PDT 
> 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.
Comment 7 Adam Barth 2011-10-20 15:38:38 PDT 
Created attachment 111859 [details]
Patch
Comment 8 Eric Seidel (no email) 2011-10-20 15:49:12 PDT 
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?
Comment 9 Adam Barth 2011-10-20 16:27:21 PDT 
Created attachment 111868 [details]
Patch for landing
Comment 10 WebKit Review Bot 2011-10-20 16:36:13 PDT 
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.
Comment 11 Adam Barth 2011-10-20 16:59:37 PDT 
Committed r98044: <http://trac.webkit.org/changeset/98044>
Comment 12 Adam Barth 2011-10-20 17:04:16 PDT 
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.
Comment 13 Ryosuke Niwa 2011-10-20 21:32:58 PDT 
It seems like this patch broke 75+ worker tests:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r98044%20(34066)/results.html
Comment 14 Ryosuke Niwa 2011-10-20 21:33:14 PDT 
The blame list contains exactly one change set:
http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Tests%29/builds/34066
Comment 15 Adam Barth 2011-10-20 21:46:24 PDT 
Yep.  https://bugs.webkit.org/show_bug.cgi?id=70576 is the bug.