1006912 - Web Audio Editor needs to be destroyed and recreated after used

Status: RESOLVED FIXED

(DevTools Graveyard :: Web Audio Editor, defect)

Description

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

From the ContentObserver refactoring I think, the 'start-context' event doesn't get emitted from the web audio actor when refreshing.

Currently, the first node created fires the start-context event, as well as a node for the AudioDestination. Since we can't hook into constructors with ContentObserver/CallWatcher, this was done instead of listening for the actual AudioContext creation. The front and back end needs to be rewritten to handle persisting across reloads.

Comment 1

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

This is actually a show stopper.

Comment 2

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

Bug 1008497 may solve this issue as we'll have real events for AudioContext creation

Comment 3

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

Depends on 1013544 due to test changes, and I don't want to deal with merging patch magic right now

Comment 4

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

Attachment: 1006912-wae-must-persist.patch

Was holding off on r?ing this as it'll need rebased as it also moves the `clickGraphNode` fn in tests to `head.js`, which is also done in bug 1013544, but I'll deal with rebasing later, just want to land this ASAP, as this is the bug that makes you close/open toolbox again upon refreshing the page

Comment 5

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

Attachment: 1006912-wae-must-persist.patch

Now that the other patches landed, rebased against that.

Comment 6

[Victor Porof [:vporof][:vp]]

Comment on attachment 8429786 [details] [diff] [review]
1006912-wae-must-persist.patch

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

::: browser/devtools/webaudioeditor/webaudioeditor-controller.js
@@ +214,5 @@
>    /**
>     * Called for each location change in the debugged tab.
>     */
>    _onTabNavigated: function(event) {
> +    let controller = this;

You could turn _onTabNavigated into a Task to avoid so much indentation. Then you don't even need to cache |this| anymore.

_onTabNavigated: Task.spawn(function*(event) {

}),

Comment 7

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

Attachment: 1006912-wae-must-persist.patch

Ah, yes, switched that over to a `Task.async`, and this horrible bug is fixed!

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Do you have a link to a recent Try run by chance? :)

Comment 9

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

Hm, thought I did but couldn't find it, reran it with success: https://tbpl.mozilla.org/?tree=Try&rev=2b69ade18951

A crash in the webgl editor, possibly related to bug 937088 or bug 1009319, but this patch wouldn't affect that tool in anyway. Rerunning 10.6 opt build for sanity checking though: https://tbpl.mozilla.org/?tree=Try&rev=9e818f9abad4

Comment 10

[Ryan VanderMeulen [:RyanVM]]

You know you can just retrigger jobs on the existing push, right? :)

See https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/27c882b46de9

Comment 12

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

Ah, that would've helped too -- thanks!

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/27c882b46de9