Bug 70483 – Event.h shouldn't need to know about every ifdef and feature that uses events

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Event.h shouldn't need to know about every ifdef and feature that uses events

[Adam Barth]

Created attachment 111742 [details]
work in progress

[Eric Seidel (no email)]

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.

[Adam Barth]

(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.

[Adam Barth]

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.

[Sam Weinig]

Do we really still support CPP bindings?

[Adam Barth]

> 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.

[Adam Barth]

Created attachment 111859 [details]
Patch

[Eric Seidel (no email)]

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?

[Adam Barth]

Created attachment 111868 [details]
Patch for landing

[WebKit Review Bot]

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.

[Adam Barth]

Committed r98044: <http://trac.webkit.org/changeset/98044>

[Adam Barth]

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.

[Ryosuke Niwa]

It seems like this patch broke 75+ worker tests:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r98044%20(34066)/results.html

[Ryosuke Niwa]

The blame list contains exactly one change set:
http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Tests%29/builds/34066

[Adam Barth]

Yep.  https://bugs.webkit.org/show_bug.cgi?id=70576 is the bug.