301298 - nsFilePicker ignores display directory setting on OS X

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Mac, defect)

Description

[Tony Goold]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-ca) AppleWebKit/412 (KHTML, like Gecko) Safari/412
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4

Setting displayDirectory on an nsFilePicker in OS X has no effect. The dialog defaults to the last directory 
used instead of respecting the displayDirectory choice.

Reproducible: Always

Steps to Reproduce:
1. Create and initialise a file picker (nsFilePicker).
2. Set its displayDirectory to an arbitrary directory.
3. Show the file picker.

Actual Results:  
The file picker displays the last directory in which it was opened.

Expected Results:  
The file picker displays the directory assigned to displayDirectory.

Comment 1

[Tony Goold]

Attachment: Patch to fix described bug

Comment 2

[jhp (no longer active)]

Comment on attachment 189762 [details] [diff] [review]
Patch to fix described bug

+      // Set the directory to mDisplayDirectory if available
+      nsCOMPtr<nsILocalFile> displayDir = self->mDisplayDirectory;
+      if (!displayDir)
+	 return;
+      nsCOMPtr<nsILocalFileMac> localDisplay = do_QueryInterface(displayDir);
+      if (!localDisplay)

Don't see why you need a local variable for that.  You could just do:
  nsCOMPtr<nsILocalFileMac> localDisplay =
	    do_QueryInterface(self->mDisplayDirectory);
  if (localDisplay) { ....

Also, all those returns don't look good.  Might be better to nest the success
cases.

Comment 3

[Mike Pinkerton (not reading bugmail)]

looks good to me, agreed on javier's comments. some small cleanup and this can
be r+'d.

Comment 4

[Tony Goold]

Attachment: Response to review comments

(In reply to comment #2)
> Don't see why you need a local variable for that.  You could just do:
>   nsCOMPtr<nsILocalFileMac> localDisplay =
>	      do_QueryInterface(self->mDisplayDirectory);
>   if (localDisplay) { ....

I wasn't sure if do_QueryInterface was safe for null values (I'm still an XPCOM
neophyte). I've eliminated the local variable.

> Also, all those returns don't look good.  Might be better to nest the success

> cases.

Done.

Comment 5

[jhp (no longer active)]

Oh, yeah, you are correct: |do_QueryInterface| does seem to work for NULL.  Just
to make it more clear, though, you may just want to wrap everything with a |if
(self->mDisplayDirectory != nsnull) { .... }|.  Also, to make it more legible,
separate the new code from the old with a blank line.

Comment 6

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 189812 [details] [diff] [review]
Response to review comments

r=pink

Comment 7

[Tony Goold]

Attachment: Fixes problem with previous patch

There's a mistake in my last patch involving use of the Carbon function
AECreateDesc: The function doesn't actually create the object, it merely
initializes it. Sorry! I added Javier's formatting suggestions while I was at
it.

Comment 8

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 192837 [details] [diff] [review]
Fixes problem with previous patch

ah yes, good catch. i missed that.

r=pink

Comment 9

[Boris Zbarsky [:bzbarsky]]

Did this patch ever get checked in?  Or does that still need to happen?

Comment 10

[Tony Goold]

(In reply to comment #9)
> Did this patch ever get checked in?  Or does that still need to happen?
> 

I'm assuming this patch still needs super-review. Am I supposed to set this flag myself? If so, from whom do I request it?

Comment 11

[Josh Aas]

Comment on attachment 192837 [details] [diff] [review]
Fixes problem with previous patch

Please put brackets on the same line as the test. That is Mozilla coding style.

+      if (self->mDisplayDirectory != nsnull)
+      {

should be

+      if (self->mDisplayDirectory != nsnull) {

Comment 12

[Josh Aas]

I'll land this.

Comment 13

[Josh Aas]

landed on trunk with bracket cleanup. thanks Tony.

Comment 14

[[e-mail]]

(In reply to comment #5)
>Oh, yeah, you are correct: |do_QueryInterface| does seem to work for NULL. Just
>to make it more clear, though, you may just want to wrap everything with a |if
>(self->mDisplayDirectory != nsnull) { .... }|.
IIRC neither null-checking the parameter of do_QueryInterface nor explicitly comparing anything to null are considered good style.

Comment 15

[Josh Aas]

Checked in on MOZILLA_1_8_BRANCH