Closed
Bug 1290021
Opened 8 years ago
Closed 8 years ago
Implement a prototype version of Houdini "Worklets Level 1" spec
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: baku, Assigned: baku)
References
(Blocks 2 open bugs, )
Details
Attachments
(6 files, 5 obsolete files)
12.47 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
25.43 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
17.14 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
13.98 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
5.16 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
725 bytes,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
With this first patch I implemented some Worklet WebIDL things and the importing of modules. The importing doesn't follow the spec yet (no cache, for instance, wrong error code returned, etc). For testing only, I also exposed window.createWorklet(); This is similar to what the spec calls "fakeWorket1" and "fakeWorklet2".
Attachment #8775485 -
Flags: feedback?(dholbert)
Assignee | ||
Comment 2•8 years ago
|
||
This second patch is about WorkletGlobalScope. The imported module is executed using this new global scope. I still use the wrong compilation flag, and the console is not really exposed yet.
Attachment #8775486 -
Flags: feedback?(dholbert)
Assignee | ||
Comment 3•8 years ago
|
||
Daniel, these 2 feedback requests are just to inform about the progress. Don't spend time to read this code yet.
Comment 4•8 years ago
|
||
Thanks! Great to see progress here.
Summary: Implement Worklet spec → Implement Houdini "Worklets Level 1" spec
Updated•8 years ago
|
Attachment #8775486 -
Flags: feedback?(dholbert)
Updated•8 years ago
|
Attachment #8775485 -
Flags: feedback?(dholbert)
Updated•8 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8775486 -
Attachment is obsolete: true
Assignee | ||
Comment 6•8 years ago
|
||
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8775485 [details] [diff] [review] WIP 1 - Worklet webIDL interface Review of attachment 8775485 [details] [diff] [review]: ----------------------------------------------------------------- This patch implements: 1. window.createWorklet() for testing only 2. Worklet.webidl 3. fetch of the script. 4. tests Missing steps: having an hashtable of fetched scripts as the spec says. This will be done in a separate patch.
Attachment #8775485 -
Flags: feedback?(bugs)
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8781941 [details] [diff] [review] part 2 - WorkletGlobalScope Review of attachment 8781941 [details] [diff] [review]: ----------------------------------------------------------------- This patch implements the WorkletGlobalScope and it executes the fetched script.
Attachment #8781941 -
Flags: feedback?(bugs)
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8781942 [details] [diff] [review] part 3 - Console API in worklet Review of attachment 8781942 [details] [diff] [review]: ----------------------------------------------------------------- Console API exposed to WorkletGlobalScope.
Attachment #8781942 -
Flags: feedback?(bugs)
Comment 10•8 years ago
|
||
Comment on attachment 8775485 [details] [diff] [review] WIP 1 - Worklet webIDL interface >+ static already_AddRefed<Promise> >+ Fetch(Worklet* aWorklet, const nsAString& aModuleURL, ErrorResult& aRv) >+ { >+ RefPtr<Promise> promise = Promise::Create(aWorklet->GetParentObject(), aRv); >+ if (NS_WARN_IF(aRv.Failed())) { >+ return nullptr; >+ } >+ >+ RequestOrUSVString request; >+ request.SetAsUSVString().Rebind(aModuleURL.Data(), aModuleURL.Length()); >+ >+ RequestInit init; >+ >+ RefPtr<Promise> fetchPromise = >+ FetchRequest(aWorklet->GetParentObject(), request, init, aRv); Does FetchRequest deal with url being relative or absolute? >+ ResolveFetch(JSContext* aCx, JS::Handle<JS::Value> aValue) >+ { >+ if (!aValue.isObject()) { >+ mPromise->MaybeReject(NS_ERROR_FAILURE); >+ return; >+ } >+ >+ RefPtr<Response> response; >+ nsresult rv = UNWRAP_OBJECT(Response, &aValue.toObject(), response); >+ if (NS_WARN_IF(NS_FAILED(rv))) { >+ mPromise->MaybeReject(rv); >+ return; >+ } >+ >+ if (!response->Ok()) { >+ mPromise->MaybeReject(NS_ERROR_FAILURE); // TODO, error TBD >+ return; >+ } >+ >+ ErrorResult error; >+ RefPtr<Promise> promise = response->Text(error); >+ if (NS_WARN_IF(error.Failed())) { >+ mPromise->MaybeReject(error); >+ return; >+ } >+ >+ promise->AppendNativeHandler(this); >+ mState = eRead; >+ } >+ >+ void >+ ResolveRead(JSContext* aCx, JS::Handle<JS::Value> aValue) >+ { >+ if (!aValue.isString()) { >+ mPromise->MaybeReject(NS_ERROR_FAILURE); >+ return; >+ } >+ >+ nsAutoJSString str; >+ if (!str.init(aCx, aValue)) { >+ mPromise->MaybeReject(NS_ERROR_FAILURE); >+ return; >+ } >+ >+ // TODO: do something with this string... Hmm, we really have no way to avoid JS stuff with Fetch. If so, I wouldn't use Fetch stuff for loading anything in C++. This is too heavy. Could we add some stuff to Fetch so that it can deal with C++ better? >+worklet.import("common.js").then(function() { >+ ok("Import should load a resource."); >+ SimpleTest.finish(); >+}); should test also some error case
Attachment #8775485 -
Flags: feedback?(bugs) → feedback+
Comment 11•8 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #10) > >+ { > >+ if (!aValue.isString()) { > >+ mPromise->MaybeReject(NS_ERROR_FAILURE); > >+ return; > >+ } > >+ > >+ nsAutoJSString str; > >+ if (!str.init(aCx, aValue)) { > >+ mPromise->MaybeReject(NS_ERROR_FAILURE); > >+ return; > >+ } > >+ > >+ // TODO: do something with this string... > Hmm, we really have no way to avoid JS stuff with Fetch. > If so, I wouldn't use Fetch stuff for loading anything in C++. This is too > heavy. > Could we add some stuff to Fetch so that it can deal with C++ better? > Hmm, or perhaps we don't end up creating a copy of the string after all. Could you ensure that? Though, still very ugly to go through JSAPI stuff here.
Comment 12•8 years ago
|
||
It does make a copy bz> smaug: Passing the C++ thing to JS will use an external string, but passing from JS to C++ will copy nowadays bz> smaug: we used to not copy in some cases, but SpiderMonkey made that really hard....
Comment 13•8 years ago
|
||
Comment on attachment 8781941 [details] [diff] [review] part 2 - WorkletGlobalScope Fine. but need to still figure out how to deal with cycle collection. Since this is now for main thread only, using main thread CC is fine, but will need to support other, non-main-thread, non-worker-thread threads.
Attachment #8781941 -
Flags: feedback?(bugs) → feedback+
Comment 14•8 years ago
|
||
padenot might have some feedback on this stuff too.
Comment 15•8 years ago
|
||
Comment on attachment 8781942 [details] [diff] [review] part 3 - Console API in worklet >+WorkletGlobalScope::GetConsole(ErrorResult& aRv) >+{ >+ if (!mConsole) { >+ mConsole = Console::Create(mWindow, aRv); >+ if (NS_WARN_IF(aRv.Failed())) { >+ return nullptr; >+ } >+ } This doesn't look right. The console object will have mWindow as the parent. It should have WorkletGlobalScope as parent. This just happens to work since this stuff is same origin and all. But I do think we should figure out the correct setup for non-main-thread worklets from beginning. So, f+, but needs work
Attachment #8781942 -
Flags: feedback?(bugs) → feedback+
Comment 16•8 years ago
|
||
FWIW, blink-dev had just email "Intent to Implement: AudioWorklet".
Assignee | ||
Updated•8 years ago
|
Blocks: audioworklet
Assignee | ||
Comment 17•8 years ago
|
||
Daniel, I guess the next step is to have a valid use-case for testing this code. I don't want to land this as it is because it's not connected to anything. I would like to see some other pieces of the Houdini architecture connected to Worklet. Or, it can also be the audioWorklet.
Flags: needinfo?(dholbert)
Updated•8 years ago
|
Blocks: houdini-painting
Comment 18•8 years ago
|
||
"CSS Paint" (bug 1302328) would be our first CSS Houdini usage of this abstraction. We were originally (optimistically) planning on having an intern work on CSS Paint this summer, but another prerequisite feature turned out to be thornier & take up more time than our optimistic guess, so CSS Paint ended up being out-of-scope for that internship. So: I don't immediately know how soon a layout/css full-timer will have cycles to work on CSS Paint, so I don't have a timeframe to give you right away. (It might be soon, I just don't know.) So, audioWorklet might be the first usage of this.
Flags: needinfo?(dholbert)
Comment 19•8 years ago
|
||
baku: would you be OK landing this but keeping dom.worklet.enabled turned off? I'm still working on staffing up the rendering bits of Houdini Paint, but it would be good to work off of Nightly as we go along. Thanks!
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 20•8 years ago
|
||
This patch introduces the Worklet object and it tests them. The fetch requests are correctly done but the content is not handled yet.
Flags: needinfo?(amarchesini)
Attachment #8801164 -
Flags: review?(bugs)
Assignee | ||
Updated•8 years ago
|
Attachment #8775485 -
Attachment is obsolete: true
Assignee | ||
Comment 21•8 years ago
|
||
I'm not sure about how I set the compileOptions.
Attachment #8781941 -
Attachment is obsolete: true
Attachment #8801165 -
Flags: review?(bugs)
Assignee | ||
Comment 22•8 years ago
|
||
This is main-thread only. I didn't apply your comments because so far I'm not considering to port it out of the main-thread.
Attachment #8781942 -
Attachment is obsolete: true
Attachment #8801169 -
Flags: review?(bugs)
Assignee | ||
Comment 23•8 years ago
|
||
Attachment #8801635 -
Flags: review?(bugs)
Assignee | ||
Comment 24•8 years ago
|
||
This is not part of the spec, but it's very useful to have.
Attachment #8801640 -
Flags: review?(bugs)
Assignee | ||
Comment 25•8 years ago
|
||
> baku: would you be OK landing this but keeping dom.worklet.enabled turned
> off? I'm still working on staffing up the rendering bits of Houdini Paint,
> but it would be good to work off of Nightly as we go along. Thanks!
As you can see, I want to have these patches in m-i.
I'm OK if you want to use this first implementation to continue with the Houdini spec, but keep in mind that it could be that we have to change the worklet code a lot if we want to go out of main-thread.
Comment 26•8 years ago
|
||
Comment on attachment 8801640 [details] [diff] [review] part 5 - dump() This must be behind a pref, perhaps the same pref as window.dump.
Comment 27•8 years ago
|
||
Comment on attachment 8801640 [details] [diff] [review] part 5 - dump() And without adding the pref now, we'll forget it easily.
Attachment #8801640 -
Flags: review?(bugs) → review-
Comment 28•8 years ago
|
||
Comment on attachment 8801164 [details] [diff] [review] part 1 - Worklet webIDL interface > >+ // Exposed only for testing Useless comment. The method is exposed whenever the pref used in .webidl is set to true, > >+// For testing worklet only >+partial interface Window { I guess this comment is fine. You might want to make it even more stronger, saying that it must not be enabled anywhere by default.
Attachment #8801164 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 29•8 years ago
|
||
Attachment #8801640 -
Attachment is obsolete: true
Attachment #8801749 -
Flags: review?(bugs)
Updated•8 years ago
|
Attachment #8801169 -
Flags: review?(bugs) → review+
Comment 30•8 years ago
|
||
Comment on attachment 8801749 [details] [diff] [review] part 5 - dump() >+// Mozilla extensions >+partial interface WorkletGlobalScope { Func=... >+ void dump(optional DOMString str); >+void >+WorkletGlobalScope::Dump(const Optional<nsAString>& aString) const >+{ >+ if (!nsContentUtils::DOMWindowDumpEnabled()) { You should move this check to .webidl. We don't want to expose .dump() in the worklet scope. (No need to make the same mistakes we've done with Window) > SimpleTest.waitForExplicitFinish(); > SpecialPowers.pushPrefEnv( > {"set": [["dom.worklet.testing.enabled", true], > ["dom.worklet.enabled", true]]}, >- function() { loadTest("file_console.html"); }); >+ function() { loadTest("file_dump.html"); }); So this test never tests anything. I mean it is missing ok() or is() checks. I think this will cause orange. At least add dummy ok()
Attachment #8801749 -
Flags: review?(bugs) → review+
Comment 31•8 years ago
|
||
Comment on attachment 8801635 [details] [diff] [review] part 4 - cache for the imports I'm a bit worried about the lifetime management. Worklet has nsRefPtrHashtable<nsCStringHashKey, WorkletFetchHandler> mImportHandlers; and those handlers own Promises which then own global etc. The cycle should be killed when resolving or rejecting, so should be fine. But better to test this a bit on debug builds with export XPCOM_MEM_LEAK_LOG=1 to see that there aren't at least shutdown leaks. >+ // Maybe we already have an handler for this URI a handler This all is of course quite against the spec. Not dealing with module loading in general. But for testing this might ok. (though, it is unclear to me why we need this caching when we don't even have module loading.)
Attachment #8801635 -
Flags: review?(bugs) → review+
Comment 32•8 years ago
|
||
Comment on attachment 8801165 [details] [diff] [review] part 2 - WorkletGlobalScope This at least should use ModuleEvaluation I think, to be a bit closer to the spec. Could we even somehow reuse scriptloader for all this? jonco might have an opinion.
Attachment #8801165 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 33•8 years ago
|
||
jonco, this is set of patches is about a new API called Worklet. Can you please give me a feedback based on patch 2 and comment 32? Thanks
Flags: needinfo?(jcoppeard)
Comment 34•8 years ago
|
||
This doesn't fetch imports or evaluate the script as a module. This is probably fine for testing but I'm not sure we should expose this until it's more spec-compliant. It would be really great if we could reuse some of the code in the script loader (and would be useful for other efforts, e.g. bug 1308512). This is not trivial unfortunately. I think it would require factoring out the module loader into a separate class with a caller-provided a fetch interface passed in. Currently nsScriptLoader uses MozPromise whereas this uses Promise so that's something else that would need to be addressed.
Flags: needinfo?(jcoppeard)
Comment 35•8 years ago
|
||
None of this is ready to be exposed to web. The implementation is just to let layout folks play with worklets.
Comment 36•8 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #35) Ah OK. I do think we should at least parse the code as a module though.
Comment 37•8 years ago
|
||
Right. And we need bug 1311726 or something simlar sooner than later to actually expose Worklets to web. The current patches are just preliminary to fulfill comment 19.
Assignee | ||
Comment 38•8 years ago
|
||
smaug, do we want to wait for bug 1311726 or continue and change the code later?
Flags: needinfo?(bugs)
Comment 39•8 years ago
|
||
Can we somehow load a module without that? I'm just a bit worried that without supporting modules from the beginning, writing tests for worklets using this preliminary implementation would be rather useless.
Flags: needinfo?(bugs) → needinfo?(jcoppeard)
Comment 40•8 years ago
|
||
(In reply to Olli Pettay [:smaug] (reviewing overload, queue closed for a day or two) from comment #39) As long as the module doesn't import anything you can load it by calling JS::CompileModule to get a module record object, and then calling JS::ModuleDeclarationInstantiation and then JS::ModuleEvaluation on that.
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 41•8 years ago
|
||
What about if we land what we have and then we fix everything as follow up? And keep everything behind pref. In the meantime CSS team can continue the implementation of Houdini spec.
Flags: needinfo?(bugs)
Comment 42•8 years ago
|
||
I guess we can do that. Script handling isn't quite right so tests written based on it may break later. Please add some explicit check, at least some assertion, that the code can't be enabled in release builds. But do we have plans how to get worklets into shippable state? And this prototype implementation doesn't really let one to start implement the audio worklets, since those really need separate thread. (though, I don't know when we have plans to start implement that stuff)
Flags: needinfo?(bugs)
Assignee | ||
Comment 43•8 years ago
|
||
We don't have real plans for audioWorklet yet but I'm happy to bring this code in a better state if we have a roadmap. Maybe this can be a topic for the next work week?
Flags: needinfo?(padenot)
Assignee | ||
Comment 44•8 years ago
|
||
Attachment #8807135 -
Flags: review?(bugs)
Assignee | ||
Updated•8 years ago
|
Attachment #8801165 -
Flags: review- → review?(bugs)
Comment 45•8 years ago
|
||
Comment on attachment 8807135 [details] [diff] [review] part 6 - assertions You need to then also ensure the tests aren't run in beta or release
Attachment #8807135 -
Flags: review?(bugs) → review+
Comment 46•8 years ago
|
||
Also, when landing the patches, the commit message could say something about this being just a prototype version, so that no one thinks we have support for worklets yet.
Summary: Implement Houdini "Worklets Level 1" spec → Implement a prototype version of Houdini "Worklets Level 1" spec
Updated•8 years ago
|
Blocks: worklets-1
Comment 47•8 years ago
|
||
Comment on attachment 8801165 [details] [diff] [review] part 2 - WorkletGlobalScope >+ nsCOMPtr<nsIStreamLoader> loader; >+ rv = NS_NewStreamLoader(getter_AddRefs(loader), this); >+ if (NS_WARN_IF(NS_FAILED(rv))) { >+ mPromise->MaybeReject(rv); >+ return; >+ } >+ >+ rv = pump->AsyncRead(loader, nullptr); >+ if (NS_WARN_IF(NS_FAILED(rv))) { >+ mPromise->MaybeReject(rv); >+ return; >+ } >+ >+ nsCOMPtr<nsIThreadRetargetableRequest> rr = do_QueryInterface(pump); >+ if (rr) { >+ nsCOMPtr<nsIEventTarget> sts = >+ do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID); >+ rv = rr->RetargetDeliveryTo(sts); >+ if (NS_FAILED(rv)) { >+ NS_WARNING("Failed to dispatch the nsIInputStreamPump to a IO thread."); >+ } >+ } >+ } ok, you copy some of the stuff from Fetch handling here. I wonder why you couldn't just use response->GetBlob and then read the data from that. But I guess this works too >+ >+ NS_IMETHOD >+ OnStreamComplete(nsIStreamLoader* aLoader, nsISupports* aContext, >+ nsresult aStatus, uint32_t aStringLen, >+ const uint8_t* aString) align the last argument >+ { MOZ_ASSERT that we're in the main thread
Attachment #8801165 -
Flags: review?(bugs) → review+
Comment 48•8 years ago
|
||
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4bed3a0e2332 Implement a prototype version of Houdini "Worklets Level 1" spec - part 1 - WebIDL interface, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/59c14257aba5 Implement a prototype version of Houdini "Worklets Level 1" spec - part 2 - WorkletGlobalScope, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/3769e657d104 Implement a prototype version of Houdini "Worklets Level 1" spec - part 3 - Console API in worklet, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/1946c5517376 Implement a prototype version of Houdini "Worklets Level 1" spec - part 4 - cache for the imports, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/c77883513897 Implement a prototype version of Houdini "Worklets Level 1" spec - part 5 - dump(), r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/f99b99cc076d Implement a prototype version of Houdini "Worklets Level 1" spec - part 6 - assertions, r=smaug
Comment 49•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4bed3a0e2332 https://hg.mozilla.org/mozilla-central/rev/59c14257aba5 https://hg.mozilla.org/mozilla-central/rev/3769e657d104 https://hg.mozilla.org/mozilla-central/rev/1946c5517376 https://hg.mozilla.org/mozilla-central/rev/c77883513897 https://hg.mozilla.org/mozilla-central/rev/f99b99cc076d
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment 50•8 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #43) > We don't have real plans for audioWorklet yet but I'm happy to bring this > code in a better state if we have a roadmap. Maybe this can be a topic for > the next work week? Yes, we can also implement some things right now. Ping me on IRC, I'm less busy that a week ago now, we can chat.
Flags: needinfo?(padenot)
Comment 51•8 years ago
|
||
I think it is a bit too early to document this (very experimental, behind a flag, and anyway would not be very useful). I've moved the dev-doc-needed flag to bug 1315239. And we will document when this bug is resolved (or when AudioWorklet, or Houdini CSS Painting API, lands, as we have a dev-doc-needed flag there too :-) )
Keywords: dev-doc-needed
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•