Bug 149576 – [ES6] Add ScriptElement::determineScriptType

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[ES6] Add isModuleType function to ScriptElements

[Yusuke Suzuki]

Created attachment 261951 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 261951 [details]
Patch

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

> Source/WebCore/dom/ScriptElement.cpp:165
> +    else if (type == "module")

Where is this type value specified?  I can't find any mentioning of the "module" type in
https://html.spec.whatwg.org/multipage/scripting.html#attr-script-type
or
https://html.spec.whatwg.org/multipage/infrastructure.html#valid-mime-type

Also, shouldn't this check for the type of the element given SVGScriptElement doesn't support modules?

> Source/WebCore/html/HTMLScriptElement.cpp:171
> +bool HTMLScriptElement::isModuleType() const
> +{
> +#if ENABLE(ES6_MODULES)
> +    return fastGetAttribute(typeAttr).string() == "module";
> +#else
> +    return false;
> +#endif
> +}

I think it's better to put this in ScriptElement so that checks for "module" type appears next to each other.

[Yusuke Suzuki]

Comment on attachment 261951 [details]
Patch

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

Thank you for your comment!

>> Source/WebCore/dom/ScriptElement.cpp:165
>> +    else if (type == "module")
> 
> Where is this type value specified?  I can't find any mentioning of the "module" type in
> https://html.spec.whatwg.org/multipage/scripting.html#attr-script-type
> or
> https://html.spec.whatwg.org/multipage/infrastructure.html#valid-mime-type
> 
> Also, shouldn't this check for the type of the element given SVGScriptElement doesn't support modules?

This is why the module tag is behind the compile time flag.
Currently, there is consensus, but not specified yet.

https://github.com/whatwg/loader/blob/master/roadmap.md#stage-0-basic-static-loading
https://github.com/whatwg/loader/issues/83#issuecomment-143219400
https://github.com/dherman/module-tag/blob/master/explainer.md
https://esdiscuss.org/topic/any-news-about-the-module-element

And oops, I've missed it. After thinking about it, I think enabling "module" for svg script element seems more consistent.
So, I'll drop the SVGScriptElement's change. Put all "module" related changes into ScriptElement.

>> Source/WebCore/html/HTMLScriptElement.cpp:171
>> +}
> 
> I think it's better to put this in ScriptElement so that checks for "module" type appears next to each other.

Looks very nice to me.

[Yusuke Suzuki]

Created attachment 261966 [details]
Patch

[Ryosuke Niwa]

Okay, why don't we wait until those discussion settle?  We can implement the other parts of the module loading without this, right?  e.g. @import inside a script element.

[Yusuke Suzuki]

(In reply to comment #5)
> Okay, why don't we wait until those discussion settle?  We can implement the
> other parts of the module loading without this, right?  e.g. @import inside
> a script element.

Pending this patch is OK to me :-)
I'll do the other parts first, like https://bugs.webkit.org/show_bug.cgi?id=149574.

[Yusuke Suzuki]

Let's reboot this.
The description about type="module" is explicitly specified in the whatwg living standard!

https://html.spec.whatwg.org/multipage/webappapis.html#integration-with-the-javascript-module-system

[Yusuke Suzuki]

Created attachment 285410 [details]
Patch

[Yusuke Suzuki]

Created attachment 285412 [details]
Patch

[Yusuke Suzuki]

Created attachment 285413 [details]
Patch

[Yusuke Suzuki]

Created attachment 285415 [details]
Patch

[Saam Barati]

Comment on attachment 285415 [details]
Patch

LGTM but I'll let other WebCore reviewers give the official review

[Yusuke Suzuki]

Comment on attachment 285415 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 285415 [details]
Patch

Clearing flags on attachment: 285415

Committed r204221: <http://trac.webkit.org/changeset/204221>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.