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.
Created attachment 73773 [details] Patch
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?
(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.
(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.
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.
Committed r72060: <http://trac.webkit.org/changeset/72060>