Bug 49647 – Merge ScriptElement and ScriptElementData

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

ScriptElementData and ScriptElement should be merged.

[Ryosuke Niwa]

Created attachment 74086 [details]
cleanup

[Darin Adler]

Comment on attachment 74086 [details]
cleanup

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

Good next refactoring step. Might be worth one more pass after this, because some members may no longer be needed.

> WebCore/dom/ScriptElement.cpp:54
> +ScriptElement::ScriptElement(Element* element, bool createdByParser, bool isEvaluated)

It occurs to me that a clearer name for these arguments and data members is “was evaluated” rather than “is evaluated”.

> WebCore/dom/ScriptElement.h:43
>      // A charset for loading the script (may be overridden by HTTP headers or a BOM).
>      String scriptCharset() const;
>      String scriptContent() const;

It’s strange to have a comment about the first function before a paragraph with 5 functions in it, and the comment does not apply to any of these others. Adding a blank line is one way to solve that.

> WebCore/dom/ScriptElement.h:46
>      void executeScript(const ScriptSourceCode&);
> +    void execute(CachedScript*);

It’s unclear why one of these is named executeScript and the other is named execute. What’s the difference between these two?

> WebCore/dom/ScriptElement.h:90
> +    bool m_createdByParser; // HTML5: "parser-inserted"
> +    bool m_requested;
> +    bool m_isEvaluated; // HTML5: "already started"
> +    bool m_firedLoad;

Better names for these data members, that would obviate the need for those HTML5 comments:

    m_wasInsertedByParser
    m_wasRequested (maybe, not sure what the flag means)
    m_wasAlreadyStarted
    m_hasFiredLoadEvent

[Ryosuke Niwa]

Committed r72222: <http://trac.webkit.org/changeset/72222>

[Ryosuke Niwa]

(In reply to comment #2)
> > WebCore/dom/ScriptElement.h:43
> >      // A charset for loading the script (may be overridden by HTTP headers or a BOM).
> >      String scriptCharset() const;
> >      String scriptContent() const;
> 
> It’s strange to have a comment about the first function before a paragraph with 5 functions in it, and the comment does not apply to any of these others. Adding a blank line is one way to solve that.

Fixed.

> > WebCore/dom/ScriptElement.cpp:54
> > +ScriptElement::ScriptElement(Element* element, bool createdByParser, bool isEvaluated)
> 
> It occurs to me that a clearer name for these arguments and data members is “was evaluated” rather than “is evaluated”.

I think we should do all the renames in a separate patch.

> > WebCore/dom/ScriptElement.h:46
> >      void executeScript(const ScriptSourceCode&);
> > +    void execute(CachedScript*);
> 
> It’s unclear why one of these is named executeScript and the other is named execute. What’s the difference between these two?

It's not clear to me either.  There's also evaluateScript and requestScript, which makes the matter worse.  I need to figure out what all of these functions are doing before I can give more appropriate names to them.

> > WebCore/dom/ScriptElement.h:90
> > +    bool m_createdByParser; // HTML5: "parser-inserted"
> > +    bool m_requested;
> > +    bool m_isEvaluated; // HTML5: "already started"
> > +    bool m_firedLoad;
> 
> Better names for these data members, that would obviate the need for those HTML5 comments:
> 
>     m_wasInsertedByParser
>     m_wasRequested (maybe, not sure what the flag means)
>     m_wasAlreadyStarted
>     m_hasFiredLoadEvent

Right, will do in a separate patch.

[Ryosuke Niwa]

Follow up filed as https://bugs.webkit.org/show_bug.cgi?id=49705.