Bug 148705 - [ES6] Implement ModuleNamespaceObject
Summary: [ES6] Implement ModuleNamespaceObject
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on: 148053
Blocks: 147340 148689
  Show dependency treegraph
 
Reported: 2015-09-01 23:29 PDT by Yusuke Suzuki
Modified: 2015-09-05 00:23 PDT (History)
1 user (show)

See Also:


Attachments
Patch (31.75 KB, patch)
2015-09-02 13:16 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (32.65 KB, patch)
2015-09-02 14:17 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (33.62 KB, patch)
2015-09-03 21:51 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (33.62 KB, patch)
2015-09-03 22:00 PDT, Yusuke Suzuki
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2015-09-01 23:29:37 PDT
Implement ModuleNamespaceObject.
Comment 1 Yusuke Suzuki 2015-09-02 13:16:41 PDT
Created attachment 260431 [details]
Patch
Comment 2 Yusuke Suzuki 2015-09-02 13:21:52 PDT
Comment on attachment 260431 [details]
Patch

This patch is based on the https://bugs.webkit.org/show_bug.cgi?id=148053
Comment 3 Yusuke Suzuki 2015-09-02 14:17:11 PDT
Created attachment 260437 [details]
Patch
Comment 4 Yusuke Suzuki 2015-09-03 21:30:21 PDT
Let's rebase it.
Comment 5 Yusuke Suzuki 2015-09-03 21:51:38 PDT
Created attachment 260562 [details]
Patch
Comment 6 Yusuke Suzuki 2015-09-03 22:00:29 PDT
Created attachment 260564 [details]
Patch
Comment 7 Yusuke Suzuki 2015-09-03 22:00:53 PDT
Slightly clean up the comment. Now patch is ready for review.
Comment 8 Geoffrey Garen 2015-09-04 14:19:41 PDT
Comment on attachment 260564 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSModuleNamespaceObject.cpp:64
> +{
> +    // http://www.ecma-international.org/ecma-262/6.0/#sec-module-namespace-exotic-objects
> +    // Quoted from the spec:
> +    //     A List containing the String values of the exported names exposed as own properties of this object.
> +    //     The list is ordered as if an Array of those String values had been sorted using Array.prototype.sort using SortCompare as comparefn.
> +    //
> +    // Sort the exported names by the code point order.
> +    Vector<UniquedStringImpl*> temporaryVector(exports.size(), nullptr);
> +    std::transform(exports.begin(), exports.end(), temporaryVector.begin(), [](const RefPtr<WTF::UniquedStringImpl>& ref) {
> +        return ref.get();
> +    });
> +    std::sort(temporaryVector.begin(), temporaryVector.end(), [] (UniquedStringImpl* lhs, UniquedStringImpl* rhs) {
> +        return codePointCompare(lhs, rhs) < 0;
> +    });
> +    for (auto* identifier : temporaryVector)
> +        m_exports.add(identifier);

This stuff should move into finishCreation. A JS object constructor should only do the bare minimum to default initialize fields in order to avoid crashing during GC. We do all meaningful work inside finishCreation in order to avoid problems that we might cause by triggering GC during a constructor. (I can see by reading this code that it doesn't cause GC, but it is preferable style to favor finishCreation so we don't have to do that reading.)

> Source/JavaScriptCore/runtime/JSModuleNamespaceObject.cpp:114
> +    // If the value is filled with TDZ value, throw the reference error.

the => a
Comment 9 Yusuke Suzuki 2015-09-04 14:55:02 PDT
Comment on attachment 260564 [details]
Patch

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

Thank you for your review! I'll revise the patch based on your review and land it :D

>> Source/JavaScriptCore/runtime/JSModuleNamespaceObject.cpp:64
>> +        m_exports.add(identifier);
> 
> This stuff should move into finishCreation. A JS object constructor should only do the bare minimum to default initialize fields in order to avoid crashing during GC. We do all meaningful work inside finishCreation in order to avoid problems that we might cause by triggering GC during a constructor. (I can see by reading this code that it doesn't cause GC, but it is preferable style to favor finishCreation so we don't have to do that reading.)

Make sense. Moving the above code to the finishCreation.

>> Source/JavaScriptCore/runtime/JSModuleNamespaceObject.cpp:114
>> +    // If the value is filled with TDZ value, throw the reference error.
> 
> the => a

Thank you! Fixed.
Comment 10 Yusuke Suzuki 2015-09-05 00:23:05 PDT
Committed r189429: <http://trac.webkit.org/changeset/189429>