Closed Bug 1007922 Opened 10 years ago Closed 10 years ago

Make 'readonly' AudioParams uneditable

Categories

(DevTools Graveyard :: Web Audio Editor, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: jsantell, Assigned: rlustin, Mentored)

References

Details

(Whiteboard: [good first bug])

Attachments

(2 files, 5 obsolete files)

Attached image 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.
No longer blocks: webaudioeditorv1
Blocks: 1022248
Mentor: jsantell
Whiteboard: [good first bug][mentor=jsantell@mozilla.com]
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.
Flags: needinfo?(nobody)
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)
Flags: needinfo?(nobody) → needinfo?(crackerplace)
Ok.I was assuming this to be related to Java.Thanks for that.Where can I find Java related bugs for a newbie.
Flags: needinfo?(crackerplace)
We don't have too many Java projects, but some are listed here http://www.whatcanidoformozilla.org/#!/progornoprog/proglang/java/
Hey !
I'm interested to fix this bug. Can somebody assigns it to me? Thank!
I'll try helping Raphaël to fix this bug, assigning him.
Assignee: nobody → raphael
OS: Mac OS X → All
Hardware: x86 → All
Attachment #8443914 - Flags: review?(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!
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 on attachment 8443914 [details] [diff] [review]
Make 'readonly' AudioParams uneditable.

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

Addressed in previous comment
Attachment #8443914 - Flags: review?(jsantell) → review-
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.
Attachment #8444074 - Flags: review?(jsantell)
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.
Attachment #8444074 - Flags: review?(jsantell)
Comment on attachment 8444087 [details] [diff] [review]
Make 'readonly' AudioParams uneditable. r=jsantell

Please take a look at my patch :)
Attachment #8444087 - Flags: review?(jsantell)
No longer depends on: 994258
Whiteboard: [good first bug][mentor=jsantell@mozilla.com] → [good first bug]
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
Attachment #8444087 - Flags: review?(jsantell) → review-
Attachment #8444629 - Flags: review?(jsantell)
Rebased from mozilla-central, as bug 1027852 was merged in it and affects the
same area.
Attachment #8444629 - Attachment is obsolete: true
Attachment #8444629 - Flags: review?(jsantell)
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.
Attachment #8445291 - Flags: review?(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
Attachment #8445291 - Flags: review?(jsantell) → review+
https://hg.mozilla.org/integration/fx-team/rev/9da0383d3ecd
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [good first bug] → [good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9da0383d3ecd
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][fixed-in-fx-team] → [good first bug]
Target Milestone: --- → Firefox 33
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.