Bug 49703 – HTMLScriptRunner and ScriptElement duplicate code

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

HTMLScriptRunner and ScriptElement implement very similar features.  We should try to share code between the two as much as possible.

[Ryosuke Niwa]

Created attachment 74787 [details]
work in progress

[Darin Adler]

Comment on attachment 74787 [details]
work in progress

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

> WebCore/dom/ScriptElement.h:48
> +    void evaluateScriptWithEvents(CachedScript*);
> +    void executeScriptWithEvents(const ScriptSourceCode&, bool errorOccurred);

I think these both can be called executeScript, no need to call one evaluate and one execute. C++ overloading will work fine.

The use of boolean for “errorOccurred” is strange, though. A boolean that when true means, “no don’t execute the script at all, just send an error event”, is not good design. Two separate functions would be better even though it’s slightly more code at the call site.

[Ryosuke Niwa]

Thanks for the feedback, Darin!

(In reply to comment #2)
> > WebCore/dom/ScriptElement.h:48
> > +    void evaluateScriptWithEvents(CachedScript*);
> > +    void executeScriptWithEvents(const ScriptSourceCode&, bool errorOccurred);
> 
> I think these both can be called executeScript, no need to call one evaluate and one execute. C++ overloading will work fine.

It turned out that ScriptController's evaluate and execute functions differ slightly.  They have slightly different rules on how to choose DOMWrapperWorld, and while execute may trigger layout, evaluate never does.  I don't like this ever-slight difference between the two and the fact names "evaluate" and "execute" do not convey any of those differences.  But we should probably resolve that in a separate patch.

> The use of boolean for “errorOccurred” is strange, though. A boolean that when true means, “no don’t execute the script at all, just send an error event”, is not good design. Two separate functions would be better even though it’s slightly more code at the call site.

This is to do with what sourceFromPendingScript does and releaseElementAndClear:
http://trac.webkit.org/browser/trunk/WebCore/html/parser/HTMLScriptRunner.cpp
128	    bool errorOccurred = false;
129	    ScriptSourceCode sourceCode = sourceFromPendingScript(pendingScript, errorOccurred);
130	
131	    // Stop watching loads before executeScript to prevent recursion if the script reloads itself.
132	    if (pendingScript.cachedScript() && pendingScript.watchingForLoad())
133	        stopWatchingForLoad(pendingScript);
134	
135	    // Clear the pending script before possible rentrancy from executeScript()
136	    RefPtr<Element> element = pendingScript.releaseElementAndClear();
137	    if (ScriptElement* scriptElement = toScriptElement(element.get())) {
138	        NestingLevelIncrementer nestingLevelIncrementer(m_scriptNestingLevel);
139	        IgnoreDestructiveWriteCountIncrementer ignoreDestructiveWriteCountIncrementer(m_document);
140	        if (errorOccurred)
141	            element->dispatchEvent(createScriptErrorEvent());
142	        else {
143	            ASSERT(isExecutingScript());
144	            scriptElement->executeScript(sourceCode);
145	            element->dispatchEvent(createScriptLoadEvent());
146	        }
147	    }
148	    ASSERT(!m_scriptNestingLevel);

We need to call releaseElementAndClear in order to avoid running the same script twice but it'll clear the script cache (removes itself from the resource loader) so we need to obtain the SourceCode before calling this function.  But then we can't pass CachedScript to executeScriptWithEvents as in evaluateScriptWithEvents.

I need to spend some time thinking about how to clean up all of these dependencies and legacy structures.

[Ryosuke Niwa]

http://trac.webkit.org/changeset/79114 fixed this.