pageloader no longer creating browser windows with chrome

RESOLVED FIXED

Status

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: anodelman, Assigned: anodelman)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
Created attachment 527411 [details] [diff] [review]
[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... ?
Assignee: nobody → anodelman
Attachment #527411 - Flags: review?(jmaher)
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!
Attachment #527411 - Flags: review?(jmaher) → review+
(Assignee)

Updated

8 years ago
Depends on: 651670
(Assignee)

Comment 3

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

changeset:   5:bffb8b1b0948
Attachment #527411 - Attachment description: one small change and chrome returns! (take 1) → [checked in]one small change and chrome returns! (take 1)
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?
(Assignee)

Comment 5

8 years ago
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.
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

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

Comment 8

8 years ago
(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 :-(
Blocks: 509124
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.
Can this be marked as fixed now?
(Assignee)

Comment 11

8 years ago
Yes, this bug is fixed.  We now draw chrome when we should.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

8 years ago
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.
Don't worry about it.  I just pushed 22 changesets to try to figure this out (see bug 655930 for the details)
(Assignee)

Comment 14

8 years ago
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.
(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?
(Assignee)

Comment 16

8 years ago
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.
You need to log in before you can comment on or make changes to this bug.