703867 - Update /dom/tests/browser/browser_popup_blocker_save_open_panel.js to use MockFilePicker.jsm

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Geoff Lankow (:darktrojan)]

Attachment: patch

Mounir, please check this test still does everything you expect it to.

Comment 1

[Johnny Stenback (:jst)]

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)]

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)]

Attachment: 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)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e381a382772c

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e381a382772c