Closed Bug 731307 Opened 12 years ago Closed 12 years ago

Add a new nsIFilePicker::ShowAsync method

Categories

(Core :: Widget, defect)

Product:
Core
Component:
Widget
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla17

People

(Reporter: mounir, Assigned: bbondy)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: completed-elm)

Attachments

(4 files, 16 obsolete files)

Patch v1. IDL
1.80 KB, patch
bbondy
: review+
mounir
: superreview+
Details | Diff | Splinter Review
Patch v7 - Widget filepicker code
3.61 KB, patch
bbondy
: review+
mounir
: superreview+
Details | Diff | Splinter Review
Patch v6 - MockFilePicker code
3.89 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
Patch v7 - Content changes
9.73 KB, patch
bbondy
: review+
Details | Diff | Splinter Review 
That should not cause any trouble for web content but it might cause some for privileged code calling directly the interface.
What does this mean?  On at least some operating systems (Windows) the file picker is modal and has to block the calling UI loop.
I can't speak about other OSes, but here's the situation on the Mac,
as I understand it:

Currently the native filepicker is an app-modal window.  It uses its
own dedicated (native) event loop, nested on top of any other
currently running event loops (native or Gecko).  We can (presumably)
count on the OS preventing any inappropriate native events running (or
being acted on) while the native filepicker is open.  And we rely on
Gecko to prevent any inappropriate Gecko events from running or being
acted on.  (This doesn't just happen automatically, because the
appshell (on all platforms) allows Gecko events	to be processed	while
a native event is being processed, and vice versa, to prevent event
starvation.)

If Mounir and I have our way, and we start using sheets for native
filepickers (as per bug 729720), things will change as follows:

The native filepicker will be window-modal, and no longer have its own
dedicated event loop.  The OS will still be responsible for filtering
native events (and selectively not acting on some of them).  Gecko
will also have to continue filtering events while the window-modal
sheet is open, but the filter will need to be a bit "leakier" -- for
example allowing other browser windows (besides the one blocked by the
window-modal sheet) to function normally.  And we'll need to add a
callback to tell Gecko when the filepicker has closed (which we don't
need if we're using an app-modal window with its own dedidated native
event loop).
> What does this mean?

See also bug 729720 comment #8.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #1)
> What does this mean?  On at least some operating systems (Windows) the file
> picker is modal and has to block the calling UI loop.

So apparently I was wrong about this.  The filepicker no longer blocks the window on Windows.  It definitely used to though ...
Judging by what you say in bug 729720 comment #8, it won't be possible to have more than one file picker open at a time even after nsIFilePicker::Show becomes asynchronous.  Do you agree, Mounir?

As best I can tell, the only way you could have more than one filepicker open at the same time would be if you make the filepicker return its results via a callback.
(Following up comment #2)

> If Mounir and I have our way, and we start using sheets for native
> filepickers (as per bug 729720), things will change as follows:
>
> The native filepicker will be window-modal, and no longer have its
> own dedicated event loop.  The OS will still be responsible for
> filtering native events (and selectively not acting on some of
> them).  Gecko will also have to continue filtering events while the
> window-modal sheet is open, but the filter will need to be a bit
> "leakier" -- for example allowing other browser windows (besides the
> one blocked by the window-modal sheet) to function normally.  And
> we'll need to add a callback to tell Gecko when the filepicker has
> closed (which we don't need if we're using an app-modal window with
> its own dedidated native event loop).

Now that I think more about it, this is the solution I'd prefer, but
isn't exactly what Mounir is aiming at.

Mounir doesn't want to add a callback to nsIFilePicker::Show, because
this would change a webfacing API, and therefore require web pages to
be rewritten.
(In reply to comment #4)

> So apparently I was wrong about this.  The filepicker no longer
> blocks the window on Windows.

As best I can tell this is wrong.  The filepicker you get using File :
Open File *does* block the window it opens above.  Even in today's
mozilla-central nightly.  I tested on Windows XP.
> Judging by what you say in bug 729720 comment #8, it won't be
> possible to have more than one file picker open at a time even after
> nsIFilePicker::Show becomes asynchronous.  Do you agree, Mounir?
>
> As best I can tell, the only way you could have more than one
> filepicker open at the same time would be if you make the filepicker
> return its results via a callback.

I'm quite sure this is correct for OS X.  But it seems to be wrong for
Windows.  In both FF 10.0.2 and today's mozilla-central nightly
(testing on Windows XP), it's possible to have two file pickers open
at the same time (each open over, and blocking, its own window).
> In both FF 10.0.2 and today's mozilla-central nightly (testing on
> Windows XP), it's possible to have two file pickers open at the same
> time (each open over, and blocking, its own window).

I'll bet this is because on Windows each window has its own event
loop.  That's not how things work on OS X.
(In reply to Steven Michaud from comment #7)
> (In reply to comment #4)
> 
> > So apparently I was wrong about this.  The filepicker no longer
> > blocks the window on Windows.
> 
> As best I can tell this is wrong.  The filepicker you get using File :
> Open File *does* block the window it opens above.  Even in today's
> mozilla-central nightly.  I tested on Windows XP.

It doesn't block the window on Windows 7.
Blocks: 671820
(In reply to Steven Michaud from comment #6)
> Mounir doesn't want to add a callback to nsIFilePicker::Show, because
> this would change a webfacing API, and therefore require web pages to
> be rewritten.

I'm not following this bit.  How would nsIFilePicker return the result async without a callback?

As I read bug 729720 comment 8, it seems to imply that a callback to fire the change event would be fine.
Indeed. I don't know what I was thinking... We would require a callback to get the selected files.
The filepicker on Windows drops into a Windows authored modal dispatch loop when we show the dialog. Our events keep going because this loop replaces our main dispatch loop. (We've had some problem with this in the past because we have unique dispatch behavior you don't see in the Windows dispatch loop.)

Are there any expectations on when this bug might be worked on / fixed? We could use this in our metro development since the file picker in metro is async only, so we've had to hack around the sync nature of nsIFilePicker.
It's my understanding that, as of Mounir's comment #12, this bug is no longer really distinct from bug 729720 (except insofar as bug 729720 is Mac-specific and this bug is cross-platform).

I have more work completed on bug 729720 than I've posted, and I really want to get back to it.  But I keep getting dragged through the wringer by other things, and I'm the only full-time Mac guy.  So it's hard to say when I'll be able to get back to bug 729720.

If you think it's worth it, though, I could bring my current work up to date and post it.  Which shouldn't take more than a day or two.
(In reply to Steven Michaud from comment #14)
> If you think it's worth it, though, I could bring my current work up to date
> and post it.  Which shouldn't take more than a day or two.

Converting to sheets on osx doesn't seem to help us at all on metro unless I'm missing something.

Maybe we could add an async Show to the existing file picker for starters without removing the current sync version. For metro work modifying the front end to use this instead should be pretty trivial. The only remaining issue for us would be content.

In bug 729720 Mounir mentioned this -

> I could work on that. It would be easy to change the webfacing filepicker to
> make it async but all internal uses will have to be audited and some might
> required some fixes. 

Sounds like fixing content callers would be "easy" too, although I have no idea how to go about that myself and selectively invoking it based on the platform.

Then over time we can migrate over to the async api in desktop.
> Converting to sheets on osx doesn't seem to help us at all on metro
> unless I'm missing something.

It's not just converting to sheets (which are window-modal).  I'll
also be adding an asynchronous variant of nsIFilePicker::Show() -- one
with a callback.
(In reply to Steven Michaud from comment #16)
> > Converting to sheets on osx doesn't seem to help us at all on metro
> > unless I'm missing something.
> 
> It's not just converting to sheets (which are window-modal).  I'll
> also be adding an asynchronous variant of nsIFilePicker::Show() -- one
> with a callback.

Ah, didn't see that in your patch. We could help out with Windows on this. Open question remains how to get content using the async variant.
I'll try to find time this week or next to post my current work at bug 729720.
(In reply to Jim Mathies [:jimm] from comment #15)
> In bug 729720 Mounir mentioned this -
> 
> > I could work on that. It would be easy to change the webfacing filepicker to
> > make it async but all internal uses will have to be audited and some might
> > required some fixes. 
> 
> Sounds like fixing content callers would be "easy" too, although I have no
> idea how to go about that myself and selectively invoking it based on the
> platform.
> 
> Then over time we can migrate over to the async api in desktop.

Migrating to the asyc API will not break content because Chrome already have an async filepicker. I don't know if that's the same in Safari and Opera but Chrome has enough market share nowadays to believe that this change will not break anything.
So, as soon as we have an async API, we could just update AsyncClickHandler::Run() in nsHTMLInputElement.cpp to use it and update the element's value when the callback is run.
Blocks: 771238
Have you had any time to look into this again Steven? If not maybe you could post your work in progress patch and I can try to pick it up.
(In reply to comment #20)

I've been on vacation for the last week.  Sorry I haven't yet posted my work-in-progress.  I should be able to do that in the next few days.

First, though, I need to figure out why current trunk won't build on OS X 10.6 :-(
> First, though, I need to figure out why current trunk won't build on OS X 10.6 :-(

See bug 733905 comment #45.
I've posted my latest work-in-progress at bug 729720.

Unfortunately I've so far only added an asynchronous option to nsIPrintDialogService::Show() -- not to nsIFilePicker::Show().  But I hope my work will still be useful, at least as an example of how we might fix this bug.
Did you still plan on working on this bug, or did you want someone else to jump in getting ideas from your patch in bug 729720?
Go ahead and take this bug, Brian, if you want it :-)

It's unlikely I'll be able to do any more on it myself anytime soon.

I do strongly suggest, though, that you don't just "make nsIFilePicker::Show async".  That's likely to create compatibility problems in current web content.  I think it'd be a lot better to add an async option to nsIFilePicker::Show, and then only use it "internally" (like I did with nsIPrintDialogService::Show).

But I still don't know exactly how to do this :-(
Assignee: nobody → netzen
Attached patch WIP (obsolete) — — Splinter Review 
ETA for patch v1 tomorrow end of day.
I'm just going to return not implemented from ShowAsync by the way on all platforms except WinRT.  We can file follow up bugs for platform specific implementations where having this support is beneficial.  

From our internal code I'll try calling ShowAsync first. When that fails it'll call Show().  For the WinRT stuff I'll return not implemented if some future addon user calls Show() since there is no way to get that working.
(In reply to Steven Michaud from comment #25)
> I do strongly suggest, though, that you don't just "make nsIFilePicker::Show
> async".  That's likely to create compatibility problems in current web
> content.  I think it'd be a lot better to add an async option to
> nsIFilePicker::Show, and then only use it "internally" (like I did with
> nsIPrintDialogService::Show).

As already said, web content (ie nsHTMLInputElement.cpp) could use an async method because Webkit as already doing that. If that was web incompatible, we would know about it.
The only issue I might see with that is internal calls like Firefox UI calling .show() and assuming it's sync. But that's something we are able to fix ourselves.
> As already said, web content (ie nsHTMLInputElement.cpp) could use an async
> method because Webkit as already doing that. If that was web incompatible,
> we would know about it.

I'm already doing that, but ya I made a second method and leaving show() sync. I made a showAsync() because that way addons with filepickers won't be broken.

> The only issue I might see with that is internal calls like Firefox UI
> calling .show() and assuming it's sync. But that's something we are able to
> fix ourselves.

I'm updating all of those in my patch.
> > The only issue I might see with that is internal calls like Firefox UI
> > calling .show() and assuming it's sync. But that's something we are able to
> > fix ourselves.

> I'm updating all of those in my patch.

After doing some searches there are about 60 js only internal uses of this, and a few C++ ones, there's no benefit as far as I need an async picker for WinRT to update them all.  I'm afraid of introducing regressions in an area that I won't be able to test easily.  So I'll just update the one that is important that I do need nsHTMLInputElement.cpp.
Attached patch Patch v1. (obsolete) — — Splinter Review 

        Attachment #649942 -
        Attachment is obsolete: true

    
    

    
  
http://hg.mozilla.org/projects/elm/rev/e89b3e5d9f0e
Will sync up review comments as I make them.
Whiteboard: completed-elm

    
  

        Attachment #650132 -
        Attachment description: WIP2 → Patch v1.

    
    

    
  
Comment on attachment 650132 [details] [diff] [review]
Patch v1.

Attached is the base code for adding a showAsync method to nsIFilePicker.
The only place we need this for Metro work is in nsHTMLInputElement.cpp.  Our front end code is in /browser/metro and it is distinct from the /browser front end code.  So anytime we need to use a filepicker we will use showAsync. 

This is a big blocker for us for Metro because there is no sync API available.  We can't use synchronization either to make the async call sync because the call needs to be made from the main thread in Metro and dispatches back to the main thread once a user picks a file.  Please see bug 771238 to see why this is needed.

If you're ok with this patch I'll follow up another bug to do the rest of the original intent of this bug.  I don't see any harm in taking this approach and landing it on m-c for now and it'll unblock us for our Metro needs.

        Attachment #650132 -
        Flags: review?(mounir)

    
    

    
  
Comment on attachment 650132 [details] [diff] [review]
Patch v1.

Review of attachment 650132 [details] [diff] [review]:
-----------------------------------------------------------------

I quite don't like this approach. There is no point in keeping a sync and an async method if some widgets will only provide one or the other and if for some widgets, using one will creates bug we can't fix.
Given the current status of various widgets, it seems to be reasonable to move all callers to a async method because anyway Windows Metro UI will not allow sync calls, Cocoa would still have various bugs when using async calls.

I understand you guys have short-term goals to make the Metro version of Firefox works but I don't believe we can go with that solution.
What I would suggest is the following:
- Audit all widgets to see if we can move to an async method;
- Change Show() to be async and takes a callback;
- If the widget allow async calls, we should use the async API;
- If the widget doesn't allow async calls (hopefully, that will not be the case), we should use the sync API, block and then call the callback so it will be more or less transparent for the consumer of the filepicker API.

The advantage of that method is that it will work long term, fix bugs and allow the Metro UI to work.
The disadvantage is that extensions will break, indeed. But anyway, extensions using .show() will break with the Metro UI because it will not be implemented...

        Attachment #650132 -
        Flags: review?(mounir) → review-

    
    

    
  
(In reply to Mounir Lamouri (:mounir) from comment #34)
> Comment on attachment 650132 [details] [diff] [review]
> Patch v1.
> 
> Review of attachment 650132 [details] [diff] [review]:

I'm afraid of breaking all extensions as you mentioned, and also of introducing regressions in the 60+ cases we use it in code that doesn't affect Metro.

We also have a pretty aggressive timeline for Metro support and I shouldn't be spending my time making implementations for each platform when there is no negative side effects with the current approach.

> -----------------------------------------------------------------
> I quite don't like this approach. There is no point in keeping a sync and an
> async method if some widgets will only provide one or the other and if for
> some widgets, using one will creates bug we can't fix.

Well for the method that would provide a bug in the widget implementation like the currnet sync show with WinRT, we would return not implemented.

We could document that on some platforms only provide an async show method.

> Given the current status of various widgets, it seems to be reasonable to
> move all callers to a async method because anyway Windows Metro UI will not
> allow sync calls, Cocoa would still have various bugs when using async calls.

What bugs would Cocoa have? Again my time should be mostly spent towards Metro and if I did get approval to make widget implementations for each platform, it sounds like you know of a lot more long tail work here.

> I understand you guys have short-term goals to make the Metro version of
> Firefox works but I don't believe we can go with that solution.

Could you detail any negative impacts this would provide? We don't have addons in Metro and the patch has a no-oop effect on every other platform.  The only problem I can see is that it makes the documentation a little messy because you have to explain why there's 2 methods. And you have to explain that some platforms only offer async variants. 

> What I would suggest is the following:
> - Audit all widgets to see if we can move to an async method;
> - Change Show() to be async and takes a callback;
> - If the widget allow async calls, we should use the async API;
> - If the widget doesn't allow async calls (hopefully, that will not be the
> case), we should use the sync API, block and then call the callback so it
> will be more or less transparent for the consumer of the filepicker API.
> 
> The advantage of that method is that it will work long term, fix bugs and
> allow the Metro UI to work.
> The disadvantage is that extensions will break, indeed. But anyway,
> extensions using .show() will break with the Metro UI because it will not be
> implemented...

My patch provides the exact same advantages you listed, but not the disadvantage.  In addition the approach of updating all code will probably cost several extra days of work at least, and possibly introduce regressions that we don't find.

    
    

    
  
We can't break extensions in desktop firefox. What we need to do is provide both, depreciate 'show' and give developers time to switch over.

Migrating firefox desktop / toolkit over to the async version should be filed in a separate bug so regressions can be tracked. This bug should be about getting the async method implemented.

As far as extensions in metrofx go, we don't currently support them, so that's not a concern.

    
    

    
  
If this bug only pertains to the Windows impl I'd suggest filing an overarching tracking bug on implementing async show with dependent bugs for linux, mac, and this bug. The migration bug would then depend on all of the platform bugs.

    
    

    
  
(In reply to Jim Mathies [:jimm] from comment #36)
> We can't break extensions in desktop firefox. What we need to do is provide
> both, depreciate 'show' and give developers time to switch over.

Having from now on Show() returning NOT_IMPLEMENTED, in which case you have to call ShowAsync() is breaking extensions as much as changing/removing Show(). Actually, the two methods way is worse because at some point we might want to only have an async method and in that case, we will break extensions again.
As soon as some platforms don't allow sync calls, we should just switch everything to async. Most platforms actually recommend/use sync calls: Android and MacOS X for the one I know. And faking async is easier and less bug-prone than faking sync.

> As far as extensions in metrofx go, we don't currently support them, so
> that's not a concern.

OOC, why?

Maybe we could find a solution that would not change the API or only change it for Metro?
For example, we could have a nsIFilePickerMetro.idl that will be a replica of nsIFilePicker.idl but with Show() getting a callback. Then nsHTMLInputElement.cpp will have a special path for the Metro UI that would use the callback. Maybe we can avaid duplicating the idl if we can use #ifdef (inside %{C++ blocks).
I think that solution would prevent us from having to take too fast API decisions based on short needs.

BTW, that bug being about making Show() async, we should probably open another bug for the Metro needs, as Jim pointed in comment 37.

    
    

    
  
(In reply to Mounir Lamouri (:mounir) from comment #38)
> As soon as some platforms don't allow sync calls [...]

You had to read *async* instead of sync here...

    
    

    
  
> Having from now on Show() returning NOT_IMPLEMENTED, in which case you have
> to call ShowAsync() is breaking extensions as much as changing/removing
> Show(). 

I don't think anyone is suggesting that.  What's being suggested is to keep the implementation of Show() as is for all platforms but to mark it in documentation as obsolete.   The only platform Show() would return not implemented on would be WinRT because ther is no possibility to build a sync filepicker.

> Actually, the two methods way is worse because at some point we
> might want to only have an async method and in that case, we will break
> extensions again.

I don't see why it's worse to give people notice over giving them no notice of an obsolete API.  

> As soon as some platforms don't allow sync calls, we should just switch
> everything to async. Most platforms actually recommend/use sync calls:
> Android and MacOS X for the one I know. And faking async is easier and less
> bug-prone than faking sync.

We agree with this argument, but we just think it should be in separate bugs.  And also when there is no harm to make steps towards this direction, and it gives us massive gains on a project due this quarter, I think it should be allowed to land on m-c. 

> > As far as extensions in metrofx go, we don't currently support them, so
> > that's not a concern.
> 
> OOC, why?

New addons will be supported when it's ready, but legacy ones will not.  Asa, rstrong, or bsmedberg are probably the best people to ping for more reasoning.


> Maybe we could find a solution that would not change the API or only change
> it for Metro?
> For example, we could have a nsIFilePickerMetro.idl that will be a replica
> of nsIFilePicker.idl but with Show() getting a callback. 
> nsHTMLInputElement.cpp will have a special path for the Metro UI that would
> use the callback. Maybe we can avaid duplicating the idl if we can use
> #ifdef (inside %{C++ blocks).

We could just add a single method in the new interface and query for existence in nsHTMLInputElement.cpp, no need to duplicate everything.  But this is basically the exact same approach my patch does with the exception that we are naming it metro only and excluding other platforms from participating ever. 

> I think that solution would prevent us from having to take too fast API
> decisions based on short needs.
> BTW, that bug being about making Show() async, we should probably open
> another bug for the Metro needs, as Jim pointed in comment 37.

If the special interface way is the way you want to go, I'll file another bug with a slightly modified patch.  Please advise. Thanks

    
    

    
  
How about adding ShowAsync with a default implementation that calls sync Show() and then immediately fires the callback? Then ShowAsync works on all platforms, and we can immediately start encouraging people to use it/convert code to it, and ensure that all new code uses it.

We definitely have to have an interregnum where both are supported --- possibly a very long one. There's nothing unusual or wrong about that.

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #41)
> How about adding ShowAsync with a default implementation that calls sync
> Show() and then immediately fires the callback? Then ShowAsync works on all
> platforms, and we can immediately start encouraging people to use it/convert
> code to it, and ensure that all new code uses it.
> 
> We definitely have to have an interregnum where both are supported ---
> possibly a very long one. There's nothing unusual or wrong about that.

I'm fine with that. 

Some users of the new function may expect ShowAsync to return right away though, but since it's a new function I think we can just document that some widget implementations may actually perform the operation synchronous.  I guess the only down side of this, is that while the user is selecting a file, the code the author wants to eagerly execute, will not until a file is picked.  I like that we can update the documentation right away though.

Are you OK with this approach Mounir? 

If so, I'll:
1) File a new bug to change the internal calls from show to showAsync (other than nsHTMLInputElement.cpp)
2) File a new bug for each widget implementation we want to support for actually doing an async operation.  (What are they?)
3) Use this bug for a patch similar to the current patch, but which has a default implementation as roc suggested in Comment 41, instead of the current default implementation of returning not implemented.  Also I'd update the patch so that nsHTMLInputElement.cpp doesn't fallback to the sync show() call anymore since it wouldn't be needed.

    
    

    
  
(In reply to Brian R. Bondy [:bbondy] from comment #42)
> 2) File a new bug for each widget implementation we want to support for
> actually doing an async operation.  (What are they?)

Bug 671820 can be used for the GTK implementation, and I'd be keen to fix that.
It was in Widget:Gtk at one stage.  With this bug covering the XP interface and input element parts, bug 671820 can revert to Widget:Gtk.

    
    

    
  
(In reply to Brian R. Bondy [:bbondy] from comment #42)
> Some users of the new function may expect ShowAsync to return right away
> though, but since it's a new function I think we can just document that some
> widget implementations may actually perform the operation synchronous.  I
> guess the only down side of this, is that while the user is selecting a
> file, the code the author wants to eagerly execute, will not until a file is
> picked.  I like that we can update the documentation right away though.

You could make it a bit more asynchronous by posting an XPCOM event which actually does the synchronous window open. Then callers won't be able to rely on the filepicker and callback happening right away. This would also be good because it would reduce the possibility of the filepicker being open while JS is on the stack.

    
    

    
  
sounds good, thanks for the suggestion.

    
    

    
  
Latest work on elm:
http://hg.mozilla.org/projects/elm/rev/7779d6482d74

Will upload patches shortly.

    
    

    
  


  
Mounir, I didn't get a response back yet in Comment 42, so I just went ahead with roc's suggestion since it seems reasonable from both of our concerns.

content/html/test/test_input_file_picker.html doesn't need to be updated since showAsync is used automatically in tests from the click events due to the mockfilepicker update and nsHTMLInputElement change.  I actually originally forgot to update mockfilepicker and tests were failing everywhere.

        Attachment #650132 -
        Attachment is obsolete: true

        Attachment #651026 -
        Flags: review?(roc)

    
    

    
  


  

        Attachment #651028 -
        Flags: review?(mounir)

    
  
Depends on: 781973

    
  
Blocks: 781973
No longer depends on: 781973

    
    

    
  
Comment on attachment 651026 [details] [diff] [review]
Patch v2 - Widget filepicker code.

Review of attachment 651026 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/MockFilePicker.jsm
@@ +135,5 @@
>      return MockFilePicker.returnValue;
> +  },
> +  showAsync: function(aFilePickerShownCallback) {
> +    let result = this.show();
> +    aFilePickerShownCallback.shown(result);

You should probably call aFilePickerShownCallback asynchronously like you do in nsBaseFilePicker.

::: widget/nsIFilePicker.idl
@@ +161,5 @@
> +
> +
> + /**
> +  * Show File Dialog asynchrounously.
> +  * The passed in object's shown method will be called upon completion.

'done' method, not 'shown'

        Attachment #651026 -
        Flags: review?(roc) → review+

    
    

    
  
Thanks for the quick review. 

> You should probably call aFilePickerShownCallback asynchronously like you do in 
> nsBaseFilePicker.

Just via setTimeout is fine allowing the closure to capture the local variables right?

    
    

    
  


  
Fixed nits. Carrying forward r+.

        Attachment #651026 -
        Attachment is obsolete: true

        Attachment #651088 -
        Flags: review+

    
    

    
  


  
Same as previous R+ed patch but I took out the MockFilePicker.jsm stuff so it can go in its own patch to be reviewed.

        Attachment #651088 -
        Attachment is obsolete: true

        Attachment #651140 -
        Flags: review+

    
    

    
  


  
Added matching code to the showAsync method so it dispatches to the main thread in the same way as the base file picker.  Also I call returnNotShown in the same way as the base file picker.

        Attachment #651141 -
        Flags: review?(roc)

    
    

    
  


  
Fix for a failing test and slight change to MockFilePicker.jsm.
The test simply tries to make sure clicks open the filepicker when they should. 

The test uses an internal shown boolean variable which only exists on the MockFilePicker, but since that is async now it was intermittently failing.  

I added a showing variable which gets set right away for showAsync calls.

        Attachment #651141 -
        Attachment is obsolete: true

        Attachment #651141 -
        Flags: review?(roc)

        Attachment #651146 -
        Flags: review?(roc)

    
  
Keywords: dev-doc-needed
Summary: Make nsIFilePicker::Show async → Add a new nsIFilePicker::ShowAsync method

    
    

    
  


  
Added missing return NS_OK after NS_ENSURE_SUCCESS, caused build problems on try run for non-Windows platforms.  Carrying forward r+.

        Attachment #651140 -
        Attachment is obsolete: true

        Attachment #651162 -
        Flags: review+

    
    

    
  
Comment on attachment 651028 [details] [diff] [review]
Patch v2 - Use async show method from nsHTMLInputElement

Review of attachment 651028 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +238,5 @@
> +                            nsHTMLInputElement* aInput,
> +                            nsIFilePicker* aFilePicker) :
> +    mMulti(aMulti),
> +    mInput(aInput),
> +    mFilePicker(aFilePicker)

I re-ordered these locally to match the declaration order.
Also I added a virtual destructor locally.

    
    

    
  
Comment on attachment 651162 [details] [diff] [review]
Patch v4 - Widget filepicker code

Review of attachment 651162 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/nsIFilePicker.idl
@@ +163,5 @@
> + /**
> +  * Show File Dialog asynchrounously.
> +  * The passed in object's done method will be called upon completion.
> +  */
> +  void showAsync(in nsIFilePickerShownCallback aFilePickerShownCallback);

What about calling that method Open() so when we remove Show() we can use Open() and not have to worry about keeping a ShowAsync() method without a ShowSync() equivalent.

BTW, could you mark show() as deprecated?

::: widget/xpwidgets/nsBaseFilePicker.cpp
@@ +38,5 @@
> +public:
> +  AsyncShowFilePicker(nsIFilePicker *aFilePicker,
> +                      nsIFilePickerShownCallback *aCallback) :
> +    mFilePicker(aFilePicker),
> +    mCallback(aCallback)

nit: usually coding style is:
Ctor()
  : foo
  , bar
{
}

@@ +42,5 @@
> +    mCallback(aCallback)
> +  {
> +  }
> +
> +  NS_IMETHOD Run() 

nit: trailing whitespace

@@ +55,5 @@
> +    nsresult rv = mFilePicker->Show(&result);
> +    if (NS_SUCCEEDED(rv)) {
> +      mCallback->Done(result);
> +    } else {
> +      mCallback->Done(nsIFilePicker::returnNotShown);

That seems wrong. I think you should do:
NS_ENSURE_SUCCESS(rv, rv); If |Show()| fails, you should make ShowAsync() to fail too.

@@ +97,5 @@
> +{
> +  nsCOMPtr<nsIRunnable> filePickerEvent = new AsyncShowFilePicker(this, aCallback);
> +  nsresult rv = NS_DispatchToMainThread(filePickerEvent);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  return NS_OK;

Remove NS_ENSURE_SUCCESS() and do |return rv;|.

        Attachment #651162 -
        Flags: review-

    
    

    
  
(In reply to Mounir Lamouri (:mounir) from comment #57)
> Comment on attachment 651162 [details] [diff] [review]
> Patch v4 - Widget filepicker code
> 
> Review of attachment 651162 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/nsIFilePicker.idl
> @@ +163,5 @@
> > + /**
> > +  * Show File Dialog asynchrounously.
> > +  * The passed in object's done method will be called upon completion.
> > +  */
> > +  void showAsync(in nsIFilePickerShownCallback aFilePickerShownCallback);
> 
> What about calling that method Open() so when we remove Show() we can use
> Open() and not have to worry about keeping a ShowAsync() method without a
> ShowSync() equivalent.

You weren't actually flagged for review on this patch, but thanks for the feedback in any case. 
I personally think it's confusing having both a show() and an open() for the long overlap period where we will offer both API. 
If roc would like me to change it to open() though, then I will. 

> BTW, could you mark show() as deprecated?

I don't believe we mark things using the XPIDL attribute anymore.
https://developer.mozilla.org/en-US/docs/XPIDL

"deprecated
This interface should no longer be used. The compiler will emit warnings if you attempt to use this."

This is why I didn't.

> 
> ::: widget/xpwidgets/nsBaseFilePicker.cpp
> @@ +38,5 @@
> > +public:
> > +  AsyncShowFilePicker(nsIFilePicker *aFilePicker,
> > +                      nsIFilePickerShownCallback *aCallback) :
> > +    mFilePicker(aFilePicker),
> > +    mCallback(aCallback)
> 
> nit: usually coding style is:
> Ctor()
>   : foo
>   , bar
> {
> }

I can update that if you want but I think it's more common in /widget to have it the way I did it.  
See GfxDriverInfo and inside the nsBaseFIlePicker.cpp itself for the class nsBaseFilePicker. 
 
> @@ +42,5 @@
> > +    mCallback(aCallback)
> > +  {
> > +  }
> > +
> > +  NS_IMETHOD Run() 
> 
> nit: trailing whitespace

I can remove the space at the end of this line before I push the patch.


> @@ +55,5 @@
> > +    nsresult rv = mFilePicker->Show(&result);
> > +    if (NS_SUCCEEDED(rv)) {
> > +      mCallback->Done(result);
> > +    } else {
> > +      mCallback->Done(nsIFilePicker::returnNotShown);
> 
> That seems wrong. I think you should do:
> NS_ENSURE_SUCCESS(rv, rv); If |Show()| fails, you should make ShowAsync() to
> fail too.

If I did that, then the caller would not get a result.
Who would you be returning the value to?

> @@ +97,5 @@
> > +{
> > +  nsCOMPtr<nsIRunnable> filePickerEvent = new AsyncShowFilePicker(this, aCallback);
> > +  nsresult rv = NS_DispatchToMainThread(filePickerEvent);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +  return NS_OK;
> 
> Remove NS_ENSURE_SUCCESS() and do |return rv;|.

I do that on purpose so that if there is a failure then it will be logged as a warning.
I do it instead of:
if (NS_FAILED(rv)) {
  NS_WARNING(...)
  return rv;
}

    
    

    
  
Comment on attachment 651162 [details] [diff] [review]
Patch v4 - Widget filepicker code

Clearing your r- on this patch since this is a widget patch for review and you weren't marked for r?.  I did add a feedback? onto it though so roc could comment on our disagreement on the widget review comments.

        Attachment #651162 -
        Flags: review- → feedback?(roc)

    
    

    
  
Comment on attachment 651028 [details] [diff] [review]
Patch v2 - Use async show method from nsHTMLInputElement

Review of attachment 651028 [details] [diff] [review]:
-----------------------------------------------------------------

Could you declare the callback class in nsHTMLInputElement (private) and implement it in in the cpp?

Not that much to change but I would like to see another version of this patch.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +238,5 @@
> +                            nsHTMLInputElement* aInput,
> +                            nsIFilePicker* aFilePicker) :
> +    mMulti(aMulti),
> +    mInput(aInput),
> +    mFilePicker(aFilePicker)

nit: coding style usually is:
nsClass()
  : mFoo
  , mBar
{
}

@@ +253,5 @@
> +      return NS_OK;
> +    }
> +
> +    // Collect new selected filenames
> +    nsCOMPtr<nsIDocument> doc = mInput->OwnerDoc();

Just call |mInput->OwnerDoc()| in the two places you are using |doc|.

@@ +266,5 @@
> +      bool loop = true;
> +      while (NS_SUCCEEDED(iter->HasMoreElements(&loop)) && loop) {
> +        iter->GetNext(getter_AddRefs(tmp));
> +        nsCOMPtr<nsIFile> localFile = do_QueryInterface(tmp);
> +        if (localFile) {

if (!localFile) {
  continue;
}

@@ +286,5 @@
> +    }
> +    else {
> +      nsCOMPtr<nsIFile> localFile;
> +      nsresult rv = mFilePicker->GetFile(getter_AddRefs(localFile));
> +      if (localFile) {

if (!localFile) {
  continue;
}

@@ +301,5 @@
> +      }
> +    }
> +
> +    // Set new selected files
> +    if (newFiles.Count()) {

if (!newFiles.Count()) {
  return NS_OK;
}

@@ +306,5 @@
> +      // The text control frame (if there is one) isn't going to send a change
> +      // event because it will think this is done by a script.
> +      // So, we can safely send one by ourself.
> +      mInput->SetFiles(newFiles, true);
> +      nsContentUtils::DispatchTrustedEvent(mInput->OwnerDoc(),

return nsContentUtils::DispatchTrustedEvent(...);

@@ +422,5 @@
>    }
>  
> +  nsCOMPtr<nsFilePickerShownCallback> callback =
> +    new nsFilePickerShownCallback(multi, mInput, filePicker);
> +  rv = filePicker->ShowAsync(callback);

return filePicker->ShowAsync(callback);

        Attachment #651028 -
        Flags: review?(mounir) → review-

    
    

    
  
Regarding the review on the /content patch, thanks, I'll implement those changes.

    
    

    
  
(In reply to Brian R. Bondy [:bbondy] from comment #58)
> You weren't actually flagged for review on this patch, but thanks for the
> feedback in any case.

Not being flagged for review doesn't mean I'm not allowed to review.

> I personally think it's confusing having both a show() and an open() for the
> long overlap period where we will offer both API. 
> If roc would like me to change it to open() though, then I will. 

IMO, having show() and open() when show() is marked as deprecated and open() as the async version of show() doesn't seem worse than having showAsync() withouth a showSync() counter-part. Given that we agreed that show() should be deprecated and removed at some point, I think we should think about long-term.

> > BTW, could you mark show() as deprecated?
> 
> I don't believe we mark things using the XPIDL attribute anymore.
> https://developer.mozilla.org/en-US/docs/XPIDL
> 
> "deprecated
> This interface should no longer be used. The compiler will emit warnings if
> you attempt to use this."
> 
> This is why I didn't.

See: https://mxr.mozilla.org/mozilla-central/search?string=deprecated&find=\.idl&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
I believe what this documentation was trying to say is that using a deprecated method will produce a warning.

> > @@ +55,5 @@
> > > +    nsresult rv = mFilePicker->Show(&result);
> > > +    if (NS_SUCCEEDED(rv)) {
> > > +      mCallback->Done(result);
> > > +    } else {
> > > +      mCallback->Done(nsIFilePicker::returnNotShown);
> > 
> > That seems wrong. I think you should do:
> > NS_ENSURE_SUCCESS(rv, rv); If |Show()| fails, you should make ShowAsync() to
> > fail too.
> 
> If I did that, then the caller would not get a result.
> Who would you be returning the value to?

Callers will get the return:
- C++ callers by reading the returned nsresult, which they should;
- Javascript callers: it will simply throw an exception.

In both cases, return NS_ERROR_FAILURE would implicitly means that the callback isn't taken into account.

> > @@ +97,5 @@
> > > +{
> > > +  nsCOMPtr<nsIRunnable> filePickerEvent = new AsyncShowFilePicker(this, aCallback);
> > > +  nsresult rv = NS_DispatchToMainThread(filePickerEvent);
> > > +  NS_ENSURE_SUCCESS(rv, rv);
> > > +  return NS_OK;
> > 
> > Remove NS_ENSURE_SUCCESS() and do |return rv;|.
> 
> I do that on purpose so that if there is a failure then it will be logged as
> a warning.
> I do it instead of:
> if (NS_FAILED(rv)) {
>   NS_WARNING(...)
>   return rv;
> }

I don't really see the point of doing that given that anyway, callers of the method will do:
nsresult rv = filePicker->Show(&result);
NS_ENSURE_SUCCESS(rv, rv);

IOW, chances that nothing in the calling chain will warn because of the failure is very low. Usually, the top of the chain will. No need to make that code less readable, IMO.

    
    

    
  
> - C++ callers by reading the returned nsresult, which they should;

I don't think so, the showAsync function would have already returned.  The dispatched xpcom event is run after that.

In any case, please wait for roc to comment on the widget patch.  I will implement whatever he suggests since he is the /widget owner.  I mean no disrespect but you are not an owner for widget, nor a peer.  If your suggestions do prompt roc to ask for changes though, then I'll gladly implement them.  In its current state, he marked it as an r+ and your r- does not override that.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v3 - Content changes. (obsolete)
        — 
          — Splinter Review
      

    


  
Implemented review comments.

        Attachment #651028 -
        Attachment is obsolete: true

        Attachment #651371 -
        Flags: review?(mounir)

    
    

    
  


  
Updated patch for the extra comments which I agree with.  Carrying forward roc's previous r+.  Re-adding flag for feedback? for recent mounir /widget objections.

Things changed:
- Added the deprecated IDL attribute before the show() method.
- Removed the trailing space character

        Attachment #651162 -
        Attachment is obsolete: true

        Attachment #651162 -
        Flags: feedback?(roc)

        Attachment #651375 -
        Flags: review+

        Attachment #651375 -
        Flags: feedback?(roc)

    
  

        Attachment #651371 -
        Attachment description: Patch v3. → Patch v3 - Content changes.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v4 - Content changes. (obsolete)
        — 
          — Splinter Review
      

    


  
Regarding the /content patch:

Forgot that you also asked for nsFilePickerShownCallback to be a declared private under nsHTMLInputElement.
So I updated the patch to do that.

I also moved AsyncClickHandler there since AsyncClickHandler::Run creates an instance of nsFilePickerShownCallback, and otherwise I'd have to use friend class AsyncClickHandler; inside nsHTMLInputElement.

For this comment: 

> > +    }
> > +    else {
> > +      nsCOMPtr<nsIFile> localFile;
> > +      nsresult rv = mFilePicker->GetFile(getter_AddRefs(localFile));
> > +      if (localFile) {
> 
> if (!localFile) {
>   continue;
> }

That's actually outside of the loop, so I did no action for that review comment.

        Attachment #651371 -
        Attachment is obsolete: true

        Attachment #651371 -
        Flags: review?(mounir)

        Attachment #651530 -
        Flags: review?(mounir)

    
    

    
  
Comment on attachment 651375 [details] [diff] [review]
Patch v5 - Widget filepicker code

Review of attachment 651375 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think open() vs showAsync() matters much so let's make Mounir happy and use open() :-).

I think calling NS_ENSURE_SUCCESS(rv,rv) in Run() is a good idea. The result returned by Run() doesn't really matter but we do want to report the error and that's an easy way to do it.

I think "return NS_DispatchToMainThread(filePickerEvent);" in Open() is a good idea too.

    
    

    
  
Thanks, the last disagreement was this:

@@ +55,5 @@
> +    nsresult rv = mFilePicker->Show(&result);
> +    if (NS_SUCCEEDED(rv)) {
> +      mCallback->Done(result);
> +    } else {
> +      mCallback->Done(nsIFilePicker::returnNotShown);

That seems wrong. I think you should do:
NS_ENSURE_SUCCESS(rv, rv); If |Show()| fails, you should make ShowAsync() to fail too.

---

I explained that this code is run in the dispatched XPCOM event after showAsync, but my point didn't get across.

I actually don't need clarification here, I know that it is correct. I'm just calling it out since it's the last uncommented on thing.

    
    

    
  
By the way Mounir, there are some companies who offer APIs that have a suffix of Async. For example see Microsoft's WinRT API: http://msdn.microsoft.com/library/windows/apps/BR207847

The API is called PickSingleFileAsync and there is no PickSingleFileSync. The world didn't end. 

I really don't care about the name, but I'll be spending a couple hours tonight uploading 3 new patches here, updating elm with those patches, and changing the WinRT elm implementation as well.  The unnecessary change makes it hard to get things done.

It's wasting a couple hours of dev time that could be spent on better priorities than a personal preference in an internal API name that doesn't matter.

I also wanted to mention that I absolutely welcome extra review comments on my patches, but it was rude to add an r- when the patch was already r+ed by the module owner.  Especially for nits that were mostly a personal preference.

    
    

    
  


  

        Attachment #651146 -
        Attachment is obsolete: true

        Attachment #651585 -
        Flags: review+

    
    

    
  


  

        Attachment #651375 -
        Attachment is obsolete: true

        Attachment #651375 -
        Flags: feedback?(roc)

        Attachment #651587 -
        Flags: review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v5 - Content changes (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #651530 -
        Attachment is obsolete: true

        Attachment #651530 -
        Flags: review?(mounir)

        Attachment #651588 -
        Flags: review?(mounir)

    
    

    
  
Implemented changes to the 3 patches in Comment 67

    
    

    
  
(In reply to Brian R. Bondy [:bbondy] from comment #69)
> By the way Mounir, there are some companies who offer APIs that have a
> suffix of Async. For example see Microsoft's WinRT API:
> http://msdn.microsoft.com/library/windows/apps/BR207847
> 
> The API is called PickSingleFileAsync and there is no PickSingleFileSync.
> The world didn't end. 
> 
> I really don't care about the name, but I'll be spending a couple hours
> tonight uploading 3 new patches here, updating elm with those patches, and
> changing the WinRT elm implementation as well.  The unnecessary change makes
> it hard to get things done.
> 
> It's wasting a couple hours of dev time that could be spent on better
> priorities than a personal preference in an internal API name that doesn't
> matter.
> 
> I also wanted to mention that I absolutely welcome extra review comments on
> my patches, but it was rude to add an r- when the patch was already r+ed by
> the module owner.  Especially for nits that were mostly a personal
> preference.

Sorry, I think there was a misunderstanding here. My r- was only about using |returnNotShown| which, I believe was a mistake because we were not using |nsresult| when we should have been. All other comments were advices, suggestions or nits. I would have been okay if you wanted to keep warning on failure instead of simply returning the code. For the method name, I would also have been okay to keep it named ShowAsync if no one was convinced.

    
    

    
  
Comment on attachment 651588 [details] [diff] [review]
Patch v5 - Content changes

Review of attachment 651588 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with all comments applied.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +253,5 @@
> +        continue;
> +      }
> +      nsString unicodePath;
> +      localFile->GetPath(unicodePath);
> +      if (!unicodePath.IsEmpty()) {

if (unicodePath.IsEmpty()) {
  continue;
}

Also, |unicodePath| is a quite disturbing name given that we can expect that there is a variable containing the path in another string format. Could you rename this variable |path| to remove any confusion?

@@ +268,5 @@
>      }
> +  }
> +  else {
> +    nsCOMPtr<nsIFile> localFile;
> +    nsresult rv = mFilePicker->GetFile(getter_AddRefs(localFile));

This that the original author forgot |NS_ENSURE_SUCCESS(rv, rv);|.

@@ +272,5 @@
> +    nsresult rv = mFilePicker->GetFile(getter_AddRefs(localFile));
> +    if (localFile) {
> +      nsString unicodePath;
> +      rv = localFile->GetPath(unicodePath);
> +      if (!unicodePath.IsEmpty()) {

Same thing for |unicodePath|.

@@ +279,5 @@
> +        newFiles.AppendObject(domFile);
> +      }
> +      // Store the last used directory using the content pref service
> +      nsHTMLInputElement::gUploadLastDir->StoreLastUsedDirectory(
> +        mInput->OwnerDoc()->GetDocumentURI(), localFile);

I think you can move these lines inside |if (!unicodePath.isEmpty())|. If the file as a non-usable path, we unlikely want to store it as last used directory (we will more than likely not be able ta get that directory anyway...).

@@ +283,5 @@
> +        mInput->OwnerDoc()->GetDocumentURI(), localFile);
> +    }
> +  }
> +
> +  // Set new selected files

This comment is misplaced.

@@ +304,3 @@
>  
>  NS_IMETHODIMP
> +nsHTMLInputElement::AsyncClickHandler::Run()

Better to have the two nsHTMLInputElement::AsyncClickHandler methods definition one after the other.

::: content/html/content/src/nsHTMLInputElement.h
@@ +748,5 @@
> +  {
> +  public:
> +    nsFilePickerShownCallback(bool aMulti,
> +                              nsHTMLInputElement* aInput,
> +                              nsIFilePicker* aFilePicker);

nit: I would have put aMulti as the last parameter.

@@ +751,5 @@
> +                              nsHTMLInputElement* aInput,
> +                              nsIFilePicker* aFilePicker);
> +    virtual ~nsFilePickerShownCallback()
> +    { }
> +    

nit: trailing whitespaces.

@@ +757,5 @@
> +
> +    NS_IMETHOD Done(PRInt16 aResult);
> +
> +  private:
> +    nsRefPtr<nsIFilePicker> mFilePicker;

nsCOMPtr<>

        Attachment #651588 -
        Flags: review?(mounir) → review+

    
    

    
  
(In reply to Mounir Lamouri (:mounir) from comment #75)
> Sorry, I think there was a misunderstanding here. My r- was only about using
> |returnNotShown|

I'm aware of this review comment as well, I called it out after roc commented in Comment 68.

> Callers will get the return:
> - C++ callers by reading the returned nsresult, which they should;
> - Javascript callers: it will simply throw an exception.
...
> I believe was a mistake because we were not using
> |nsresult| when we should have been. 

If we did use an nsresult, it would have to be passed up into the callback.  And so the javascript wouldn't throw an exception. We wouldn't be returning it back up the callstack to nsThread.cpp who called event->Run(); in its thread.

As it stands now the existing code that calls show() differentiates from the 'short' return code and not the nsresult, so that is the one I decided to preserve.  Should we find an important failure condition we want to cover, we can just add a new filepicker specific 'short' error code.

---

Thanks for the review on the /content patch. I'll implement the changes and push to try.  The last push to try passed by the way.

> Also, |unicodePath| is a quite disturbing name

NP I'll update it, by the way this isn't my code it's just moved into the callback.

    
    

    
  
Comment on attachment 651587 [details] [diff] [review]
Patch v6 - Widget filepicker code

Review of attachment 651587 [details] [diff] [review]:
-----------------------------------------------------------------

Indeed, the callback being called asynchronously, returning an error will not change that much. I missed your comment. Sorry about that :(
However, what is the value of |returnNotShown|? What would callers do if they know the file picker was not even shown to the user? IMO, they would do exactly like if they got |returnCancel| because the only meaning is that they should not try to read the selected file(s).
Also, do we have any backend where we can expect to have the file picker not shown to the user without being in front of a complete failure. I looked at Cocoa and GTK backends and it seems that they only return something different than NS_OK if things are quite broken (like OOM or wrong arguments).

Given all that, I would recommend doing something like:
if (NS_FAILED(mFilePicker->Show(&result)) {
  NS_ERROR("FilePicker's Show() implementation failed!");
  mCallback->Done(nsIFilePicker::returnCancel);
  return NS_OK;
}

return mCallback->Done(result);

BTW, given that we are talking about API design, I'm using my super-reviewer hat.

        Attachment #651587 -
        Flags: superreview-

    
    

    
  
(In reply to Mounir Lamouri (:mounir) from comment #78)
> Comment on attachment 651587 [details] [diff] [review]
> Patch v6 - Widget filepicker code
> 
> Review of attachment 651587 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Indeed, the callback being called asynchronously, returning an error will
> not change that much. I missed your comment. Sorry about that :(

No problem, by the way I did explain 4 times: in Comment 58, Comment 63, Comment 68, and Comment 77.

> However, what is the value of |returnNotShown|? 

3

> What would callers do if
> they know the file picker was not even shown to the user? IMO, they would do
> exactly like if they got |returnCancel| because the only meaning is that
> they should not try to read the selected file(s).

Most likely they would do the same, but being able to differentiate won't cause any harm. 
You can think of it as NS_ERROR_FAILURE, which is a general error, and other error codes are added when we need to differentiate further.

> Also, do we have any backend where we can expect to have the file picker not
> shown to the user without being in front of a complete failure. I looked at
> Cocoa and GTK backends and it seems that they only return something
> different than NS_OK if things are quite broken (like OOM or wrong
> arguments).

Usually NS_OK is returned, as far as I an tell the other nsresults are used for things like a nullptr was passed in as an argument.

> Given all that, I would recommend doing something like:
> if (NS_FAILED(mFilePicker->Show(&result)) {
>   NS_ERROR("FilePicker's Show() implementation failed!");
>   mCallback->Done(nsIFilePicker::returnCancel);
>   return NS_OK;
> }
> 
> return mCallback->Done(result);
> 
> BTW, given that we are talking about API design, I'm using my super-reviewer
> hat.

I don't agree with your assessment, but I will respect the super review even know it is for a nit involving an error code. 
I'm tired of fighting over pointless things. 

Sometimes it is more important to move forward than to pick the in-your-mind ideal route and fight about it.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v1. IDL
        — 
          — Splinter Review
      

    


  
Carrying forward roc's R+ on the idl which used to be within the widget patch.

        Attachment #651757 -
        Flags: superreview?(mounir)

        Attachment #651757 -
        Flags: review+

    
    

    
  


  
Carried forward roc's r+.
Added mounir as a super reviewer since he deemed it necessary for the widget code.

        Attachment #651587 -
        Attachment is obsolete: true

        Attachment #651760 -
        Flags: superreview?(mounir)

        Attachment #651760 -
        Flags: review+

    
    

    
  


  
Updated mockfilepicker to use less specific error code.
Carried forward r+.

        Attachment #651585 -
        Attachment is obsolete: true

        Attachment #651762 -
        Flags: review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v6 - Content changes (obsolete)
        — 
          — Splinter Review
      

    


  
Implemented nits. Carrying forward r+.

        Attachment #651773 -
        Flags: review+

    
    

    
  


  
Forgot to remove an instance of returnNotShown

        Attachment #651588 -
        Attachment is obsolete: true

        Attachment #651773 -
        Attachment is obsolete: true

        Attachment #651792 -
        Flags: review+

    
  

        Attachment #651757 -
        Flags: superreview?(mounir) → superreview+

    
    

    
  
Comment on attachment 651760 [details] [diff] [review]
Patch v7 - Widget filepicker code

Review of attachment 651760 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/xpwidgets/nsBaseFilePicker.cpp
@@ +52,5 @@
> +    // to be on the main thread, so that's why we're not dispatching to another
> +    // thread and calling back to the main after it's done.
> +    PRInt16 result;
> +    nsresult rv = mFilePicker->Show(&result);
> +    if (NS_FAILED(rv)) {

nit: if (NS_FAILED(mFilePicker->Show(&result))) {
would work as well given that you don't re-use |rv|.

        Attachment #651760 -
        Flags: superreview?(mounir) → superreview+

    
    

    
  
FYI, this seems to have broken our built-in file picker - the one
you get with ui.allow_platform_file_picker set to false.
I've filed bug 784842.
Depends on: 784842

    
    

    
  
Thanks, fixed in that bug.  I'll land it as soon as possible.

    
  
Depends on: 785744

    
  
Blocks: 796994

    
  
Blocks: 112134

    
    

    
  
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10)
> (In reply to Steven Michaud from comment #7)
> > (In reply to comment #4)
> > 
> > > So apparently I was wrong about this.  The filepicker no longer
> > > blocks the window on Windows.
> > 
> > As best I can tell this is wrong.  The filepicker you get using File :
> > Open File *does* block the window it opens above.  Even in today's
> > mozilla-central nightly.  I tested on Windows XP.
> 
> It doesn't block the window on Windows 7.

It doesn't block the window on Windows 7 for me either. That's very annoying for keyboard users like me. Any chance on fixing this for Windows 7? Or do I need to open another bug for that? Or is that bug 808722, but then for Firefox?

    
    

    
  
Please post a new bug with a detailed report of the bug. You can use the Clone this Bug link below to save a bit of time.  Then just modify the subject and description and maybe other fields.

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: