269280 - BeOS needs moz-icon implementation

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Simon]

Mozilla and Firefox have started using moz-icon:// urls in order to get the icon
the underlying OS would use for the file. This should be added to BeOS.

At the moment, confusing "External handler" messages pop up whenever a file is
downloaded or the downloads window is opened.

BeOS has a nifty function
GetIconForType(const char *file_type, BBitmap *icon, icon_size which)
which should do the business.

GTK implementation here:
https://bugzilla.mozilla.org/show_bug.cgi?id=225148

Comment 1

[Simon]

Current Status:

I have an implementation (very basic) to get an icon. The code does get called,
and the native BBitmap is correct. However I can't work out how to pass that
back to mozilla - the windows implementation seems to stuff a windows-native
icon into a stream somewhere, the GTK one saves it as a temporary png file and
passes that somewhere else - I want to avoid that approach if possible.

Surely there's a better way? The simple format suggested in the IconDecoder
seems great, but how can I make a bitmap in that format get decoded?

Any help appreciated - pavlov?

Simon

Comment 2

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

the icon channel needs to make nsIStreamListener callbacks on the passed-in
listener to asyncOpen, with the appropriate data in ondataavailable's stream.
windows uses an inputstreampump for this, which probably simplifies your code.
note that these callbacks should happen asynchronously (nsIInputStreamPump
ensures that)

if you want to use nsIconDecoder.cpp your channel's content type should be
image/icon

Comment 3

[tqh]

I'd like to see you work in progress. Could you upload them?

Comment 4

[Simon]

Sorry about the late response, just got back from skiing.

Hoping to catch up with everything in the near future, then I'll upload what
I've done so far. Basically it is just a copy from the gnome bug, with the
actual icon-getting code stripped out and replaced with a skeletal implementation.

Comment 5

[Sergei Dolgov]

<sadness_and_flame mode=ON>
Ohh, tons of such mess
https://bugzilla.mozilla.org/attachment.cgi?id=148530
only for little bit of IMHO useless "coolness":(

Coolness == straight way to bloatware
<sadness_and_flame mode=OFF>

Comment 6

[tqh]

Personally, I think using BeOS icons is good for integration.

Comment 7

[Niels Reedijk]

If you look at the patch closely, the largest part of it consists of modifying
the build system and the license headers. The actual code is quite small. I
wouldn't call it bloatware at this point (though I guess you should draw a line
between what platform dependent additions are usefull, and which are just
playful crap). I think in this case it might even be a good addition. Else,
Firefox would just have to have its own database of icons, which is something I
would consider bloatware.

Comment 8

[Simon]

As Niels says, most of the patch is to do with adding files to the build. The
actual file addition itself is quite small (and will be much simpler that the
gnome implementation anyway).

I have a little more free time now, I'll try and get this basically working this
weekend.

Comment 9

[Simon]

Attachment: Apply in mozilla/ directory

I've been having huge difficulty creating a patch that works for files in
different directories, so I'm afraid there will be lots of attachments on the
way. I've been fighting with diff far too long now though, and I know some
people want to see the code, so I thought it was best to do it this way.

Firstly I'll upload the necessary files for the beos moz-icon implementation -
the changes to the build system and the implementation itself.

That's 6 files worth I'm afraid...

Comment 10

[Simon]

Attachment: Apply in mozilla/modules/libpr0n/decoders/

Comment 11

[Simon]

Attachment: Apply in mozilla/modules/libpr0n/decoders/icon/

Comment 12

[Simon]

Attachment: beos makefile.in

Create a "beos" dir in mozilla/modules/libpr0n/decoders/icon/ and drop this in
it

Comment 13

[Simon]

Attachment: put this in ......decoders/icon/beos/ too

Comment 14

[Simon]

Attachment: ...and this

Comment 15

[Simon]

Attachment: temporary - just for configure

This is just a patch for configure. I think the idea is to alter configure.in,
then re-run autoconf. For that reason configure.in is included in the first
patch in this bug.

However I don't think autoconf works correctly on beos, so as a temporary
solution this patch will modify the configure script directly. Run it from
mozilla/

Comment 16

[Simon]

Attachment: Enable icons in the firefox download manager for BeOS as well as windows

The firefox download manager only asks for icons on windows. This alters the
ifdefs so they are requested on BeOS too.

This might belong in another bug, but I'll put it here for now.
Apply this one in mozilla/toolkit/mozapps/downloads/content/

Comment 17

[Simon]

I've opened a new bug for the download manager in ff generating the wrong
moz-icon:// strings - that is bug 280944

Comment 18

[Simon]

Attachment: Fix nsIconURI to parse file:// URIs correctly

From the description of moz-icon, if a direct reference to a local file is
required it should be:
"moz-icon://file://path/to/file.ext"

However nsIconURI.cpp doesn't look for the first // (eg expects
"moz-icon:file://"), so local files never get parsed correctly.

This one gets applied in mozilla/modules/libpr0n/decoders/icon

That concludes all my files...sorry for the mass of emails!

Comment 19

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

Comment on attachment 173295 [details] [diff] [review]
Fix nsIconURI to parse file:// URIs correctly

this is already being patched in bug 233461 comment 40, which already has
reiew...

Comment 20

[Simon]

Comment on attachment 173295 [details] [diff] [review]
Fix nsIconURI to parse file:// URIs correctly

(In reply to comment #19)
> (From update of attachment 173295 [details] [diff] [review] [edit])
> this is already being patched in bug 233461 comment 40, which already has
> reiew...
OK, obseleting my patch.

About the rest of the implementation: It is an exact copy of the windows
version besides GetContentType being changed to return "image/icon" and
MakeInputStream being written with the BeOS-specific stuff. Hopefully that
should make it easier to get review.

I expect 1 patch would be much easier to get into the tree though, but "cvs
diff -up4N Makefile.in Configure.in modules/libpr0n/decoders/Makefile.in [etc]"
produced a patch that didn't work ("patch -i patchfile" tried to apply
everything in the root mozilla directory.)

Comment 21

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

> ("patch -i patchfile" tried to apply everything in the root mozilla directory.)

you also need -p0 as an option to patch

Comment 22

[Simon]

Attachment: Unified patch

Thanks for the comments above. Now it is in one patch - apply in mozilla/ with
"patch -p0 < patchfile" or similar.

There are a few changes too - MakeInputStream has been cleaned up a little, and
error checking restructured to better fit mozilla's coding guidelines. Also if
the local file exists but has not yet been identified by Tracker, we force an
identify and get the MIME type from beos, as that is more accurate (it seems
mozilla's mime service doesn't know things like .mpg)

Comment 23

[tqh]

I'm having a weird issue with this patch (although I don't think it's the patch
itself). The makefile in decoders don't work for me. The lines

ifeq ($(OS_ARCH),BeOS)
DIRS = icon/beos icon
endif

don't cause make to go into icon at all. If I move DIRS out of the ifeq it works
though. I wonder if it's just my computer, the improved build-system or this
bug. I'm a bit worried that OS_ARCH is not ok currently (for BeOS at least).

Comment 24

[Simon]

I had exactly the same thing - took me ages to figure it out...

The patch modifies configure.in, but not configure itself. It seems the build 
system doesn't automatically run autoconf to create a new configure file (ad 
autoconf doesn't work for me anyway). However, I'm sure the idea of 
configure.in is that you just modifty that, and let someone run autoconf when 
you check it in (that's what the gnome bug does anyway).

If yo apply the patch called "temporary - just for configure", and do a new 
build it should pick up the changes.

Comment 25

[Simon]

I had exactly the same thing - took me ages to figure it out...

The patch modifies configure.in, but not configure itself. It seems the build 
system doesn't automatically run autoconf to create a new configure file (and 
autoconf doesn't work for me anyway). However, I'm sure the idea of 
configure.in is that you just modifty that, and let someone run autoconf when 
they check it in (that's what the gnome bug does anyway).

If you apply the patch called "temporary - just for configure", and do a new 
build it should pick up the changes.

Comment 26

[Niels Reedijk]

I've noticed this issue as well, because in my latest build, with the patch
applied, I still get warnings about a missing moz_icon protocol.

Comment 27

[Niels Reedijk]

Our messages crossed (apparantly). I'll try the configure patch.

Comment 28

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

(In reply to comment #25)
> let someone run autoconf when 
> they check it in (that's what the gnome bug does anyway).

autoconf is run automatically when a change to configure.in gets checked in.
(note that when you run autoconf yourself, you need version 2.13 - 2.5x won't work)

Comment 29

[Doug Shelton]

The changes here seem to work.  What other efforts are required to get this into
CVS?

Comment 30

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

the patch needs review. and if it wants to land before the 1.8 branch is made,
it needs approval too (once it has review).

Comment 31

[Doug Shelton]

(In reply to comment #27)
> Our messages crossed (apparantly). I'll try the configure patch.

Neilx, did you ever have a chance to configure the patch appropriately?  This
implementation has been working here for quite a while.  I'm concerned that the
patch file is getting stale and wonder if an updated patch would allow us to
request review.

Comment 32

[Doug Shelton]

Attachment: updated patch for use w/current CVS

no substantive changes, just a more current patch that's easier to apply.

Comment 33

[Niels Reedijk]

Let's get this thing ready for take-off. Only one issue that needs to be solved
(I think). When Firefox is restarted, the old items in the download window have
a single (boring) icon. We need to find out why and how this happens, before
landing this patch. Plus, configure.in needs to be updated (the patch currently
fails). 

Comment 34

[Niels Reedijk]

It seems that the issue of icons mis-appearing when Firefox is started and there
are old downloads in the download window, are an issue named in bug 280944 .
This means that this patch will go into the second round of pre-review. As Simon
is no longer able to do this himself, I will take over and make sure this patch
will land well before the release of Firefox 1.5.

Comment 35

[Niels Reedijk]

Attachment: Testing: apply this patch to configure to enable testing

Comment 36

[Niels Reedijk]

Attachment: Unified patch (2): updated configure.in

Comment 37

[Doug Shelton]

seems difficult to keep the patch files fresh... I'm having trouble applying to
2005-09-29 CVS.

Comment 38

[Niels Reedijk]

Depend on bug 280944, because frankly it is required to fix this one if we want to have a good moz_icon support.

Comment 39

[Doug Shelton]

Attachment: updated patch to work with recent changes to configure.in

configure.in was changed recently, in the area that invokes these BeOS changes.  This patch adds beos to the list of OS with icon support, in the new configure.in file.

Comment 40

[Doug Shelton]

Attachment: Testing: apply this after mozIconBeOS.patch

After configure.in is read and /mozilla/configure exists, apply this patch to enable testing.  This patch and its companion obsolete the patches from 2005-09-18.  Patches should be applied in the /mozilla directory, not in subdirectories.

Since we've been successfully using this code for almost a year (successfully), I'm requesting a review - not sure who needs to look at this, but I'll start with biesi.  I hope this is OK.

Comment 41

[tqh]

Doug, this page is good to know who to ask: http://www.mozilla.org/owners.html

Comment 42

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

Comment on attachment 204655 [details] [diff] [review]
Testing: apply this after mozIconBeOS.patch

this one should not be checked in

Comment 43

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

Comment on attachment 204654 [details] [diff] [review]
updated patch to work with recent changes to configure.in 

+  nsCOMPtr<nsIMozIconURI> iconURI (do_QueryInterface(mUrl, &rv));

maybe mUrl should be of type nsIMozIconURI and the QI done in Init

+  if(desiredImageSize > 16)
+  	iconSize = 32;

wrong indentation on the second line here

+  PRUint32 iconLength = 2 + iconSize*(iconSize*3 + alphaBytesPerRow);

can you add a comment where these numbers come from?

more later...

Comment 44

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

Comment on attachment 204654 [details] [diff] [review]
updated patch to work with recent changes to configure.in 

why is nativeIcon allocated on the heap rather than the stack? and, aren't you leaking it?

Comment 45

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

Comment on attachment 204654 [details] [diff] [review]
updated patch to work with recent changes to configure.in 

+  BBitmap* nativeIcon = new BBitmap(BRect(0,0,iconSize - 1,iconSize - 1), B_CMAP8);

can you be a bit more consistent with spaces after commas?

+    if(localNode.ReadAttr("BEOS:TYPE", B_STRING_TYPE, 0, NULL, 0) != B_OK)
+    	update_mime_info(filePath.get(), 0, 1, 1);

Couldn't you avoid the if and just always call update_mime_info with force=false?

I'm noticing that you're inconsistent with whether there's a space after the if and before the opening paren, please decide on one style and use it :)

+  if(!gotBitmap)
+	return NS_ERROR_NOT_AVAILABLE;

wrong indentation

more later...

Comment 46

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

Comment on attachment 204654 [details] [diff] [review]
updated patch to work with recent changes to configure.in 

I think it'd be better to move the iconLength declaration here:
+  uint8 *buffer = new uint8[iconLength];

Comment 47

[Doug Shelton]

Biesi, thank you for the review.  I'll correct what I can (especially formatting/spacing)  I wish Simon was still participating, as I don't understand the structure of the code well enough to answer your questions in comments 44 and 46.

Comment 48

[Sergei Dolgov]

Looks like it is really leaking.


Doug, 
replace 
BBitmap* nativeIcon = new BBitmap(BRect(0,0,iconSize - 1,iconSize - 1), B_CMAP8);
with 
BBitmap nativeIcon(BRect(0,0,iconSize - 1,iconSize - 1), B_CMAP8);

replace
if (!nativeIcon)
with
if (!nativeIcon.IsValid())

and replace all remaining occurencies of 
nativeIcon
with
&nativeIcon

and see if it works

Comment 49

[Doug Shelton]

Attachment: corrected spacing per biesi, fixed leak per fyysik

I think this gets us closer.

Comment 50

[Doug Shelton]

Attachment: really corrected spacing, etc, per biesi and fixed leak per fyysik

Comment 51

[Doug Shelton]

Comment on attachment 205599 [details] [diff] [review]
really corrected spacing, etc, per biesi and fixed leak per fyysik

found problems with patch file.  will upload revised patch after testing.

Comment 52

[Doug Shelton]

Attachment: cleaned spacing, fixed leak, applied against clean tree and tested OK here.

Comment 53

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

Comment on attachment 205676 [details] [diff] [review]
cleaned spacing, fixed leak, applied against clean tree and tested OK here.

(this patch doesn't seem to address some of the previous comments. some new comments follow)

+  PRUint32 iconLength = 2 + iconSize*(iconSize*3 + alphaBytesPerRow);

You should be consistent with spacing around operators...

+      else
+      {
+        destByte += 3;
+      }

hm... uninitialized bytes... I think it'd be better to initialize this to black or something.

+      if (iconCol < iconSize - 1)

I don't really get the point of these ifs... when the condition is false, the variables are reset anyway, right?

r=biesi with all my comments addressed

Comment 54

[Doug Shelton]

(In reply to comment #53)
> (From update of attachment 205676 [details] [diff] [review] [edit])
> (this patch doesn't seem to address some of the previous comments. some new
> comments follow)
> 
> +  PRUint32 iconLength = 2 + iconSize*(iconSize*3 + alphaBytesPerRow);
> 
> You should be consistent with spacing around operators...
> 

Biesi, what is considered "best practice"?  In looking through, I see the code consistently puts spaces around "+" while omitting them around "*" operators.  I'll happily change this to whatever is considered "correct".

> +      else
> +      {
> +        destByte += 3;
> +      }
> 
> hm... uninitialized bytes... I think it'd be better to initialize this to black
> or something.
> 
stupid noob question: where/how would this be initialized?  

> +      if (iconCol < iconSize - 1)
> 
> I don't really get the point of these ifs... when the condition is false, the
> variables are reset anyway, right?
> 
Do you think they can simply be eliminated?


> r=biesi with all my comments addressed
> 
I'll do my best.  Please bear in mind, this patch has been cooking for almost 3 years and the original author seems to have left the BeOS community.  I'm doing what I can to keep it fresh but don't have much insight into the code itself, as I'm really more a build tester than a true dev.  I really appreciate your help on this.  Seems like we're very close here and apologize for all the iterations.

Comment 55

[Doug Shelton]

(In reply to comment #53)
> +  PRUint32 iconLength = 2 + iconSize*(iconSize*3 + alphaBytesPerRow);
> 
fixed - will be in next revision of patch

> 
> +      if (iconCol < iconSize - 1)
> 
> I don't really get the point of these ifs... when the condition is false, the
> variables are reset anyway, right?

I don't know Simon's original reason for including them.  I created a version without the conditional but with the operations that follow and it seems to work well for BeOS purposes (displaying icons in download window and download destination of prefs panel).  If Simon is still around, maybe he can remember why he included these if statements.  Otherwise, my testing has shown no trouble bypassing them.

Comment 56

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

(In reply to comment #54)
> Biesi, what is considered "best practice"?  In looking through, I see the code
> consistently puts spaces around "+" while omitting them around "*" operators. 
> I'll happily change this to whatever is considered "correct".

Oh :-/ Well, I think that's not such a good idea. Most Mozilla code uses spaces around all operators (except stuff like ->). But making this file internally inconsistent is probably not a good idea... so, I guess best to follow the file style (or to convert the file completely)

> stupid noob question: where/how would this be initialized?  

Just something like:
  *destByte++ = 0;
  *destByte++ = 0;
  *destByte++ = 0;
At the same place where currently the pointer is just incremented.

> Do you think they can simply be eliminated?

Yes, I think they can just be replaced with the if body.

> I'll do my best.  Please bear in mind, this patch has been cooking for almost 3
> years and the original author seems to have left the BeOS community. 

I'm sorry about that. This bug seems to have had the patch only for a year, but that's a long time too... It seems to me that review should've been requested earlier...

Comment 57

[Doug Shelton]

Attachment: addressed Biesi's additional comments

Addressed Biesi's addtional comments.  Spacing around operands is consistent and according to convention.  Uninitialized bytes now initialized.  Removed conditionals (but left comment in case unforseen problems arise in the future).  

Biesi, please take a look.  If it looks good to you, is an SR required? (I'd assume yes, as toolkit/components/downloads/src/nsDownloadManager.cpp is changed along with libpr0n.)

Comment 58

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

Comment on attachment 209065 [details] [diff] [review]
addressed Biesi's additional comments

can you attach a patch with the usual diff flags? (something like -Nu7p)

Comment 59

[Doug Shelton]

(In reply to comment #58)
> (From update of attachment 209065 [details] [diff] [review] [edit])
> can you attach a patch with the usual diff flags? (something like -Nu7p)
> 
I can try - but I don't know how to force cvs diff to create patch entries for files that aren't in CVS.  To create this, I first created a patch for all the files in the repository using CVS diff, then manually appended a patch created by a local "diff" against /dev/null.

I'm sure there's a better way - please let me know.

Comment 60

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

those diff flags work with the standalone diff program too, but there is indeed a better way: If you have CVS access, use "cvs add"; otherwise use http://viper.haque.net/~timeless/redbean/

(doesn't work though if the files are in a new directory, because that actually creates the dir on the server immediately)

Comment 61

[Doug Shelton]

Attachment: created w/correct format

Comment 62

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

      if ((bitNo%8)==0)

spacing around operators?

I'd be happier if you didn't add that comment, or at least comment it out with #if 0, or use a more common commenting out style (that is, one of:
  // line 1
  // line 2
and
  /*
   * line 1
   * line 2
   */
)

Comment 63

[Doug Shelton]

(In reply to comment #62)
>       if ((bitNo%8)==0)
> 
> spacing around operators?
> 

Rats!  Caught me again.

> I'd be happier if you didn't add that comment, or at least comment it out with
> #if 0, or use a more common commenting out style (that is, one of:
>   // line 1
>   // line 2
> and
>   /*
>    * line 1
>    * line 2
>    */
> )
> 
Will do.

Comment 64

[Doug Shelton]

Attachment: fixed last two formatting issues, as per comment #62

Comment 65

[Doug Shelton]

Attachment: reformatted comments. hopefully clean now.

Comment 66

[Doug Shelton]

Comment on attachment 209158 [details] [diff] [review]
reformatted comments.  hopefully clean now.

requesting toolkit peer review, as this changes download.xml.

Comment 67

[Mike Connor [:mconnor]]

Comment on attachment 209158 [details] [diff] [review]
reformatted comments.  hopefully clean now.

r=me on the toolkit parts

Comment 68

[Doug Shelton]

thanks, biesi and mconnor!  Biesi, if I understand, this needs no further review.  I do not have the ability to commit changes.  Is there some place I can request help to commit these changes?  Please advise. g

Comment 69

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

checked in on trunk! if you want this in firefox 2 / seamonkey 1.1, you need to request 1.8 approval.

Comment 70

[Doug Shelton]

Comment on attachment 209158 [details] [diff] [review]
reformatted comments.  hopefully clean now.

requesting this change be incorporated in 1.8.

Comment 71

[Benjamin Smedberg]

Comment on attachment 209158 [details] [diff] [review]
reformatted comments.  hopefully clean now.

1.8.0.1 is already out the door. This doesn't really fit the 1.8.0 branch criteria (being neither a regression or a stability issue).

Comment 72

[Doug Shelton]

(In reply to comment #71)
> ...This doesn't really fit the 1.8.0 branch
> criteria (being neither a regression or a stability issue).

My apologies if not fitting for this release.  It's been hanging over our heads, implemented as patches in BeOS builds for so long, it just seems like it belongs.  Thank you for the review.

Comment 73

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

Attachment: patch merged to MOZILLA_1_8_BRANCH

Comment 74

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

checked in on MOZILLA_1_8_BRANCH, but I'm not 100% sure I merged this correctly, please verify

Checking in allmakefiles.sh;
/cvsroot/mozilla/allmakefiles.sh,v  <--  allmakefiles.sh
new revision: 1.579.2.8; previous revision: 1.579.2.7
done
Checking in configure;
/cvsroot/mozilla/configure,v  <--  configure
new revision: 1.1492.2.29; previous revision: 1.1492.2.28
done
Checking in configure.in;
/cvsroot/mozilla/configure.in,v  <--  configure.in
new revision: 1.1503.2.25; previous revision: 1.1503.2.24
done
Checking in modules/libpr0n/decoders/Makefile.in;
/cvsroot/mozilla/modules/libpr0n/decoders/Makefile.in,v  <--  Makefile.in
new revision: 1.13.18.1; previous revision: 1.13
done
Checking in modules/libpr0n/decoders/icon/Makefile.in;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/Makefile.in,v  <--  Makefile.in
new revision: 1.17.8.1; previous revision: 1.17
done
Checking in modules/libpr0n/decoders/icon/nsIconModule.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/nsIconModule.cpp,v  <--  nsIconModule.cpp
new revision: 1.7.12.1; previous revision: 1.7
done
Checking in modules/libpr0n/decoders/icon/beos/Makefile.in;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/beos/Makefile.in,v  <--  Makefile.in
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in modules/libpr0n/decoders/icon/beos/nsIconChannel.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/beos/nsIconChannel.cpp,v  <--  nsIconChannel.cpp
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in modules/libpr0n/decoders/icon/beos/nsIconChannel.h;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/beos/nsIconChannel.h,v  <--  nsIconChannel.h
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in toolkit/mozapps/downloads/content/download.xml;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/download.xml,v  <--  download.xml
new revision: 1.20.2.1; previous revision: 1.20
done

Comment 75

[Sergei Dolgov]

I think we need some effective cache for types/icons, instead regetting it everytime from system.
I'm very unsatisfied with SeaMonkey download manager behaviour after icon support appeared there.

Comment 76

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

Comment on attachment 209158 [details] [diff] [review]
reformatted comments.  hopefully clean now.

out of scope for security/stability updates