EventTarget.h shouldn't need to know about every feature and ifdef

• Component: UI Events
• Status: RESOLVED FIXED

[Adam Barth]

EventTarget.h shouldn't need to know about every feature and ifdef

[Adam Barth]

Created attachment 112040 [details]
work in progress

[Adam Barth]

Created attachment 112059 [details]
Patch

[Early Warning System Bot]

Comment on attachment 112059 [details]
Patch

Attachment 112059 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10203066

[Gyuyoung Kim]

Comment on attachment 112059 [details]
Patch

Attachment 112059 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10203067

[Gustavo Noronha (kov)]

Comment on attachment 112059 [details]
Patch

Attachment 112059 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10202093

[Adam Barth]

Created attachment 112101 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 112101 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112101&action=review

> Source/WebCore/CMakeLists.txt:2351
> +GENERATE_EVENT_FACTORY(${WEBCORE_DIR}/dom/EventTargetFactory.in EventInterfaces.h)

Not immediately obvious to me why one of these is passed a .cpp file and the other a .h.

> Source/WebCore/dom/EventTarget.h:-109
> -        virtual XMLHttpRequest* toXMLHttpRequest();
> -        virtual XMLHttpRequestUpload* toXMLHttpRequestUpload();
> -        virtual DOMApplicationCache* toDOMApplicationCache();

Can you remove a zillion includes to this file now?  Or certainly class declarations?

> Source/WebCore/dom/make_event_factory.pl:65
> +printFactoryFile("$outputDir/${namespace}Factory.cpp") if $namespace eq "Event";

Feels like a hack.  Why not have a --factory option like the other generator has?

> Source/WebCore/dom/make_event_factory.pl:134
> +    return "EVENT" if $camelCase eq "Event";
> +    return "EVENT_TARGET" if $camelCase eq "EventTarget";

Why not pass this in?  Or at least put this at the very top of the file where other assumptions like this should be made?

> Source/WebCore/dom/make_event_factory.pl:151
> +    print F "#include \"${namespace}Factory.h\"\n";
>      print F "\n";
> -    print F "#include \"EventHeaders.h\"\n";
> +    print F "#include \"${namespace}Headers.h\"\n";

Namespace seems like the wrong word.  Just Name?  Or prefix?

> Source/WebCore/dom/make_event_factory.pl:248
> +    print F "#ifndef ${namespace}Headers_h\n";

Does the other script use namespace for this varaible?  Certainly the make_names script must have something like this.

[Adam Barth]

> > Source/WebCore/CMakeLists.txt:2351
> > +GENERATE_EVENT_FACTORY(${WEBCORE_DIR}/dom/EventTargetFactory.in EventInterfaces.h)
> 
> Not immediately obvious to me why one of these is passed a .cpp file and the other a .h.

The Event one creates a CPP file that backs document.createEvent.  There is not document.createEventTarget, so no CPP file is needed.

> > Source/WebCore/dom/EventTarget.h:-109
> > -        virtual XMLHttpRequest* toXMLHttpRequest();
> > -        virtual XMLHttpRequestUpload* toXMLHttpRequestUpload();
> > -        virtual DOMApplicationCache* toDOMApplicationCache();
> 
> Can you remove a zillion includes to this file now?  Or certainly class declarations?

Yes!  I can also remove a zillion includes from JSEventTarget.cpp.  Will do in a followup patch.

> > Source/WebCore/dom/make_event_factory.pl:65
> > +printFactoryFile("$outputDir/${namespace}Factory.cpp") if $namespace eq "Event";
> 
> Feels like a hack.  Why not have a --factory option like the other generator has?

It is a hack!  We can generalize now, or wait until we actually need the generality.

> > Source/WebCore/dom/make_event_factory.pl:134
> > +    return "EVENT" if $camelCase eq "Event";
> > +    return "EVENT_TARGET" if $camelCase eq "EventTarget";
> 
> Why not pass this in?  Or at least put this at the very top of the file where other assumptions like this should be made?

We could.  I have a dream of actually just doing the case conversion at some point.  It seemed like overkill to do it now though.

> > Source/WebCore/dom/make_event_factory.pl:151
> > +    print F "#include \"${namespace}Factory.h\"\n";
> >      print F "\n";
> > -    print F "#include \"EventHeaders.h\"\n";
> > +    print F "#include \"${namespace}Headers.h\"\n";
> 
> Namespace seems like the wrong word.  Just Name?  Or prefix?

The namespace terminology came from the HTML and SVG "in" files which use HTML and SVG as their namespace, respectively.  I'm happy to rename the variable to whatever you like.

> > Source/WebCore/dom/make_event_factory.pl:248
> > +    print F "#ifndef ${namespace}Headers_h\n";
> 
> Does the other script use namespace for this varaible?  Certainly the make_names script must have something like this.

I'm not sure how make_names create include guards.  I can certainly investigate.  The make_names code generator uses a somewhat different approach than we're using here.  For example, make_names needs to know about both JSC and V8 where as this script doesn't know about either.

[Adam Barth]

Created attachment 112104 [details]
Patch

[Early Warning System Bot]

Comment on attachment 112104 [details]
Patch

Attachment 112104 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10203305

[Gyuyoung Kim]

Comment on attachment 112104 [details]
Patch

Attachment 112104 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10203306

[Darin Adler]

Comment on attachment 112104 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112104&action=review

review+ but note that this broke both Qt and EFL so it could have been a "review- because it breaks the build"

> Source/WebCore/bindings/js/JSEventTarget.cpp:119
> +    AtomicString desiredInterface = target->interfaceName();

This can be const AtomicString& because there is no need to increment/decrement the reference count here.

> Source/WebCore/bindings/js/WorkerScriptController.cpp:89
> -        m_workerContextWrapper.set(*m_globalData, JSDedicatedWorkerContext::create(*m_globalData, structure, m_workerContext->toDedicatedWorkerContext()));
> +        m_workerContextWrapper.set(*m_globalData, JSDedicatedWorkerContext::create(*m_globalData, structure, static_cast<DedicatedWorkerContext*>(m_workerContext)));

Would be nice to have the cast checked in debug builds.

> Source/WebCore/bindings/v8/V8DOMWrapper.cpp:346
> +    AtomicString desiredInterface = target->interfaceName();

Ditto.

> Source/WebCore/dom/EventTargetFactory.in:4
> +namespace="EventTarget"
> +
> +EventSource
> +MessagePort

Since these files are often in alphabetical order, I’d like to see a comment explaining what the order is and why.

[Adam Barth]

Thanks for the review.  I'm make sure to the patch builds before landing it.

> Since these files are often in alphabetical order, I’d like to see a comment explaining what the order is and why.

I think I just used the same order they used to be in EventTarget.h.  I'll alphabetize them.

[Darin Adler]

(In reply to comment #13)
> > Since these files are often in alphabetical order, I’d like to see a comment explaining what the order is and why.
> 
> I think I just used the same order they used to be in EventTarget.h.  I'll alphabetize them.

Could that have any effects? It might have some performance effect, if the common types were at the top of the cascade of ifs before and aren’t any more. I suppose there is no overlap of these interfaces, by definition. (If there was, then we’d need to be careful about specific before general.)

[Adam Barth]

I'll write a microbenchmark to see if I can measure a difference.  I suspect we won't be able to, but it's worth checking.

[Adam Barth]

Created attachment 112245 [details]
for EWS

[Gyuyoung Kim]

Comment on attachment 112245 [details]
for EWS

Attachment 112245 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10201902

[Adam Barth]

Created attachment 112249 [details]
Microbenchmark

[Adam Barth]

Comment on attachment 112249 [details]
Microbenchmark

Oops.  Not for review.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 112245 [details]
for EWS

View in context: https://bugs.webkit.org/attachment.cgi?id=112245&action=review

> Source/WebCore/CMakeLists.txt:2319
> +GENERATE_EVENT_FACTORY(${WEBCORE_DIR}/dom/EventTargetFactory.in EventInterfaces.h)

Shouldn't this be EventTargetInterfaces.h?

[Adam Barth]

Comment on attachment 112245 [details]
for EWS

View in context: https://bugs.webkit.org/attachment.cgi?id=112245&action=review

>> Source/WebCore/CMakeLists.txt:2319
>> +GENERATE_EVENT_FACTORY(${WEBCORE_DIR}/dom/EventTargetFactory.in EventInterfaces.h)
> 
> Shouldn't this be EventTargetInterfaces.h?

You're totally right.  Thanks!

[Adam Barth]

Created attachment 112363 [details]
for EWS

[Adam Barth]

Sigh, that didn't help.

[Gyuyoung Kim]

Comment on attachment 112363 [details]
for EWS

Attachment 112363 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10208419

[Raphael Kubo da Costa (:rakuco)]

Alright, so the macro call in Source/WebCore/CMakeLists.txt called add_custom_output, which generated a make target but nothing depended on it, so the Perl script was never called.

Adding

  ADD_SOURCE_WEBCORE_DERIVED_DEPENDENCIES(${WEBCORE_DIR}/dom/EventNames.cpp EventTargetInterfaces.h)

after the GENERATE_EVENT_FACTORY call should fix the problem.

[Adam Barth]

That makes sense.  I'll give that a try.

[Adam Barth]

Created attachment 112381 [details]
for EWS

[Adam Barth]

Yay!  Success.

[Adam Barth]

Committed r98388: <http://trac.webkit.org/changeset/98388>

[Adam Barth]

Sorting the file (with benchmark) are in https://bugs.webkit.org/show_bug.cgi?id=70855