Bug 54676 – Make ScriptElement match the HTML5 spec

• Component: New Bugs
• Status: RESOLVED FIXED

[James Simonsen]

Make ScriptElement match the HTML5 spec

[James Simonsen]

Created attachment 82837 [details]
Patch

[James Simonsen]

As requested offline, this gets us to match up with the spec. It'll make it much easier to implement bug 50115 so that it matches the spec.

[Tony Gentilcore]

James and I talked offline about making sure to separate refactoring from functionality changes, so I expect a new patch is coming, but I'll still take a first pass over this one.

[James Simonsen]

Created attachment 82839 [details]
Patch

[James Simonsen]

Created attachment 82843 [details]
Patch

[Adam Barth]

Comment on attachment 82843 [details]
Patch

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

Some random notes below.

> Source/WebCore/dom/ScriptElement.cpp:103
> +    /*

We usually use C++ type comments.

> Source/WebCore/dom/ScriptElement.cpp:149
> +    // FIXME: isLegacySupportedJavaScriptLanguage() is not valid HTML5. It is used here to maintain backwards compatibility with existing layout tests.

Compatibility with layout tests?  Maybe we should change the spec to match our list of mime types?  I don't know the history here.

> Source/WebCore/dom/ScriptElement.cpp:161
> +        m_wasParserInserted = true;
> +        m_parserInserted = false;

Woah, there's wasParserInserted, which is different than parserInserted?

> Source/WebCore/dom/ScriptElement.cpp:181
> +    // FIXME: Eventually we'd like to evaluate scripts which are inserted into a
>      // viewless document but this'll do for now.
>      // See http://bugs.webkit.org/show_bug.cgi?id=5727

Running scripts in viewless documents is pretty scary...  I'm not sure we want to do that, but that's not really related to this patch.

> Source/WebCore/dom/ScriptElement.cpp:197
> +        eventAttribute = eventAttribute.stripWhiteSpace();
> +        if (!equalIgnoringCase(eventAttribute, "onload") && !equalIgnoringCase(eventAttribute, "onload()"))
> +            return false;

Do we have tests for these cases?

[James Simonsen]

(In reply to comment #6)
> (From update of attachment 82843 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82843&action=review
> 
> Some random notes below.
> 
> > Source/WebCore/dom/ScriptElement.cpp:103
> > +    /*
> 
> We usually use C++ type comments.

Done.

> > Source/WebCore/dom/ScriptElement.cpp:149
> > +    // FIXME: isLegacySupportedJavaScriptLanguage() is not valid HTML5. It is used here to maintain backwards compatibility with existing layout tests.
> 
> Compatibility with layout tests?  Maybe we should change the spec to match our list of mime types?  I don't know the history here.

Updated comment to explain the situation. The spec doesn't care about specific MIME types, just that we use them consistently.

> > Source/WebCore/dom/ScriptElement.cpp:161
> > +        m_wasParserInserted = true;
> > +        m_parserInserted = false;
> 
> Woah, there's wasParserInserted, which is different than parserInserted?

Yep.

> > Source/WebCore/dom/ScriptElement.cpp:181
> > +    // FIXME: Eventually we'd like to evaluate scripts which are inserted into a
> >      // viewless document but this'll do for now.
> >      // See http://bugs.webkit.org/show_bug.cgi?id=5727
> 
> Running scripts in viewless documents is pretty scary...  I'm not sure we want to do that, but that's not really related to this patch.

Yeah, I just moved that block of code without changing it.

> > Source/WebCore/dom/ScriptElement.cpp:197
> > +        eventAttribute = eventAttribute.stripWhiteSpace();
> > +        if (!equalIgnoringCase(eventAttribute, "onload") && !equalIgnoringCase(eventAttribute, "onload()"))
> > +            return false;
> 
> Do we have tests for these cases?

Yep. fast/dom/HTMLScriptElement/script-for-attribute-unexpected-execution.html

[James Simonsen]

Created attachment 82855 [details]
Patch

[Tony Gentilcore]

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

> Source/WebCore/dom/ScriptElement.cpp:60
>      , m_wasAlreadyStarted(wasAlreadyStarted)

Throughout the spec it refers to "already started" not "was already started." Should this be m_alreadyStarted?

> Source/WebCore/dom/ScriptElement.cpp:133
> +bool ScriptElement::isScriptTypeSupported(LegacyTypeSupport supportLegacyTypes) const

Are there layout tests that cover the various ways the type and language can be fixed up here?

>> Source/WebCore/dom/ScriptElement.cpp:161
>> +        m_parserInserted = false;
> 
> Woah, there's wasParserInserted, which is different than parserInserted?

was-parser-inserted seems to only be used for the new async=false.

Regarding this step, the spec says:
"This is done so that if parser-inserted script elements fail to run when the parser tries to run them, e.g. because they are empty or specify an unsupported scripting language, another script can later mutate them and cause them to run again."

This sounds like a behavior change. Is it possible to hold off on this part until the async=false patch so we can be sure to layout test the scenario above.

> Source/WebCore/dom/ScriptElement.cpp:164
> +

This would be a good place for a FIXME to check force-async here.

> Source/WebCore/dom/ScriptElement.cpp:184
> +

What about step 10 (the document changed)? Should there at least be a fixme?

> Source/WebCore/dom/ScriptElement.cpp:188
> +    String eventAttribute = eventAttributeValue();

I like the way you factored out isScriptTypeSupported(). I think we should also factor this out to a method like isScriptForEventSupported()..

> Source/WebCore/dom/ScriptElement.cpp:205
> +    if (hasSourceAttribute())

Do we also need an empty string check here or does hasSourceAttribute do that? Or this that behavior change we talked about before and it should just be a FIXME.

> Source/WebCore/dom/ScriptElement.cpp:305
> +    for (Node* n = m_element->firstChild(); n; n = n->nextSibling()) {

How did this work before? Did any functionality change and is it possible to add new tests?

> Source/WebCore/dom/XMLDocumentParserLibxml2.cpp:884
> +    if (!successfullyPrepared) {

I'm guessing xhtml picked up some new behaviors here. Should we have any new tests?

[James Simonsen]

(In reply to comment #9)
> View in context: https://bugs.webkit.org/attachment.cgi?id=82843&action=review
> 
> > Source/WebCore/dom/ScriptElement.cpp:60
> >      , m_wasAlreadyStarted(wasAlreadyStarted)
> 
> Throughout the spec it refers to "already started" not "was already started." Should this be m_alreadyStarted?

Done.

> > Source/WebCore/dom/ScriptElement.cpp:133
> > +bool ScriptElement::isScriptTypeSupported(LegacyTypeSupport supportLegacyTypes) const
> 
> Are there layout tests that cover the various ways the type and language can be fixed up here?

There's no new behavior. I've restructured the code to look more like the old code and less like the spec. The legacy stuff was added because some of the SVG layout tests depend on it.

> >> Source/WebCore/dom/ScriptElement.cpp:161
> >> +        m_parserInserted = false;
> > 
> > Woah, there's wasParserInserted, which is different than parserInserted?
> 
> was-parser-inserted seems to only be used for the new async=false.
> 
> Regarding this step, the spec says:
> "This is done so that if parser-inserted script elements fail to run when the parser tries to run them, e.g. because they are empty or specify an unsupported scripting language, another script can later mutate them and cause them to run again."
> 
> This sounds like a behavior change. Is it possible to hold off on this part until the async=false patch so we can be sure to layout test the scenario above.

The net effect is no new functionality. This code represents what had been in insertedIntoDocument, childrenChanged, and finishParsingChildren. We still need wasParserInserted to keep track of whether to reset isParserInserted at step 8, since we might remove parserInserted-ness as we used to in finishParsingChildren.

> > Source/WebCore/dom/ScriptElement.cpp:164
> > +
> 
> This would be a good place for a FIXME to check force-async here.

Done.

> > Source/WebCore/dom/ScriptElement.cpp:184
> > +
> 
> What about step 10 (the document changed)? Should there at least be a fixme?

Added fixme.

> > Source/WebCore/dom/ScriptElement.cpp:188
> > +    String eventAttribute = eventAttributeValue();
> 
> I like the way you factored out isScriptTypeSupported(). I think we should also factor this out to a method like isScriptForEventSupported().

Done.

> > Source/WebCore/dom/ScriptElement.cpp:205
> > +    if (hasSourceAttribute())
> 
> Do we also need an empty string check here or does hasSourceAttribute do that? Or this that behavior change we talked about before and it should just be a FIXME.

It matches the old behavior. Fixing it is new behavior (it fixes the < parsing on empty scripts), so I added a fixme.

> > Source/WebCore/dom/ScriptElement.cpp:305
> > +    for (Node* n = m_element->firstChild(); n; n = n->nextSibling()) {
> 
> How did this work before? Did any functionality change and is it possible to add new tests?

It's new functionality, so I reverted it to a fixme and restored the old check (which is incorrect, but works).

> > Source/WebCore/dom/XMLDocumentParserLibxml2.cpp:884
> > +    if (!successfullyPrepared) {
> 
> I'm guessing xhtml picked up some new behaviors here. Should we have any new tests?

It should behave like the HTML parser when it comes to "prepare a script." Is there a good way to add new tests for XHTML? I can copy & paste a bunch of the HTML tests to XHTML, but that seems a bit redundant.

[James Simonsen]

Created attachment 82876 [details]
Patch

[Tony Gentilcore]

This looks great to me! It removes duplication, lines us up with the spec, and paves the way for future changes to become even more compliant with the spec.

I don't have any more comments but would feel better if at least one other person took a pass through the final version as this is a lot of moving and opportunity to introduce subtle changes/bugs.

> > I'm guessing xhtml picked up some new behaviors here. Should we have any new tests?
> It should behave like the HTML parser when it comes to "prepare a script." Is there a good way to add new tests for XHTML? I can copy & paste a bunch of the HTML tests to XHTML, but that seems a bit redundant.

I'm not really sure what do to here. There is a lot of missing coverage, but I agree it would be silly to duplicate all the tests in HTML and XHTML form. Anyone else have an opinion?

[Ryosuke Niwa]

Comment on attachment 82876 [details]
Patch

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

I don't think your changes to the layout tests are correct.  You're essentially disabling these tests.

> LayoutTests/ChangeLog:22
> +        * fast/dom/HTMLScriptElement/move-in-beforeload.html: Original author says test was only meant to check for crashes. Beforeload is not specified by HTML5. Test was modified to assume moved script wouldn't execute because wasAlreadyStarted is set.

You should make sure that we don't execute the script.  Just removing the check isn't sufficient.

> LayoutTests/ChangeLog:23
> +        * fast/dom/script-clone-rerun-src.xhtml: According to HTML5 spec, load should only fire after a script executes. Test was modified to match that. The old broken behavior was that load fired on the cloned element, even though it didn't execute.

What are you talking about?  This test was testing that load() is called exactly once.

> LayoutTests/fast/dom/HTMLScriptElement/move-in-beforeload.html:21
> -    }, false);
> -    s.addEventListener("error", function() {
> -        testFailed("error event should not fire.")
> -    }, false);
> -    s.addEventListener("load", function() {
>          testPassed("");
> -        if (window.layoutTestController)
> -            layoutTestController.notifyDone();
>      }, false);

I don't think removing all these checks is great.  If we think the script shouldn't execute, then we should verify that it doesn't.

> LayoutTests/fast/dom/script-clone-rerun-src.xhtml:43
> +loaded();
> +

Why do we need to call loaded here?  It seems that we're breaking the test.  r- for this.

[James Simonsen]

(In reply to comment #13)
> (From update of attachment 82876 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82876&action=review
> > LayoutTests/ChangeLog:22
> > +        * fast/dom/HTMLScriptElement/move-in-beforeload.html: Original author says test was only meant to check for crashes. Beforeload is not specified by HTML5. Test was modified to assume moved script wouldn't execute because wasAlreadyStarted is set.
> 
> You should make sure that we don't execute the script.  Just removing the check isn't sufficient.

Done.

> > LayoutTests/ChangeLog:23
> > +        * fast/dom/script-clone-rerun-src.xhtml: According to HTML5 spec, load should only fire after a script executes. Test was modified to match that. The old broken behavior was that load fired on the cloned element, even though it didn't execute.
> 
> What are you talking about?  This test was testing that load() is called exactly once.

No, it was testing that it was executed only once. It relied on loaded() being called twice in order to complete. With the refactoring, load only fires once, so the test hangs without an extra call to loaded(). I've restructured the test to make it more clear what's going on. Let me know if that helps.

> > LayoutTests/fast/dom/HTMLScriptElement/move-in-beforeload.html:21
> > -    }, false);
> > -    s.addEventListener("error", function() {
> > -        testFailed("error event should not fire.")
> > -    }, false);
> > -    s.addEventListener("load", function() {
> >          testPassed("");
> > -        if (window.layoutTestController)
> > -            layoutTestController.notifyDone();
> >      }, false);
> 
> I don't think removing all these checks is great.  If we think the script shouldn't execute, then we should verify that it doesn't.

Done.

> > LayoutTests/fast/dom/script-clone-rerun-src.xhtml:43
> > +loaded();
> > +
> 
> Why do we need to call loaded here?  It seems that we're breaking the test.  r- for this.

See above.

[James Simonsen]

Created attachment 82882 [details]
Patch

[Ryosuke Niwa]

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

Ok, I'm still in the middle of reviewing but will do on the new patch.

> Source/WebCore/dom/ScriptElement.cpp:54
> -ScriptElement::ScriptElement(Element* element, bool wasInsertedByParser, bool wasAlreadyStarted)
> +ScriptElement::ScriptElement(Element* element, bool parserInserted, bool alreadyStarted)

Please do not make these renames.  See comments in https://bugs.webkit.org/show_bug.cgi?id=49705.

> Source/WebCore/dom/ScriptElement.cpp:58
> +    , m_parserInserted(parserInserted)
> +    , m_wasParserInserted(false)

Even if we're making the behavior change, I don't think it's great to have two variables called m_parserInserted and m_wasParserInserted.  We should definitely come with better variable names.

> Source/WebCore/dom/ScriptElement.cpp:150
> +    if (!type.isEmpty()) {
> +        if (!MIMETypeRegistry::isSupportedJavaScriptMIMEType(type.stripWhiteSpace().lower()) && !(supportLegacyTypes == AllowLegacyTypeInTypeAttribute && isLegacySupportedJavaScriptLanguage(type)))
> +            return false;
> +    } else {
> +        String language = languageAttributeValue();
> +        if (!language.isEmpty()) {
> +            String type = "text/" + language.lower();
> +            if (!MIMETypeRegistry::isSupportedJavaScriptMIMEType(type) && !isLegacySupportedJavaScriptLanguage(language))
> +                return false;
> +        }
> +    }
> +
> +    return true;

There are too many not's in these statements.  Let's negate them do something along the line of:

if (type.isEmpty()) {
    String language = languageAttributeValue();
    if (language.isEmpty())
        return true;
    String type = "text/" + language.lower();
    if (MIMETypeRegistry::isSupportedJavaScriptMIMEType(type) || isLegacySupportedJavaScriptLanguage(language))
        return true;
} else if (MIMETypeRegistry::isSupportedJavaScriptMIMEType(type.stripWhiteSpace().lower()) || (supportLegacyTypes == AllowLegacyTypeInTypeAttribute && isLegacySupportedJavaScriptLanguage(type)))
    return true;

return false;

> Source/WebCore/dom/ScriptElement.cpp:163
> +    if (m_parserInserted) {
> +        m_wasParserInserted = true;
> +        m_parserInserted = false;
> +    } else
> +        m_wasParserInserted = false;

It's totally unclear what we're doing here.  Again, we need better variable names so that this code is self-evident.

> Source/WebCore/dom/ScriptElement.cpp:199
> +    if (!charsetAttributeValue().isEmpty())
> +        m_characterEncoding = charsetAttributeValue();
> +    else
> +        m_fallbackCharacterEncoding = m_element->document()->charset();

It seems like we only need one variable for this.

> Source/WebCore/dom/ScriptElement.cpp:212
> +    } else if (hasSourceAttribute() && m_parserInserted && !asyncAttributeValue())
> +        m_willBeParserExecuted = true;
> +    else if (!hasSourceAttribute() && m_parserInserted && !m_element->document()->haveStylesheetsLoaded()) {
> +        m_willBeParserExecuted = true;
> +        m_readyToBeParserExecuted = true;

It seems like we don't set m_readyToBeParserExecuted true anywhere else.  But then what does the former case mean?  It will be executed by parser but never ready?

> Source/WebCore/dom/ScriptElement.cpp:321
> +    if (!m_characterEncoding.isEmpty())
> +        charset = m_characterEncoding;
> +    else if (!m_fallbackCharacterEncoding.isEmpty())
> +        charset = m_fallbackCharacterEncoding;

Why do we need two variables to do this?  We should just have one m_characterEncoding and set it in ScriptElement::prepareScript.

[James Simonsen]

(In reply to comment #16)

All of the names and structure comes from the HTML5 spec. The variable names have specific meaning in the spec. You should be able to read the spec and the code side-by-side and see exactly what's going on. See:

http://www.whatwg.org/specs/web-apps/current-work/multipage/scripting-1.html#prepare-a-script

I agree the spec can make the code look a bit convoluted. We can diverge from it some to help readability, but I'd like to keep it as close as possible, which is what we did in the parser code. The closer it is, the easier it'll be to stay in sync with it in the future.

[Ryosuke Niwa]

Comment on attachment 82882 [details]
Patch

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

> LayoutTests/ChangeLog:20
> +        https://bugs.webkit.org/show_bug.cgi?id=54676

This URL needs to be moved right beneath the bug title.

> LayoutTests/fast/dom/script-clone-rerun-src.xhtml:30
> +result = 'FAIL: script never ran';
> +if (i == 1)
> +  result = 'PASS';
> +else if (i > 1)
> +  result = 'FAIL: script ran ' + i + ' times';
> +document.body.appendChild(document.createTextNode(result));

But is this code guaranteed to run after the script loads for the second time?

> Source/WebCore/dom/ScriptElement.cpp:148
> +    if (!type.isEmpty()) {
> +        if (!MIMETypeRegistry::isSupportedJavaScriptMIMEType(type.stripWhiteSpace().lower()) && !(supportLegacyTypes == AllowLegacyTypeInTypeAttribute && isLegacySupportedJavaScriptLanguage(type)))
> +            return false;
> +    } else {
> +        String language = languageAttributeValue();
> +        if (!language.isEmpty()) {
> +            String type = "text/" + language.lower();
> +            if (!MIMETypeRegistry::isSupportedJavaScriptMIMEType(type) && !isLegacySupportedJavaScriptLanguage(language))
> +                return false;
> +        }
> +    }

See my comment about negating these statements.

> Source/WebCore/dom/ScriptElement.cpp:163
> +    if (m_parserInserted) {
> +        m_wasParserInserted = true;
> +        m_parserInserted = false;
> +    } else
> +        m_wasParserInserted = false;

I still think we need to explain why we do this.  Can we add an inline function of a descriptive name and call it here?

> Source/WebCore/dom/ScriptElement.cpp:178
> +    if (m_wasParserInserted)
> +        m_parserInserted = true;

It seems that the value of m_wasParserInserted is determined solely by the value of m_parserInserted at the time we entered this function.  We set it true if m_parserInserted is true and false otherwise.  Since none of functions we call here doesn't modify the value of m_wasParserInserted, we should make m_wasParserInserted a local variable.  Correct me if I'm wrong.

> Source/WebCore/dom/ScriptElement.cpp:209
> +    } else if (hasSourceAttribute() && m_parserInserted && !asyncAttributeValue())
> +        m_willBeParserExecuted = true;

Ok, so I guess this is the case where we need to wait for the script to load.

> Source/WebCore/dom/ScriptElement.cpp:247
> +    if (sourceCode.isEmpty())
>          return;

Should we assert some other conditions here?  e.g. script hasn't already started

> Source/WebCore/dom/ScriptElement.cpp:321
> +    String charset;
> +    if (!m_characterEncoding.isEmpty())
> +        charset = m_characterEncoding;
> +    else if (!m_fallbackCharacterEncoding.isEmpty())
> +        charset = m_fallbackCharacterEncoding;

As I said earlier, I don't think we need two member variables for this.

> Source/WebCore/dom/ScriptElement.h:36
> +enum LegacyTypeSupport { DisallowLegacyTypeInTypeAttribute, AllowLegacyTypeInTypeAttribute };
> +

We should declare this enum inside ScriptElement.

> Source/WebCore/dom/ScriptElement.h:104
> -    bool m_wasInsertedByParser;
> +    bool m_parserInserted;
> +    bool m_wasParserInserted;
>      bool m_isExternalScript;
> -    bool m_wasAlreadyStarted;
> +    bool m_alreadyStarted;
>      bool m_haveFiredLoad;
> +    bool m_willBeParserExecuted; // Same as "The parser will handle executing the script."
> +    bool m_readyToBeParserExecuted;
> +    bool m_willExecuteWhenDocumentFinishedParsing;

We should use bitfields here.

[Alexey Proskuryakov]

> > +        https://bugs.webkit.org/show_bug.cgi?id=54676
> This URL needs to be moved right beneath the bug title.

Right above the title is even better - that way, you guarantee that CIA-bot messages on IRC will have a URL to click on.

It would certainly be great to have a more specific title. We are never going to eliminate all bugs, and we are never going to match the spec. The ChangeLog has a more detailed description, but it's unclear what implementing a section from HTML5 means in plain terms.

The only place I could find practically useful information was in LayoutTests/ChangeLog.

[Eric Seidel (no email)]

(In reply to comment #19)
> > > +        https://bugs.webkit.org/show_bug.cgi?id=54676
> > This URL needs to be moved right beneath the bug title.
> 
> Right above the title is even better - that way, you guarantee that CIA-bot messages on IRC will have a URL to click on.

It's possible to customize the CIA message or change the prepare-ChangeLog generated format.  BUt it seems we should match the generated format until we change it. :)

[Alexey Proskuryakov]

> BUt it seems we should match the generated format until we change it. :)

All prepare-ChangeLog emits is one line, "Need a short description and bug URL (OOPS!)". But looking at ChangeLogs, looks like almost everyone now puts description first, so I'm just outdated, and will change my ways.

[Eric Seidel (no email)]

(In reply to comment #21)
> > BUt it seems we should match the generated format until we change it. :)
> 
> All prepare-ChangeLog emits is one line, "Need a short description and bug URL (OOPS!)". But looking at ChangeLogs, looks like almost everyone now puts description first, so I'm just outdated, and will change my ways.

If you pass -b NUMBER to prepare-ChangeLog which fills in the url and title for you (that's what webkit-patch does).  You should feel welcome to edit prepare-ChagneLog or the CIA template of course.

[Tony Gentilcore]

> > Source/WebCore/dom/ScriptElement.cpp:54
> > -ScriptElement::ScriptElement(Element* element, bool wasInsertedByParser, bool wasAlreadyStarted)
> > +ScriptElement::ScriptElement(Element* element, bool parserInserted, bool alreadyStarted)
> 
> Please do not make these renames.  See comments in https://bugs.webkit.org/show_bug.cgi?id=49705.

I disagree. For members that behave exactly as the spec suggests, there's a lot of value in explicitly tying them to the names used in the spec. This has worked really well in the rest of the parser code. Given that the spec uses parserInserted and wasParserInserted separately, I don't think we can ignore or append verbs unless you want a wasWasParserInserted or isWasParserInserted.

If we vary the names from the flags used in the spec, then I'd like to see comments tying each to the flag in the spec. But I think it is just more clear to match the names and skip the comments.

> 
> > Source/WebCore/dom/ScriptElement.cpp:58
> > +    , m_parserInserted(parserInserted)
> > +    , m_wasParserInserted(false)
> 
> Even if we're making the behavior change, I don't think it's great to have two variables called m_parserInserted and m_wasParserInserted.  We should definitely come with better variable names.

The thing is that tying them to the spec names allows a reader to go look at the spec and see exactly how they are used, what their purpose is, and notes about the behavior. We basically get documentation for free without having to embed it in the code or maintain it. It also helps maintainers to know to keep them in line with the spec. Again, if we rename, we'd needs comments saying tying them to the flags in the spec.

[Alexey Proskuryakov]

> For members that behave exactly as the spec suggests, there's a lot of value in explicitly tying them to the names used in the spec. This has worked really well in the rest of the parser code.

FWIW, the same didn't work as well for Application Cache code - trying to closely match the spec language backfired with every spec change. We ended up with obsolete concepts that are hard to deal with, because we relied on HTML5 for "documentation".

[James Simonsen]

(In reply to comment #18)
> (From update of attachment 82882 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82882&action=review
> 
> > LayoutTests/ChangeLog:20
> > +        https://bugs.webkit.org/show_bug.cgi?id=54676
> 
> This URL needs to be moved right beneath the bug title.

Done.

> > LayoutTests/fast/dom/script-clone-rerun-src.xhtml:30
> > +result = 'FAIL: script never ran';
> > +if (i == 1)
> > +  result = 'PASS';
> > +else if (i > 1)
> > +  result = 'FAIL: script ran ' + i + ' times';
> > +document.body.appendChild(document.createTextNode(result));
> 
> But is this code guaranteed to run after the script loads for the second time?

I see what you're saying now. You're worried about detecting failures. I'm not really sure how to robustly wait for something that might not (and should not) happen. I added a setTimeout() and verified it fails before my change though, so this seems to work.

> 
> > Source/WebCore/dom/ScriptElement.cpp:148
> > +    if (!type.isEmpty()) {
> > +        if (!MIMETypeRegistry::isSupportedJavaScriptMIMEType(type.stripWhiteSpace().lower()) && !(supportLegacyTypes == AllowLegacyTypeInTypeAttribute && isLegacySupportedJavaScriptLanguage(type)))
> > +            return false;
> > +    } else {
> > +        String language = languageAttributeValue();
> > +        if (!language.isEmpty()) {
> > +            String type = "text/" + language.lower();
> > +            if (!MIMETypeRegistry::isSupportedJavaScriptMIMEType(type) && !isLegacySupportedJavaScriptLanguage(language))
> > +                return false;
> > +        }
> > +    }
> 
> See my comment about negating these statements.

Done.

> > Source/WebCore/dom/ScriptElement.cpp:163
> > +    if (m_parserInserted) {
> > +        m_wasParserInserted = true;
> > +        m_parserInserted = false;
> > +    } else
> > +        m_wasParserInserted = false;
> 
> I still think we need to explain why we do this.  Can we add an inline function of a descriptive name and call it here?

Hmm. I've thought about it for a bit, but can't come up with anything that succinctly explains it. I could copy the note from the spec. Any other ideas?

> > Source/WebCore/dom/ScriptElement.cpp:178
> > +    if (m_wasParserInserted)
> > +        m_parserInserted = true;
> 
> It seems that the value of m_wasParserInserted is determined solely by the value of m_parserInserted at the time we entered this function.  We set it true if m_parserInserted is true and false otherwise.  Since none of functions we call here doesn't modify the value of m_wasParserInserted, we should make m_wasParserInserted a local variable.  Correct me if I'm wrong.

Done.

> > Source/WebCore/dom/ScriptElement.cpp:209
> > +    } else if (hasSourceAttribute() && m_parserInserted && !asyncAttributeValue())
> > +        m_willBeParserExecuted = true;
> 
> Ok, so I guess this is the case where we need to wait for the script to load.

Yep.

We should also refactor HTMLScriptRunner to match the spec. We've diverged there too.

> > Source/WebCore/dom/ScriptElement.cpp:247
> > +    if (sourceCode.isEmpty())
> >          return;
> 
> Should we assert some other conditions here?  e.g. script hasn't already started

Sure. BTW, it _should_ have already started. This is a good check to ensure that prepareScript() has been called.

> > Source/WebCore/dom/ScriptElement.cpp:321
> > +    String charset;
> > +    if (!m_characterEncoding.isEmpty())
> > +        charset = m_characterEncoding;
> > +    else if (!m_fallbackCharacterEncoding.isEmpty())
> > +        charset = m_fallbackCharacterEncoding;
> 
> As I said earlier, I don't think we need two member variables for this.

Done.

> > Source/WebCore/dom/ScriptElement.h:36
> > +enum LegacyTypeSupport { DisallowLegacyTypeInTypeAttribute, AllowLegacyTypeInTypeAttribute };
> > +
> 
> We should declare this enum inside ScriptElement.

Done.

> > Source/WebCore/dom/ScriptElement.h:104
> > -    bool m_wasInsertedByParser;
> > +    bool m_parserInserted;
> > +    bool m_wasParserInserted;
> >      bool m_isExternalScript;
> > -    bool m_wasAlreadyStarted;
> > +    bool m_alreadyStarted;
> >      bool m_haveFiredLoad;
> > +    bool m_willBeParserExecuted; // Same as "The parser will handle executing the script."
> > +    bool m_readyToBeParserExecuted;
> > +    bool m_willExecuteWhenDocumentFinishedParsing;
> 
> We should use bitfields here.

Done.

[James Simonsen]

Created attachment 83003 [details]
Patch

[Adam Barth]

Comment on attachment 83003 [details]
Patch

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

Looks great.  Thanks!

> Source/WebCore/dom/ScriptElement.cpp:222
> +    if (!m_element->document()->contentSecurityPolicy()->canLoadExternalScriptFromSrc(sourceUrl))
> +        return false;

This is wrong, but that's my job to deal with, not yours.

> Source/WebCore/dom/ScriptElement.h:44
> +    bool prepareScript(const TextPosition1& scriptStartPosition = TextPosition1::minimumPosition(), LegacyTypeSupport supportLegacyTypes = DisallowLegacyTypeInTypeAttribute);

supportLegacyTypes parameter name isn't needed.

[Ryosuke Niwa]

Comment on attachment 83003 [details]
Patch

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

> Source/WebCore/dom/ScriptElement.cpp:145
> +    } else
> +        if (MIMETypeRegistry::isSupportedJavaScriptMIMEType(type.stripWhiteSpace().lower()) || (supportLegacyTypes == AllowLegacyTypeInTypeAttribute && isLegacySupportedJavaScriptLanguage(type)))

else and if needs to be on the same line.  You should split at || or && (with || or && on the next line).

[Adam Barth]

Created attachment 83043 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 83043 [details]
Patch for landing

Clearing flags on attachment: 83043

Committed r79114: <http://trac.webkit.org/changeset/79114>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.