[webkit-reviews] review denied: [Bug 48966] Script runs more than once after a clone : [Attachment 73631] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 48966: Script runs more than once after a clone
https://bugs.webkit.org/show_bug.cgi?id=48966

Attachment 73631: Patch
https://bugs.webkit.org/attachment.cgi?id=73631&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=73631&action=review

Looks like you are on the right track, but I’d like you to consider some
changes.

> WebCore/dom/Element.cpp:160
> +// This function needs to be synced with HTMLScriptElement and
SVGScriptElement

It’s not obvious to me what “synced with” means here. Do we really need a copy
of the function? It’s really unfortunate to have a copied and pasted copy of
this function body. Can’t we call the base class version of 
cloneElementWithoutChildren and then set the evaluated flag after the fact?

Or, since we have to add virtual function overhead, how about making a new
small function that covers just the part of the operation that has to be
different, rather than making cloneElementWithoutChildren itself virtual? That
would resolve the issue above too.

> WebCore/dom/ScriptElement.cpp:369
> +    ASSERT(element);
> +    if (element->isHTMLElement() &&
element->hasTagName(HTMLNames::scriptTag))
> +	  
static_cast<HTMLScriptElement*>(element)->m_data.executeScript(sourceCode);
> +#if ENABLE(SVG)
> +    else if (element->isSVGElement() &&
element->hasTagName(SVGNames::scriptTag))
> +	  
static_cast<SVGScriptElement*>(element)->m_data.executeScript(sourceCode);
> +#endif

The isHTMLElement and isSVGElement checks here are not needed for correctness.
Are you doing them for performance reasons?

Can we do this in a way that uses a virtual function in ScriptElement or
perhaps even in Element for this rather than hasTagName checks for the two
specific derived classes? It’s not good encapsulation for ScriptElement to have
to know about all classes derived from it, so if we can avoid it that would be
better.

> WebCore/dom/ScriptElement.h:81
> +    bool isEvaluated() const { return m_evaluated; }

Might be worthwhile to rename the data member too to add the “is”

> WebCore/dom/ScriptElement.h:91
> +    void executeScript(const ScriptSourceCode& sourceCode);

No need for the argument name here.