Bug 49469 – ScriptElement rather than HTMLScriptElement and SVGScriptElement should have ScriptElementData

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

HTMLScriptElement and SVGScriptElement have a lot of duplicated code because each class declares ScriptElementData as a member variable m_data separately.
We should declare m_data in ScriptElement and share the duplicated code.

[Ryosuke Niwa]

Created attachment 73773 [details]
Patch

[Darin Adler]

Comment on attachment 73773 [details]
Patch

I don’t understand why ScriptElement and ScriptElementData are two separate classes. Could you instead merge them into one class?

[Ryosuke Niwa]

(In reply to comment #2)
> (From update of attachment 73773 [details])
> I don’t understand why ScriptElement and ScriptElementData are two separate classes. Could you instead merge them into one class?

We could do that.  However, I rather merge ScriptElementData with PendingScript in HTMLScriptRunner.cpp than with ScriptElement because ScriptElementData and PendingScript have similar abstractions and provide similar functionalities.

Regardless, moving m_data to ScriptElement and making it private (done in a separate patch) should be a good thing since there are several places in HTMLScriptElement and SVGScriptElement that directly access m_data.

[Darin Adler]

(In reply to comment #3)
> However, I rather merge ScriptElementData with PendingScript in HTMLScriptRunner.cpp than with ScriptElement because ScriptElementData and PendingScript have similar abstractions and provide similar functionalities.

After this patch, a ScriptElementData has all the data in a ScriptElement object. It’s bizarre to have all the data members of one class stored in an object of another class that’s not used elsewhere.

And there are a substantial number of functions in ScriptElement that just call through to a function in ScriptElementData of the same name.

Factoring to share more code with PendingScript is OK, but probably not nearly as important or helpful. I see quite a bit in ScriptElementData and PendingScript that does not overlap.

[Darin Adler]

Comment on attachment 73773 [details]
Patch

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

> WebCore/dom/ScriptElement.h:79
> +    : m_data(this, element, isEvaluated)

Formatting is wrong here. This should be indented.

> WebCore/dom/ScriptElement.h:101
> +    virtual String sourceAttributeValue() const = 0;
> +    virtual String charsetAttributeValue() const = 0;
> +    virtual String typeAttributeValue() const = 0;
> +    virtual String languageAttributeValue() const = 0;
> +    virtual String forAttributeValue() const = 0;
> +    virtual String eventAttributeValue() const = 0;
> +    virtual bool asyncAttributeValue() const = 0;
> +    virtual bool deferAttributeValue() const = 0;
> +
> +    virtual void dispatchLoadEvent() = 0;
> +    virtual void dispatchErrorEvent() = 0;

I think these should all be private. We can refactor to accomplish this in a later patch.

[Ryosuke Niwa]

Committed r72060: <http://trac.webkit.org/changeset/72060>