Closed Bug 1204595 Opened 4 years ago Closed 4 years ago

Serialize via form several static properties for AudioNodeFront

Categories

(DevTools Graveyard :: Web Audio Editor, defect)

41 Branch
defect
Not set

Tracking

(firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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
Attachment #8660980 - Flags: review?(jryans)
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
Attachment #8660980 - Flags: review?(jryans)
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)
Attachment #8660980 - Attachment is obsolete: true
Attachment #8661439 - Flags: review?(jryans)
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
Attachment #8661439 - Flags: review?(jryans) → review+
Added docs for async preEvent functionality!


https://hg.mozilla.org/integration/fx-team/rev/93a6681ccc23
https://hg.mozilla.org/mozilla-central/rev/93a6681ccc23
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.