Avoid destroying the markup iframe on navigation

RESOLVED FIXED in Firefox 64

Status

enhancement
P3
normal
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: gl, Assigned: gl)

Tracking

unspecified
Firefox 64

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

8 months ago
No description provided.
Assignee

Comment 1

8 months ago
Posted 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)
Assignee

Comment 3

7 months ago
Attachment #9016773 - Flags: review?(pbrosset)
Assignee

Updated

7 months ago
Attachment #9015923 - Attachment is obsolete: true
Attachment #9016773 - Flags: review?(pbrosset) → review+

Comment 5

7 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbae2c874b44
Avoid destroying the markup iframe on navigation. r=pbro

Comment 6

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bbae2c874b44
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in before you can comment on or make changes to this bug.