WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
149574
[ES6] Implement ES6 Module loader hook stubs in WebCore
https://bugs.webkit.org/show_bug.cgi?id=149574
Summary
[ES6] Implement ES6 Module loader hook stubs in WebCore
Yusuke Suzuki
Reported
2015-09-25 15:35:46 PDT
[ES6] Implement ES6 Module loader hook stubs in WebCore
Attachments
Patch
(31.26 KB, patch)
2015-09-25 15:41 PDT
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(31.74 KB, patch)
2015-09-25 18:09 PDT
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(31.77 KB, patch)
2015-09-25 18:22 PDT
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(32.08 KB, patch)
2015-09-26 02:06 PDT
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(32.08 KB, patch)
2015-09-26 02:08 PDT
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(32.08 KB, patch)
2015-09-26 02:15 PDT
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(32.26 KB, patch)
2015-09-26 17:58 PDT
,
Yusuke Suzuki
rniwa
: review+
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1
2015-09-25 15:41:36 PDT
Created
attachment 261949
[details]
Patch
Yusuke Suzuki
Comment 2
2015-09-25 15:42:29 PDT
Waiting EWS.
Yusuke Suzuki
Comment 3
2015-09-25 15:45:30 PDT
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.
Yusuke Suzuki
Comment 4
2015-09-25 17:34:41 PDT
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
Yusuke Suzuki
Comment 5
2015-09-25 18:09:06 PDT
Created
attachment 261957
[details]
Patch
Yusuke Suzuki
Comment 6
2015-09-25 18:22:25 PDT
Created
attachment 261958
[details]
Patch
Ryosuke Niwa
Comment 7
2015-09-26 00:06:27 PDT
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; }
Yusuke Suzuki
Comment 8
2015-09-26 01:42:39 PDT
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.
Yusuke Suzuki
Comment 9
2015-09-26 02:06:27 PDT
Created
attachment 261967
[details]
Patch
Yusuke Suzuki
Comment 10
2015-09-26 02:08:37 PDT
Created
attachment 261968
[details]
Patch
Yusuke Suzuki
Comment 11
2015-09-26 02:10:23 PDT
Waiting my local build and EWS.
Yusuke Suzuki
Comment 12
2015-09-26 02:15:27 PDT
Created
attachment 261969
[details]
Patch Build fix
Yusuke Suzuki
Comment 13
2015-09-26 02:22:00 PDT
Local working copy passed the build.
Yusuke Suzuki
Comment 14
2015-09-26 17:58:41 PDT
Created
attachment 261980
[details]
Patch Add more comments
Ryosuke Niwa
Comment 15
2015-09-26 22:42:50 PDT
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.
Yusuke Suzuki
Comment 16
2015-09-27 00:29:44 PDT
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`.
Yusuke Suzuki
Comment 17
2015-09-27 15:24:45 PDT
Committed
r190272
: <
http://trac.webkit.org/changeset/190272
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug