113745 - moz-icon returns wrong icons to directory references

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Robert John Churchill]

At least on Mac (haven't tried it on windows), using moz-icon with file:/// URLs 
which reference directories returns a wrong icon... looks like some file icon 
instead.

See bug # 113742 for details.  I added a special check for directories at:
  mozilla/rdf/datasource/src/FileSystemDataSource.cpp
in GetTarget() [around line #500] for the moment... it should be removed once 
this bug is fixed.

Comment 1

[Robert John Churchill]

I looked into libpr0n a bit... the icon code is currently using the Desktop 
database, which is wrong... it doesn't know about icons for volumes, folders, or 
files/folders/volumes with custom icons.

The icon decoder code should be using the Mac OS's Icon Services which can get 
the correct icons for ANY file, folder, or volume.  :)

Comment 2

[Robert John Churchill]

Attachment: Use Icon Services; works with files, folders & custom icons

Comment 3

[Robert John Churchill]

The diff I attached switches the moz-icon: code over to using Mac OS's Icon 
Services, only falling back to mimetype info if absolutely necessary (the file/
folder/volume didn't exist, for example).  By using Icon Services, we can now 
also get icons for volumes, folders, remote volumes, and anything with a custom 
icon.

Reviews, please!

Comment 4

[Robert John Churchill]

Attachment: Also, use SetFollowLinks(FALSE) and free icon handles if error exit

Comment 5

[Simon Fraser [no longer active]]

+        if (::GetIconRefFromFile (&spec, &icnRef, &label)) 
return(NS_ERROR_FAILURE);

Please be more explicit about the error checking:

  if (::GetIconRefFromFile (&spec, &icnRef, &label) != noErr) 
return(NS_ERROR_FAILURE);

Same here:

+    if (fileExists && (!::IconRefToIconFamily(icnRef,
+        kSelectorAllAvailableData, &icnFamily)))

->

+    if (fileExists && (::IconRefToIconFamily(icnRef,
+        kSelectorAllAvailableData, &icnFamily) == noErr))

Same here:
+        if (::GetIconRef(kOnSystemDisk, macCreator, macType, &icnRef))

and here:

+        if (!::IconRefToIconFamily(icnRef, kSelectorAllAvailableData, &
icnFamily))

In GetIconData() is this code:

+    if (*iconDataSize > 0)
+    {
+        ::HLock(iconDataH);
+    }

It seems odd to me that GetIconData() has to return a locked handle. Why doesn't 
the caller do the locking?

Comment 6

[Robert John Churchill]

> Please be more explicit about the error checking:

OK, I'll make the changes regarding comparing against "noErr".


> It seems odd to me that GetIconData() has to return a locked handle. Why doesn't the caller do the locking?


I wanted it to happen in one spot instead of requiriong each caller to lock the 
handle.

Comment 7

[Simon Fraser [no longer active]]

Doesn't the handle only need to be locked for the code in 
nsIconChannel::AsyncOpen()? The code should only lock the handle when it needs to 
be locked, not lock it eagerly.

Comment 8

[Robert John Churchill]

Well, only nsIconChannel::AsyncOpen() calls GetIconData().  I don't actually care 
either way, but I don't see why having each caller lock the icon data is 
preferrable, especially since it isn't a public routine that others can call.  
However about a compromise, where GetIconData() is renamed to GetLockedIconData() 
?

Comment 9

[Robert John Churchill]

Attachment: Compare against "noErr", rename icon funcs

Comment 10

[Mike Pinkerton (not reading bugmail)]

Comments:

1) change spacing to 2 spaces per indent

2) add a comment that ::GetIconFamilyData will resize the handles when you
create them to zero size. I had to go searching through the docs to make sure
that was ok. A comment would help ease fears.

3) Instead of:

+    PRUint32  dataCount = 0L, maskCount = 0L;
+    Handle    iconH = ::NewHandle(dataCount);

Why not just:

PRUint32  dataCount = 0L, maskCount = 0L;
Handle iconH = ::NewHandle(0L);

Along with the above comment, this is easier for the reader to quickly see your
intent (creating a zero sized handle) rather than having to mentally parse the code.

4) Do we have any kind of stack-based class for Handles so you don't need all
the error handling code when we return early? It really clutters things up. If
not, you should write one, i'm sure we could use it elsewhere.

5) What is the 72 in:

+        cTabHandle = ::GetCTable(72);

is there a constant somewhere for this table size?

Comment 11

[Robert John Churchill]

Claiming this bug since I'm rewriting the Mac's icon decoder.   cc'ing mscott

Comment 12

[Robert John Churchill]

.

Comment 13

[Robert John Churchill]

Attachment: Incorporate sfraser/pinkerton comments

Incorporate sfraser/pinkerton comments.  Regarding pink's comments:

1) done

2) & 3)  I've added a comment which states that we start with a
	 zero-sized handle which gets resized; however, I'm keeping:
	       Handle iconH = ::NewHandle(dataCount); 
	 as that's my preferred usage.

4) I am unaware of a stack-based class for auto-disposing of Handles
   and don't plan to write one at this time, so the code  stays as is.

5) The 72 is what mscott had in his original code.  I originally went
   looking for a constant but didn't find one.

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 61503 [details] [diff] [review]
Incorporate sfraser/pinkerton comments

r=[e-mail]

Comment 15

[Simon Fraser [no longer active]]

Comment on attachment 61503 [details] [diff] [review]
Incorporate sfraser/pinkerton comments

sr=sfraser

Comment 16

[Robert John Churchill]

Fixed.