moz-icon returns wrong icons to directory references

RESOLVED FIXED

Status

()

RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: mozilla, Assigned: mozilla)

Tracking

Trunk
PowerPC
Mac System 9.x
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

17 years ago
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.
(Assignee)

Comment 1

17 years ago
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.  :)
(Assignee)

Updated

17 years ago
Blocks: 113742
(Assignee)

Comment 2

17 years ago
Created attachment 60867 [details] [diff] [review]
Use Icon Services; works with files, folders & custom icons
(Assignee)

Comment 3

17 years ago
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!
(Assignee)

Comment 4

17 years ago
Created attachment 61060 [details] [diff] [review]
Also, use SetFollowLinks(FALSE) and free icon handles if error exit
Attachment #60867 - Attachment is obsolete: true

Comment 5

17 years ago
+        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?
(Assignee)

Comment 6

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

17 years ago
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.
(Assignee)

Comment 8

17 years ago
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() 
?
(Assignee)

Comment 9

17 years ago
Created attachment 61220 [details] [diff] [review]
Compare against "noErr", rename icon funcs
Attachment #61060 - Attachment is obsolete: true
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?


(Assignee)

Comment 11

17 years ago
Claiming this bug since I'm rewriting the Mac's icon decoder.   cc'ing mscott
(Assignee)

Comment 12

17 years ago
.
Assignee: mscott → rjc
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 13

17 years ago
Created attachment 61503 [details] [diff] [review]
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.
Attachment #61220 - Attachment is obsolete: true
Comment on attachment 61503 [details] [diff] [review]
Incorporate sfraser/pinkerton comments

r=ben@netscape.com
Attachment #61503 - Flags: review+
(Assignee)

Updated

17 years ago
Blocks: 114811

Comment 15

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

sr=sfraser
Attachment #61503 - Flags: superreview+
(Assignee)

Comment 16

17 years ago
Fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.