47553 - Change uses of nsIFileWidget to nsIFilePicker in nsEditorShell.cpp

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[Stuart Parmenter]

mozilla/editor/base/nsEditorShell.cpp uses nsIFileWidget which
will be removed towards the end of beta3.  Please change this code to use
nsIFilePicker instead.

Comment 1

[Charles Manske]

Simon had some good ideas for the required changes to make this conversion.

Comment 2

[Simon Fraser [no longer active]]

cc ryan. Did you move all the code that requires file pickers from 
nsEditorShell.cpp to the JS?

Comment 3

[Ryan Cassin]

The only code which has nsIFileWidget that I'll be touching is SaveDocument. 
I'm removing all UI-related code out of SaveDocument (which includes two uses 
of nsIFileWidget) and putting it in JS. There are other places outside of 
SaveDocument where nsIFileWidget is used that I will not be touching. I'll make 
sure to use nsIFilePicker in my JS.

Comment 4

[rubydoo123]

Setting to nsbeta3+

Ryan & Simon -- what's left on this bug?

Comment 5

[Ryan Cassin]

The only function that use nsIFileWidget and that needs to be changed are as 
follows:

nsEditorShell::GetLocalFileURL(nsIDOMWindow *parent, const PRUnichar 
*filterType, PRUnichar **_retval) (2 instances)

There is also #include "nsIFileWidget.h" that may be removed. As I said 
earlier, the 2 instances of nsIFileWidget in SaveDocument don't need to be 
worried about, those will be removed by me.

Comment 6

[Simon Fraser [no longer active]]

There's a bunch of work here, like make nsDiskDocument use nsIFile. I'll do this.

Comment 7

[rubydoo123]

setting the milestone

Comment 8

[Makoto Kato [:m_kato]]

I already report to bug 39234 and attach proposed patch.

Comment 9

[Makoto Kato [:m_kato]]

Attachment: a patch

Comment 10

[rubydoo123]

need to move this one over to m19

Comment 11

[Stuart Parmenter]

*** Bug 39234 has been marked as a duplicate of this bug. ***

Comment 12

[Stuart Parmenter]

there is a patch on here, why are you moving this out?  we need this for beta3
so that everyone is using the same file picker.  the old file picker has
numerous bugs.  Please review this patch and check it in.  thanks for the patch
Makoto.

Comment 13

[Peter Trudelle]

Pav is anxious to get rid of the old filepicker code once all callers are
upgraded, which will allow us to get rid of some completely wasteful bloat.

Comment 14

[rubydoo123]

we are currently resource constrained and have higher level issues to address 
before b3, setting this to m19 so the patch can be reviewed properly with the 
appropriate testing.

Comment 15

[Simon Fraser [no longer active]]

I have the changes in my tree.

Comment 16

[Simon Fraser [no longer active]]

The patch does not work on Mac, because the conversion from nsLocalFile to 
nsFileSpec, via nsFilePath, does not work. In addition, nsLocalFileMac has bugs 
which prevent alternative conversion paths.

Comment 17

[rubydoo123]

setting to rtm+/p1 per review with bij and beppe

Comment 18

[rubydoo123]

Simon, please include the required information per the rtm checkin rules

Comment 19

[Michael La Guardia]

PDT would like to know why this is so critical.  Thanks.

Comment 20

[Stuart Parmenter]

if this bug is not fixed, we will have 2 different file pickers, which will
confuse the users a great deal, espically on linux.  the old file picker does
not behave properly in many cases, nor does it support things like filtering on
linux or mac.  I think that is very important and needs to be fixed.

Comment 21

[Simon Fraser [no longer active]]

This bug is blocked by bug 51932.

Comment 22

[Phil Peterson]

PDT marking [rtm-] because the downside of not fixing doesn't sound like P1

Comment 23

[rubydoo123]

moving to future

Comment 24

[Makoto Kato [:m_kato]]

Attachment: new patch

Comment 25

[Makoto Kato [:m_kato]]

I cannot test on MacOS since I have no Mac build env.
please, does anyone test on Mac.

Comment 26

[Simon Fraser [no longer active]]

Makoto Kato: how is that last patch different? Does it resolve the Mac OS file 
spec conversion problems?

Comment 27

[Simon Fraser [no longer active]]

*** Bug 53533 has been marked as a duplicate of this bug. ***

Comment 28

[Simon Fraser [no longer active]]

Gotta get this done for embedding too.

Comment 29

[Masaki Katakai]

*** Bug 60934 has been marked as a duplicate of this bug. ***

Comment 30

[Simon Fraser [no longer active]]

I've put a nsFileSpec -> nsIFile patch in bug 62567.
See http://bugzilla.mozilla.org/showattachment.cgi?attach_id=21020

That subsumes these patches.

Comment 31

[Simon Fraser [no longer active]]

Fix checked in. Testing should cover open/save of files in Composer, and opening 
of image files in the image dialog, checking that the file filters are working 
properly.

Comment 32

[sujay]

Simon, can you verify this one...thanks!

Comment 33

[Simon Fraser [no longer active]]

sujay: did you do the testing that I described in the penultimate comment? If 
that all works, you can verify this.

Comment 34

[sujay]

Yes I did the testing which involved opening/saving of files in Composer, and
opening of image files in the image dialog, checking that the file filters are
working properly.

marking verified...