240723 - nsIFilePicker cleanup.

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Johnny Stenback (:jst)]

nsIFilePicker uses nsIDOMInternalWindow, that interface is not supposed to be
used in any interface that others might see (that's why it's named *internal*)!
And while I was at it, I made the interface use AString strings in stead of
silly malloc happy wstring strings. Patch coming up.

Comment 1

[Johnny Stenback (:jst)]

Attachment: API cleanup, and fix all callers.

Oh, and this also makes filepickers now require a parent, whereas before they
didn't. I forget the details now, but I think I convinced myself that the old
code wouldn't have worked w/o a parent anyways, so I just made Init() fail if
it didn't get a parent. I can change that back if someone feels that that's too
risky.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146286 [details] [diff] [review]
API cleanup, and fix all callers.

>Index: widget/public/nsIFilePicker.idl
>+  * @param      parent   nsIDOMWindow parent.  This dialog should be dependent on this parent.

This is now _required_, right?	And it has to have an associated widget, right?
 We should probably document that.  And are we sure that none of the callers
are passing in null and expecting it to work?

>Index: widget/src/beos/nsFilePicker.h

>+	virtual void InitNative(nsIWidget *aParent, const nsAString& aTitle,
>+                          PRInt16 aMode);

Weird indent.

>Index: widget/src/cocoa/nsFilePicker.mm

>+nsFilePicker::AppendFilter(const nsAString& aTitle, const nsAString& aFilter)
> {
>   // XXX this assumes that these buffers outlive us. Might want to copy
>   // the strings.

mFilters already copies.  The comment should go.

>Index: gfx/src/windows/nsDeviceContextSpecWin.cpp

>-  rv = filePicker->Init(nsnull, title.get(), nsIFilePicker::modeSave);
>+  rv = filePicker->Init(nsnull, title, nsIFilePicker::modeSave);

This will no longer work, no?  We don't support not having a parent window
anymore, with your other changes.

>Index: layout/html/forms/src/nsFileControlFrame.cpp

>+  result = filePicker->Init(parentWindow, title, nsIFilePicker::modeOpen);

Should probably change parentWindow to an nsIDOMWindow?

>Index: mailnews/addrbook/src/nsAddressBook.cpp

>+  rv = filePicker->Init(nsnull, title, nsIFilePicker::modeSave);

This will no longer work.

>Index: mailnews/base/src/nsMessenger.cpp

>             filePicker->Init(nsnull,

Nor will this.

>@@ -811,21 +811,21 @@ nsMessenger::SaveAttachment(const char *

>+  filePicker->Init(nsnull, GetString(NS_LITERAL_STRING("SaveAttachment")),
>                    nsIFilePicker::modeSave);

Nor this.

>@@ -862,19 +862,19 @@ nsMessenger::SaveAllAttachments(PRUint32

>     filePicker->Init(nsnull,

Again.

>@@ -950,33 +950,33 @@ nsMessenger::SaveAs(const char *aURI, PR

>+    filePicker->Init(nsnull, GetString(NS_LITERAL_STRING("SaveMailAs")),
>                      nsIFilePicker::modeSave);

And here.

>Index: security/manager/ssl/src/nsCrypto.cpp

>+  filePicker->Init(nsnull, filePickMessage, nsIFilePicker::modeSave);

And here.

I'm fine with requiring callers to pass in a window (because posing parentless
modals is totally evil), but then they need to pass in a window....

Comment 3

[Johnny Stenback (:jst)]

Attachment: Always pass in a parent.

Duh, yeah, of course. Fixed all that now.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146616 [details] [diff] [review]
Always pass in a parent.

I guess there's no better way to get the window in the device context and
crypto code, eh?  :(

Please fix the comment in the idl like I asked, and sr=bzbarsky, assuming
you've checked that JS callers don't pass in null windows.

Comment 5

[Boris Zbarsky [:bzbarsky]]

bieis, once this lands, a change to not require a context in uriloader will need
an accompanying change to the nsHelperAppDlg files (all 3(!!?!) of them) to deal...

Comment 6

[Johnny Stenback (:jst)]

Oops, sorry. Comment updated locally.

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 146616 [details] [diff] [review]
Always pass in a parent.

>Index: widget/public/nsIFilePicker.idl

> [scriptable, uuid(c47de916-1dd1-11b2-8141-82507fa02b21)]

Rev the uuid?

> interface nsIFilePicker : nsISupports
> {
>   const short modeOpen        = 0;              // Load a file or directory
>   const short modeSave        = 1;              // Save a file or directory
>   const short modeGetFolder   = 2;              // Select a folder/directory
>   const short modeOpenMultiple= 3;              // Load multiple files
>@@ -61,56 +61,56 @@ interface nsIFilePicker : nsISupports
>   const long filterText       = 0x04;           // *.txt
>   const long filterImages     = 0x08;           // *.png; *.gif; *.jpg; *.jpeg
>   const long filterXML        = 0x10;           // *.xml
>   const long filterXUL        = 0x20;           // *.xul
>   const long filterApps       = 0x40;           // Applications (per-platform implementation)
> 
>  /**
>   * Initialize the file widget.
>   *
>-  * @param      parent   nsIDOMWindowInternal parent.  This dialog should be dependent on this parent.
>+  * @param      parent   nsIDOMWindow parent.  This dialog should be dependent on this parent.

Can we document that parent can't be null?


>Index: widget/src/beos/nsFilePicker.h
>-	nsFilePicker();
>-	virtual ~nsFilePicker();
>+  nsFilePicker();
>+  virtual ~nsFilePicker();

Does this need to be virtual?  I forget the rule of when a dtor needs to be
virtual and when it doesn't.  Same for the other dtors in your patch.


>+NS_IMETHODIMP nsBaseFilePicker::Init(nsIDOMWindow *aParent,
>+                                     const nsAString& aTitle,
>                                      PRInt16 aMode)
> {
>-  nsCOMPtr<nsIWidget> widget;
>-  nsresult rv = DOMWindowToWidget(aParent, getter_AddRefs(widget));

How about an NS_PRECONDITION(aParent, "You suck") here to make it easier to
catch braindead callers?

>+  nsIWidget *widget = DOMWindowToWidget(aParent);
>+  NS_ENSURE_TRUE(widget, NS_ERROR_FAILURE);


r=caillon

Comment 8

[Johnny Stenback (:jst)]

(In reply to comment #7)
> > [scriptable, uuid(c47de916-1dd1-11b2-8141-82507fa02b21)]
> 
> Rev the uuid?

Done.

> Can we document that parent can't be null?

Done (as stated earlier here).

> >Index: widget/src/beos/nsFilePicker.h
> >-	nsFilePicker();
> >-	virtual ~nsFilePicker();
> >+  nsFilePicker();
> >+  virtual ~nsFilePicker();
> 
> Does this need to be virtual?  I forget the rule of when a dtor needs to be
> virtual and when it doesn't.  Same for the other dtors in your patch.

If you have virtual methods, you better have a virtual dtor, or some copilers
will warn and the real destructor might not get called (in cases where an object
is deleted through a pointer of an inherited type).

> How about an NS_PRECONDITION(aParent, "You suck") here to make it easier to
> catch braindead callers?

Done.

Thanks for the reviews!

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #8)
> If you have virtual methods, you better have a virtual dtor, or some copilers
> will warn 

note that gcc will not warn, because the build system passes a -Wno-virtual-dtor
or so flag which disables that warning

Comment 10

[Johnny Stenback (:jst)]

Fix checked in.

Comment 11

[Stuart Morgan]

Camino has been busted since this was checked in.

Comment 12

[Johnny Stenback (:jst)]

Sorry about that, I landed a fix this morning.

Comment 13

[timeless]

would someone please post to some newsgroup indicating the change to nsIFilePicker?

Comment 14

[Scott MacGregor]

jst, this checkin caused a regression.  see bug #242991 (fix in hand)

Comment 15

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I'm going to try to get this on the branch, so that I can get caillon's gtk24
stuff there as well.  Blah.

Comment 16

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: Bonsai-derived patch for aviary.

Works on Linux, at least! =)

Comment 17

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: Pretend, if you will, that I am not a moron.

Comment 18

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 154807 [details] [diff] [review]
Pretend, if you will, that I am not a moron.

Chris, can you check my work?

Comment 19

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 154807 [details] [diff] [review]
Pretend, if you will, that I am not a moron.

I'm making it very hard to pretend that I'm not a moron, what with diffing
trees from the wrong machine and attaching them.  I'll try that again tomorrow
AM.

Comment 20

[Boris Zbarsky [:bzbarsky]]

I assume that patch has the fix for the regression too, right?

Comment 21

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: aviary: as I committed

This is what caillon agreed was not evidence of being a moron.

Comment 22

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Yeah, the regression fix (using parentWindow in nsAddressBook.cpp, as near as I
can tell from CVS history and bug comments) made it in as well.

Closing this out, now that I've roughed up the tree a little.

Comment 23

[Scott MacGregor]

Hi Mike, this patch broke the file picker when exporting address book data on
the aviary branch: Bug #255733.

I'm guesssing the patch that landed didn't have the regresion fix Boris cited
before landing :). It's linked to this bug as a dependency blocker, Bug #242991. 

Comment 24

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Yeah, I got part of it (making sure we don't pass null as a parent window at
some point) but I obviously didn't get it all.  Thanks for wiping my chin.

Comment 25

[Ger Teunis]

Mike: some people are reporting download failure, most likely a regression from
this fix.

Could you please checkout the conversation on:
http://forums.mozillazine.org/viewtopic.php?p=728478#728478

Comment 26

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I can't reproduce that failure with a 08-17 Win32 build, though I do note that I
don't get a filepicker prompt if I have told Firefox to always download to a
given folder (as expected).  I'm on XP without SP2.

Which is not to say that it _couldn't_ be related to this, but I don't see the
relationship.  What makes you say that it's most likely related to this bug?

Comment 27

[Ger Teunis]

The bug was introduced on the 20040811 build.
The only bugfix mentioned on Peter(6)'s post on 20040811 in relation to download
functionality was this fix. That's why we jumped to this conclusion.
(from our point of view the most likely case, if it isn't: mea culpa)

I was confinced by vertigo451 here:
http://forums.mozillazine.org/viewtopic.php?p=728441#728441

Comment 28

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I can't reproduce it, so I'm not sure what I can do to help at this point.  If
someone gets some errors on the console with a debug build, or can verify that
it goes away by backing out my patch and the follow-ons (painful, I fear), I'm
happy to help chase it, though.

The paths through the download manager seem to use the new filepicker APIs
correctly, so I'm at a bit of a loss to explain a connection.

Comment 29

[Ger Teunis]

Javascript console displays this error after failing download window

Error: [Exception... "Component returned failure code: 0x80004005
(NS_ERROR_FAILURE) [nsIFilePicker.init]"  nsresult: "0x80004005
(NS_ERROR_FAILURE)"  location: "JS frame ::
file:///D:/Internet/firefox/components/nsHelperAppDlg.js :: anonymous :: line
155"  data: no]
Source File: file:///D:/Internet/firefox/components/nsHelperAppDlg.js
Line: 155

Comment 30

[Ger Teunis]

We just thought it was related to this, we aren't Moz devs so there is a big
change we are wrong :)
This is perhaps not the place to research this download error, but I am hesitant
of filing a new bug at this point (too early to file it?)

Hope the above Java exception will help us find the real problem, thank you for
your effort looking into this Mike.

Comment 31

[Christopher Aillon (sabbatical, not receiving bugmail)]

Ger Teunis, can you please file a new bug for that issue?  Whether or not it was
caused by this bug, it is still not this bug and therefore deserves its own bug.
 Please feel free to note your suspicions as to this bug causing the issues
you're seeing.

Comment 32

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Yes, please file another bug against me.  I don't want to believe it, I still
don't see where we go wrong, but that Java_Script_ error message sure does point
the finger at some nsIFilePicker jiggery and pokery.

Comment 33

[Johnny Stenback (:jst)]

This comment belongs in the other bug that doesn't exist yet, so until then,
I'll put it here... Maybe the problem is that a closed window happens to be used
as the parent of a file picker, seems like that could cause this exception to be
thrown... 

Comment 34

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

This XPSP2 strangeness is bug 256235.  Future comments to there, pls.

Comment 35

[timeless]

for the record the silly change to require a non null parent broke various code
snippets we had in our product and screws anyone (ok me) trying to use xpcshell
with the filepicker (on windows or beos or os/2 or macosx or gtk2.4+).

Comment 36

[Simon Fraser [no longer active]]

This patch broke the Camino branch build.

Comment 37

[Simon Fraser [no longer active]]

Attachment: Fix cocoa file picker on the MOZILLA_1_7_BRANCH branch

Comment 38

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 169733 [details] [diff] [review]
Fix cocoa file picker on the MOZILLA_1_7_BRANCH branch

r=pink

Comment 39

[Simon Fraser [no longer active]]

Cocoa file picker patch checked in.