Last Comment Bug 703867 - Update /dom/tests/browser/browser_popup_blocker_save_open_panel.js to use MockFilePicker.jsm
: Update /dom/tests/browser/browser_popup_blocker_save_open_panel.js to use Moc...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Geoff Lankow (:darktrojan)
:
Mentors:
Depends on:
Blocks: 668710
  Show dependency treegraph
 
Reported: 2011-11-19 04:32 PST by Geoff Lankow (:darktrojan)
Modified: 2012-02-01 14:00 PST (History)
1 user (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.68 KB, patch)
2011-11-19 04:32 PST, Geoff Lankow (:darktrojan)
jst: review+
mounir: review+
Details | Diff | Review
patch, comments addressed (3.67 KB, patch)
2011-11-24 03:19 PST, Geoff Lankow (:darktrojan)
no flags Details | Diff | Review

Description Geoff Lankow (:darktrojan) 2011-11-19 04:32:22 PST
Created attachment 575651 [details] [diff] [review]
patch

Mounir, please check this test still does everything you expect it to.
Comment 1 Johnny Stenback (:jst, jst@mozilla.com) 2011-11-23 19:18:55 PST
Comment on attachment 575651 [details] [diff] [review]
patch

Looks good to me, but Mounir should have a look too since he wrote this test I think.
Comment 2 Mounir Lamouri (:mounir) 2011-11-24 03:02:51 PST
Comment on attachment 575651 [details] [diff] [review]
patch

Review of attachment 575651 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, the tests changes seem correct but I think we could improve a bit the design of MockFilePicker. Anyhow, thanks for writing this, it is going to save quite some time for the next tests! ;)

::: dom/tests/browser/browser_popup_blocker_save_open_panel.js
@@ +6,5 @@
>  
>  var gLoaded = false;
>  
> +var MockFilePicker = SpecialPowers.MockFilePicker;
> +MockFilePicker.reset();

Could you move .reset() to the place register() was called?

@@ +7,5 @@
>  var gLoaded = false;
>  
> +var MockFilePicker = SpecialPowers.MockFilePicker;
> +MockFilePicker.reset();
> +MockFilePicker.returnValue = MockFilePicker.returnCancel;

I would prefer that to be moved to the place |return returnCancel| was called.

@@ +38,5 @@
>      gBrowser.removeEventListener("DOMPopupBlocked", arguments.callee, true);
>      ok(true, "The popup has been blocked");
>      prefs.setBoolPref("dom.disable_open_during_load", gDisableLoadPref);
>  
> +    MockFilePicker.reset();

I really dislike that name. Actually I don't even know why that needs to be called here (except to prevent bugs if other tests forget to init the MockFilePicker).

IMO, we should rename reset() to init() and create a finish() methods that resets stuff and ideally unregister the mock object.
If you happen to open a bug about that, please CC me :)
Comment 3 Geoff Lankow (:darktrojan) 2011-11-24 03:19:31 PST
Created attachment 576712 [details] [diff] [review]
patch, comments addressed

I think I do need to make some changes to the MockFilePicker, but this'll do for now.
Comment 4 Geoff Lankow (:darktrojan) 2011-11-24 03:33:53 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/e381a382772c
Comment 5 Ed Morley [:emorley] 2011-11-24 08:15:18 PST
https://hg.mozilla.org/mozilla-central/rev/e381a382772c

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