Closed Bug 592525 Opened 9 years ago Closed 9 years ago

Txul Regression between 1PM-3PM PDT

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(blocking2.0 beta7+)

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: rcampbell, Assigned: rcampbell)

References

Details

(Whiteboard: [kd4b6])

Attachments

(1 file, 3 obsolete files)

currently only linux64 but waiting on windows builds.
looks like there is a slight regression in win7 as well.

preparing a backout of patch:

http://hg.mozilla.org/mozilla-central/rev/9d7e7ac57f10

from bug 572038.
Depends on: 572038
Whiteboard: [kd4b6]
Severity: normal → blocker
I believe this should be resolved/fixed now with the landing of the updated tree panel patch.
It's not resolved, according to new notifications in m.d.tree-management.

Adding the <iframe> to browser.xul means that the document inside the iframe needs to finish loading before the browser window opens. This might explain the slowdown.
Maybe you could change the iframe's src to about:blank and only load inspector.html when the inspector is opened?
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Severity: blocker → normal
Assignee: nobody → rcampbell
(In reply to comment #4)
> It's not resolved, according to new notifications in m.d.tree-management.

I didn't see any regressions in the group the next day. I must have missed them. Disappointing!

> Adding the <iframe> to browser.xul means that the document inside the iframe
> needs to finish loading before the browser window opens. This might explain the
> slowdown.
> Maybe you could change the iframe's src to about:blank and only load
> inspector.html when the inspector is opened?

That's a good suggestion. I'll try about:blank or data:text/html,basic for starters.
Status: NEW → ASSIGNED
Having issues getting this to work due to some load listener weirdness on xul iframes. I should have a fix for this tomorrow.
(In reply to comment #6)
> (In reply to comment #4)
> > It's not resolved, according to new notifications in m.d.tree-management.
> 
> I didn't see any regressions in the group the next day. I must have missed
> them. Disappointing!

I didn't check the graphs, but these are the notifications I was referring to:
http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/a16d5ec899aad310
those really didn't show up on Try, or I misinterpreted the lines. They looked well within the acceptable range of noise.

At any rate, I have a patch! hopefully this'll fix the problem.
Attached patch Inspector iframe loader (obsolete) — Splinter Review
made a simple iframe loader section in openTreePanel(). Tested locally and does the job.

Only loads once. Subsequent Inspector openings will use the existing document.
Attachment #472375 - Flags: review?(gavin.sharp)
Blocks: devtools4b7
I've just found this comment at the end of browser.xul:

> <iframe id="tab-view"> is dynamically appended as the 2nd child of #tab-view-deck.
>     Introducing the iframe dynamically, as needed, was found to be better than
>     starting with an empty iframe here in browser.xul from a Ts standpoint.
I did a search for references to other iframes in xul and didn't find this. I actually considered building the iframe dynamically but worried that it would be considered too hacky. Thanks for the tip.
programmatically adding the iframe in openTreePanel now.
Attachment #472375 - Attachment is obsolete: true
Attachment #473048 - Flags: review?(gavin.sharp)
Attachment #472375 - Flags: review?(gavin.sharp)
Attachment #473048 - Attachment is patch: true
Attachment #473048 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 473048 [details] [diff] [review]
Inspector iframe builder and loader

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>+#     Creating programmatically in inspector.js::IUI_openTreePanel()
>+#     to address Txul regression. See bug 592525.
>+#     <iframe id="inspector-tree-iframe"
>+#             flex="1"
>+#             type="content"
>+#             src="about:blank"
>+#             onclick="InspectorUI.onTreeClick(event);" />

Not sure there's much benefit to having this here - just remove it?

>diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js

>+      this.treeIFrame.addEventListener("DOMContentLoaded", function() {
>+        self.treeIFrame.removeEventListener("DOMContentLoaded", arguments.callee, true);
>+        InspectorUI.initializeTreePanel();

self.initializeTreePanel()?
Attachment #473048 - Flags: review?(gavin.sharp) → review+
blocking2.0: --- → beta6+
(In reply to comment #14)
> Comment on attachment 473048 [details] [diff] [review]
> Inspector iframe builder and loader
> 
> >diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
> 
> >+#     Creating programmatically in inspector.js::IUI_openTreePanel()
> >+#     to address Txul regression. See bug 592525.
> >+#     <iframe id="inspector-tree-iframe"
> >+#             flex="1"
> >+#             type="content"
> >+#             src="about:blank"
> >+#             onclick="InspectorUI.onTreeClick(event);" />
> 
> Not sure there's much benefit to having this here - just remove it?

There was a similar block at the end of browser.xul for tabview. I thought it might be useful to save someone else from having a similar txul regression in the future.

> >diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js
> 
> >+      this.treeIFrame.addEventListener("DOMContentLoaded", function() {
> >+        self.treeIFrame.removeEventListener("DOMContentLoaded", arguments.callee, true);
> >+        InspectorUI.initializeTreePanel();
> 
> self.initializeTreePanel()?

ooh yeah. Thanks!
Sorry to make you revisit this, Gavin. I had to make some changes because of test failures.

The dynamic loader was causing timing failures with the observer notification. I broke the notification out into a separate method (notifyReady) and added a treeLoaded state variable. Retested and this works well.

Additionally, I noticed that I was not adding the id="inspector-tree-iframe" attribute to the iframe at construction time. This meant that subsequent opens would create new iframes. Added a check to the initialization test to make sure it's there as well as the attribute.
Attachment #473048 - Attachment is obsolete: true
Attachment #473621 - Flags: review?(gavin.sharp)
and now with the actual patch.
Attachment #473621 - Attachment is obsolete: true
Attachment #473622 - Flags: review?(gavin.sharp)
Attachment #473621 - Flags: review?(gavin.sharp)
Attachment #473622 - Flags: review?(gavin.sharp) → review+
Comment on attachment 473622 [details] [diff] [review]
[checked-in] Inspector iframe builder and loader

http://hg.mozilla.org/mozilla-central/rev/a2be9c644f47
Attachment #473622 - Attachment description: Inspector iframe builder and loader → [checked-in] Inspector iframe builder and loader
from dev.tree-management:
http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/3dc0346c14f1250a#

Haven't heard from any of the windows talos builders yet, but I'm taking this as a good sign. Marking fixed.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.