Closed Bug 1053805 Opened 7 years ago Closed 7 years ago

Tweak canvas, webgl and webaudio actors to support frame switching

Categories

(DevTools :: Framework, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file, 4 obsolete files)

Followup for bug 977043, in order to ensure that all these actors support frame switching.
Blocks: 981748
Depends on: 977043
Attached patch patch (obsolete) — Splinter Review
(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
(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?
It looks like DOMWindowCreated could be emitted at the time as "content-document-global-created"? Found bug 567357.
(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 on attachment 8473072 [details] [diff] [review]
patch

(forgot to reflag when posting the patch...)
Attachment #8473072 - Flags: review?(vporof)
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.
Attachment #8473072 - Flags: review?(vporof) → review+
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #8473072 - Attachment is obsolete: true
Attached patch patch v3 (obsolete) — Splinter Review
Rebased.
Attachment #8476658 - Attachment is obsolete: true
Attached patch additional fix v1 (obsolete) — Splinter Review
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.
Attachment #8479847 - Flags: review+
Attachment #8479850 - Flags: review?(vporof)
Attachment #8479850 - Flags: review?(past)
Assignee: nobody → poirot.alex
Attachment #8479850 - Flags: review?(vporof) → review+
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.
Attachment #8479850 - Flags: review?(past) → review+
https://tbpl.mozilla.org/?tree=Try&rev=f20889af2f3d
Attachment #8479847 - Attachment is obsolete: true
Attachment #8479850 - Attachment is obsolete: true
Attachment #8482210 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a45254af12a6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.