Bug 49578 - ScriptElementData should be a private member of ScriptElement
Summary: ScriptElementData should be a private member of ScriptElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-15 22:14 PST by Ryosuke Niwa
Modified: 2010-11-17 12:22 PST (History)
3 users (show)

See Also:


Attachments
cleanup (10.08 KB, patch)
2010-11-15 22:29 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixed haveFiredLoadEvent (10.09 KB, patch)
2010-11-16 09:45 PST, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2010-11-15 22:14:04 PST
ScriptElementData is a protected membre variable of ScriptElement but it should be a private member variable since HTMLScriptElement and SVGScriptElement only use a small subset of ScriptElementData's methods.
Comment 1 Ryosuke Niwa 2010-11-15 22:29:31 PST
Created attachment 73963 [details]
cleanup
Comment 2 Build Bot 2010-11-16 05:31:42 PST
Attachment 73963 [details] did not build on win:
Build output: http://queues.webkit.org/results/6102015
Comment 3 Ryosuke Niwa 2010-11-16 09:45:17 PST
Created attachment 74005 [details]
fixed haveFiredLoadEvent
Comment 4 Darin Adler 2010-11-16 10:29:00 PST
Comment on attachment 74005 [details]
fixed haveFiredLoadEvent

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

> WebCore/dom/ScriptElement.h:95
>  protected:
> +    bool haveFiredLoadEvent() const { return m_data.haveFiredLoadEvent(); }

Why are any members of ScriptElementData protected? Is there a class that derives from ScriptElementData? If not, then protected and private mean the same thing, and these should be private rather than protected.

> WebCore/html/HTMLScriptElement.h:41
> -    bool haveFiredLoadEvent() const { return m_data.haveFiredLoadEvent(); }
> +    bool haveFiredLoadEvent() const { return ScriptElement::haveFiredLoadEvent(); }

You can accomplish the same thing here by just removing this function altogether.
Comment 5 Ryosuke Niwa 2010-11-16 10:48:35 PST
Comment on attachment 74005 [details]
fixed haveFiredLoadEvent

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

>> WebCore/dom/ScriptElement.h:95
>> +    bool haveFiredLoadEvent() const { return m_data.haveFiredLoadEvent(); }
> 
> Why are any members of ScriptElementData protected? Is there a class that derives from ScriptElementData? If not, then protected and private mean the same thing, and these should be private rather than protected.

On this isn't a part of ScriptElementData.  It's a part of ScriptElement, which is inherited by HTMLScriptElement and SVGScriptElement.

>> WebCore/html/HTMLScriptElement.h:41
>> +    bool haveFiredLoadEvent() const { return ScriptElement::haveFiredLoadEvent(); }
> 
> You can accomplish the same thing here by just removing this function altogether.

SVGScriptElement doesn't define this function so making haveFiredLoadEvent() public in ScriptElement means we're adding haveFiredLoadEvent to SVGScriptElement as well.  If that's desirable, I'll do.
Comment 6 Ryosuke Niwa 2010-11-16 21:56:56 PST
Committed r72172: <http://trac.webkit.org/changeset/72172>
Comment 7 Darin Adler 2010-11-17 12:22:48 PST
(In reply to comment #5)
> >> WebCore/html/HTMLScriptElement.h:41
> >> +    bool haveFiredLoadEvent() const { return ScriptElement::haveFiredLoadEvent(); }
> > 
> > You can accomplish the same thing here by just removing this function altogether.
> 
> SVGScriptElement doesn't define this function so making haveFiredLoadEvent() public in ScriptElement means we're adding haveFiredLoadEvent to SVGScriptElement as well.  If that's desirable, I'll do.

It’s fine to add it to SVGScriptElement.

If we did not want it public in SVGScriptElement, there are two solutions:

    1) Make this protected in ScriptElement, and put "using ScriptElement::haveFiredLoadingEvent" in the public section of HTMLScriptElement.

    2) Make this public in ScriptElement, and put "using ScriptElement::haveFiredLoadingEvent" in the private section of SVGScriptElement.