Bug 49705 – [HTML5] Rename member variables of HTMLScriptElement

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

[Ryosuke Niwa]

We should rename member variables of HTMLScriptElement to match the terminology used in HTML5 spec.

[Ryosuke Niwa]

Created attachment 74189 [details]
renamed

[Darin Adler]

Comment on attachment 74189 [details]
renamed

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

> WebCore/dom/ScriptElement.h:92
> +    bool m_insertedByParser;
> +    bool m_extrnalScript;
> +    bool m_alreadyStarted;
>      bool m_firedLoad;

Boolean data members should be named so that they finish a sentence "script element <xxx>". That’s why they often start with something like “was”.

I suggest:

    m_wasInsertedByParser
    m_isExternalScript
    m_wasAlreadyStarted
    m_hasFiredLoadEvent

I also suggest renaming functions, not just the data members.

"extrnalScript" needs an "e".

[Ryosuke Niwa]

Created attachment 74256 [details]
fixed per Darin's comment

[Ryosuke Niwa]

Created attachment 74259 [details]
updated ChangeLog

[Darin Adler]

Comment on attachment 74256 [details]
fixed per Darin's comment

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

> WebCore/dom/ScriptElement.cpp:164
>      // m_createdByParser is never reset - always resied at the initial value set while parsing.
>      // m_evaluated is left untouched as well to avoid script reexecution, if a <script> element

Didn’t we rename m_createdByParser and m_evaluated? Comment should be fixed.

> WebCore/dom/ScriptElement.cpp:178
> -    if (m_isEvaluated || sourceCode.isEmpty() || !shouldExecuteAsJavaScript())
> +    if (wasAlreadyStarted() || sourceCode.isEmpty() || !shouldExecuteAsJavaScript())

Why the change from looking at the data member to calling the function?

> WebCore/dom/ScriptElement.cpp:205
> -    if (m_isEvaluated || sourceCode.isEmpty())
> +    if (wasAlreadyStarted() || sourceCode.isEmpty())

Same question.

[Darin Adler]

Comment on attachment 74259 [details]
updated ChangeLog

Later we could rename the haveFiredLoadEvent function to hasFiredLoadEvent or maybe an even better name.

[Ryosuke Niwa]

(In reply to comment #5)
> > WebCore/dom/ScriptElement.cpp:164
> >      // m_createdByParser is never reset - always resied at the initial value set while parsing.
> >      // m_evaluated is left untouched as well to avoid script reexecution, if a <script> element
> 
> Didn’t we rename m_createdByParser and m_evaluated? Comment should be fixed.

Oops, will do.

> > WebCore/dom/ScriptElement.cpp:178
> > -    if (m_isEvaluated || sourceCode.isEmpty() || !shouldExecuteAsJavaScript())
> > +    if (wasAlreadyStarted() || sourceCode.isEmpty() || !shouldExecuteAsJavaScript())
> 
> Why the change from looking at the data member to calling the function?

> > WebCore/dom/ScriptElement.cpp:205
> > -    if (m_isEvaluated || sourceCode.isEmpty())
> > +    if (wasAlreadyStarted() || sourceCode.isEmpty())
> 
> Same question.

So we're inconsistent in calling accessor vs. accessing member variable directly.  I tried to make this consistent by always calling the accessor but I can do the other way, which is to replace all calls to accessor by accessing member variables directly.

[Ryosuke Niwa]

Created attachment 74263 [details]
fixed comment

[Ryosuke Niwa]

Hi Darin, could you review this patch?

[Ryosuke Niwa]

Thanks for the review!

[Ryosuke Niwa]

Committed r72995: <http://trac.webkit.org/changeset/72995>