1007922 - Make 'readonly' AudioParams uneditable

Status: RESOLVED FIXED

(DevTools Graveyard :: Web Audio Editor, defect)

Description

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

Attachment: parambug.png

For AudioParams that aren't editable (like an ArrayBuffer), it should not be possible for me to change that to a string or a number, or anything at all.

Comment 1

[crackerplace]

Hi,

I am interested to contribute as this is a good first bug and I believe its in Java. 
Could someone guide me how to approach or start working on this issue.

Comment 2

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

All of this work is in JavaScript, not Java, are you comfortable in that language? Also, with the rendering of objects (like AudioBuffer) different now, this may no longer be an issue (the editability of non-editable properties)

Comment 3

[crackerplace]

Ok.I was assuming this to be related to Java.Thanks for that.Where can I find Java related bugs for a newbie.

Comment 4

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

We don't have too many Java projects, but some are listed here http://www.whatcanidoformozilla.org/#!/progornoprog/proglang/java/

Comment 5

[Raphaël Lustin [:rlustin] [:rlubitel] [:lubitel]]

Hey !
I'm interested to fix this bug. Can somebody assigns it to me? Thank!

Comment 6

[Jan Keromnes [:janx]]

I'll try helping Raphaël to fix this bug, assigning him.

Comment 7

[Raphaël Lustin [:rlustin] [:rlubitel] [:lubitel]]

Attachment: Make 'readonly' AudioParams uneditable.

Comment 8

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

Nice! This adds the 'lock' icon to the VariablesView component to the appropriate parameters, but still allows the `_onEval` method to be called, and then the change sent to the actual underlying component. Since the object displayed isn't an exact representation of the underlying AudioNode, I think some extra magic will be necessary, and probably just in the `_onEval` method, check again to see if it's readonly/un-writable and just ignore the attempted change.

Thanks again Raphaël!

Comment 9

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

And the test should probably be very similar to ./browser_wa_properties-view-edit, but in a different file (using the doc_complex-content.html, and using the script processor's bufferSize param there). Probably just call it `./browser_wa_properties_view-edit-02.js` or something, using the same techniques as the first one, but different nodes and check to make sure that the property is not updated.

Comment 10

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

Comment on attachment 8443914 [details] [diff] [review]
Make 'readonly' AudioParams uneditable.

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

Addressed in previous comment

Comment 11

[Raphaël Lustin [:rlustin] [:rlubitel] [:lubitel]]

Attachment: Make 'readonly' AudioParams uneditable. r=jsantell

Comment 12

[Raphaël Lustin [:rlustin] [:rlubitel] [:lubitel]]

Attachment: Make 'readonly' AudioParams uneditable. r=jsantell

Comment 13

[Raphaël Lustin [:rlustin] [:rlubitel] [:lubitel]]

Comment on attachment 8444074 [details] [diff] [review]
Make 'readonly' AudioParams uneditable. r=jsantell

Hi Jordan,

I addressed your nits, but it seems that bug 994258 prevents this line from being called:
http://dxr.mozilla.org/mozilla-central/source/browser/devtools/webaudioeditor/webaudioeditor-view.js#483

I updated this bug to be blocked on bug 994258, because the latter needs to be fixed for this test to work.

Comment 14

[Raphaël Lustin [:rlustin] [:rlubitel] [:lubitel]]

Comment on attachment 8444074 [details] [diff] [review]
Make 'readonly' AudioParams uneditable. r=jsantell

Nevermind, just found a way to make the test works independently from the other bug.

Comment 15

[Raphaël Lustin [:rlustin] [:rlubitel] [:lubitel]]

Attachment: Make 'readonly' AudioParams uneditable. r=jsantell

Comment 16

[Raphaël Lustin [:rlustin] [:rlubitel] [:lubitel]]

Comment on attachment 8444087 [details] [diff] [review]
Make 'readonly' AudioParams uneditable. r=jsantell

Please take a look at my patch :)

Comment 17

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

Comment on attachment 8444087 [details] [diff] [review]
Make 'readonly' AudioParams uneditable. r=jsantell

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

This will also need UI_SET_PARAM_ERROR defined in webaudioeditor-view.js.

I also suspect that this will cause merge conflicts due to bug 1027852 affecting the same area, so once the review comments are addressed, this may need to be rebased if bug 1027852 has already landed so we can get a clean merge.

Thanks! Almost there :)

::: browser/devtools/webaudioeditor/test/browser.ini
@@ +35,5 @@
>  [browser_wa_properties-view.js]
>  [browser_wa_properties-view-media-nodes.js]
>  # [browser_wa_properties-view-edit.js]
>  # Disabled for too many intermittents bug 1010423
> +[browser_wa_properties_view-edit-02.js]

This should be `browser_wa_properties-view-edit-02.js` (note the differences between _ and -), and if we name this, we should also change the name of `browser_wa_properties-view-edit` and add a -01 there.

Also, this test should also be commented out as it will have the same failures as the other test due to bug 1010423

::: browser/devtools/webaudioeditor/test/browser_wa_properties_view-edit-02.js
@@ +26,5 @@
> +  yield Promise.all([
> +    once(panelWin, EVENTS.UI_INSPECTOR_NODE_SET),
> +    once(panelWin, EVENTS.UI_INSPECTOR_TOGGLED)
> +  ]);
> +

We should start a listener for EVENTS.UI_SET_PARAM_ERROR here, but yield after the failure below:

let errorEvent = once(panelWin, EVENTS.UI_SET_PARAM_ERROR);

@@ +28,5 @@
> +    once(panelWin, EVENTS.UI_INSPECTOR_TOGGLED)
> +  ]);
> +
> +  try {
> +    yield modifyVariableView(panelWin, gVars, 0, 'bufferSize', 2048);

Use double quotes "

@@ +32,5 @@
> +    yield modifyVariableView(panelWin, gVars, 0, 'bufferSize', 2048);
> +  } catch(e) {
> +    // we except modifyVariableView to fail here, because bufferSize is not writable
> +  }
> +

Here we should yield for the above `errorEvent` to ensure it was called at some point during the modification:

`yield errorEvent`

@@ +33,5 @@
> +  } catch(e) {
> +    // we except modifyVariableView to fail here, because bufferSize is not writable
> +  }
> +
> +  checkVariableView(gVars, 0, {bufferSize: 4096}, 'check that unwritable variable is not updated');

Double quotes "

::: browser/devtools/webaudioeditor/webaudioeditor-view.js
@@ +467,5 @@
>      let propName = variable.name;
>      let error;
>  
> +    if (!variable._initialDescriptor.writable) {
> +      error = new Error('Variable ' + propName + ' is not writable.');

Double quotes

Comment 18

[Raphaël Lustin [:rlustin] [:rlubitel] [:lubitel]]

Attachment: Make 'readonly' AudioParams uneditable. r=jsantell

Comment 19

[Raphaël Lustin [:rlustin] [:rlubitel] [:lubitel]]

Attachment: Make 'readonly' AudioParams uneditable.

Rebased from mozilla-central, as bug 1027852 was merged in it and affects the
same area.

Comment 20

[Raphaël Lustin [:rlustin] [:rlubitel] [:lubitel]]

Comment on attachment 8445291 [details] [diff] [review]
Make 'readonly' AudioParams uneditable.

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

Rebased from mozilla-central, as bug 1027852 was merged in it and affects the
same area.

Comment 21

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

Comment on attachment 8445291 [details] [diff] [review]
Make 'readonly' AudioParams uneditable.

Looks great! Pushing to try: https://tbpl.mozilla.org/?tree=Try&rev=219a3602b818

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/9da0383d3ecd

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9da0383d3ecd