302569 - moz-icon urls broken on Mac OS X

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Scott MacGregor]

today's trunk build. 7/27

mozicons don't seem to be working in Mac OS X.

read a message with attachments. Notice that we don't draw a 16x16 icon to the
left of the attachment name anymore.

Weird, I can hack the CSS to make the attachment field use a background image
for the moz-icon and that draws just fine. So the problem may not be with
moz-icon but with how we are trying to display the icon inside that widget.

Comment 1

[Scott MacGregor]

For the Mac

nsIconChannel::AsyncOpen calls imgRequest::OnStartRequest. OnStartRequest
cancels the image load if there are no image listeners associated with the load.

http://lxr.mozilla.org/mozilla/source/modules/libpr0n/src/imgRequest.cpp#709

Our image frame code calls AsyncOpen on the icon channel and then it adds an
image listener after that routine returns. Hence the listener is added too late
and the image is canceled.

This effects Firefox as well.

Comment 2

[Scott MacGregor]

Attachment: the fix

The fix is to rework the mac implementation of the moz-icon output stream to
look more like windows. Using an input stream pump. This means the call to
OnStartRequest now happens after the AyncOpen call has returned to the caller,
so we no longer generate an error, which canceled the image load.

I'm not sure what changed on the trunk that broke this but this implementation
is much better than what we used to do and it fixes the problem.

Comment 3

[Scott MacGregor]

Comment on attachment 192563 [details] [diff] [review]
the fix

I'd like to get biesi to review this change as well since he made the original
change for the windows implementation to use an in put stream pump.

Comment 4

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

Comment on attachment 192563 [details] [diff] [review]
the fix

very nice

+    // Store our real listener
+	mListener = aListener;
+    // Add ourself to the load group, if available

The mListener assignment seems wrongly indented.

+nsresult nsIconChannel::MakeInputStream(nsIInputStream** _retval, PRBool
nonBlocking)
   {

as is this bracket

+  nsCAutoString filePath;
+  nsresult rv = ExtractIconInfoFromUrl(getter_AddRefs(fileloc),
&desiredImageSize, contentType, filePath);

why filePath? It looks like it really is the extension? (hm... similar in the
windows code... but it overwrites filePath with the path from the nsILocalFile
if it has one? I don't know why it does that...) It looks like this code would
be better just naming this fileExt, though.

+    PRUint32 written;
+	rv = outStream->Write(iconBuffer.get(), iconSize, &written);

indentation is wrong here...

Looks like you can also remove mLoadAttributes from the header file.

looks good otherwise, r=biesi

Comment 5

[Scott MacGregor]

Comment on attachment 192563 [details] [diff] [review]
the fix

We need this to fix a regression from 1.0 on Mac OS X. This regression is in
Firefox and Thunderbird. The mime type icons for mail attachments and in the
browser prefs / downloads UI are no longer getting drawn because of this bug.

Comment 6

[Scott MacGregor]

fixed on the trunk

Comment 7

[Scott MacGregor]

fixed on the branch