717963 - Use getBrowserURL() in Firefox tests to ease porting them, part 1

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Serge Gautherie (:sgautherie)]

"Found 2 matching lines in 2 files"

See bug 717753 as an example.

Comment 1

[Mark Capella [:capella]]

Attachment: Possible fix ... [Moved to bug 725647]

The comment says two files found, I currently find / change: test_embeds.xul.

   What's the other? I'll fix it...

Comment 2

[Serge Gautherie (:sgautherie)]

Comment on attachment 595679 [details] [diff] [review]
Possible fix ...
[Moved to bug 725647]

(In reply to Mark Capella from comment #1)

> The comment says two files found, I currently find / change: test_embeds.xul.

Good catch!
Though this is not exactly this bug: I filed bug 725647 and assigned it to you.
Also, you copied the code I initially attached rather than the code I eventually checked in.

NB: In this case, I don't think you need to add the bug reference to the test, but reviewer would tell.

> What's the other? I'll fix it...

Thanks!
See 'URL' field of this bug.

Comment 3

[Mark Capella [:capella]]

FYI and comments / feedback please...

   This bug would refer to changes in:

      File#1) /browser/components/sessionstore/test/browser_580512.js
      File#2) /browser/base/content/test/browser_bug422590.js

   File#1 is already using Services.prefs so the change seems to be:

      From ===>
      var newWin = openDialog("chrome://browser/content/", "_blank",
                          "chrome,dialog=no,toolbar=no", "about:blank");
      To   ===>
      var newWin = openDialog(Services.prefs.getCharPref("browser.chromeURL"), "_blank",
                          "chrome,dialog=no,toolbar=no", "about:blank");

   File#2 is not apparently using Services.prefs, so the change would be:

      From ===>
      var win = openDialog("chrome://browser/content/", "_blank",
                           "chrome,all,dialog=no", argURIs.join("|"));

      To   ===>
      Components.utils.import("resource://gre/modules/Services.jsm");       // If import required
      var win = openDialog(Services.prefs.getCharPref("browser.chromeURL"), "_blank",
                           "chrome,all,dialog=no", argURIs.join("|"));

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to Mark Capella [:capella] from comment #3)

>       var newWin =
> openDialog(Services.prefs.getCharPref("browser.chromeURL"), "_blank",

Yes, that should be it.

>       Components.utils.import("resource://gre/modules/Services.jsm");      
> // If import required

Services.jsm should already be available to browser-chrome tests, iirc.

Comment 5

[Mark Capella [:capella]]

Attachment: Use "browser.chromeURL"

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 599556 [details] [diff] [review]
Use "browser.chromeURL"

Please use getBrowserURL().

Comment 7

[Mark Capella [:capella]]

Attachment: Use getBrowserURL()

After a review+ I can do a TRY server AUTOLAND or ask someone to push it for me.

Comment 8

[Dão Gottwald [:dao]]

You're replacing openDialog with window.openDialog for seemingly no good reason. Please don't do this.

Comment 9

[Mark Capella [:capella]]

Attachment: Use getBrowserURL() [Has CRLF :-(]

Sorry, I grabbed the wrong code snip.

Comment 10

[Serge Gautherie (:sgautherie)]

["Mid-air collision detected!", on review then on comment. Bah :-|]

(In reply to Dão Gottwald [:dao] from comment #6)
> Please use getBrowserURL().

Good catch: browser-chrome tests have access to this function (which SeaMonkey has too) :-)

(In reply to Mark Capella [:capella] from comment #7)
> After a review+ I can do a TRY server AUTOLAND or ask someone to push it for
> me.

No need for this trivial patch.

Comment 11

[Mark Capella [:capella]]

Thanks to all :-P

Comment 12

[Serge Gautherie (:sgautherie)]

> [autoland-try]

And, for this patch, you should have used "-u mochitest-o" to save resources.

Comment 13

[Mark Capella [:capella]]

serge: Found this... http://trychooser.pub.build.mozilla.org/, will read

Comment 14

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 599571
	Branch: mozilla-central => try
Error applying patch 599571 to mozilla-central.
patching file browser/base/content/test/browser_bug422590.js
Hunk #1 FAILED at 2
1 out of 1 hunks FAILED -- saving rejects to file browser/base/content/test/browser_bug422590.js.rej
patching file browser/components/sessionstore/test/browser_580512.js
Hunk #1 FAILED at 34
1 out of 1 hunks FAILED -- saving rejects to file browser/components/sessionstore/test/browser_580512.js.rej
abort: patch failed to apply

Could not apply and push patchset:

Comment 15

[Serge Gautherie (:sgautherie)]

(In reply to Mozilla RelEng Bot from comment #14)
> abort: patch failed to apply

Ftr,
Good wrt this bug, but rather odd wrt AutoLand :-/
I wonder whether the issue is with the patch (which looks good) or the (experimental) service.

Comment 16

[Ed Morley [:emorley]]

(In reply to Serge Gautherie (:sgautherie) from comment #15)
> I wonder whether the issue is with the patch (which looks good) or the
> (experimental) service.

Autoland applies the patch to m-c tip. So either the patch needs unbitrotting, or it only applies cleanly on top of something that had not yet merged from inbound to m-c, but will be fine when landed directly on inbound.

Comment 17

[Mark Capella [:capella]]

I reviewed the DIFF file locally, and see my text editor had added WIN style CRLF's ... I can post the corrected DIFF if it's easiest to all...

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to Mark Capella [:capella] from comment #17)
> I reviewed the DIFF file locally, and see my text editor had added WIN style
> CRLF's ... I can post the corrected DIFF if it's easiest to all...

Yes, please, we usually work with LF only.

Comment 19

[Mark Capella [:capella]]

Attachment: Use getBrowserURL()

Comment 20

[Serge Gautherie (:sgautherie)]

Mark, please add author and description to your patch, to ease c-n:
see http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Comment 21

[Mark Capella [:capella]]

Attachment: Use getBrowserURL() [Checked in: Comment 27]

Sorry Serge, I'm still getting used to the process. Comment please.

Comment 22

[Mark Capella [:capella]]

Comment on attachment 600409 [details] [diff] [review]
Use getBrowserURL()
[Checked in: Comment 27]

I added Author / Title to attachment ... looks like I need a re-review before checkin

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 600409 [details] [diff] [review]
Use getBrowserURL()
[Checked in: Comment 27]

(In reply to Mark Capella [:capella] from comment #22)
> I added Author / Title to attachment ... looks like I need a re-review
> before checkin

No, you don't: the code is exactly the same.

Comment 24

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89fbc0fd6006

Comment 25

[Serge Gautherie (:sgautherie)]

Ah, it looks like there is a few more:
http://mxr.mozilla.org/mozilla-central/search?string="chrome%3A%2F%2Fbrowser%2Fcontent%2Fbrowser.xul&case=1&find=%2Fbrowser%2F.*%2Ftest
"Found 4 matching lines in 3 files"

Comment 26

[Mark Capella [:capella]]

I might have preferred to allow this patch to finish the move to production, and then to address these three files in a followup bug. With all the back and forth already on this, I'm getting the feeling of shooting a moving target. Are we sure the research is done now, and there won't be additional scope creep later?

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/89fbc0fd6006

Comment 28

[Serge Gautherie (:sgautherie)]

(In reply to Mark Capella [:capella] from comment #26)

> I might have preferred to allow this patch to finish the move to production,
> and then to address these three files in a followup bug. With all the back

Sure. I was thinking about a second patch: this is still this bug.

> and forth already on this, I'm getting the feeling of shooting a moving
> target. Are we sure the research is done now, and there won't be additional
> scope creep later?

Sorry not to have been explicit about that earlier: the additional search is taken from bug 17969.
Please take it easy: "sure" vs "real life" :-|

Comment 29

[Mark Capella [:capella]]

I'm going to have to drop off this one due to my backlog and recent involvement with new development projects in the browser. I'd hoped to have had it completed by now, but it's fairly straight-forward, so I'll ask someone else to carry it to completion.

Comment 30

[Dão Gottwald [:dao]]

File a new bug.

Comment 31

[Serge Gautherie (:sgautherie)]

(In reply to Dão Gottwald [:dao] from comment #30)
> File a new bug.

Bug 730840