Support JS type reflections in the JS-API, an overview of the API changes are outlined here: https://github.com/WebAssembly/js-types/blob/master/proposals/js-types/Overview.md
Created attachment 415554 [details] Patch
Comment on attachment 415554 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415554&action=review Looks good, but put r- since I found bugs, And, can you update the LayoutTests results to make EWS green? > Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:74 > + JSValue minSizeValue = memoryDescriptor->get(globalObject, minimum); We need RETURN_IF_EXCEPTION(throwScope, encodedJSValue()); here since `->get(...)` can throw an error. > Source/JavaScriptCore/wasm/js/WebAssemblyTableConstructor.cpp:85 > + JSValue minSizeValue = memoryDescriptor->get(globalObject, minimumIdent); Ditto.
Comment on attachment 415554 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415554&action=review > Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:77 > + return JSValue::encode(throwException(globalObject, throwScope, createTypeError(globalObject, "WebAssembly.Memory specify exactly one of 'initial' or 'minimum'"))); Use `return throwVMTypeError(...)`. > Source/JavaScriptCore/wasm/js/WebAssemblyTableConstructor.cpp:87 > + return JSValue::encode(throwException(globalObject, throwScope, createTypeError(globalObject, "WebAssembly.Table exactly one of 'initial' or 'minimum'"))); Ditto.
Created attachment 415736 [details] Patch
Comment on attachment 415736 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415736&action=review Can you update LayoutTests results to make EWS green? Currently, win, mac-wk1, mac-wk2, and mac-debug-wk1 are red. > Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:78 > + return throwVMTypeError(globalObject, throwScope, "WebAssembly.Memory specify exactly one of 'initial' or 'minimum'"); Maybe, "'initial' and 'minimum' options are specified at the same time" would be better. > Source/JavaScriptCore/wasm/js/WebAssemblyTableConstructor.cpp:88 > + return throwVMTypeError(globalObject, throwScope, "WebAssembly.Table exactly one of 'initial' or 'minimum'"); Ditto.
<rdar://problem/72295339>
Created attachment 416294 [details] Patch
I've amended the patch to include the improved error message. I took a look at the failing tests and it looks like they're web-platform tests for the .type() method on WebAssembly.Memory, WebAssembly.Table and WebAssembly.Global. Those along with WebAssembly.Function I plan to make in a future pull requests, they should be failing currently on master too as it's currently missing. I was planning to land this in smaller sections (minimum constructor parameter, .type method and then WebAssembly.Function implementation). Thanks for the review!
(In reply to jtallon from comment #8) > I've amended the patch to include the improved error message. I took a look > at the failing tests and it looks like they're web-platform tests for the > .type() method on WebAssembly.Memory, WebAssembly.Table and > WebAssembly.Global. Those along with WebAssembly.Function I plan to make in > a future pull requests, they should be failing currently on master too as > it's currently missing. I was planning to land this in smaller sections > (minimum constructor parameter, .type method and then WebAssembly.Function > implementation). > > Thanks for the review! In WebKit, we cannot land the patch with failing patch. Can you update the xxx-expected.txt files in LayoutTests to make them green? :)
Comment on attachment 416294 [details] Patch Needing rebaseline of LayoutTests expected files.
You can see what LayoutTests are failing. If this failure is due to updated expected.txt files, you can update `LayoutTests/.../test-name-expected.txt` to update the expectation of these tests. Then you can make it green. If the failure is due to the bug of the patch, we need to fix the bug before landing. In this case, it seems that tests are failing because expectation is updated. So please update xxx-expected.txt files to update expectations of these tests. https://ews-build.s3-us-west-2.amazonaws.com/macOS-Mojave-Release-WK2-Tests-EWS/r416294-24678/results.html Thanks!
Created attachment 416502 [details] Patch
Created attachment 416508 [details] Patch
Created attachment 416510 [details] Patch
Comment on attachment 416510 [details] Patch r=me
/Volumes/Data/worker/Commit-Queue/build/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Comment on attachment 416510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416510&action=review > Source/JavaScriptCore/ChangeLog:5 > + Please keep `Reviewed by NOBODY (OOPS!).` thing. This will be replaced by commit-queue, and we cannot land it without that. ChangeLog format is, 2020-12-07 Jessica Tallon <jtallon@igalia.com> [JSC] Add minimum parameter to the WASM JS-API for Memory & Table. https://bugs.webkit.org/show_bug.cgi?id=219600 Reviewed by NOBODY (OOPS!). Comments. > LayoutTests/ChangeLog:5 > + Please keep `Reviewed by NOBODY (OOPS!).` thing. This will be replaced by commit-queue, and we cannot land it without that.
Created attachment 416600 [details] Patch
Comment on attachment 416600 [details] Patch r=me
Committed r271039: <https://trac.webkit.org/changeset/271039> All reviewed patches have been landed. Closing bug and clearing flags on attachment 416600 [details].
The changes in r271039 appear to have broken fast/images/webp-as-image.html on Big Sur History: https://results.webkit.org/?suite=layout-tests&test=fast%2Fimages%2Fwebp-as-image.html Diff: https://build.webkit.org/results/Apple-BigSur-Release-WK2-Tests/r271064%20(686)/fast/images/webp-as-image-diffs.html it looks like this test is failing to load an image
(In reply to Truitt Savell from comment #21) > The changes in r271039 appear to have broken fast/images/webp-as-image.html > on Big Sur > > History: > https://results.webkit.org/?suite=layout-tests&test=fast%2Fimages%2Fwebp-as- > image.html > > Diff: > https://build.webkit.org/results/Apple-BigSur-Release-WK2-Tests/ > r271064%20(686)/fast/images/webp-as-image-diffs.html > > it looks like this test is failing to load an image Hmmmmmmmmm, this sounds super strange. Literally, this patch only changes WebAssembly part.
(In reply to Yusuke Suzuki from comment #22) > (In reply to Truitt Savell from comment #21) > > The changes in r271039 appear to have broken fast/images/webp-as-image.html > > on Big Sur > > > > History: > > https://results.webkit.org/?suite=layout-tests&test=fast%2Fimages%2Fwebp-as- > > image.html > > > > Diff: > > https://build.webkit.org/results/Apple-BigSur-Release-WK2-Tests/ > > r271064%20(686)/fast/images/webp-as-image-diffs.html > > > > it looks like this test is failing to load an image > > Hmmmmmmmmm, this sounds super strange. Literally, this patch only changes > WebAssembly part. That test is ``` <body> <img src="resources/green-400x400.webp"> </body> ``` And the test failure is because the image is not loaded in the actual result. I don't think this patch is related to this test failure. My guess is library update for webp etc.
(In reply to Yusuke Suzuki from comment #23) > (In reply to Yusuke Suzuki from comment #22) > > (In reply to Truitt Savell from comment #21) > > > The changes in r271039 appear to have broken fast/images/webp-as-image.html > > > on Big Sur > > > > > > History: > > > https://results.webkit.org/?suite=layout-tests&test=fast%2Fimages%2Fwebp-as- > > > image.html > > > > > > Diff: > > > https://build.webkit.org/results/Apple-BigSur-Release-WK2-Tests/ > > > r271064%20(686)/fast/images/webp-as-image-diffs.html > > > > > > it looks like this test is failing to load an image > > > > Hmmmmmmmmm, this sounds super strange. Literally, this patch only changes > > WebAssembly part. > > That test is > > ``` > <body> > <img src="resources/green-400x400.webp"> > </body> > ``` > > And the test failure is because the image is not loaded in the actual result. > I don't think this patch is related to this test failure. > My guess is library update for webp etc. Reproduced this failure with r271038. So this revision is not related to the failure.
Right, we had the version of macOS updated right then. This is almost certainly a dupe of bug 219977.