Closed Bug 785744 Opened 8 years ago Closed 8 years ago

Async file picker cleanup

Categories

(Firefox :: General, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 18

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

Attachments

(2 files, 2 obsolete files)

- Should ensure check for a valid argument in nsBaseFilePicker::Open(nsIFilePickerShownCallback *aCallback)
- mockfilepicker and xul filepicker should have the callback called outside of the try statement.
- Should use Components.interfaces.nsIFilePicker.returnCancel instead of this.returnCancel inside xul filepicker
Attached patch Patch v1. (obsolete) — Splinter Review
Haven't pushed to try yet, but I will before I push to m-i after the review.
Attachment #655454 - Flags: review?(neil)
Severity: major → normal
Target Milestone: --- → Firefox 17
Comment on attachment 655454 [details] [diff] [review]
Patch v1.

> NS_IMETHODIMP
> nsBaseFilePicker::Open(nsIFilePickerShownCallback *aCallback)
> {
>+  NS_ENSURE_ARG_POINTER(aCallback);
The GTK filepicker actually depends on being able to pass null to its open method :-( so I'm not sure what the right thing to do here is. (By comparison, the mock and xul filepickers don't null-check either, they just log an exception to the console when the picker is closed.)
Thanks for the quick feedback.

(In reply to neil@parkwaycc.co.uk from comment #2)
> Comment on attachment 655454 [details] [diff] [review]
> Patch v1.
> 
> > NS_IMETHODIMP
> > nsBaseFilePicker::Open(nsIFilePickerShownCallback *aCallback)
> > {
> >+  NS_ENSURE_ARG_POINTER(aCallback);
> The GTK filepicker actually depends on being able to pass null to its open
> method :-( so I'm not sure what the right thing to do here is. 

Oh that was fast, I didn't realize any extra implementations had already landed.
Couldn't we just fail the Gtk2 implementation in the same way and cleanup the code in Done?

> (By
> comparison, the mock and xul filepickers don't null-check either, they just
> log an exception to the console when the picker is closed.)

What's the right way to fail them via throw?
(In reply to Brian R. Bondy from comment #3)
> (In reply to comment #2)
> > (From update of attachment 655454 [details] [diff] [review])
> > > NS_IMETHODIMP
> > > nsBaseFilePicker::Open(nsIFilePickerShownCallback *aCallback)
> > > {
> > >+  NS_ENSURE_ARG_POINTER(aCallback);
> > The GTK filepicker actually depends on being able to pass null to its open
> > method :-( so I'm not sure what the right thing to do here is. 
> 
> Oh that was fast, I didn't realize any extra implementations had already
> landed.
> Couldn't we just fail the Gtk2 implementation in the same way and cleanup
> the code in Done?

Unfortunately their Show() deliberately passes nullptr to Open(). The only workaround is to make the filepicker implement its own callback. I have some code I could attach if you would like to see it.

> > (By comparison, the mock and xul filepickers don't null-check either,
> > they just log an exception to the console when the picker is closed.)
> 
> What's the right way to fail them via throw?

Well, you could throw new TypeError("foo is null") I guess.
How about I just allow nullptr in the base filepicker implementation?
(In reply to Brian R. Bondy from comment #5)
> How about I just allow nullptr in the base filepicker implementation?

Sure, as long as it doesn't crash.
Attached patch Patch v2 (obsolete) — Splinter Review
How about this?
Attachment #655454 - Attachment is obsolete: true
Attachment #655454 - Flags: review?(neil)
Attachment #655788 - Flags: review?(neil)
Comment on attachment 655788 [details] [diff] [review]
Patch v2

>+      if (mCallback) {
>+        mCallback->Done(nsIFilePicker::returnCancel);
>+      }
>       return NS_OK;
[Rather than duplicating code, could you not just set result to returnCancel?]
Attachment #655788 - Flags: review?(neil) → review+
Which way would you prefer? I don't mind either way.
(In reply to Brian R. Bondy from comment #10)
> Which way would you prefer? I don't mind either way.

I put the code in for posterity in case someone else wants it the other way.
Attached patch Patch v3.Splinter Review
Same as before just fixed nit.
Attachment #655788 - Attachment is obsolete: true
Attachment #656534 - Flags: review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/ca7e9d87a4f1
Target Milestone: Firefox 17 → Firefox 18
https://hg.mozilla.org/mozilla-central/rev/ca7e9d87a4f1
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.