1128974 - We should not hard code default AudioParam values

Status: RESOLVED FIXED

(DevTools Graveyard :: Web Audio Editor, defect)

Description

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

We currently hard code the expected default values of AudioParams for testing:
https://github.com/mozilla/gecko-dev/blob/9fe5980ccdc16e6477ebcfe37b6cfb1c38c2bcf1/browser/devtools/webaudioeditor/test/head.js#L447-L519

This causes failures when the spec changes, and ruins Paul's day: bug 1128494#c2

We should not use hard coded values, but check the `defaultValue` of an AudioParam in the context being checked, and ensure that's what the tool is reporting. This will require message manager usage, which is used in some of our other tools test, to communicate with content[0].

[0] https://github.com/mozilla/gecko-dev/blob/9fe5980ccdc16e6477ebcfe37b6cfb1c38c2bcf1/browser/devtools/canvasdebugger/test/head.js#L247-L272

Comment 1

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Hi Jordan,

I'm interested in this bug, Where should I start?

Thanks.

Comment 2

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

Hey Mahdi, thanks for checking this out!

What needs to be done here is get rid of all hardcoded default properties audio nodes have (at the bottom of browser/devtools/webaudioeditor/test/head.js) and get those values from the content themselves. So any test that uses the current NODE_DEFAULT_VALUES values, we need to get those from the actual nodes themselves.

In tests we have the `evalInDebuggee` function in test/head.js that we can use to fetch the default value like `evalInDebuggee("aNode.PARAM_NAME.value")` to compare to, so that we can ensure in the tests that the web audio tool is displaying the correct current value

Comment 3

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

Assigning you the bug Mahdi, let me know if you have any questions!

Comment 4

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 1128974 - Fetch default values for AudioNode params instead of hardcoding

Hi Jordan,

In order to ignore event properties like onended, onaudioprogress, I tested the property key against |prop.startsWith("on")|, do you think it might break in the future and that it should be changed?

I ran all the webaudioeditor tests locally and they passed, pushed to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ce32a08cb4e

Thanks!

Comment 5

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

Comment on attachment 8571931 [details] [diff] [review]
Bug 1128974 - Fetch default values for AudioNode params instead of hardcoding

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

Mahdi, this looks great! I think a better way to go about it is to use the audio nodes definition recently added[0], (and a look at the source[1]), this will get you a list of properties that will be displayed in the editor, rather than iterating over the properties on the node and checking for "on" etc., this will also tell you whether it's an audio param or not to check for defaultValue. This might make things a bit cleaner. I think with that change, everything else looks good!

[0] `require("devtools/server/actors/utils/audionodes.json");`
[1] https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/actors/utils/audionodes.json

Comment 6

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 1128974 - Fetch default values for AudioNode params instead of hardcoding

Thanks Jordan! that really helped with making code cleaner.

A few params in the definitions missed the |param: true|, so I added them.

I noticed that evalInDebuggee was added to head.js in another patch (it appeared after doing hg pull -u), so the patch required a rebase.

All the tests passed locally, try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=12d8c492fdd2

Comment 7

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

Comment on attachment 8573871 [details] [diff] [review]
Bug 1128974 - Fetch default values for AudioNode params instead of hardcoding

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

Looks great! I just am wondering how the node creation works without throwing an error (try creating a MediaElement/MediaStream node without an argument), and wondering why this passes with that case. Maybe it'd be easier just using the current nodes in the document rather than creating them again?

::: browser/devtools/webaudioeditor/test/head.js
@@ +491,5 @@
>  
>    return deferred.promise;
>  }
>  
> +function nodeDefaultValues(param) {

Should have a comment describing what this function does. Also `param` should probably be `nodeName` or something more descriptive

@@ +497,4 @@
>  
> +  if(typeof fn === 'undefined') return {};
> +
> +  let init = param === "AudioDestinationNode" ? "destination" : `create${fn}()`;

Shouldn't this fail for the MediaElement and MediaStream source nodes without a proper stream/element?

`ctx.createMediaStreamSource();` -> TypeError: Not enough arguments to AudioContext.createMediaStreamSource.

A possibility is to store all nodes created in a document in a var `nodes`, and this script just iterates over them, rather than having to create new nodes (look at doc_media-node-creation.html for some of the extra work needed just to create a node)

::: toolkit/devtools/server/actors/utils/audionodes.json
@@ +52,5 @@
>      "properties": {
>        "threshold": { "param": true },
>        "knee": { "param": true },
>        "ratio": { "param": true },
> +      "reduction": { "param": true },

According to the spec this should not be an AudioParam, but it is in implementation... I'll see if this is intentional, but this is good for now as it matches implementation

@@ +100,5 @@
>      }
>    },
>    "StereoPannerNode": {
>      "properties": {
> +      "pan": { "param": true }

Good catch!

Comment 8

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

Comment on attachment 8573871 [details] [diff] [review]
Bug 1128974 - Fetch default values for AudioNode params instead of hardcoding

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

Once the issue is figured out with node creation and no parameters, and a few comments added, I think we're good to go :)

Comment 9

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

Comment on attachment 8573871 [details] [diff] [review]
Bug 1128974 - Fetch default values for AudioNode params instead of hardcoding

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

Once the issue is figured out with node creation and no parameters, and a few comments added, I think we're good to go :)

::: browser/devtools/webaudioeditor/test/head.js
@@ +497,2 @@
>  
> +  if(typeof fn === 'undefined') return {};

Ah ok, I see here now we abort if using one of those media source nodes, which does not have any parameters

Comment 10

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 1128974 - Fetch default values for AudioNode params instead of hardcoding; r=jsantell

Thanks Jordan!

Comment 11

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

Perfect, thanks Mahdi!

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/05cbf8d10c3e

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/05cbf8d10c3e