Last Comment Bug 717963 - Use getBrowserURL() in Firefox tests to ease porting them, part 1
: Use getBrowserURL() in Firefox tests to ease porting them, part 1
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Firefox 13
Assigned To: Mark Capella [:capella]
:
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on:
Blocks: 717969 730840
  Show dependency treegraph
 
Reported: 2012-01-13 09:18 PST by Serge Gautherie (:sgautherie)
Modified: 2012-02-27 09:04 PST (History)
3 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Possible fix ... [Moved to bug 725647] (1.66 KB, patch)
2012-02-09 00:43 PST, Mark Capella [:capella]
bugzillamozillaorg_serge_20140323: feedback-
Details | Diff | Review
Use "browser.chromeURL" (1.77 KB, patch)
2012-02-22 04:03 PST, Mark Capella [:capella]
dao+bmo: review-
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Review
Use getBrowserURL() (1.96 KB, patch)
2012-02-22 04:31 PST, Mark Capella [:capella]
no flags Details | Diff | Review
Use getBrowserURL() [Has CRLF :-(] (1.94 KB, patch)
2012-02-22 04:51 PST, Mark Capella [:capella]
dao+bmo: review+
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Review
Use getBrowserURL() (1.89 KB, patch)
2012-02-22 07:51 PST, Mark Capella [:capella]
no flags Details | Diff | Review
Use getBrowserURL() [Checked in: Comment 27] (2.02 KB, patch)
2012-02-24 08:39 PST, Mark Capella [:capella]
bugzillamozillaorg_serge_20140323: checkin+
Details | Diff | Review

Description Serge Gautherie (:sgautherie) 2012-01-13 09:18:23 PST
"Found 2 matching lines in 2 files"

See bug 717753 as an example.
Comment 1 Mark Capella [:capella] 2012-02-09 00:43:38 PST
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...
Comment 2 Serge Gautherie (:sgautherie) 2012-02-09 07:08:49 PST
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] 2012-02-21 19:21:36 PST
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) 2012-02-21 19:56:44 PST
(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] 2012-02-22 04:03:42 PST
Created attachment 599556 [details] [diff] [review]
Use "browser.chromeURL"
Comment 6 Dão Gottwald [:dao] 2012-02-22 04:07:35 PST
Comment on attachment 599556 [details] [diff] [review]
Use "browser.chromeURL"

Please use getBrowserURL().
Comment 7 Mark Capella [:capella] 2012-02-22 04:31:06 PST
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.
Comment 8 Dão Gottwald [:dao] 2012-02-22 04:43:14 PST
You're replacing openDialog with window.openDialog for seemingly no good reason. Please don't do this.
Comment 9 Mark Capella [:capella] 2012-02-22 04:51:55 PST
Created attachment 599571 [details] [diff] [review]
Use getBrowserURL()
[Has CRLF :-(]

Sorry, I grabbed the wrong code snip.
Comment 10 Serge Gautherie (:sgautherie) 2012-02-22 05:03:03 PST
["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] 2012-02-22 05:05:09 PST
Thanks to all :-P
Comment 12 Serge Gautherie (:sgautherie) 2012-02-22 05:07:50 PST
> [autoland-try]

And, for this patch, you should have used "-u mochitest-o" to save resources.
Comment 13 Mark Capella [:capella] 2012-02-22 05:18:49 PST
serge: Found this... http://trychooser.pub.build.mozilla.org/, will read
Comment 14 Mozilla RelEng Bot 2012-02-22 05:20:39 PST
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) 2012-02-22 05:59:17 PST
(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] 2012-02-22 07:06:09 PST
(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] 2012-02-22 07:20:05 PST
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) 2012-02-22 07:47:30 PST
(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] 2012-02-22 07:51:50 PST
Created attachment 599616 [details] [diff] [review]
Use getBrowserURL()
Comment 20 Serge Gautherie (:sgautherie) 2012-02-24 08:18:28 PST
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] 2012-02-24 08:39:51 PST
Created attachment 600409 [details] [diff] [review]
Use getBrowserURL()
[Checked in: Comment 27]

Sorry Serge, I'm still getting used to the process. Comment please.
Comment 22 Mark Capella [:capella] 2012-02-25 04:45:35 PST
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) 2012-02-25 06:14:15 PST
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 25 Serge Gautherie (:sgautherie) 2012-02-26 00:16:19 PST
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] 2012-02-26 13:16:56 PST
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) 2012-02-26 16:23:22 PST
https://hg.mozilla.org/mozilla-central/rev/89fbc0fd6006
Comment 28 Serge Gautherie (:sgautherie) 2012-02-26 23:54:26 PST
(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] 2012-02-27 00:55:32 PST
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] 2012-02-27 04:50:48 PST
File a new bug.
Comment 31 Serge Gautherie (:sgautherie) 2012-02-27 09:04:47 PST
(In reply to Dão Gottwald [:dao] from comment #30)
> File a new bug.

Bug 730840

Note You need to log in before you can comment on or make changes to this bug.