1204595 - Serialize via form several static properties for AudioNodeFront

Status: RESOLVED FIXED

(DevTools Graveyard :: Web Audio Editor, defect)

Description

[Jordan Santell [:jsantell] [@jsantell]]

We have a few async methods fetching data about these audio nodes when they first hit the front -- not only do these properties not change, we can just send them in the form so they're available immediately. Without this, we get a few lingering async calls during refresh which is the cause of a few intermittents, as well as preventing some other async stuff landing like automation views.

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1204595-cache-audionode-props.patch

This'll fix up a lot of race conditions and complexities in this tool, and will let us land the automation panel, I believe.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7101bde9037f

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8660980 [details] [diff] [review]
1204595-cache-audionode-props.patch

Review of attachment 8660980 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/webaudioeditor/test/browser_audionode-actor-bypassable.js
@@ +16,5 @@
> +  let expectedBypassability = [
> +    false, // AudioDestinationNode
> +    true, // AudioBufferSourceNode
> +    true, // ScriptProcessorNode
> +    true, // AnalyserNode 

Nit: whitespace, other lines here too

::: toolkit/devtools/server/actors/webaudio.js
@@ +892,5 @@
>      this.manage(this);
> +  },
> +
> +  /**
> +   * If connecting to older geckos, where audio node actor's do not

Nit: Add the Gecko version where this behavior changed.

@@ +900,5 @@
> +  _onCreateNode: preEvent("create-node", function (audionode) {
> +    if (!audionode.type) {
> +      return audionode.getType().then(type => {
> +        audionode.type = type;
> +        audionode.source = !!AUDIO_NODE_DEFINITION[type].source;

This means the values for `source` / `bypassable` are determined from the client side map which changes over time, instead of the server's logic which is fixed to Gecko version in use on the target.

Are you okay with that difference?  Have these values ever changed for a given node?  Can you imagine them ever changing in the future?

::: toolkit/devtools/server/protocol.js
@@ +1201,5 @@
> +        let results = event.pre.map(pre => pre.apply(this, args));
> +
> +        // Check to see if any of the preEvents returned a promise -- if so,
> +        // wait for their resolution before emitting. Otherwise, emit synchronously.
> +        if (results.some(result => result && typeof result.then === "function")) {

Promise.all handles the case[1] where some values are non-promises, so this test is not needed.

Or maybe you knew that, but just tried to leave the non promise path synchronous?

It would be nice to have once path here for less complexity.  Does always using promise.all cause regressions?

[1]: https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/toolkit/modules/Promise-backend.js#580-581

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8660980 [details] [diff] [review]
1204595-cache-audionode-props.patch

Review of attachment 8660980 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/webaudio.js
@@ +900,5 @@
> +  _onCreateNode: preEvent("create-node", function (audionode) {
> +    if (!audionode.type) {
> +      return audionode.getType().then(type => {
> +        audionode.type = type;
> +        audionode.source = !!AUDIO_NODE_DEFINITION[type].source;

Right -- this is ok and desired, the `source` field was just added, so otherwise we'd have to make additional requests for legacy servers -- none of these fields specifically will ever change

::: toolkit/devtools/server/protocol.js
@@ +1201,5 @@
> +        let results = event.pre.map(pre => pre.apply(this, args));
> +
> +        // Check to see if any of the preEvents returned a promise -- if so,
> +        // wait for their resolution before emitting. Otherwise, emit synchronously.
> +        if (results.some(result => result && typeof result.then === "function")) {

While Promise.all handles all a|sync events, if there are any async events, they're all handled async -- If we always used promise.all, I fear it'd lead to slowdown in many events, which I've noticed in implementing queues that are almost always sync (when dealing with hundreds of events, adding a tick delay to each adds up)

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1204595-cache-audionode-props.patch

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8661439 [details] [diff] [review]
1204595-cache-audionode-props.patch

Review of attachment 8661439 [details] [diff] [review]:
-----------------------------------------------------------------

Would be nice to document[1] the now sometimes async behavior in p.js docs, since that's unusual / surprising.

[1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/docs/protocol.js.md#420

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Added docs for async preEvent functionality!


https://hg.mozilla.org/integration/fx-team/rev/93a6681ccc23

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/93a6681ccc23