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

RESOLVED FIXED in mozilla11

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

Trunk
mozilla11
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 575651 [details] [diff] [review]
patch

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

Updated

6 years ago
Attachment #575651 - Flags: review?(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.
Attachment #575651 - Flags: review?(mounir)
Attachment #575651 - Flags: review?(jst)
Attachment #575651 - Flags: review+
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 :)
Attachment #575651 - Flags: review?(mounir) → review+
Flags: in-testsuite+
Version: unspecified → Trunk
(Assignee)

Comment 3

6 years ago
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.
Attachment #575651 - Attachment is obsolete: true
(Assignee)

Comment 4

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e381a382772c
Target Milestone: --- → mozilla11

Comment 5

6 years ago
https://hg.mozilla.org/mozilla-central/rev/e381a382772c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.