Bug 49469

Summary: ScriptElement rather than HTMLScriptElement and SVGScriptElement should have ScriptElementData
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eric, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch darin: review+

Ryosuke Niwa
Reported 2010-11-12 13:29:58 PST
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.
Attachments
Patch (19.83 KB, patch)
2010-11-12 13:48 PST, Ryosuke Niwa
darin: review+
Ryosuke Niwa
Comment 1 2010-11-12 13:48:27 PST
Darin Adler
Comment 2 2010-11-15 13:53:21 PST
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
Comment 3 2010-11-15 14:11:01 PST
(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
Comment 4 2010-11-15 14:20:11 PST
(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 5 2010-11-15 14:21:46 PST
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
Comment 6 2010-11-15 21:23:49 PST
Note You need to log in before you can comment on or make changes to this bug.