1053805 - Tweak canvas, webgl and webaudio actors to support frame switching

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Alexandre Poirot [:ochameau]]

Followup for bug 977043, in order to ensure that all these actors support frame switching.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch

(In reply to Victor Porof [:vporof][:vp] from comment #106)
> Comment on attachment 8468473 [details] [diff] [review]
> Tweak webgl and webaudio actors for frames selection.
> 
> Review of attachment 8468473 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/shadereditor/shadereditor.js
> @@ +124,5 @@
> >      switch (event) {
> >        case "will-navigate": {
> >          Task.spawn(function() {
> >            // Make sure the backend is prepared to handle WebGL contexts.
> > +          if (!isFakeEvent) {
> 
> I don't fully understand what isFakeEvent represents, and neither would
> anyone else that's reading any code that's using it. I would definitely
> recommend naming it differently, and adding comments describing it.

I renamed it to isFrameSwitching, hopefully it is clear now.

> 
> @@ +132,5 @@
> >            // Reset UI.
> >            ShadersListView.empty();
> > +          if (isFakeEvent) {
> > +            $("#reload-notice").hidden = false;
> > +            $("#waiting-notice").hidden = true;
> 
> Please add a comment here too, like you did for webaudioeditor. Also, this
> needs to be tested.

done.

> 
> ::: browser/devtools/webaudioeditor/webaudioeditor-controller.js
> @@ -200,5 @@
> >     * for an audio context notice.
> >     */
> >    reset: function () {
> > -    $("#reload-notice").hidden = true;
> > -    $("#waiting-notice").hidden = false;
> 
> Are these now handled in _onTabNavigated?

Yes otherwise I can't control it from tabNavigated when we receive a frame switch.
> 
> @@ +246,5 @@
> > +          // so we don't need to reload anymore and should receive
> > +          // new node events.
> > +          $("#reload-notice").hidden = true;
> > +          $("#waiting-notice").hidden = false;
> > +        }
> 
> Testme.

Added browser_wa_reset-04.js for that.

> 
> ::: toolkit/devtools/server/actors/call-watcher.js
> @@ -289,5 @@
> >      this._tracedGlobals = tracedGlobals || [];
> >      this._tracedFunctions = tracedFunctions || [];
> >      this._holdWeak = !!holdWeak;
> >      this._storeCalls = !!storeCalls;
> > -    this._contentObserver = new ContentObserver(this.tabActor);
> 
> Was the ContentObserver assimilated by the tab actor? If so, where? and I
> need to review that. There are a lot of assumptions here about when
> _onGlobalCreated and _onGlobalDestroyed are invoked.

It's already landed, bug 977043 attachment 8408432 [details] [diff] [review].
Or you can just look at webbrowser.js:
- Listen:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webbrowser.js#1558
- Dispatch:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webbrowser.js#1038

It is based on what already existed within the inspector and conciliate the various subtle different needs of existing actors like ThreadActor and inspector.
It will also replace the beginning of implementation of content observer-like in storage.js.

> 
> @@ +381,3 @@
> >      let self = this;
> >  
> > +    if (!isTopLevel) {
> 
> Please add a comment here to bug 981748.

done

> 
> @@ +382,5 @@
> >  
> > +    if (!isTopLevel) {
> > +      return;
> > +    }
> > +    this._tracedWindowId = id;
> 
> Is this the same inner window id?

yes

> 
> ::: toolkit/devtools/server/actors/webaudio.js
> @@ -334,5 @@
> >      });
> > -    // Bind to the `global-destroyed` event on the content observer so we can
> > -    // unbind events between the global destruction and the `finalize` cleanup
> > -    // method on the actor.
> > -    // TODO expose these events on CallWatcherActor itself, bug 1021321
> 
> I guess you can now wontfix that bug?

done

> 
> ::: toolkit/devtools/server/actors/webgl.js
> @@ +391,5 @@
> > +    if (isTopLevel) {
> > +      removeFromArray(this._programActorsCache, e => e.ownerWindow == id);
> > +      this._webglObserver.unregisterContextsForWindow(id);
> > +      events.emit(this, "global-destroyed", id);
> > +    }
> 
> Do all tests still pass?

They used to pass in the previous patch, but here is a new try:
https://tbpl.mozilla.org/?tree=Try&rev=de5285bccc94

Comment 2

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

(In reply to Alexandre Poirot [:ochameau] from comment #1)
> > 
> > Was the ContentObserver assimilated by the tab actor? If so, where? and I
> > need to review that. There are a lot of assumptions here about when
> > _onGlobalCreated and _onGlobalDestroyed are invoked.
> 
> It's already landed, bug 977043 attachment 8408432 [details] [diff] [review].
> Or you can just look at webbrowser.js:
> - Listen:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> webbrowser.js#1558
> - Dispatch:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> webbrowser.js#1038
> 
> It is based on what already existed within the inspector and conciliate the
> various subtle different needs of existing actors like ThreadActor and
> inspector.
> It will also replace the beginning of implementation of content
> observer-like in storage.js.
> 

Is "window-ready" invoked before the content global is created? (i.e., before or at the exact same time "content-document-global-created" is emitted). "DOMWindowCreated" is too late, IIRC. The assumption is that content window globals can be overridden *before* any code gets a chance to be executed. If this isn't the case anymore, all the tools that use ContentObserver will be subtly broken!

Same question goes for "window-destroyed".

Can you please double check this?

Comment 3

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

It looks like DOMWindowCreated could be emitted at the time as "content-document-global-created"? Found bug 567357.

Comment 4

[Alexandre Poirot [:ochameau]]

(In reply to Victor Porof [:vporof][:vp] from comment #3)
> It looks like DOMWindowCreated could be emitted at the time as
> "content-document-global-created"? Found bug 567357.

Yes, they are duplicated events. There is just one that is a DOM event and the other one is an observer service notification.
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#2787
DOMWindowCreated is even dispatched right before content-document-global-created.
DOMWindowCreated also has the advantage to be dispatched for chrome and content document, which may help us support chrome documents.

window-destroyed is sent during inner-window-destroyed observer notification, which is the same than content-observer.

But that's not all! window-ready and window-destroyed are also emitted for pages going into/out of the bfcache when navigatting through tab history. It should help supporting bfcache correctly!
window-ready is dispatched during page-show (if the page is resurrected from the bfcache)
and window-destroyed is emitted during page-hide (if frozen into the bfcache)

Comment 5

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

<3

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8473072 [details] [diff] [review]
patch

(forgot to reflag when posting the patch...)

Comment 7

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

Comment on attachment 8473072 [details] [diff] [review]
patch

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

::: browser/devtools/webaudioeditor/test/browser_wa_reset-04.js
@@ +19,5 @@
> +    "The tool's content should initially be hidden.");
> +
> +  let btn = toolbox.doc.getElementById("command-button-frames");
> +  ok(!btn.firstChild.getAttribute("hidden"), "The frame list button is visible");
> +  let frameBtns = Array.slice(btn.firstChild.querySelectorAll("[data-window-id]"));

Why the slice here?

@@ +41,5 @@
> +  let started = once(gFront, "start-context");
> +
> +  reload(target);
> +
> +  yield Promise.all([navigating, started]);

Nit: I think all those tests use lowercase promise.

::: browser/devtools/webaudioeditor/test/doc_iframe-context.html
@@ +8,5 @@
> +    <title>Web Audio Editor test page with an iframe</title>
> +  </head>
> +
> +  <body>
> +

Nit: remove unnecessary newline.

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

https://tbpl.mozilla.org/?tree=Try&rev=e42eaefacfb2

(In reply to Victor Porof [:vporof][:vp] from comment #7)
> Comment on attachment 8473072 [details] [diff] [review]
> patch
> 
> Review of attachment 8473072 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/webaudioeditor/test/browser_wa_reset-04.js
> @@ +19,5 @@
> > +    "The tool's content should initially be hidden.");
> > +
> > +  let btn = toolbox.doc.getElementById("command-button-frames");
> > +  ok(!btn.firstChild.getAttribute("hidden"), "The frame list button is visible");
> > +  let frameBtns = Array.slice(btn.firstChild.querySelectorAll("[data-window-id]"));
> 
> Why the slice here?

I must have been trying to use filter() or some array function and then stopped doing it...
Removed.

> 
> @@ +41,5 @@
> > +  let started = once(gFront, "start-context");
> > +
> > +  reload(target);
> > +
> > +  yield Promise.all([navigating, started]);
> 
> Nit: I think all those tests use lowercase promise.
> 

Nope, *P*romise all over the place in this folder.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

Rebased.

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: additional fix v1

To fix a failing test, I had to tweak webgl actor even more.
That to better support bfcache navigation where we don't expect
to do anything for pages going into the bfcache.
So that when a page revives out ofthe bfcache,
we still have the hooks and the actors in cache (in programActorsCache). 

In order to do that, I had to add yet another flag in window-destroyed
'isFrozen' to know if that's a bfcache event or a complete destruction of the document.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8479850 [details] [diff] [review]
additional fix v1

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

::: toolkit/devtools/server/actors/webbrowser.js
@@ +1224,5 @@
>      // Fake a will-navigate on the previous document
>      // to let a chance to unregister it
>      this._willNavigate(this.window, window.location.href, null, true);
>  
> +    this._windowDestroyed(this.window, null, true);

This is a signal that _windowDestroyed should have an option object parameter instead of many optional ones.

Comment 12

[Alexandre Poirot [:ochameau]]

https://tbpl.mozilla.org/?tree=Try&rev=538fbd362bfa

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: rebased, merged patch

https://tbpl.mozilla.org/?tree=Try&rev=f20889af2f3d

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/a45254af12a6