Last Comment Bug 661132 - Fix test for bug 642338 so it doesn't rely on firefox.js prefs
: Fix test for bug 642338 so it doesn't rely on firefox.js prefs
Status: RESOLVED FIXED
[Blocks (SM) bug 653996] [good first ...
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- major (vote)
: mozilla7
Assigned To: Justin Lebar (not reading bugmail)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 653996 642338
  Show dependency treegraph
 
Reported: 2011-06-01 06:11 PDT by Justin Lebar (not reading bugmail)
Modified: 2011-06-15 10:03 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected


Attachments
Patch v1 (1.96 KB, patch)
2011-06-06 10:18 PDT, Justin Lebar (not reading bugmail)
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Splinter Review
Patch v2 (2.24 KB, patch)
2011-06-06 15:15 PDT, Justin Lebar (not reading bugmail)
bugs: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2011-06-01 06:11:39 PDT
The test I added in bug 642338 relies on a Firefox-specific pref, that you can't hide the location bar.  Since this is a core test, it shouldn't rely on this pref being set.
Comment 1 Frédéric Buclin 2011-06-01 06:17:09 PDT
Wrong bug ID.
Comment 2 Justin Lebar (not reading bugmail) 2011-06-01 06:22:38 PDT
Thanks.
Comment 3 Serge Gautherie (:sgautherie) 2011-06-02 13:38:05 PDT
Bug 642338 comment 37:
{
Serge Gautherie (:sgautherie)      2011-05-31 17:07:04 PDT

> If not, I'll review a patch to modify the test so it checks the pref before
> the is() call.

To be explicit, is() check is fine in both cases, but its expected value should simply depend on "dom.disable_window_open_feature.location"(!?) preference value.
}

More details are in bug 653996.
Comment 4 Justin Lebar (not reading bugmail) 2011-06-06 09:46:47 PDT
So just to be clear, I *can* rely on a pref in all.js?

> pref("dom.disable_window_open_feature.status",      true);
Comment 5 Justin Lebar (not reading bugmail) 2011-06-06 10:18:24 PDT
Created attachment 537580 [details] [diff] [review]
Patch v1
Comment 6 Justin Lebar (not reading bugmail) 2011-06-06 10:18:56 PDT
> So just to be clear, I *can* rely on a pref in all.js?

Actually, nevermind.  That's not necessary.
Comment 7 Serge Gautherie (:sgautherie) 2011-06-06 15:09:15 PDT
Comment on attachment 537580 [details] [diff] [review]
Patch v1

[Mozilla/5.0 (Windows NT 5.0; rv:7.0a1) Gecko/20110605 Firefox/7.0a1 SeaMonkey/2.2a1pre] (nightly, SM 2.4a1pre !)

I like this code. And it fixes bug 653996 :-)

From my bug 653996 comment 3:
{
Nit: you might want to add "from HTML" (or "content" or whatever applies) to the comment, "ftr".
}
Comment 8 Justin Lebar (not reading bugmail) 2011-06-06 15:15:25 PDT
Created attachment 537657 [details] [diff] [review]
Patch v2
Comment 9 Justin Lebar (not reading bugmail) 2011-06-11 14:04:41 PDT
Comment on attachment 537657 [details] [diff] [review]
Patch v2

I meant to ask for Smaug's review on this patch, but I apparently missed that.
Comment 10 Matt Brubeck (:mbrubeck) 2011-06-15 10:03:49 PDT
http://hg.mozilla.org/mozilla-central/rev/1b46a093e413

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