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...
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla11
Assigned To: Geoff Lankow (:darktrojan)
: Andrew Overholt [:overholt]
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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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

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

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

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

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 User image 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 User image Geoff Lankow (:darktrojan) 2011-11-24 03:33:53 PST
Comment 5 User image Ed Morley [:emorley] 2011-11-24 08:15:18 PST

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