[webkit-reviews] review granted: [Bug 49647] Merge ScriptElement and ScriptElementData : [Attachment 74086] cleanup

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 49647: Merge ScriptElement and ScriptElementData
https://bugs.webkit.org/show_bug.cgi?id=49647

Attachment 74086: cleanup
https://bugs.webkit.org/attachment.cgi?id=74086&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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