Brian R. Bondy [:bbondy] Assignee
	Description • 12 years ago

- 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

	Brian R. Bondy [:bbondy] Assignee
	Comment 1 • 12 years ago

Haven't pushed to try yet, but I will before I push to m-i after the review.

	neil@parkwaycc.co.uk
	Comment 2 • 12 years ago

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

	Brian R. Bondy [:bbondy] Assignee
	Comment 3 • 12 years ago

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?

	neil@parkwaycc.co.uk
	Comment 4 • 12 years ago

(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.

	Brian R. Bondy [:bbondy] Assignee
	Comment 5 • 12 years ago

How about I just allow nullptr in the base filepicker implementation?

	neil@parkwaycc.co.uk
	Comment 6 • 12 years ago

(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.

	Brian R. Bondy [:bbondy] Assignee
	Comment 7 • 12 years ago

How about this?

	neil@parkwaycc.co.uk
	Comment 8 • 12 years ago

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

	Brian R. Bondy [:bbondy] Assignee
	Comment 10 • 12 years ago

Which way would you prefer? I don't mind either way.

	neil@parkwaycc.co.uk
	Comment 11 • 12 years ago

(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.

	Brian R. Bondy [:bbondy] Assignee
	Comment 12 • 12 years ago

Same as before just fixed nit.

	Brian R. Bondy [:bbondy] Assignee
	Comment 13 • 12 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/ca7e9d87a4f1

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 12 years ago

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