Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2780693003:
[wasm] response-based compile APIs (Closed)
|
Created:
3 years, 8 months ago by Mircea Trofin Modified:
3 years, 8 months ago CC:
chromium-reviews, blink-reviews, bradnelson, domenic, esprehn, ikilpatrick Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[wasm] response-based compile APIs
Spec available at: https://github.com/WebAssembly/design/blob/master/Web.md#webassemblycompile
This CL introduces the WebAssembly.compile overload in Blink.
Notably, we do not currently check the mime type (chromium:707149).
The V8 side is still under development. We currently exercise a skeleton API with a
trivial implementation. Once the full implementation is implemented, we will enhance
the blink side of the feature - for instance, we can introduce back pressure
information.
BUG=chromium:697028
Review-Url: https://codereview.chromium.org/2780693003
Cr-Commit-Position: refs/heads/master@{#462917}
Committed: https://chromium.googlesource.com/chromium/src/+/380bc71f286663403e7706aedcf66f724c89b584
Patch Set 1 #
Total comments: 1
Patch Set 2 : . #
Total comments: 3
Patch Set 3 : initialization #Patch Set 4 : fix win #Patch Set 5 : . #Patch Set 6 : . #
Total comments: 16
Patch Set 7 : rebase #
Total comments: 17
Patch Set 8 : . #
Total comments: 8
Patch Set 9 : feedback, more comments, tracking bug #
Total comments: 19
Patch Set 10 : feedback #
Total comments: 4
Patch Set 11 : feedback #Patch Set 12 : Moved wasm extensions to their own module. #Patch Set 13 : no specific OWNERS for now #
Total comments: 41
Patch Set 14 : . #Patch Set 15 : . #
Total comments: 10
Patch Set 16 : simplify #
Total comments: 3
Messages
Total messages: 79 (49 generated)
Description was changed from ========== [wasm] response-based compile APIs even more progress more progress - can compile, but that's about it work in progress BUG= ========== to ========== [wasm] response-based compile APIs <work in progress> BUG= ==========
https://codereview.chromium.org/2780693003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/fetch/Body.h (right): https://codereview.chromium.org/2780693003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/Body.h:65: public: Probably safe to ignore for now. https://codereview.chromium.org/2780693003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp (right): https://codereview.chromium.org/2780693003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp:100: scriptState->isolate()->SetWasmCompileCallback( This is the problem: we need to set this callback somewhere; we can't set it in bindings/core/v8/V8Initializer.cpp, which may be natural, because of layering. https://codereview.chromium.org/2780693003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/2780693003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Response.cpp:126: class FetchDataLoaderAsWasm final : public FetchDataLoader, Probably belongs to its own file, or in Body.cpp. https://codereview.chromium.org/2780693003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Response.cpp:203: class WasmConsumer final : public GarbageCollectedFinalized<WasmConsumer>, Not 100% convinced this brings much value here. Added it to conform to existent pattern.
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [wasm] response-based compile APIs <work in progress> BUG= ========== to ========== [wasm] response-based compile APIs Spec available at: https://github.com/WebAssembly/design/blob/master/Web.md#webassemblycompile This CL introduces the WebAssembly.compile overload in Blink. Notably, we do not currently check the mime type (chromium:707149). The V8 side is still under development. We currently exercise a skeleton API with a trivial implementation. Once the full implementation is implemented, we will enhance the blink side of the feature - for instance, we can introduce back pressure information. BUG=chromium:697028 ==========
mtrofin@chromium.org changed reviewers: + hiroshige@chromium.org, tyoshino@chromium.org, yhirano@chromium.org
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal - thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/31 07:02:52, Mircea Trofin wrote: > ptal - thanks! Gentle nudge - thank you!
mtrofin@chromium.org changed reviewers: + horo@chromium.org
On 2017/04/03 05:51:51, Mircea Trofin wrote: > On 2017/03/31 07:02:52, Mircea Trofin wrote: > > ptal - thanks! > > Gentle nudge - thank you! +horo@
Sorry for the late response. https://codereview.chromium.org/2780693003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/2780693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:124: class FetchDataLoaderAsWasm final : public FetchDataLoader, Please define this (and related classes) in FetchDataLoader.cpp. https://codereview.chromium.org/2780693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:153: case BytesConsumer::Result::Ok: { Why do we need this bracket? https://codereview.chromium.org/2780693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:155: result = m_consumer->endRead(available); What happens when this returns an error? https://codereview.chromium.org/2780693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:176: m_client->didFetchDataLoadedStream(); This function is not for loading a WASM. It's for streams, so please don't call it here. The right way is to define a new function in FetchDataLoader::Client. https://codereview.chromium.org/2780693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:207: reinterpret_cast<const uint8_t*>(bufferCopy)), Is this safe? I think this is confusing; can you remove this reinterpret_cast and declare a unique pointer from the beginning? https://codereview.chromium.org/2780693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:247: void CompileFromResponseCallback( Please use camelCase instead of CamelCase. https://codereview.chromium.org/2780693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:297: bool WasmCompileOverload(const v8::FunctionCallbackInfo<v8::Value>& args) { ditto https://codereview.chromium.org/2780693003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Response.h (right): https://codereview.chromium.org/2780693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.h:30: const WrapperTypeInfo* wrapperTypeInfo() const override { Can you add some comments explaining why these custom settings are needed?
haraken@chromium.org changed reviewers: + blink-reviews-bindings@chromium.org, haraken@chromium.org
https://codereview.chromium.org/2780693003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/2780693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:129: explicit FetchDataLoaderAsWasm(v8::Isolate* isolate, Drop explicit. https://codereview.chromium.org/2780693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:148: BytesConsumer::Result result = m_consumer->beginRead(&buffer, &available); Who allocates the buffer memory and who deallocates the memory? https://codereview.chromium.org/2780693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:167: if (maybeModule.ToLocal(&module)) { Nit: Use m_builder.ToLocal(&module). We want to avoid using MaybeLocal handles explicitly. https://codereview.chromium.org/2780693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:169: ScriptValue sv(m_scriptState.get(), module); sv => scriptValue Blink prefers a fully qualified name. https://codereview.chromium.org/2780693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:174: trycatch.Reset(); Why do you need to Reset it? https://codereview.chromium.org/2780693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:200: void SendBytes(const char* bytes, size_t size) { sendBytes https://codereview.chromium.org/2780693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:201: char* bufferCopy = new char[size]; Use fastMalloc(). Who deletes the bufferCopy? https://codereview.chromium.org/2780693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:221: class WasmConsumer final : public GarbageCollectedFinalized<WasmConsumer>, "Finalized" wouldn't be needed. https://codereview.chromium.org/2780693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:247: void CompileFromResponseCallback( I'm not really happy about the hand-written callbacks and the wrapper type info overriding. Would you help me understand why you need the hack?
(CC blink-reviews-bindings@ when you heavily touch V8 APIs.)
https://codereview.chromium.org/2780693003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/2780693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:124: class FetchDataLoaderAsWasm final : public FetchDataLoader, On 2017/04/04 00:44:35, yhirano (slow) wrote: > Please define this (and related classes) in FetchDataLoader.cpp. Done. Except that we need the WasmDataLoaderClient (was: WasmConsumer) here, so moved just FetchDataLoaderAsWasmModule (was: FetchDataLoaderAsWasm), and added the corresponding create*** API. Alternatively, WasmDataLoaderClient could have its own .h file, but it's ultimately and implementation detail of this pipeline here, and adding a separate file seems a bit overkill. https://codereview.chromium.org/2780693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:153: case BytesConsumer::Result::Ok: { On 2017/04/04 00:44:35, yhirano (slow) wrote: > Why do we need this bracket? I prefer adding braces for more than 1 line + the break. https://codereview.chromium.org/2780693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:155: result = m_consumer->endRead(available); On 2017/04/04 00:44:35, yhirano (slow) wrote: > What happens when this returns an error? Good catch - we can reject at that point. Done. https://codereview.chromium.org/2780693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:176: m_client->didFetchDataLoadedStream(); On 2017/04/04 00:44:35, yhirano (slow) wrote: > This function is not for loading a WASM. It's for streams, so please don't call > it here. The right way is to define a new function in FetchDataLoader::Client. Done. https://codereview.chromium.org/2780693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:207: reinterpret_cast<const uint8_t*>(bufferCopy)), On 2017/04/04 00:44:35, yhirano (slow) wrote: > Is this safe? I think this is confusing; can you remove this reinterpret_cast > and declare a unique pointer from the beginning? It is safe because bufferCopy doesn't get aliased out of this scope, but you're right, it's just cleaner to start and end with std::unique_ptr. Done. https://codereview.chromium.org/2780693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:247: void CompileFromResponseCallback( On 2017/04/04 00:44:35, yhirano (slow) wrote: > Please use camelCase instead of CamelCase. Done. https://codereview.chromium.org/2780693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:297: bool WasmCompileOverload(const v8::FunctionCallbackInfo<v8::Value>& args) { On 2017/04/04 00:44:35, yhirano (slow) wrote: > ditto Done. https://codereview.chromium.org/2780693003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Response.h (right): https://codereview.chromium.org/2780693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.h:30: const WrapperTypeInfo* wrapperTypeInfo() const override { On 2017/04/04 00:44:35, yhirano (slow) wrote: > Can you add some comments explaining why these custom settings are needed? Done. https://codereview.chromium.org/2780693003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/2780693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:129: explicit FetchDataLoaderAsWasm(v8::Isolate* isolate, On 2017/04/04 02:39:38, haraken wrote: > > Drop explicit. Done. https://codereview.chromium.org/2780693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:148: BytesConsumer::Result result = m_consumer->beginRead(&buffer, &available); On 2017/04/04 02:39:38, haraken wrote: > > Who allocates the buffer memory and who deallocates the memory? the consumer allocates, and deallocates at endRead. I got inspiration from what we do elsewhere, e.g. FetchDataLoaderAsArrayBuffer. I'm assuming that this pattern of allocation/deallocation by m_consumer->beginRead/endRead is part of the contract with that object? https://codereview.chromium.org/2780693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:167: if (maybeModule.ToLocal(&module)) { On 2017/04/04 02:39:38, haraken wrote: > > Nit: Use m_builder.ToLocal(&module). We want to avoid using MaybeLocal handles > explicitly. Done. I'm curious though, what is the motivation against using MaybeLocals explicitly (style, something else)? Thanks! https://codereview.chromium.org/2780693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:169: ScriptValue sv(m_scriptState.get(), module); On 2017/04/04 02:39:38, haraken wrote: > > sv => scriptValue > > Blink prefers a fully qualified name. Done. https://codereview.chromium.org/2780693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:174: trycatch.Reset(); On 2017/04/04 02:39:38, haraken wrote: > > Why do you need to Reset it? We don't want the exception to survive past the catch, since we're rejecting with it. https://codereview.chromium.org/2780693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:201: char* bufferCopy = new char[size]; On 2017/04/04 02:39:38, haraken wrote: > > Use fastMalloc(). > > Who deletes the bufferCopy? Changed to using std::unique_ptr from the get-go, as per +yhirano's suggestion. Re. destruction - the destructor of the unique_ptr it's getting handed to. That'd be the case both before and now. Does the fastMalloc comment still apply, now that we start from a std::unique_ptr? https://codereview.chromium.org/2780693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:221: class WasmConsumer final : public GarbageCollectedFinalized<WasmConsumer>, On 2017/04/04 02:39:38, haraken wrote: > > "Finalized" wouldn't be needed. You mean just "GarbageCollected"? Removing the "Finalized" triggers compiler error because FetchDataLoader::Client is GarbageCollectedMixin (if I understand this correctly) https://codereview.chromium.org/2780693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:247: void CompileFromResponseCallback( On 2017/04/04 02:39:38, haraken wrote: > > I'm not really happy about the hand-written callbacks and the wrapper type info > overriding. > > Would you help me understand why you need the hack? Added you to a thread. The very succinct explanation is that we need blink-side overloads for wasm APIs (defined in v8), and currently there's no alternative, from what I understand.
https://codereview.chromium.org/2780693003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (right): https://codereview.chromium.org/2780693003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:314: ScriptState* scriptState) Drop explicit. https://codereview.chromium.org/2780693003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:384: std::unique_ptr<uint8_t[]> bufferCopy(new uint8_t[size]); We should use fastMalloc / fastFree instead of new. In the first place, however, is there any way to avoid creating the copy? i.e., pass the bytes directly to v8::WasmModuleObjectBuilde and make it work. https://codereview.chromium.org/2780693003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/2780693003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:155: void compileFromResponseCallback( I'd prefer moving these wasm-specific methods to Source/bindings/core/v8/WASMHelper.cpp or something. https://codereview.chromium.org/2780693003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:230: isolate->SetWasmCompileCallback(wasmCompileOverload); Just to confirm: This statement is the only reason you need to override the wrapper type info, right? Also would Response the only object that will need the statement? Or do you have a plan to use it in other objects as well? If yes, we should consider introducing an IDL extended attribute like [SupportWebAssembly] and let the IDL compiler generate the statement. That said, I'm confused. Why do we not have WebAssembly.idl in the first place? I'm feeling that this CL is trying to implement WebAssembly.compile without explicitly exposing it to the IDL. That's why you need to implement it yourself without relying on the IDL comipler. Which seems strange to me.
Thanks for the feedback - I'll address the remainder shortly (tomorrow morning PST). https://codereview.chromium.org/2780693003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (right): https://codereview.chromium.org/2780693003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:384: std::unique_ptr<uint8_t[]> bufferCopy(new uint8_t[size]); On 2017/04/04 06:05:31, haraken wrote: > > We should use fastMalloc / fastFree instead of new. > > In the first place, however, is there any way to avoid creating the copy? i.e., > pass the bytes directly to v8::WasmModuleObjectBuilde and make it work. Oh, good point - thats anyway what we want to do once the wasm side is fully implemented (right now it's just a dumb skeleton). I'll prep the v8 side. https://codereview.chromium.org/2780693003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/2780693003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:155: void compileFromResponseCallback( On 2017/04/04 06:05:31, haraken wrote: > > I'd prefer moving these wasm-specific methods to > Source/bindings/core/v8/WASMHelper.cpp or something. Won't layering cause an issue? These implementations take a dependency on Response APIs. https://codereview.chromium.org/2780693003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:230: isolate->SetWasmCompileCallback(wasmCompileOverload); On 2017/04/04 06:05:31, haraken wrote: > > Just to confirm: This statement is the only reason you need to override the > wrapper type info, right? > Yes. It'll be followed by a similar statement for the WebAssembly.instantiate API, but that's all. > Also would Response the only object that will need the statement? Or do you have > a plan to use it in other objects as well? If yes, we should consider > introducing an IDL extended attribute like [SupportWebAssembly] and let the IDL > compiler generate the statement. > We don't have more plans, but (as we were chatting on the offline thread), the scenario limiting synchronous wasm compilation to certain sizes also "feels" like overriding, so would benefit from a similar mechanism. Currently, it's done the same way (injection), just that there was a natural, layering-preserving location for that initialization (V8Initializer). I'd feel better with a streamlined solution here, too, and if that's enhancing the IDL, sgtm. For completeness - we also had on the table the idea of using "V8 Extras", but that has its own challenges. To conclude - happy to do the IDL (or "V8 Extras"?) work in a subsequent CL! > That said, I'm confused. Why do we not have WebAssembly.idl in the first place? > I'm feeling that this CL is trying to implement WebAssembly.compile without > explicitly exposing it to the IDL. That's why you need to implement it yourself > without relying on the IDL comipler. Which seems strange to me. > >
On 2017/04/04 06:48:07, Mircea Trofin wrote: > Thanks for the feedback - I'll address the remainder shortly (tomorrow morning > PST). > > https://codereview.chromium.org/2780693003/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (right): > > https://codereview.chromium.org/2780693003/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:384: > std::unique_ptr<uint8_t[]> bufferCopy(new uint8_t[size]); > On 2017/04/04 06:05:31, haraken wrote: > > > > We should use fastMalloc / fastFree instead of new. > > > > In the first place, however, is there any way to avoid creating the copy? > i.e., > > pass the bytes directly to v8::WasmModuleObjectBuilde and make it work. > > Oh, good point - thats anyway what we want to do once the wasm side is fully > implemented (right now it's just a dumb skeleton). I'll prep the v8 side. FYI: https://codereview.chromium.org/2797653002/ > > https://codereview.chromium.org/2780693003/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/fetch/Response.cpp (right): > > https://codereview.chromium.org/2780693003/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/fetch/Response.cpp:155: void > compileFromResponseCallback( > On 2017/04/04 06:05:31, haraken wrote: > > > > I'd prefer moving these wasm-specific methods to > > Source/bindings/core/v8/WASMHelper.cpp or something. > > Won't layering cause an issue? These implementations take a dependency on > Response APIs. > > https://codereview.chromium.org/2780693003/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/fetch/Response.cpp:230: > isolate->SetWasmCompileCallback(wasmCompileOverload); > On 2017/04/04 06:05:31, haraken wrote: > > > > Just to confirm: This statement is the only reason you need to override the > > wrapper type info, right? > > > Yes. It'll be followed by a similar statement for the WebAssembly.instantiate > API, but that's all. > > > Also would Response the only object that will need the statement? Or do you > have > > a plan to use it in other objects as well? If yes, we should consider > > introducing an IDL extended attribute like [SupportWebAssembly] and let the > IDL > > compiler generate the statement. > > > We don't have more plans, but (as we were chatting on the offline thread), the > scenario limiting synchronous wasm compilation to certain sizes also "feels" > like overriding, so would benefit from a similar mechanism. Currently, it's done > the same way (injection), just that there was a natural, layering-preserving > location for that initialization (V8Initializer). > > I'd feel better with a streamlined solution here, too, and if that's enhancing > the IDL, sgtm. For completeness - > we also had on the table the idea of using "V8 Extras", but that has its own > challenges. > > To conclude - happy to do the IDL (or "V8 Extras"?) work in a subsequent CL! > > > That said, I'm confused. Why do we not have WebAssembly.idl in the first > place? > > I'm feeling that this CL is trying to implement WebAssembly.compile without > > explicitly exposing it to the IDL. That's why you need to implement it > yourself > > without relying on the IDL comipler. Which seems strange to me. > > > >
https://codereview.chromium.org/2780693003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/2780693003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:230: isolate->SetWasmCompileCallback(wasmCompileOverload); On 2017/04/04 06:48:07, Mircea Trofin wrote: > On 2017/04/04 06:05:31, haraken wrote: > > > > Just to confirm: This statement is the only reason you need to override the > > wrapper type info, right? > > > Yes. It'll be followed by a similar statement for the WebAssembly.instantiate > API, but that's all. > > > Also would Response the only object that will need the statement? Or do you > have > > a plan to use it in other objects as well? If yes, we should consider > > introducing an IDL extended attribute like [SupportWebAssembly] and let the > IDL > > compiler generate the statement. > > > We don't have more plans, but (as we were chatting on the offline thread), the > scenario limiting synchronous wasm compilation to certain sizes also "feels" > like overriding, so would benefit from a similar mechanism. Currently, it's done > the same way (injection), just that there was a natural, layering-preserving > location for that initialization (V8Initializer). > > I'd feel better with a streamlined solution here, too, and if that's enhancing > the IDL, sgtm. For completeness - > we also had on the table the idea of using "V8 Extras", but that has its own > challenges. > > To conclude - happy to do the IDL (or "V8 Extras"?) work in a subsequent CL! > > > That said, I'm confused. Why do we not have WebAssembly.idl in the first > place? > > I'm feeling that this CL is trying to implement WebAssembly.compile without > > explicitly exposing it to the IDL. That's why you need to implement it > yourself > > without relying on the IDL comipler. Which seems strange to me. > > > > > Yeah, I was confused about this CL because it's introducing a new programming pattern to the V8 - Blink bindings :) I'd prefer sticking to the well-established IDL pattern. I'm fine with introducing a new IDL extended attribute like [WebAssembly] if necessary.
PTAL - Updated to avoid copying bytes on the blink side (we need to wait for autoroller to pull the v8 side before running trybots, though) - Created tracking bug for the codegen issue.
On 2017/04/04 18:42:59, Mircea Trofin wrote: > PTAL > > - Updated to avoid copying bytes on the blink side (we need to wait for > autoroller to pull the v8 side before running trybots, though) Thanks! > > - Created tracking bug for the codegen issue. Regarding this, I want to get feedback from WebAssembly experts before landing this CL. My concern about this CL is not that this CL is introducing a bunch of hand-written code but that this CL is introducing a new way to inject platform APIs (without using IDL files). In that sense, replacing the hand-written code with the auto-generated code in a follow-up CL won't resolve the problem. i.e., I might want to think a bit more about how to implement the WebAssembly bindings before going with this CL.
https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (right): https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:24: FetchDataLoaderAsBlobHandle(const String& mimeType) : m_mimeType(mimeType) {} Why did you change this line? https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:307: class FetchDataLoaderAsWasmModule final : public FetchDataLoader, This FechDataLoader calls m_resolver->reject with an exception sometimes, m_resolver->reject with undefined sometimes and m_client->didFetchDataLoadFailed in other cases. Error handling should be consistent, so I think it's a good idea to state that this FetchDataLoader doesn't call didFetchDataLoadFailed but uses the given resolver to notify errors. https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:341: DCHECK_GT(available, 0U); |available| can be zero. See comments in BytesConsumer.h. https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:344: result = m_consumer->endRead(available); |result| can be |Done|. See comments in BytesConsumer.h. https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:379: void cancel() override { m_consumer->cancel(); } I think you need to do something with m_resolver. https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:1: - https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:221: compileFromResponseCallback(args); I think this should work as wasmCompileOverLoad(Promise.resolve(args[0])) instead of compileFromResponseCallback(args) .
Patchset #10 (id:180001) has been deleted
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Addressed, except for comment in Response.cpp - please see my question on that one. Thanks! https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (right): https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:24: FetchDataLoaderAsBlobHandle(const String& mimeType) : m_mimeType(mimeType) {} On 2017/04/05 03:48:03, yhirano (slow) wrote: > Why did you change this line? Hmm... weird. I think I misread a previous comment. Changing back. https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:307: class FetchDataLoaderAsWasmModule final : public FetchDataLoader, On 2017/04/05 03:48:03, yhirano (slow) wrote: > This FechDataLoader calls m_resolver->reject with an exception sometimes, > m_resolver->reject with undefined sometimes and m_client->didFetchDataLoadFailed > in other cases. Error handling should be consistent, so I think it's a good idea > to state that this FetchDataLoader doesn't call didFetchDataLoadFailed but uses > the given resolver to notify errors. Makes sense. Also noticed the wasm spec doesn't seem to cover faulty channel/cancellation. Throwing TypeError for now (consistent to passing an unsupported parameter). Will follow up with a subsequent CL when the spec clarifies. https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:341: DCHECK_GT(available, 0U); On 2017/04/05 03:48:03, yhirano (slow) wrote: > |available| can be zero. See comments in BytesConsumer.h. Hmm... I'm looking at these 2 lines: // Returns Ok when readable. [...] // |*buffer| will be set to null and |*available| will be set to 0 if not // readable. I assumed that meant the only state buffer and available are non-null/zero is Result::OK. I realize that's not strictly implied. Perhaps I can add a clarifying comment to avoid confusion? Changed the code here, meanwhile, to match the rest of this file. https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:344: result = m_consumer->endRead(available); On 2017/04/05 03:48:03, yhirano (slow) wrote: > |result| can be |Done|. See comments in BytesConsumer.h. Acknowledged. https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:379: void cancel() override { m_consumer->cancel(); } On 2017/04/05 03:48:03, yhirano (slow) wrote: > I think you need to do something with m_resolver. Acknowledged. https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:1: On 2017/04/05 03:48:03, yhirano (slow) wrote: > - Acknowledged. https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:221: compileFromResponseCallback(args); On 2017/04/05 03:48:03, yhirano (slow) wrote: > I think this should work as > wasmCompileOverLoad(Promise.resolve(args[0])) > > instead of > compileFromResponseCallback(args) > > . So I understand - what would be the motivation - ensuring one path into compileFromResponseCallback? (I wanted to avoid the extra prep work, that would be my motivation for the current state of things).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:221: compileFromResponseCallback(args); On 2017/04/05 05:12:22, Mircea Trofin wrote: > On 2017/04/05 03:48:03, yhirano (slow) wrote: > > I think this should work as > > wasmCompileOverLoad(Promise.resolve(args[0])) > > > > instead of > > compileFromResponseCallback(args) > > > > . > > So I understand - what would be the motivation - ensuring one path into > compileFromResponseCallback? (I wanted to avoid the extra prep work, that would > be my motivation for the current state of things). The current implementation calls compileFromResponseCallback synchronously and my suggestion doesn't. The latter is correct based on https://www.w3.org/2001/tag/doc/promises-guide#resolve-arguments. https://codereview.chromium.org/2780693003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (right): https://codereview.chromium.org/2780693003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:371: // m_outStream->abort(); What does this line mean? https://codereview.chromium.org/2780693003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/2780693003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:138: void didFetchDataLoadFailed() override { NOTREACHED();
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:221: compileFromResponseCallback(args); On 2017/04/05 12:02:50, yhirano wrote: > On 2017/04/05 05:12:22, Mircea Trofin wrote: > > On 2017/04/05 03:48:03, yhirano (slow) wrote: > > > I think this should work as > > > wasmCompileOverLoad(Promise.resolve(args[0])) > > > > > > instead of > > > compileFromResponseCallback(args) > > > > > > . > > > > So I understand - what would be the motivation - ensuring one path into > > compileFromResponseCallback? (I wanted to avoid the extra prep work, that > would > > be my motivation for the current state of things). > > The current implementation calls compileFromResponseCallback synchronously and > my suggestion doesn't. The latter is correct based on > https://www.w3.org/2001/tag/doc/promises-guide#resolve-arguments. Thanks for the clarification and the pointer. I made the change Still not sure though it buys us much: compileFromResponseCallback just starts the asynchronous fetch and returns a promise; and we still need to do typechecking (as the CL demonstrates), to determine if the API needs to call back to v8's original behavior: if the parameter passed to the blink-side overload isn't a promise nor a Response, we still need to do the v8 behavior. Am I missing something? https://codereview.chromium.org/2780693003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (right): https://codereview.chromium.org/2780693003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:371: // m_outStream->abort(); On 2017/04/05 12:02:50, yhirano wrote: > What does this line mean? It's part of the TODO, I added more text around to avoid confusion. https://codereview.chromium.org/2780693003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/2780693003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:138: void didFetchDataLoadFailed() override { On 2017/04/05 12:02:50, yhirano wrote: > NOTREACHED(); Uh, yes. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #12 (id:240001) has been deleted
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Moved extensions out of fetch into their own module.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:221: compileFromResponseCallback(args); On 2017/04/05 17:17:49, Mircea Trofin wrote: > On 2017/04/05 12:02:50, yhirano wrote: > > On 2017/04/05 05:12:22, Mircea Trofin wrote: > > > On 2017/04/05 03:48:03, yhirano (slow) wrote: > > > > I think this should work as > > > > wasmCompileOverLoad(Promise.resolve(args[0])) > > > > > > > > instead of > > > > compileFromResponseCallback(args) > > > > > > > > . > > > > > > So I understand - what would be the motivation - ensuring one path into > > > compileFromResponseCallback? (I wanted to avoid the extra prep work, that > > would > > > be my motivation for the current state of things). > > > > The current implementation calls compileFromResponseCallback synchronously and > > my suggestion doesn't. The latter is correct based on > > https://www.w3.org/2001/tag/doc/promises-guide#resolve-arguments. > > Thanks for the clarification and the pointer. I made the change > > Still not sure though it buys us much: compileFromResponseCallback just starts > the asynchronous fetch and returns a promise; and we still need to do > typechecking (as the CL demonstrates), to determine if the API needs to call > back to v8's original behavior: if the parameter passed to the blink-side > overload isn't a promise nor a Response, we still need to do the v8 behavior. > > Am I missing something? Ah, I see, thank you. Returning false means redirecting to the original function. I have another question about the interface, but will ask on anther thread. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchDataLoader.h (right): https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchDataLoader.h:8: #include "bindings/core/v8/ScriptPromiseResolver.h" These two lines are no longer needed. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchDataLoader.h:50: virtual void didFetchDataLoadedCustomFormat() { NOTREACHED(); } // This function is called when a "custom" FetchDataLoader (not listed below) finishes loading. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp (right): https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:11: #include "wtf/RefPtr.h" + modules/fetch/FetchDataLoader.h + bindings/core/v8/ScriptPomise.h + bindings/core/v8/ScriptPomiseResolver.h + platform/heap/Handle.h https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:135: Member<ScriptPromiseResolver> m_resolver; You don't need this member. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:150: args.GetReturnValue().Set(ScriptPromise().v8Value()); ScriptPromise().v8Value() is mere an empty v8 value. Do we need this line? https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:160: "Promise argument must produce a Response object")) Is "This function must be called with Promise<Response>" clearer? https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:186: "Promise argument must produce a Response object")); I think this message is confusing (from a different reason than the above). This is the case where the response has a null body.
https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:221: compileFromResponseCallback(args); On 2017/04/07 02:39:36, yhirano wrote: > On 2017/04/05 17:17:49, Mircea Trofin wrote: > > On 2017/04/05 12:02:50, yhirano wrote: > > > On 2017/04/05 05:12:22, Mircea Trofin wrote: > > > > On 2017/04/05 03:48:03, yhirano (slow) wrote: > > > > > I think this should work as > > > > > wasmCompileOverLoad(Promise.resolve(args[0])) > > > > > > > > > > instead of > > > > > compileFromResponseCallback(args) > > > > > > > > > > . > > > > > > > > So I understand - what would be the motivation - ensuring one path into > > > > compileFromResponseCallback? (I wanted to avoid the extra prep work, that > > > would > > > > be my motivation for the current state of things). > > > > > > The current implementation calls compileFromResponseCallback synchronously > and > > > my suggestion doesn't. The latter is correct based on > > > https://www.w3.org/2001/tag/doc/promises-guide#resolve-arguments. > > > > Thanks for the clarification and the pointer. I made the change > > > > Still not sure though it buys us much: compileFromResponseCallback just starts > > the asynchronous fetch and returns a promise; and we still need to do > > typechecking (as the CL demonstrates), to determine if the API needs to call > > back to v8's original behavior: if the parameter passed to the blink-side > > overload isn't a promise nor a Response, we still need to do the v8 behavior. > > > > Am I missing something? > > Ah, I see, thank you. Returning false means redirecting to the original > function. I have another question about the interface, but will ask on anther > thread. Sorry, I was wrong. Please replace the last response with below. Ah, I see, thank you. Returning false means redirecting to the original function. We need the type checking code. But I still think compile(response) should behave exactly same as compile(Promise.resolve(response)), and that should be clear without closer look at compileFromResponseCallback. In the current implmentation, there is a microtask timing difference, right?
Discussed offline. The overall approach looks good. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/ModulesInitializer.cpp:65: WasmResponseExtensions::initialize(V8PerIsolateData::mainThreadIsolate()); Would you move this call to V8Initializer? https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/wasm/BUILD.gn (right): https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/BUILD.gn:10: "WasmResponseExtensions.h", WasmResponseExtensions is heavily using V8 APIs, so would you move the files to Source/bindings/core/v8/? https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp (right): https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:22: FetchDataLoaderAsWasmModule(v8::Isolate* isolate, Remove the isolate parameter. You can use scriptSTate->isolate(). https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:39: // buffer is allocated by beginRead and de-allocated by endRead. beginRead just sets buffer to some address in an already-allocated buffer, and endRead does nothing on the buffer, right? https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:103: void ThrowTypeError() { throwTypeError I'd prefer "rejectPromise" or something though. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:150: args.GetReturnValue().Set(ScriptPromise().v8Value()); Nit: v8SetReturnValue() https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:156: args.GetReturnValue().Set( Nit: v8SetReturnValue() https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:189: args.GetReturnValue().Set(promise.v8Value()); Nit: v8SetReturnValue() https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:199: // See https://crbug.com/708238 for tracking avoiding the hand-generated code. Add a comment about what spec this method is implementing. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:209: args.GetReturnValue().Set( Nit: v8SetReturnValue() https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:216: ScriptValue wrappedResponse = ScriptValue::from(scriptState, args[0]); ScriptValue wrappedResponse(scriptState, args[0]) will work? https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:217: args.GetReturnValue().Set( Nit: v8SetReturnValue() https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.h (right): https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.h:13: class MODULES_EXPORT WasmResponseExtensions { Add a class-level comment.
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Response.cpp (right): https://codereview.chromium.org/2780693003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Response.cpp:221: compileFromResponseCallback(args); On 2017/04/07 02:52:29, yhirano wrote: > On 2017/04/07 02:39:36, yhirano wrote: > > On 2017/04/05 17:17:49, Mircea Trofin wrote: > > > On 2017/04/05 12:02:50, yhirano wrote: > > > > On 2017/04/05 05:12:22, Mircea Trofin wrote: > > > > > On 2017/04/05 03:48:03, yhirano (slow) wrote: > > > > > > I think this should work as > > > > > > wasmCompileOverLoad(Promise.resolve(args[0])) > > > > > > > > > > > > instead of > > > > > > compileFromResponseCallback(args) > > > > > > > > > > > > . > > > > > > > > > > So I understand - what would be the motivation - ensuring one path into > > > > > compileFromResponseCallback? (I wanted to avoid the extra prep work, > that > > > > would > > > > > be my motivation for the current state of things). > > > > > > > > The current implementation calls compileFromResponseCallback synchronously > > and > > > > my suggestion doesn't. The latter is correct based on > > > > https://www.w3.org/2001/tag/doc/promises-guide#resolve-arguments. > > > > > > Thanks for the clarification and the pointer. I made the change > > > > > > Still not sure though it buys us much: compileFromResponseCallback just > starts > > > the asynchronous fetch and returns a promise; and we still need to do > > > typechecking (as the CL demonstrates), to determine if the API needs to call > > > back to v8's original behavior: if the parameter passed to the blink-side > > > overload isn't a promise nor a Response, we still need to do the v8 > behavior. > > > > > > Am I missing something? > > > > Ah, I see, thank you. Returning false means redirecting to the original > > function. I have another question about the interface, but will ask on anther > > thread. > > Sorry, I was wrong. Please replace the last response with below. > > > Ah, I see, thank you. Returning false means redirecting to the original > function. We need the type checking code. > > But I still think compile(response) should behave exactly same as > compile(Promise.resolve(response)), and that should be clear without closer look > at compileFromResponseCallback. In the current implmentation, there is a > microtask timing difference, right? Ah! I think I see what you mean. The example at that link, basically, says it all. I think I got it now. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/ModulesInitializer.cpp:65: WasmResponseExtensions::initialize(V8PerIsolateData::mainThreadIsolate()); On 2017/04/07 04:47:46, haraken wrote: > > Would you move this call to V8Initializer? I think that's not allowed, layering-wise, see Source/bindings/core/DEPS, it explicitly disallows a dependency on modules. In fact, that's how I got to the Response hack initially - after having tried and failed to make the change in V8Initializer :) https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchDataLoader.h (right): https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchDataLoader.h:8: #include "bindings/core/v8/ScriptPromiseResolver.h" On 2017/04/07 02:39:36, yhirano wrote: > These two lines are no longer needed. Done. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchDataLoader.h:50: virtual void didFetchDataLoadedCustomFormat() { NOTREACHED(); } On 2017/04/07 02:39:36, yhirano wrote: > // This function is called when a "custom" FetchDataLoader (not listed below) > finishes loading. Done. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/wasm/BUILD.gn (right): https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/BUILD.gn:10: "WasmResponseExtensions.h", On 2017/04/07 04:47:46, haraken wrote: > > WasmResponseExtensions is heavily using V8 APIs, so would you move the files to > Source/bindings/core/v8/? Same dependency problem as with moving initialization to V8Initializer. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp (right): https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2017/04/07 04:47:46, haraken wrote: > > 2017 Done. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:11: #include "wtf/RefPtr.h" On 2017/04/07 02:39:37, yhirano wrote: > + modules/fetch/FetchDataLoader.h > + bindings/core/v8/ScriptPomise.h > + bindings/core/v8/ScriptPomiseResolver.h > + platform/heap/Handle.h > done. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:22: FetchDataLoaderAsWasmModule(v8::Isolate* isolate, On 2017/04/07 04:47:46, haraken wrote: > > Remove the isolate parameter. You can use scriptSTate->isolate(). Done. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:39: // buffer is allocated by beginRead and de-allocated by endRead. On 2017/04/07 04:47:46, haraken wrote: > > beginRead just sets buffer to some address in an already-allocated buffer, and > endRead does nothing on the buffer, right? Yes, looking at https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/fetch/... and also from other implementations of FetchDataLoader, it seems that beginRead does what you mentioned. endRead invalidates the buffer. We can't take ownership of the buffer, hence the wasm side copies. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:103: void ThrowTypeError() { On 2017/04/07 04:47:46, haraken wrote: > > throwTypeError > > I'd prefer "rejectPromise" or something though. Done. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:135: Member<ScriptPromiseResolver> m_resolver; On 2017/04/07 02:39:37, yhirano wrote: > You don't need this member. Done. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:156: args.GetReturnValue().Set( On 2017/04/07 04:47:47, haraken wrote: > > Nit: v8SetReturnValue() I like that! Done. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:160: "Promise argument must produce a Response object")) On 2017/04/07 02:39:36, yhirano wrote: > Is "This function must be called with Promise<Response>" clearer? Done. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:186: "Promise argument must produce a Response object")); On 2017/04/07 02:39:37, yhirano wrote: > I think this message is confusing (from a different reason than the above). This > is the case where the response has a null body. Done. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:189: args.GetReturnValue().Set(promise.v8Value()); On 2017/04/07 04:47:46, haraken wrote: > > Nit: v8SetReturnValue() Done. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:199: // See https://crbug.com/708238 for tracking avoiding the hand-generated code. On 2017/04/07 04:47:46, haraken wrote: > > Add a comment about what spec this method is implementing. Done. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:209: args.GetReturnValue().Set( On 2017/04/07 04:47:47, haraken wrote: > > Nit: v8SetReturnValue() Done. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:216: ScriptValue wrappedResponse = ScriptValue::from(scriptState, args[0]); On 2017/04/07 04:47:46, haraken wrote: > > ScriptValue wrappedResponse(scriptState, args[0]) will work? Ya, but I moved that line in the resolve() call below, I think it reads closer to the "Promise.resolve(parameter)" js equivalent. So I'm still using the ::from there, to avoid a temp variable. https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:217: args.GetReturnValue().Set( On 2017/04/07 04:47:46, haraken wrote: > > Nit: v8SetReturnValue() Done.
https://codereview.chromium.org/2780693003/diff/320001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/wasm/wasm_response_apis.js (right): https://codereview.chromium.org/2780693003/diff/320001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/wasm/wasm_response_apis.js:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2780693003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/wasm/BUILD.gn (right): https://codereview.chromium.org/2780693003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2780693003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp (right): https://codereview.chromium.org/2780693003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:203: V8SetReturnValue(args, ScriptPromise::cast(scriptState, args[0]).then(compileCallback).v8Value()); return;
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/wasm/BUILD.gn (right): https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/BUILD.gn:10: "WasmResponseExtensions.h", On 2017/04/07 06:11:16, Mircea Trofin wrote: > On 2017/04/07 04:47:46, haraken wrote: > > > > WasmResponseExtensions is heavily using V8 APIs, so would you move the files > to > > Source/bindings/core/v8/? > > Same dependency problem as with moving initialization to V8Initializer. Ah, makes sense. Then can you move the files to Source/bindings/modules/v8/ ? It's fine to call the initialize method in ModulesInitializer. https://codereview.chromium.org/2780693003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp (right): https://codereview.chromium.org/2780693003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:44: // buffer is allocated by beginRead and de-allocated by endRead. allocated / deallocated sounds misleading to me. beginRead / endRead never allocates / deallocates. https://codereview.chromium.org/2780693003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:78: trycatch.Reset(); I don't fully understand why you need to reset it. You're not using trycatch after this line, right?
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/wasm/BUILD.gn (right): https://codereview.chromium.org/2780693003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/BUILD.gn:10: "WasmResponseExtensions.h", On 2017/04/07 10:22:40, haraken wrote: > On 2017/04/07 06:11:16, Mircea Trofin wrote: > > On 2017/04/07 04:47:46, haraken wrote: > > > > > > WasmResponseExtensions is heavily using V8 APIs, so would you move the files > > to > > > Source/bindings/core/v8/? > > > > Same dependency problem as with moving initialization to V8Initializer. > > Ah, makes sense. Then can you move the files to Source/bindings/modules/v8/ ? > > It's fine to call the initialize method in ModulesInitializer. That worked, and... bonus! We can then move the initialization there, too, just like we do for serialization; and we can get rid of BUILD.gn and DEPS. Thanks! https://codereview.chromium.org/2780693003/diff/320001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/wasm/wasm_response_apis.js (right): https://codereview.chromium.org/2780693003/diff/320001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/wasm/wasm_response_apis.js:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/04/07 07:39:47, yhirano wrote: > 2017 Done. https://codereview.chromium.org/2780693003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/wasm/BUILD.gn (right): https://codereview.chromium.org/2780693003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2017/04/07 07:39:47, yhirano wrote: > 2017 Done. https://codereview.chromium.org/2780693003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp (right): https://codereview.chromium.org/2780693003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:44: // buffer is allocated by beginRead and de-allocated by endRead. On 2017/04/07 10:22:40, haraken wrote: > > allocated / deallocated sounds misleading to me. beginRead / endRead never > allocates / deallocates. > True - rephrased. https://codereview.chromium.org/2780693003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:78: trycatch.Reset(); On 2017/04/07 10:22:40, haraken wrote: > > I don't fully understand why you need to reset it. You're not using trycatch > after this line, right? Granted, the destructor of TryCatch clears the exception (https://cs.chromium.org/chromium/src/v8/include/v8.h?l=7997), but perhaps my paranoia is to blame :) I refactored a bit and commented. https://codereview.chromium.org/2780693003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/wasm/WasmResponseExtensions.cpp:203: On 2017/04/07 07:39:47, yhirano wrote: > V8SetReturnValue(args, ScriptPromise::cast(scriptState, > args[0]).then(compileCallback).v8Value()); > return; Nice! Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mtrofin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2780693003/#ps340001 (title: "simplify")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1491586990008820,
"parent_rev": "8927c43c9dfd0fa73d092eea903f9f3a1f3292f1", "commit_rev":
"380bc71f286663403e7706aedcf66f724c89b584"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] response-based compile APIs Spec available at: https://github.com/WebAssembly/design/blob/master/Web.md#webassemblycompile This CL introduces the WebAssembly.compile overload in Blink. Notably, we do not currently check the mime type (chromium:707149). The V8 side is still under development. We currently exercise a skeleton API with a trivial implementation. Once the full implementation is implemented, we will enhance the blink side of the feature - for instance, we can introduce back pressure information. BUG=chromium:697028 ========== to ========== [wasm] response-based compile APIs Spec available at: https://github.com/WebAssembly/design/blob/master/Web.md#webassemblycompile This CL introduces the WebAssembly.compile overload in Blink. Notably, we do not currently check the mime type (chromium:707149). The V8 side is still under development. We currently exercise a skeleton API with a trivial implementation. Once the full implementation is implemented, we will enhance the blink side of the feature - for instance, we can introduce back pressure information. BUG=chromium:697028 Review-Url: https://codereview.chromium.org/2780693003 Cr-Commit-Position: refs/heads/master@{#462917} Committed: https://chromium.googlesource.com/chromium/src/+/380bc71f286663403e7706aedcf6... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:340001) as https://chromium.googlesource.com/chromium/src/+/380bc71f286663403e7706aedcf6...
Message was sent while issue was closed.
https://codereview.chromium.org/2780693003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/modules/v8/wasm/WasmResponseExtensions.cpp (right): https://codereview.chromium.org/2780693003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/wasm/WasmResponseExtensions.cpp:210: ScriptPromiseResolver* scriptPromiseResolver = You don't need this. https://codereview.chromium.org/2780693003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/wasm/WasmResponseExtensions.cpp:218: ScriptPromise parameterAsPromise = scriptPromiseResolver->promise(); ditto https://codereview.chromium.org/2780693003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/wasm/WasmResponseExtensions.cpp:224: scriptPromiseResolver->resolve(ScriptValue::from(scriptState, args[0])); ditto
Message was sent while issue was closed.
On 2017/04/10 04:38:54, yhirano wrote: > https://codereview.chromium.org/2780693003/diff/340001/third_party/WebKit/Sou... > File > third_party/WebKit/Source/bindings/modules/v8/wasm/WasmResponseExtensions.cpp > (right): > > https://codereview.chromium.org/2780693003/diff/340001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/modules/v8/wasm/WasmResponseExtensions.cpp:210: > ScriptPromiseResolver* scriptPromiseResolver = > You don't need this. > > https://codereview.chromium.org/2780693003/diff/340001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/modules/v8/wasm/WasmResponseExtensions.cpp:218: > ScriptPromise parameterAsPromise = scriptPromiseResolver->promise(); > ditto > > https://codereview.chromium.org/2780693003/diff/340001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/modules/v8/wasm/WasmResponseExtensions.cpp:224: > scriptPromiseResolver->resolve(ScriptValue::from(scriptState, args[0])); > ditto yup - already fixed: https://codereview.chromium.org/2805403002/ |
