Bug 149574 – [ES6] Implement ES6 Module loader hook stubs in WebCore

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[ES6] Implement ES6 Module loader hook stubs in WebCore

[Yusuke Suzuki]

Created attachment 261949 [details]
Patch

[Yusuke Suzuki]

Waiting EWS.

[Yusuke Suzuki]

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 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]

Created attachment 261957 [details]
Patch

[Yusuke Suzuki]

Created attachment 261958 [details]
Patch

[Ryosuke Niwa]

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 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]

Created attachment 261967 [details]
Patch

[Yusuke Suzuki]

Created attachment 261968 [details]
Patch

[Yusuke Suzuki]

Waiting my local build and EWS.

[Yusuke Suzuki]

Created attachment 261969 [details]
Patch

Build fix

[Yusuke Suzuki]

Local working copy passed the build.

[Yusuke Suzuki]

Created attachment 261980 [details]
Patch

Add more comments

[Ryosuke Niwa]

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 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]

Committed r190272: <http://trac.webkit.org/changeset/190272>