1472324 - Update Worklet: s/import/addModule

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Arnaud Bienner]

Spec has been updated and |import| has been replaced by |addModule|, which has an extra WorkletOptions argument [1].
Looks like this arguments will be used by the underlying Fetch request.
In our case, I think it means passing it to RequestInit init.mCredentials object in WorkletFetchHandler (Worklet.cpp) when creating the FetchRequest; in addition to updating the WebIDL (and maybe bindings?)

I'm not sure if more work is required for the Fetch to work with new types of RequestCredentials in the Worklet scope.
Andrea: any thoughts?

[1]: https://drafts.css-houdini.org/worklets/#dictdef-workletoptions

Comment 1

[Andrea Marchesini [:baku]]

> I'm not sure if more work is required for the Fetch to work with new types
> of RequestCredentials in the Worklet scope.
> Andrea: any thoughts?

As first step, what you are saying is correct and I'm happy to review a patch, if you want to work on this renaming.

But there is still a bigger issue to solve related to the importing of scripts as module in workers (and worklet). See bug 1247687. Currently we support script modules only on main-thread via <script type="module">. In order to support script modules in workers/worklet, we must unify WorkletFetchHandler, workerinternals::Load/LoadMainScript and mozilla::dom::ScriptLoader.

Comment 2

[Karl Tomlinson (:karlt)]

Thank you, Andrea.  I filed bug 1473463 to track module evaluation for
worklets.

Arnaud, the change to addModule would be useful now, so that we don't need to
use the wrong syntax in any new tests.

RequestInit has a public mCredentials member, and so I assume that can be set
near here
https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/dom/worklet/Worklet.cpp#126

Comment 3

[Arnaud Bienner]

Attachment: Bug 1472324: Update Worklet: s/import/addModule.

Review commit: https://reviewboard.mozilla.org/r/255954/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255954/

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8990965 [details]
Bug 1472324: Update Worklet: s/import/addModule.

https://reviewboard.mozilla.org/r/255954/#review262814

File a bug to implement module support for worklet.

Comment 5

[Arnaud Bienner]

I've updated the webidl + corresponding code and tests.
I'm not sure uf we want to test the WorkletOptions' credentials attribute more, knowing that this is already tested in dom/tests/mochitest/fetch/test_fetch_cors.js
I believe a simple test to make sure the credentials attribute is passed correctly would be nice at least, what do you think?

On a side note, I see the default credentials value for worklet is "same-origin" [1] but Fetch specification says the default value is "omit" for Request object. I believe it's technically fine, though not coherent.
But the weird thing is that Firefox has "same-origin" as default value for Request. I wonder whether the Request spec has changed recently.

[1]: https://drafts.css-houdini.org/worklets/#dictdef-workletoptions
[2]: https://fetch.spec.whatwg.org/#concept-request-credentials-mode

Comment 6

[Arnaud Bienner]

(In reply to Andrea Marchesini [:baku] from comment #4)
> File a bug to implement module support for worklet.

I think that bug 1473463 Karl already filled is what you are looking for, right?

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to Arnaud Bienner from comment #5)
> I believe a simple test to make sure the credentials attribute is passed
> correctly would be nice at least, what do you think?

I expect
testing/web-platform/tests/worklets/resources/credentials-tests.js would
test this, but there are no tests for AudioWorklet yet.
If some would pass for an AudioWorklet now, then adding tests in the parent
directory would be good.  (It's easier to add tests if you can run them.)
See
https://developer.mozilla.org/en-US/docs/Mozilla/QA/web-platform-tests#Setting_prefs
(Those tests assume paintWorklet is on the CSS object, but Gecko currently
has paintWorklet on window and so 'paint' worklets are not going to work
well right now.)

Web platform tests are typically best for spec-compliance tests that do not
depend on browser-specific behavior.

> On a side note, I see the default credentials value for worklet is
> "same-origin" [1] but Fetch specification says the default value is "omit"
> for Request object. I believe it's technically fine, though not coherent.
> But the weird thing is that Firefox has "same-origin" as default value for
> Request. I wonder whether the Request spec has changed recently.

It seems browsers are changing their behavior before the spec is changed.
https://bugzilla.mozilla.org/show_bug.cgi?id=1394399
https://github.com/whatwg/fetch/pull/585

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8990965 [details]
Bug 1472324: Update Worklet: s/import/addModule.

https://reviewboard.mozilla.org/r/255954/#review262988

Thanks!

Comment 9

[Arnaud Bienner]

Comment on attachment 8990965 [details]
Bug 1472324: Update Worklet: s/import/addModule.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255954/diff/1-2/

Comment 10

[Arnaud Bienner]

Patch update to add test in wpt.
All credentials tests are green.
Some of the other audio worklet tests are green, others not, so I've left them in FAIL test for now.
I could have turn the pref on only for credentials test, but IMO it's better to activate as much tests as possible, to make sure we don't introduce regressions on what is currently working.

Note that for pref to work on wpt, they should be in all.js (which also make them available in about:config) unlike mochitest.

Comment 11

[Karl Tomlinson (:karlt)]

Arnaud, I think it's best to add the Audio Worklet tests in a separate patch.
Changes to wpt test files are automatically upstreamed to
https://github.com/web-platform-tests/wpt
and so they are better with their own commit messages.

Turning on the prefs to activate as much as possible seems sensible.

An OfflineAudioContext may be better given
https://bugzilla.mozilla.org/show_bug.cgi?id=1476514#c1

Even the passing tests are going to unexpectedly fail when the changes are
uplifted to beta due to
https://hg.mozilla.org/mozilla-central/rev/f99b99cc076d06251bf16725647d52f65bf17d4d#l1.13

The release_or_beta token can be used in conditionals in the manifest files.
Assuming that works for the prefs too, I guess it's simplest to just disable
the prefs when release_or_beta, to make it easier to predict the result.

I'm surprised the prefs need to be in all.js.  I'd prefer not to expose them
there until ready for testing.  I wonder whether defining them in
testing/profiles/unittest/user.js is sufficient.

Comment 12

[Arnaud Bienner]

(In reply to Karl Tomlinson (:karlt) from comment #11)
> Arnaud, I think it's best to add the Audio Worklet tests in a separate patch.

So the first version of my patch (before activating the wpt tests) was OK to land?
How can I easily modify the patch attachment to push to try/land only this part? 

> Turning on the prefs to activate as much as possible seems sensible.
> 
> An OfflineAudioContext may be better given
> https://bugzilla.mozilla.org/show_bug.cgi?id=1476514#c1

Not sure what you meant here: do you imply that tests should run using an AudioWorklet from OfflineAudioContext, instead of AudioContext? (regarding my changes to worklet-test-utils.js)
 
> I'm surprised the prefs need to be in all.js.  I'd prefer not to expose them
> there until ready for testing.  I wonder whether defining them in
> testing/profiles/unittest/user.js is sufficient.

I will try that. But I'm 100% sure adding them to all.js fixed the issue. Hopefully adding them to user.js will work too.

Comment 13

[Karl Tomlinson (:karlt)]

(In reply to Arnaud Bienner from comment #12)
> (In reply to Karl Tomlinson (:karlt) from comment #11)
> So the first version of my patch (before activating the wpt tests) was OK to
> land?

Yes, it was.

Sorry if what I said suggested otherwise.  Mozilla has a convention where the
patch author indicates when the patch is ready to land, usually through
checkin-needed.  Often this is unnecessary procedure, but it gives the author
the opportunity to wait for final try results, any other patches that need to
land first, etc.

> How can I easily modify the patch attachment to push to try/land only this
> part? 

https://reviewboard.mozilla.org/r/255952/ (Review Summary) has a list of
previous commits.  It shows the old commit was 64be88359f1e.  If this is still
in your tree, then (assuming the reviewboard extension is enabled) the
following would tell reviewboard that you want the same commit again.

hg push -c 64be88359f1e ssh://reviewboard-hg.mozilla.org/gecko

For try the same revision cannot be pushed again.  If it has not yet been
pushed then

hg push -r 64be88359f1e ssh://hg.mozilla.org/try

If 64be88359f1e is no longer in your tree, you can first fetch with

hg pull -r 64be88359f1e https://reviewboard-hg.mozilla.org/gecko

Alternatively, or if reviewboard just gets confused with the above, hg update
can be used to checkout the parent of 4fb144bea0f3, and then hg revert is
useful for changing just some files or directories to their state in
4fb144bea0f3, after which a new commit can be made.

Sometimes I use "hg export <commit> | hg import -" to copy one changeset.

> > Turning on the prefs to activate as much as possible seems sensible.
> > 
> > An OfflineAudioContext may be better given
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1476514#c1
> 
> Not sure what you meant here: do you imply that tests should run using an
> AudioWorklet from OfflineAudioContext, instead of AudioContext? (regarding
> my changes to worklet-test-utils.js)

That's what I'm suggesting, yes.
There's nothing wrong with AudioContext, but I expect we'll have
OfflineAudioContext passing tests first, and I suspect AudioContext may
have problems in Chromium's test harness.

> > I'm surprised the prefs need to be in all.js.
> 
> I will try that. But I'm 100% sure adding them to all.js fixed the issue.

Thank you for tracking that down.

Comment 14

[Arnaud Bienner]

Comment on attachment 8990965 [details]
Bug 1472324: Update Worklet: s/import/addModule.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255954/diff/2-3/

Comment 15

[Arnaud Bienner]

Thanks Karl for the tips!
Green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae063801b7815bbf2797e3182deac62f39fb6ab2

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b5946bda04c
Update Worklet: s/import/addModule. r=baku,karlt

Comment 17

[Arnaud Bienner]

Karl: how do you think we should proceed with the wpt (audio)worklet tests? Should we activate them now? (as I started to do in the second part of the patch) or do you prefer to leave that for later?
I believe the more activated tests we have, the better it is, but I'm not sure if there are problems that should be taken into account, in addition to what you already mentioned (disabling the test for beta/release and trying to avoid having the pref in all.js).
If you're OK, I will open a follow up bug.

Comment 18

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7b5946bda04c

Comment 19

[Karl Tomlinson (:karlt)]

(In reply to Arnaud Bienner from comment #17)
> Karl: how do you think we should proceed with the wpt (audio)worklet tests?
> Should we activate them now? (as I started to do in the second part of the
> patch)

Yes, please do that.

> I believe the more activated tests we have, the better it is, but I'm not
> sure if there are problems that should be taken into account, in addition to
> what you already mentioned (disabling the test for beta/release and trying
> to avoid having the pref in all.js).

I'm not expecting additional problems.

It may also be worthwhile setting the prefs in
testing/web-platform/meta/webaudio
as there are tests in at least these places

testing/web-platform/tests/webaudio/idlharness.https.html
testing/web-platform/tests/webaudio/the-audio-api/the-audioworklet-interface

That would require similar conditional expectation data, and
that is really separate from adding the tests in tests/worklet.

The only other possible problem I can imagine is if enabling the pref causes a
test to stall or throw, for example, in a way that means that currently
passing tests no longer run.  I don't expect that is likely.

> If you're OK, I will open a follow up bug.

Sounds good, thanks.

Comment 20

[Chris Mills [:cmills]]

We have docs for these:

https://developer.mozilla.org/en-US/docs/Web/API/Worklet
https://developer.mozilla.org/en-US/docs/Web/API/Worklet/addModule

I've edited them and made a few fixes, but I've also added a note to the Fx63 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/63#Worklets

And submitted a PR to add browser compat data for this interface:
https://github.com/mdn/browser-compat-data/pull/2755

Let me know if you need anything else; thanks!

Comment 21

[Karl Tomlinson (:karlt)]

Thank you, Chris.
Worklets are disabled by pref, and not in a usable state on any branch (incl Nightly), and so I think a release note at this stage would just lead to confusion.

Comment 22

[Chris Mills [:cmills]]

(In reply to Karl Tomlinson (:karlt) from comment #21)
> Thank you, Chris.
> Worklets are disabled by pref, and not in a usable state on any branch (incl
> Nightly), and so I think a release note at this stage would just lead to
> confusion.

Oops, I missed that they were disabled by default. I've removed the note. And one of our contributors is working on an update for the compat data that specifies this.