Bug 49578 – ScriptElementData should be a private member of ScriptElement

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 73963 [details]
cleanup

[Build Bot]

Attachment 73963 [details] did not build on win:
Build output: http://queues.webkit.org/results/6102015

[Ryosuke Niwa]

Created attachment 74005 [details]
fixed haveFiredLoadEvent

[Darin Adler]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Committed r72172: <http://trac.webkit.org/changeset/72172>

[Darin Adler]

(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.