592525 - Txul Regression between 1PM-3PM PDT

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Rob Campbell [:rc] (:robcee)]

Coincided with landing:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ccf0c898b4c8&tochange=6505b9f8aad0

backed out portion in:
http://hg.mozilla.org/mozilla-central/rev/3105d6159ffa

.tree-planning logs:

http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/be1d7aef39f8a9ea#



http://graphs.mozilla.org/graph.html#tests=[[17,1,431],[17,1,432],[17,1,433],[17,1,434],[17,1,435],[17,1,436],[17,1,437],[17,1,438],[17,1,439],[17,1,440],[17,1,441],[17,1,442],[17,1,443],[17,1,444],[17,1,445],[17,1,446],[17,1,447],[17,1,448],[17,1,449],[17,1,450],[17,1,688],[17,1,689],[17,1,690],[17,1,691],[17,1,692],[17,1,693],[17,1,694],[17,1,695],[17,1,696],[17,1,697],[17,1,698],[17,1,699],[17,1,700],[17,1,701],[17,1,702],[17,1,703],[17,1,704],[17,1,705],[17,1,706],[17,1,707],[17,1,708],[17,1,709],[17,1,710],[17,1,711],[17,1,712],[17,1,713],[17,1,714],[17,1,715],[17,1,716],[17,1,717],[17,1,895],[17,1,896],[17,1,897],[17,1,898],[17,1,899]]&sel=1283173041,1283295966

Comment 1

[Rob Campbell [:rc] (:robcee)]

currently only linux64 but waiting on windows builds.

Comment 2

[Rob Campbell [:rc] (:robcee)]

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.

Comment 3

[Kevin Dangoor]

I believe this should be resolved/fixed now with the landing of the updated tree panel patch.

Comment 4

[Markus Stange [:mstange]]

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?

Comment 5

[Kevin Dangoor]

Reprioritizing bugs. You can filter the mail on the word TEABAGS.

Comment 6

[Rob Campbell [:rc] (:robcee)]

(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.

Comment 7

[Rob Campbell [:rc] (:robcee)]

Having issues getting this to work due to some load listener weirdness on xul iframes. I should have a fix for this tomorrow.

Comment 8

[Markus Stange [:mstange]]

(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

Comment 9

[Rob Campbell [:rc] (:robcee)]

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.

Comment 10

[Rob Campbell [:rc] (:robcee)]

Attachment: Inspector iframe loader

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.

Comment 11

[Markus Stange [:mstange]]

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.

Comment 12

[Rob Campbell [:rc] (:robcee)]

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.

Comment 13

[Rob Campbell [:rc] (:robcee)]

Attachment: Inspector iframe builder and loader

programmatically adding the iframe in openTreePanel now.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

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()?

Comment 15

[Rob Campbell [:rc] (:robcee)]

(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!

Comment 16

[Rob Campbell [:rc] (:robcee)]

Attachment: Inspector iframe builder and loader

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.

Comment 17

[Rob Campbell [:rc] (:robcee)]

Attachment: [checked-in] Inspector iframe builder and loader

and now with the actual patch.

Comment 18

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 473622 [details] [diff] [review]
[checked-in] Inspector iframe builder and loader

http://hg.mozilla.org/mozilla-central/rev/a2be9c644f47

Comment 19

[Rob Campbell [:rc] (:robcee)]

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.