651659 - pageloader no longer creating browser windows with chrome

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[alice nodelman [:alice] [:anode]]

Looking at talos while it runs the browser window generated for page cycling no longer has any browser chrome.  I believe that this must have occurred as a change in the browser instead of the code, as the code for window creation hasn't been altered in a long time and was provably working at earlier dates.

Comment 1

[alice nodelman [:alice] [:anode]]

Attachment: [checked in]one small change and chrome returns! (take 1)

Made this change on linux staging and chrome magically returned.  Wonder how long we have been running without... ?

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 527411 [details] [diff] [review]
[checked in]one small change and chrome returns! (take 1)

if this doesn't affect mobile talos, r=me!

Comment 3

[alice nodelman [:alice] [:anode]]

Comment on attachment 527411 [details] [diff] [review]
[checked in]one small change and chrome returns! (take 1)

changeset:   5:bffb8b1b0948

Comment 4

[Dão Gottwald [:dao]]

This is a big deal, since we now have figure out why the pageload numbers on Windows suck so badly (bug 655930). Is there any way to find out when the pageload test broke and how this affected (i.e. "improved") the results back then?

Comment 5

[alice nodelman [:alice] [:anode]]

When I looked through the check-in queue for pageloader I did not find that the pageloader had changed, I concluded that it was a browser change that caused the chrome to be become disabled.

You can see from the patch that the fix to this was a very minor change to the window creation code:

       browserWindow = wwatch.openWindow
         (null, "chrome://browser/content/", "_blank",
-         "chrome,dialog=no,width=" + winWidth + ",height=" + winHeight, blank);
+         "chrome,all,dialog=no,width=" + winWidth + ",height=" + winHeight, blank);

I think that it would have been some more stringent interpretation of the openWindow method.  Not sure how to go digging for that.

Comment 6

[Dão Gottwald [:dao]]

I just tested Firefox 3.6.17 and 4.0.1. This opens a normal window with the former, a chromeless window with the latter:

Components.classes["@mozilla.org/embedcomp/window-watcher;1"].getService(Components.interfaces.nsIWindowWatcher).openWindow(null, "chrome://browser/content/", "_blank","chrome,dialog=no,width=300,height=300", {});

Comment 7

[[e-mail]]

Interestingly window.openDialog gets this right (always opens as popup unless "all" specified).

Comment 8

[[e-mail]]

(In reply to comment #7)
> Interestingly window.openDialog gets this right (always opens as popup
> unless "all" specified).
This is a "regression" from bug 509124. The problem is that there was an edge case when opening chrome windows where the chrome flags were getting ignored. Bug 509124 fixed the edge case but it turns out that there were consumers who were passing the wrong chrome flags and getting lucky :-(

Comment 9

[Dão Gottwald [:dao]]

That's August 2009... do we still have the talos numbers from that time? I'm trying to figure out if bug 655930 regressed since then or if the chrome overhead has always been that large on Windows.

Comment 10

[Shawn Wilsher :sdwilsh]

Can this be marked as fixed now?

Comment 11

[alice nodelman [:alice] [:anode]]

Yes, this bug is fixed.  We now draw chrome when we should.

Comment 12

[alice nodelman [:alice] [:anode]]

In terms of data from 2009, I'm seeing what I can find.

We switched to the new rev3 talos minis around Jan 2010 - and that seems to be as far back as the graph server has data for.  We may have decided to store the data from the older rev2 machines to declutter the graph server, I'll see if I can find evidence of that.

Comment 13

[Shawn Wilsher :sdwilsh]

Don't worry about it.  I just pushed 22 changesets to try to figure this out (see bug 655930 for the details)

Comment 14

[alice nodelman [:alice] [:anode]]

Well, I found it anyway.

This happened in bug 535009 - the data is still there but the rev2 machines have been marked as inactive and are thus no longer displayed on the graph server front end.

Comment 15

[Shawn Wilsher :sdwilsh]

(In reply to comment #14)
> This happened in bug 535009 - the data is still there but the rev2 machines
> have been marked as inactive and are thus no longer displayed on the graph
> server front end.
Did we run chrome and no chrome then?  If so, what's the difference in times for the two on those machines?

Comment 16

[alice nodelman [:alice] [:anode]]

We should have been running chrome/nochrome back then, yes.  To see the data we'd have to re-activate the machines marked as inactive - or have IT pull the data from the db some other way.

Since you are already collecting new data we can probably let those old dogs lie.