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

RESOLVED FIXED in Firefox 13

Status

()

Firefox
General
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: capella)

Tracking

Trunk
Firefox 13
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

5 years ago
"Found 2 matching lines in 2 files"

See bug 717753 as an example.
(Reporter)

Updated

5 years ago
Blocks: 717969
(Reporter)

Updated

5 years ago
No longer depends on: 717753
(Assignee)

Comment 1

5 years ago
Created attachment 595679 [details] [diff] [review]
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...
(Reporter)

Comment 2

5 years ago
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.
Attachment #595679 - Attachment description: Possible fix ... → Possible fix ... [Moved to bug 725647]
Attachment #595679 - Attachment is obsolete: true
Attachment #595679 - Flags: feedback-
(Assignee)

Updated

5 years ago
Assignee: nobody → markcapella
(Assignee)

Comment 3

5 years ago
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("|"));
(Reporter)

Comment 4

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

Comment 5

5 years ago
Created attachment 599556 [details] [diff] [review]
Use "browser.chromeURL"
Attachment #599556 - Flags: review?(sgautherie.bz)
Comment on attachment 599556 [details] [diff] [review]
Use "browser.chromeURL"

Please use getBrowserURL().
Attachment #599556 - Flags: review?(sgautherie.bz) → review-
(Assignee)

Comment 7

5 years ago
Created attachment 599566 [details] [diff] [review]
Use getBrowserURL()

After a review+ I can do a TRY server AUTOLAND or ask someone to push it for me.
Attachment #599556 - Attachment is obsolete: true
Attachment #599566 - Flags: review?(sgautherie.bz)
You're replacing openDialog with window.openDialog for seemingly no good reason. Please don't do this.
(Assignee)

Comment 9

5 years ago
Created attachment 599571 [details] [diff] [review]
Use getBrowserURL()
[Has CRLF :-(]

Sorry, I grabbed the wrong code snip.
Attachment #599566 - Attachment is obsolete: true
Attachment #599566 - Flags: review?(sgautherie.bz)
Attachment #599571 - Flags: review?(sgautherie.bz)

Updated

5 years ago
Attachment #599571 - Flags: review?(sgautherie.bz) → review+
(Assignee)

Updated

5 years ago
Whiteboard: [good first bug] → [autoland-try]

Updated

5 years ago
Whiteboard: [autoland-try] → [autoland-in-queue]
(Reporter)

Comment 10

5 years ago
["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.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Target Milestone: --- → Firefox 13
(Assignee)

Comment 11

5 years ago
Thanks to all :-P
(Reporter)

Comment 12

5 years ago
> [autoland-try]

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

Updated

5 years ago
Attachment #599571 - Flags: feedback+
(Reporter)

Updated

5 years ago
Attachment #599556 - Flags: feedback+
(Assignee)

Comment 13

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

Comment 14

5 years ago
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:

Updated

5 years ago
Whiteboard: [autoland-in-queue]
(Reporter)

Comment 15

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

Comment 17

5 years ago
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...
(Reporter)

Comment 18

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

Comment 19

5 years ago
Created attachment 599616 [details] [diff] [review]
Use getBrowserURL()
(Reporter)

Updated

5 years ago
Attachment #599616 - Attachment description: DIFF no CR → Use getBrowserURL()
(Reporter)

Updated

5 years ago
Attachment #599571 - Attachment description: 3d Try Patch → Use getBrowserURL() [Has CRLF :-(]
(Reporter)

Updated

5 years ago
Attachment #599566 - Attachment description: 2nd Try Patch / DIFF File → Use getBrowserURL()
(Reporter)

Updated

5 years ago
Attachment #599556 - Attachment description: Patch / DIFF File → USe "browser.chromeURL"
(Reporter)

Updated

5 years ago
Attachment #599556 - Attachment description: USe "browser.chromeURL" → Use "browser.chromeURL"
(Reporter)

Updated

5 years ago
Whiteboard: [c-n: patch 599616]
(Reporter)

Comment 20

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

Comment 21

5 years ago
Created attachment 600409 [details] [diff] [review]
Use getBrowserURL()
[Checked in: Comment 27]

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

Updated

5 years ago
Whiteboard: [c-n: patch 599616] → [c-n: patch 600409]
(Assignee)

Comment 22

5 years ago
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
Attachment #600409 - Flags: review?(dao)
(Reporter)

Updated

5 years ago
Attachment #599616 - Attachment is obsolete: true
(Reporter)

Comment 23

5 years ago
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.
Attachment #600409 - Attachment description: Use getBrowserURL(v2) No CR Has Author/Title → Use getBrowserURL()
Attachment #600409 - Flags: review?(dao)

Updated

5 years ago
Attachment #600409 - Flags: checkin?

Updated

5 years ago
Whiteboard: [c-n: patch 600409]
https://hg.mozilla.org/integration/mozilla-inbound/rev/89fbc0fd6006
Keywords: checkin-needed
Summary: Use "browser.chromeURL" preference in Firefox tests, to ease porting them → Use getBrowserURL() in Firefox tests to ease porting them
(Reporter)

Comment 25

5 years ago
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"

Updated

5 years ago
Attachment #599571 - Attachment is obsolete: true
(Assignee)

Comment 26

5 years ago
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?
https://hg.mozilla.org/mozilla-central/rev/89fbc0fd6006
(Reporter)

Updated

5 years ago
Attachment #600409 - Attachment description: Use getBrowserURL() → Use getBrowserURL() [Checked in: Comment 27]
Attachment #600409 - Flags: checkin? → checkin+
(Reporter)

Comment 28

5 years ago
(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" :-|
(Assignee)

Comment 29

5 years ago
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.
Assignee: markcapella → nobody
(Reporter)

Updated

5 years ago
Status: ASSIGNED → NEW
Whiteboard: [ToDo: comment 25] [good first bug][mentor=sgautherie][lang=js]
File a new bug.
Assignee: nobody → markcapella
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [ToDo: comment 25] [good first bug][mentor=sgautherie][lang=js] → [good first bug][mentor=sgautherie][lang=js]
(Reporter)

Updated

5 years ago
Blocks: 730840
(Reporter)

Comment 31

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

Bug 730840
Flags: in-testsuite+
Summary: Use getBrowserURL() in Firefox tests to ease porting them → Use getBrowserURL() in Firefox tests to ease porting them, part 1
Whiteboard: [good first bug][mentor=sgautherie][lang=js]
You need to log in before you can comment on or make changes to this bug.