Bug 70483 - Event.h shouldn't need to know about every ifdef and feature that uses events
Summary: Event.h shouldn't need to know about every ifdef and feature that uses events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-20 01:49 PDT by Adam Barth
Modified: 2011-10-20 21:46 PDT (History)
6 users (show)

See Also:


Attachments
work in progress (76.15 KB, patch)
2011-10-20 01:50 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (80.71 KB, patch)
2011-10-20 15:38 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch for landing (78.84 KB, patch)
2011-10-20 16:27 PDT, Adam Barth
abarth: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.