We should not hard code default AudioParam values

RESOLVED FIXED in Firefox 39

Status

()

Firefox
Developer Tools: Web Audio Editor
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jsantell, Assigned: mahdi, Mentored)

Tracking

37 Branch
Firefox 39
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

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
(Reporter)

Updated

3 years ago
Whiteboard: [good first bug][lang=js]
(Reporter)

Updated

3 years ago
Mentor: jsantell@mozilla.com
(Assignee)

Comment 1

3 years ago
Hi Jordan,

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

Thanks.
(Assignee)

Updated

3 years ago
Flags: needinfo?(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
Flags: needinfo?(jsantell)
Assigning you the bug Mahdi, let me know if you have any questions!
Assignee: nobody → mdibaiee
(Assignee)

Comment 4

3 years ago
Created attachment 8571931 [details] [diff] [review]
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!
Attachment #8571931 - Flags: review?(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
Attachment #8571931 - Flags: review?(jsantell)
(Assignee)

Comment 6

3 years ago
Created attachment 8573871 [details] [diff] [review]
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
Attachment #8571931 - Attachment is obsolete: true
Attachment #8573871 - Flags: review?(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 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 :)
Attachment #8573871 - Flags: review?(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
(Assignee)

Comment 10

3 years ago
Created attachment 8574828 [details] [diff] [review]
Bug 1128974 - Fetch default values for AudioNode params instead of hardcoding; r=jsantell

Thanks Jordan!
Attachment #8573871 - Attachment is obsolete: true
Attachment #8574828 - Flags: review?(jsantell)
(Reporter)

Updated

3 years ago
Attachment #8574828 - Flags: review?(jsantell) → review+
Perfect, thanks Mahdi!
Keywords: checkin-needed
(Reporter)

Updated

3 years ago
Blocks: 1134046
https://hg.mozilla.org/integration/fx-team/rev/05cbf8d10c3e
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/05cbf8d10c3e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 39
You need to log in before you can comment on or make changes to this bug.