nsIFilePicker cleanup.

RESOLVED FIXED in mozilla1.8alpha1

Status

()

RESOLVED FIXED
15 years ago
14 years ago

People

(Reporter: jst, Assigned: shaver)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.8alpha1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

15 years ago
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.
(Reporter)

Comment 1

15 years ago
Created attachment 146286 [details] [diff] [review]
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.
(Reporter)

Updated

15 years ago
Attachment #146286 - Flags: superreview?(bzbarsky)
Attachment #146286 - Flags: review?(bzbarsky)
(Reporter)

Updated

15 years ago
Attachment #146286 - Flags: review?(caillon)
Attachment #146286 - Flags: review?(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....
Attachment #146286 - Flags: superreview?(bzbarsky) → superreview-
(Reporter)

Comment 3

15 years ago
Created attachment 146616 [details] [diff] [review]
Always pass in a parent.

Duh, yeah, of course. Fixed all that now.
(Reporter)

Updated

15 years ago
Attachment #146616 - Flags: superreview?(bzbarsky)
Attachment #146616 - Flags: review?(caillon)
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.
Attachment #146616 - Flags: superreview?(bzbarsky) → superreview+
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...
(Reporter)

Comment 6

15 years ago
Oops, sorry. Comment updated locally.
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
Attachment #146616 - Flags: review?(caillon) → review+
(Reporter)

Comment 8

15 years ago
(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!
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8alpha
(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
(Reporter)

Comment 10

15 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 11

15 years ago
Camino has been busted since this was checked in.
(Reporter)

Comment 12

15 years ago
Sorry about that, I landed a fix this morning.

Comment 13

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

Updated

15 years ago
Blocks: 242991

Comment 14

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

Updated

15 years ago
Blocks: 245554
I'm going to try to get this on the branch, so that I can get caillon's gtk24
stuff there as well.  Blah.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 154804 [details] [diff] [review]
Bonsai-derived patch for aviary.

Works on Linux, at least! =)
Assignee: jst → shaver
Attachment #146286 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Created attachment 154807 [details] [diff] [review]
Pretend, if you will, that I am not a moron.
Attachment #154804 - Attachment is obsolete: true
Comment on attachment 154807 [details] [diff] [review]
Pretend, if you will, that I am not a moron.

Chris, can you check my work?
Attachment #154807 - Flags: review?(caillon)
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.
Attachment #154807 - Attachment is obsolete: true
Attachment #154807 - Flags: review?(caillon)
I assume that patch has the fix for the regression too, right?
Created attachment 155761 [details] [diff] [review]
aviary: as I committed

This is what caillon agreed was not evidence of being a moron.
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago14 years ago
Resolution: --- → FIXED

Comment 23

14 years ago
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. 
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

14 years ago
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
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

14 years ago
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
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

14 years ago
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

14 years ago
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.
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.
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.
(Reporter)

Comment 33

14 years ago
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... 
This XPSP2 strangeness is bug 256235.  Future comments to there, pls.

Comment 35

14 years ago
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

14 years ago
This patch broke the Camino branch build.

Comment 37

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

r=pink
Attachment #169733 - Flags: review+

Comment 39

14 years ago
Cocoa file picker patch checked in.
You need to log in before you can comment on or make changes to this bug.