Skip to content

Conversation

@littledan
Copy link
Collaborator

@littledan
Copy link
Collaborator Author

I'll merge this patch, but happy to have further reviews.

@littledan littledan merged commit df18a01 into WebAssembly:master Mar 4, 2019
Copy link
Contributor

@xtuc xtuc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update @littledan.

I feel like that some changes could be upstreamed directly, especially to sync with other proposals.

</div>

<div algorithm>
To <dfn>instantate the core of a WebAssembly module</dfn> from a module |module| and imports |imports|, perform the following steps:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"instantiate the core of a WebAssembly module" is a new term? It's not part of the spec

Typo:
instantate -> instantiate

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo PR is #23.

What about "instantiate the internals of a WebAssembly module" instead?

1. [=Construct a WebAssembly module object=] from |module| and |bytes|, and return the result.
1. [=Construct a WebAssembly module object=] from |module| and |stableBytes|, and return the result.

Note: Some implementations may enforce a size limitation on |bytes|. Use of this API is discouraged, in favor of asynchronous APIs.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this note is correct, I hope that it will change in the future. Currently only Chrome (as far as I remember) enforce a < 4KB module.


<pre class="idl">
dictionary GlobalDescriptor {
required USVString value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this should be a DOMString, but it's off-topic here.

@xtuc
Copy link
Contributor

xtuc commented Mar 4, 2019

Sorry I forgot that you rebased, some of my comments are not relevant to this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants