[ES6] Add isModuleType function to ScriptElements
Created attachment 261951 [details] Patch
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.
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.
Created attachment 261966 [details] Patch
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.
(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.
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
Created attachment 285410 [details] Patch
Created attachment 285412 [details] Patch
Created attachment 285413 [details] Patch
Created attachment 285415 [details] Patch
Comment on attachment 285415 [details] Patch LGTM but I'll let other WebCore reviewers give the official review
Comment on attachment 285415 [details] Patch Thanks!
Comment on attachment 285415 [details] Patch Clearing flags on attachment: 285415 Committed r204221: <http://trac.webkit.org/changeset/204221>
All reviewed patches have been landed. Closing bug.