EventTarget.h shouldn't need to know about every feature and ifdef
Created attachment 112040 [details] work in progress
Created attachment 112059 [details] Patch
Comment on attachment 112059 [details] Patch Attachment 112059 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10203066
Comment on attachment 112059 [details] Patch Attachment 112059 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10203067
Comment on attachment 112059 [details] Patch Attachment 112059 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10202093
Created attachment 112101 [details] Patch
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.
> > 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.
Created attachment 112104 [details] Patch
Comment on attachment 112104 [details] Patch Attachment 112104 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10203305
Comment on attachment 112104 [details] Patch Attachment 112104 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10203306
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.
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.
(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.)
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.
Created attachment 112245 [details] for EWS
Comment on attachment 112245 [details] for EWS Attachment 112245 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10201902
Created attachment 112249 [details] Microbenchmark
Comment on attachment 112249 [details] Microbenchmark Oops. Not for review.
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?
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!
Created attachment 112363 [details] for EWS
Sigh, that didn't help.
Comment on attachment 112363 [details] for EWS Attachment 112363 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10208419
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.
That makes sense. I'll give that a try.
Created attachment 112381 [details] for EWS
Yay! Success.
Committed r98388: <http://trac.webkit.org/changeset/98388>
Sorting the file (with benchmark) are in https://bugs.webkit.org/show_bug.cgi?id=70855