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

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

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

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

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.

<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>

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

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.

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.

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...

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

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/

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/

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

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 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 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.)

> ("patch -i patchfile" tried to apply everything in the root mozilla directory.) you also need -p0 as an option to 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)

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).

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.

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.

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.

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

(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)

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

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).

(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.

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

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).

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.

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

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

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.

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.

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

Comment on attachment 204655 [details] [diff] [review] Testing: apply this after mozIconBeOS.patch this one should not be checked in

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 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 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 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];

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.

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

I think this gets us closer.

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 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

(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.

(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.

(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...

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 on attachment 209065 [details] [diff] [review] addressed Biesi's additional comments can you attach a patch with the usual diff flags? (something like -Nu7p)

(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.

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)

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 */ )

(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 on attachment 209158 [details] [diff] [review] reformatted comments. hopefully clean now. requesting toolkit peer review, as this changes download.xml.

Comment on attachment 209158 [details] [diff] [review] reformatted comments. hopefully clean now. r=me on the toolkit parts

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

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

Comment on attachment 209158 [details] [diff] [review] reformatted comments. hopefully clean now. requesting this change be incorporated in 1.8.

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).

(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.

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

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 on attachment 209158 [details] [diff] [review] reformatted comments. hopefully clean now. out of scope for security/stability updates