[ES6] Implement ES6 Module loader hook stubs in WebCore
Created attachment 261949 [details] Patch
Waiting EWS.
Comment on attachment 261949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261949&action=review Added comments for ease of reviews. > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:285 > + if (RefPtr<Document> document = thisObject->impl().document()) Retain document during module loader hook execution. > Source/WebCore/bindings/js/ModuleLoader.cpp:61 > + completedURL = m_document.completeURL(moduleName); So in the module loader, m_document is always effective. > Source/WebCore/bindings/js/ModuleLoader.cpp:90 > + // FIXME: Implement the module fetcher. This will be filled in the subsequent patch. The final shape is already uploaded here https://bugs.webkit.org/show_bug.cgi?id=148897 > Source/WebCore/bindings/js/ModuleLoader.cpp:110 > + // FIXME: Implement evaluating module code. Ditto.
Comment on attachment 261949 [details] Patch I'll fix based on the comment from ap@ at https://bugs.webkit.org/show_bug.cgi?id=148897
Created attachment 261957 [details] Patch
Created attachment 261958 [details] Patch
Comment on attachment 261958 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261958&action=review > Source/WebCore/bindings/js/JSDOMWindowBase.h:81 > + // Module Loader hooks. I don't think this comment adds additional information not provided by the code below. Remove? > Source/WebCore/bindings/js/ModuleLoader.cpp:40 > +ModuleLoader::ModuleLoader(Document& document) Why don't we call this JSModuleLoader since CSS has modules so we have WebCore/modules. > Source/WebCore/bindings/js/ModuleLoader.cpp:45 > +JSC::JSInternalPromise* ModuleLoader::resolve(JSC::JSGlobalObject* globalObject, JSC::ExecState* exec, JSC::JSValue moduleNameValue, JSC::JSValue referrerValue) What does "referrerValue" mean in this context? Is this the module name of the importer? If so, something like importerModuleName might be more appropriate. > Source/WebCore/bindings/js/ModuleLoader.cpp:87 > + if (moduleKeyValue.isSymbol()) > + return deferred->reject(exec, JSC::createTypeError(exec, "Symbol module key should be already fulfilled with the inlined resource.")); Is this something that's supposed to happen? Or does this indicate an internal implementation error. The error message "should be already fulfilled" seems to indicate the latter but it's weird to spit out an error message like this instead of asserting it if that were the case. > Source/WebCore/bindings/js/ModuleLoader.cpp:103 > + // FIXME: Currently, we only support JSModuleRecord. What else do we need to support? It seems like it's more valuable to say that in this FIXME. > Source/WebCore/bindings/js/ModuleLoader.h:30 > +#include "JSDOMBinding.h" > +#include <runtime/JSInternalPromise.h> Do we really need to include these headers? Given all the types used below except JSValue are pointers, it seems like we just need forward declarations. > Source/WebCore/bindings/js/ModuleLoader.h:37 > +class ModuleLoader { Again, I think we should name this JSModuleLoader instead. > Source/WebCore/bindings/js/ModuleLoader.h:45 > + Document& document() > + { > + return m_document; > + } I'd put this into one line if I were you as in: document() { return m_document; }
Comment on attachment 261958 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261958&action=review Thank you for your review! >> Source/WebCore/bindings/js/JSDOMWindowBase.h:81 >> + // Module Loader hooks. > > I don't think this comment adds additional information not provided by the code below. > Remove? Make sense. Dropped. >> Source/WebCore/bindings/js/ModuleLoader.cpp:40 >> +ModuleLoader::ModuleLoader(Document& document) > > Why don't we call this JSModuleLoader since CSS has modules so we have WebCore/modules. Make sense. I'll change this to JSModuleLoader. >> Source/WebCore/bindings/js/ModuleLoader.cpp:45 >> +JSC::JSInternalPromise* ModuleLoader::resolve(JSC::JSGlobalObject* globalObject, JSC::ExecState* exec, JSC::JSValue moduleNameValue, JSC::JSValue referrerValue) > > What does "referrerValue" mean in this context? > Is this the module name of the importer? > If so, something like importerModuleName might be more appropriate. The name "referrer" itself is derived from the loader draft[1]. But your verbose name looks better. I'll change "referrer" to "importerModuleKey". (resolve function resolves "module name" to "module key".) [1]: https://whatwg.github.io/loader/#resolve >> Source/WebCore/bindings/js/ModuleLoader.cpp:87 >> + return deferred->reject(exec, JSC::createTypeError(exec, "Symbol module key should be already fulfilled with the inlined resource.")); > > Is this something that's supposed to happen? > Or does this indicate an internal implementation error. > The error message "should be already fulfilled" seems to indicate the latter > but it's weird to spit out an error message like this instead of asserting it if that were the case. Good question. Currently, it does not happen. Because we don't implement reflective part of the module loader yet. I think we should wait implementing reflective part because reflective part will be exposed directly to users and the spec of reflective part is under development yet. But once the loader exposes these internal APIs to users (like https://whatwg.github.io/loader/#browser-loader-prototype-%40%40fetch), these hook functions may take any values. So now, I wrote this part defensively. At the first patch, I implemented this part with assertion. But after receiving comments from Alexey, I changed my mind. https://bugs.webkit.org/show_bug.cgi?id=148897#c23 >> Source/WebCore/bindings/js/ModuleLoader.cpp:103 >> + // FIXME: Currently, we only support JSModuleRecord. > > What else do we need to support? It seems like it's more valuable to say that in this FIXME. Sounds reasonable. We need to support arbitrary objects here once we implement reflective part of the module loader. >> Source/WebCore/bindings/js/ModuleLoader.h:30 >> +#include <runtime/JSInternalPromise.h> > > Do we really need to include these headers? > Given all the types used below except JSValue are pointers, it seems like we just need forward declarations. Right. I'll just include <runtime/JSCJSValue.h> and drop the above inclusions. And add the forward declarations for JSC::JSInternalPromise*. >> Source/WebCore/bindings/js/ModuleLoader.h:37 >> +class ModuleLoader { > > Again, I think we should name this JSModuleLoader instead. Fixed. >> Source/WebCore/bindings/js/ModuleLoader.h:45 >> + } > > I'd put this into one line if I were you as in: document() { return m_document; } OK, fixed.
Created attachment 261967 [details] Patch
Created attachment 261968 [details] Patch
Waiting my local build and EWS.
Created attachment 261969 [details] Patch Build fix
Local working copy passed the build.
Created attachment 261980 [details] Patch Add more comments
Comment on attachment 261980 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261980&action=review > Source/WebCore/bindings/js/JSModuleLoader.h:32 > +namespace JSC { There should be a blank line around forward decorations here (i.e. one blank line after { and another blank line before }.). > Source/WebCore/bindings/js/JSModuleLoader.h:49 > + JSC::JSInternalPromise* resolve(JSC::JSGlobalObject*, JSC::ExecState*, JSC::JSValue moduleName, JSC::JSValue referrer); Why don't we can "referrer" "importerModuleKey" to be consistent? It's very confusing to use the term "referrer" here because it normally refers to https://developer.mozilla.org/en-US/docs/Web/API/Document/referrer in WebCore.
Comment on attachment 261980 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261980&action=review Thank you for your review :D >> Source/WebCore/bindings/js/JSModuleLoader.h:32 >> +namespace JSC { > > There should be a blank line around forward decorations here (i.e. one blank line after { and another blank line before }.). Looks nice. >> Source/WebCore/bindings/js/JSModuleLoader.h:49 >> + JSC::JSInternalPromise* resolve(JSC::JSGlobalObject*, JSC::ExecState*, JSC::JSValue moduleName, JSC::JSValue referrer); > > Why don't we can "referrer" "importerModuleKey" to be consistent? > It's very confusing to use the term "referrer" here > because it normally refers to https://developer.mozilla.org/en-US/docs/Web/API/Document/referrer in WebCore. Thanks. I missed the direct hook part. Fixed to `importerModuleKey`.
Committed r190272: <http://trac.webkit.org/changeset/190272>