1497905 - Avoid destroying the markup iframe on navigation

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1497905.patch [1.0]

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 9015923 [details] [diff] [review]
1497905.patch [1.0]

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

Looks great! I have a few comments below that I'd like addressed please.
Also, what does this change look like in terms of perf? Are you seeing some improvements on reload?

::: devtools/client/inspector/inspector.js
@@ +1946,5 @@
> +        INSPECTOR_L10N.getStr("inspector.panelLabel.markupView"));
> +      this._markupFrame.setAttribute("flex", "1");
> +      // This is needed to enable tooltips inside the iframe document.
> +      this._markupFrame.setAttribute("tooltip", "aHTMLTooltip");
> +      this._markupFrame.addEventListener("contextmenu", this._onContextMenu);

this event listener isn't being removed anymore now. It should be removed in the destroy method of the panel.

@@ +1954,2 @@
>  
> +      this._markupFrame.addEventListener("load", this._onMarkupFrameLoad, true);

This event listener is not removed anymore now. We should either add a {once} type on it, or remove it in the destroy method of the panel.

@@ +1977,5 @@
>        destroyPromise = promise.resolve();
>      }
>  
> +    this._markupBox.style.visibility = "hidden";
> +    this._markupFrame.contentDocument.getElementById("root").innerHTML = "";

I would actually leave this to the MarkupView class itself. This class is given an empty document, and fills it with its content. So it's only logical that it would remove its content when it destroys itself. So when calling this.markup.destroy() this should happen automatically.

@@ -1986,5 @@
> -      this._markupFrame.remove();
> -      this._markupFrame = null;
> -    }
> -
> -    this._markupBox = null;

this._markupFrame and this._markupBox are not nullified anymore it seems now.
We should still do this in the inspector's destroy method.

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1497905.patch [2.0]

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

DAMP https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=ef3ddb2b402b4daf3b8002d11aafa5a6cd308c46&newProject=try&newRevision=352d1db3bff2f01bef576f64ede4c6c4175ed860&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=inspector&framework=12

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbae2c874b44
Avoid destroying the markup iframe on navigation. r=pbro

Comment 6

[Natalia Csoregi [:nataliaCs]]

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