[webkit-reviews] review granted: [Bug 48966] Script runs more than once after a clone : [Attachment 73659] Fixed more

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 73659: Fixed more
https://bugs.webkit.org/attachment.cgi?id=73659&action=review

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

> WebCore/dom/Element.cpp:177
> +    // This will catch HTML elements in the wrong namespace that are not
correctly copied.
> +    // This is a sanity check as HTML overloads some of the DOM methods.
> +    ASSERT(isHTMLElement() == clone->isHTMLElement());

I think it would be better to leave the assertion behind in the non-virtual
function rather than moving it into the virtual function.

> WebCore/dom/Element.h:202
> +    virtual PassRefPtr<Element> cloneElementWithoutAttributesAndChildren()
const;

I think this function should be private. Nobody needs to call it except
cloneElementWithoutChildren.

> WebCore/html/HTMLScriptElement.h:75
> +    void executeScript(const ScriptSourceCode& sourceCode);

No need to give the argument a name here.

> WebCore/html/parser/HTMLScriptRunner.cpp:138
> +    ScriptElement* scriptElement = toScriptElement(element.get());
> +    if (scriptElement) {

Could put this declaration inside the if.