717966 - pageloader.js: use "browser.chromeURL" preference, to support non-Firefox applications

Status: RESOLVED WONTFIX

(Core :: Layout, defect)

Description

[Serge Gautherie (:sgautherie)]

I assume SeaMonkey (and other "non-Firefox" applications) is not using this (yet),
but let's future-proof this anyway.

Code is
{
148       browserWindow = wwatch.openWindow
149         (null, "chrome://browser/content/", "_blank",
150          "chrome,dialog=no,width=" + winWidth + ",height=" + winHeight, blank);
}

See bug 717753 as an example.

Comment 1

[Mark Capella [:capella]]

Attachment: bug717966 fix initial try

Looking at bugs regarding "browser.chromeURL" Firefox vs Seamonkey ...

Comment 2

[Serge Gautherie (:sgautherie)]

Comment on attachment 596622 [details] [diff] [review]
bug717966 fix initial try

Can't "browser.chromeURL" preference be used here?

Comment 3

[Mark Capella [:capella]]

Attachment: Diff2 try ... using preferences

Ok ... using Services.prefs.getCharPref() vs. navigator.userAgent.match() ...

Comment 4

[Serge Gautherie (:sgautherie)]

Comment on attachment 596824 [details] [diff] [review]
Diff2 try ... using preferences

Did you test this or does it need a run on Try?

Comment 5

[Mark Capella [:capella]]

   No test ... (that's why I was only asking for feedback) ... I hadn't figured that next part out yet.

   So I guess the answer to your question is yes, it needs a test run on try ... and if you can explain that to me a little I'll appreciate it :-)

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to Mark Capella [:capella] from comment #5)
> if you can explain that to me a little I'll appreciate it :-)

A first pointer should be:
https://wiki.mozilla.org/Build:TryServer

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 596824 [details] [diff] [review]
Diff2 try ... using preferences

NB:
The idea of this patch is what I expected.
But I don't know whether Services.prefs is already available, or if Services.jsm should be imported, or Cc[].getService() used...

Comment 8

[Mark Capella [:capella]]

   I can't do a push to try as I'm not Level 1 ... I'll need a review+ or to have someone do it for me.

Comment 9

[Mozilla RelEng Bot]

Try run for 4605c5af6ad2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4605c5af6ad2
Results (out of 274 total builds):
    exception: 2
    success: 236
    warnings: 21
    failure: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4605c5af6ad2

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 596824 [details] [diff] [review]
Diff2 try ... using preferences

(In reply to Mozilla RelEng Bot from comment #9)
> https://tbpl.mozilla.org/?tree=Try&rev=4605c5af6ad2

I don't know where pageloader.js is used exactly,
but afaict this patch didn't break anything.

Comment 11

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

Comment on attachment 596824 [details] [diff] [review]
Diff2 try ... using preferences

Services.jsm isn't imported here, AFAICT.

Comment 12

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

If this file isn't used, we shouldn't spend time making untested arbitrary changes to it. It can be fixed when it poses a problem in practice.

Comment 13

[Serge Gautherie (:sgautherie)]

Ftr,

http://mxr.mozilla.org/comm-central/source/mozilla/testing/tools/pageloader/README
http://mxr.mozilla.org/build/search?string=pageloader.&case=1

I assumed pageloader was used by Talos (runs) or something, but maybe it is not (currently/anymore)...

PS: If there is no plan to reuse it, maybe it should just be deleted from trees?

Comment 14

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

Yes, it should. But determining whether it is or isn't used requires work, and there are generally more important things to work on.

Comment 15

[Serge Gautherie (:sgautherie)]

Bug 727705 removed this file.
Let's see if it still exists elsewhere after bug 725414...