Closed Bug 1497905 Opened 2 years ago Closed 2 years ago

Avoid destroying the markup iframe on navigation

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox64 fixed)

RESOLVED FIXED
Firefox 64
Tracking Status
firefox64 --- fixed

People

(Reporter: gl, Assigned: gl)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch 1497905.patch [1.0] (obsolete) — Splinter Review
Attachment #9015923 - Flags: review?(pbrosset)
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.
Attachment #9015923 - Flags: review?(pbrosset)
Attachment #9016773 - Flags: review?(pbrosset)
Attachment #9015923 - Attachment is obsolete: true
Attachment #9016773 - Flags: review?(pbrosset) → review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbae2c874b44
Avoid destroying the markup iframe on navigation. r=pbro
https://hg.mozilla.org/mozilla-central/rev/bbae2c874b44
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in before you can comment on or make changes to this bug.