280944 - Downloader manager problems creating moz-icon:// URIs

Status: VERIFIED WORKSFORME

(Toolkit :: Downloads API, defect)

Description

[Simon]

I've just implemented moz-icon support on BeOS.
See bug 269280 for information on that.

When trying to get icon support in the downloader, I came across some bugs there.

In nsDownloadsDataSource::GetTarget seems treats path as a native file, but
inserting printfs there showed that it is in fact already a "file://" URI. The
unix nsLocalFile::InitWithNativePath fails with an assertion when passed in this
fileURI and the moz-icon URI generated misses out the fileURI completely
("moz-icon://?size=32").

Patch that follows simple comments out all the localfile stuff, and adds path to
the iconURI instead. I don't understand the thinking behind the code though, so
it's not really a patch to check in.

Comment 1

[Simon]

Attachment: Comment out code to treat path as a local file

Apply this in the mozilla/toolkit/components/downloads/src directory.

Comment 2

[Doug Shelton]

There's been no discussion for quite a while.  Is this still a valid adjunct to
the mozIcon patch?

Comment 3

[Niels Reedijk]

(In reply to comment #2)
> There's been no discussion for quite a while.  Is this still a valid adjunct to
> the mozIcon patch?
Yes

Comment 4

[Niels Reedijk]

We need to get this one out in the light, in order to get it fixed. Who's responsible for the Download Manager subsystem?

Comment 5

[tqh]

I'm not sure, but wasn't the patch in bug 233461 comment 40 related to this as well?
It needs looking in to.

Comment 6

[tqh]

Btw, Niels can you take assignment on this one and bug 269280?

Comment 7

[Doug Shelton]

For what it's worth, I've been including this patch in my builds for several months and have noticed no adverse effects. 

Comment 8

[Niels Reedijk]

As soon as FF 1.5 is released and the dust settles down, I'll go over to IRC and see if I can get some help on getting this in (after I cleaned up the patch to just delete the lines instead of commenting them out).

Comment 9

[Doug Shelton]

Neilx, I'm currently running with 269280.  Do you mind if I try to finish this up also?  I'd like to get them both committed and be done with mozIcon.  

Comment 10

[Doug Shelton]

(In reply to comment #5)
> I'm not sure, but wasn't the patch in bug 233461 comment 40 related to this as
> well?
> It needs looking in to.
> 
tqh, seems like bug 233461 was fixed in 2005-03.  This patch has peacefully coexisted in builds since that time.  I'll be happy to do the "looking into" as best as I can.  What shall I look for?

Comment 11

[tqh]

I think this patch shouldn't be needed. So try without it, if it's not needed it's more or less a dupe of the other bug.

Comment 12

[Doug Shelton]

(In reply to comment #11)
> I think this patch shouldn't be needed. So try without it, if it's not needed
> it's more or less a dupe of the other bug.
> 
Without this patch, there's still a problem.  Download manager shows icons of files downloaded during the current session.  If you close the browser without cleaning up the download manager, then open the browser and DM window, the icons are missing.  Put in this patch and the icons persist between program executions.

Comment 13

[Doug Shelton]

It's hard to believe simply removing this code won't have a negative effect on other platforms, even though it is required for proper functioning on BeOS.

Comment 14

[Doug Shelton]

sergei, could you please look at the code around this change?  I can't imagine we could simply ask for it to be removed without breaking something on another platform.  But with the commented code in place, the download manager only shows appropriate icons during the current session.  DM entries present when Firefox starts display empty space where the icon should be.

I suppose I could just remove the comments, create a patch to remove the code and ask for a review from the module owner but that's not quality work, IMO.

Comment 15

[Sergei Dolgov]

will do, as far as i can without firefox build tree.
or maybe i will take yet another attempt to build FF first

Comment 16

[Sergei Dolgov]

I'm wondering what must be type which we getting from ns(I)RDFResource with GetValue(), if resource itself is obtained with gNC_File via
mInner->GetTarget(aSource, gNC_File, aTruthValue, getter_AddRefs(target));

As no other platform claimed troubles here, i suspect some flacky implementation of BeOS RDF part...at least for BeOS bookmarks there are bugs in rdf code for sure.

Comment 17

[Sergei Dolgov]

looks like bug
https://bugzilla.mozilla.org/show_bug.cgi?id=236988
 checkin was concerned about it. Will investigate

Comment 18

[Sergei Dolgov]

Btw, i don't see such problem in SeaMonkey DL manager.
It polulates with icons all entries in list.
Though, doggy slowly.

Comment 19

[Doug Shelton]

Recent builds no longer exhibit this behavior.  Maybe fixed by bug 326273 or 335725.  Either way, if no other bezilla members observe this bug, maybe this can be marked "worksforme" and closed.

Comment 20

[Shawn Wilsher :sdwilsh]

per comment #19

Comment 21

[Stephen Donner [:stephend] Not actively reading bugmail]

verified, too (per comment 19)