Fennec displays bookmark favicons as 24x24px, and we'd like to ship defaults that are that size. Unforunately, when we do that we end up bumping up against the 1kb favicon file size limit. Increasing the limit to 2kb would be sufficient for our purposes (our largest favicon is 1784 bytes), but we could also live with just making this a pref that we can adjust in our defaults. Thoughts?

This is probably just fine to bump up the limit. Worst case is that it'll double the size of moz_favicons, but most favicons on the web aren't that big anyway, so we'll never hit the limit. I can review a patch if you post one!

that can cause db size to increase, and performances to decrease. can we check in places-stats avg/max number of stored favicon, calculate a guess of the space taken and double it? does fennec really wants a bigger db? what's the use case, mobile devices have a quite smaller screen than a desktop, and we never had problems on desktop, so far

so, looking at stats the avg number is 1300, max is about 15000, so we are talking about a possible db increase from 1MB to 15MB. Does not look alarming, so we can probably take it. I'm mostly concerned by the fact we keep on saving binary data in places.sqlite, i'd be so happier if we had separate dbs for binary data :\

This won't make all favicons bigger. Most favicons on the web are 16x16, and they already fall under our 1KB limit. Increasing this just means that we get a better images when a site might actually provide a larger icon, or when we want to for defaults.

Right - the specific use case is just our 5 or so default bookmarks, which we display as 24x24px in the UI. 16px icons scaled up look really ugly, so we want to store them as 24x24px. I haven't finished collecting the icons we'll be using, so I'm a bit wary of just picking 2kb and pushing that through now (in case we end up with a 2.1kb icon we want to use). Any objections to making it pref-based?

I'm not happy about pref bloat, but I suppose Firefox may not want the bigger size yet. If you want bigger favicons on fennec, you should also probably change this code to pref based for size: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsFaviconService.cpp#1105

i think there's no reason to complicate things with a pref, just change the default

We'd actually like to change the "optimization" size as well, since all of our favicons will be larger than 16x16 (likely 32x32), so I think a pref makes the most sense.

16x16 optimization is applied only if the icon goes over the size limit, so it should not touch your case at all

(In reply to comment #9) > 16x16 optimization is applied only if the icon goes over the size limit, so it > should not touch your case at all But we don't to optimize images that are too large and make them smaller than their preferred size.

Right, we want to be optimizing larger favicons to the actual size we'll be displaying the images, which in Fennec's case is never 16x16px. For example, if some site specifies a 64x64px favicon, we want to resize that to our preferred size directly rather than sizing to 16x16px and then rescaling larger every time we display it.

Comment on attachment 407633 [details] [diff] [review] patch >+ PRInt32 prefVal; >+ if (NS_SUCCEEDED(pb->GetIntPref("places.favicon.optimizeSize", &prefVal))) >+ mOptimizedIconSize = prefVal; >+ if (NS_SUCCEEDED(pb->GetIntPref("places.favicon.dimensions", &prefVal))) >+ mOptimizedIconDimensions = prefVal; XPCOM rules state that outparms must not be written to if failure happens, so you can just pass in the member variable. r=sdwilsh with that change.

Comment on attachment 407633 [details] [diff] [review] patch Sorry but this will hurt Ts, so i can't r+ If you really want to go for a pref you should read it through a getter at first favicon request (that happens async). That said, i really don't see the need for 2 prefs to set a favicon size, if 32x32 is an acceptable size for us, just change the default to 32x32, downscaling in our UI should not be a problem. How many large favicons do we expect to find in the web? they are intended mostly for the locationbars where the preffered size is 16x16, so making this settable per app does not bring any real useful gain. I could create an app that accept 100x100 favicons, but then the vast majority of pages will look ugly.

and prefs should be checked for sanity, i don't want some extension or user setting those to too large/small values, causing any sort of issue (from disappearing favicons, to no more saved favicons, to bloated dbs)

finally, if we really want them the prefs should be places.favicons.optimizeToFilesize places.favicons.optimizeToDimension (plural is wrong)

and probably the fileSize limit can directly be extracted by the dimensions limit, 2 prefs look redundant... if you know that you accept 32x32 favicons, you can easily calculate what's an acceptable filesize for such sized icon, same for 16x16, 24x24 and so on.

(In reply to comment #14) > (From update of attachment 407633 [details] [diff] [review]) > Sorry but this will hurt Ts, so i can't r+ I doubt this will have any significant impact on Ts, given that the preferences service is pretty much guaranteed to already be initialized by this point. GetService cost should be negligible. I don't really want to add a member to track whether the prefs have been read... > That said, i really don't see the need for 2 prefs to set a favicon size, if > 32x32 is an acceptable size for us, just change the default to 32x32, > downscaling in our UI should not be a problem. That sounds far from optimal, and much more likely to affect Ts (we'll have to downscale every favicon, every time they're displayed). (In reply to comment #15) > and prefs should be checked for sanity, i don't want some extension or user > setting those to too large/small values I don't think we need to worry about this. It's quite unlikely that an extension would bother changing these prefs since they're unlikely to have any noticeable visible effect, and we shouldn't be adding runtime checks just to protect people from shooting themselves in the foot with hidden prefs. (In reply to comment #16) > finally, if we really want them the prefs should be > places.favicons.optimizeToFilesize > places.favicons.optimizeToDimension (plural is wrong) optimizeToFilesize isn't quite right, because we're not optimizing "to" 1024 bytes, we're optimizing if the file starts off larger than that. The plural isn't really wrong, because it affects both dimensions, but I can use optimizeFilesizeLimit and optimizeDimension if you'd prefer. (In reply to comment #17) > and probably the fileSize limit can directly be extracted by the dimensions > limit, 2 prefs look redundant... I thought about this, but I figured we may want to tweak them separately, and I don't think we really gain anything from reducing the number from 2 prefs to 1.

I still don't understand the problem in lazy initializing the pref, we already have a member var for the pref, we should only add a GetMaxFilesize helper. Actually, iirc we use the value only starting from the getData method, then it could just check if the member var is null, and read the pref if it is, passing the value to optimizeFavicon... that would not need to add anything. We're shaving off ms in various parts of browser to have Ts wins, and we don't bother reading a useless pref on startup? > > That said, i really don't see the need for 2 prefs to set a favicon size, if > > 32x32 is an acceptable size for us, just change the default to 32x32, > > downscaling in our UI should not be a problem. > > That sounds far from optimal, and much more likely to affect Ts (we'll have to > downscale every favicon, every time they're displayed). no, we would scale to 32px only icons that are larger than filesize limit, and only those would be downscaled. As i said most of the favicons on web are 16x16. Instead Fennec will have to upscale most of the icons since it wants 32x32. So thinking to an 80% of 16x16 favicons, Firefox will have to downsize 20% of the icons, while Fennec will have to upsize 80% of the icons. But actually if we don't read the pref on startup i'm fine with that solution. > (In reply to comment #16) > optimizeToFilesize isn't quite right, because we're not optimizing "to" 1024 > bytes, we're optimizing if the file starts off larger than that ok for optimizeFilesizeLimit The plural > isn't really wrong, because it affects both dimensions but you specify only one, so the plural pref name would be confusing, imo > optimizeFilesizeLimit and optimizeDimension if you'd prefer. optimizeToDimension still sounds better to me, we optimize to that dimension > > and probably the fileSize limit can directly be extracted by the dimensions > > limit, 2 prefs look redundant... > > I thought about this, but I figured we may want to tweak them separately why? instead it could bring to issues having them separately, since if i set a filesize limit that is not enough for a certain dimension, even icons at a good ratio would be optimized to the same size... why asking something we can know by ourselves? , and I > don't think we really gain anything from reducing the number from 2 prefs to 1. Removing useless code is a win, avoid 1 xpcom crossing is a win, being more dynamic and less error-prone from external implementers is a win.

(In reply to comment #19) > I still don't understand the problem in lazy initializing the pref, we already > have a member var for the pref, we should only add a GetMaxFilesize helper. Initializing to the default and then checking the prefs once at component initialization is much simpler than initializing to zero, checking the value at each invocation of SetFaviconData, and then checking whether the prefs are present and assigning either the pref or the default, depending. > > That sounds far from optimal, and much more likely to affect Ts (we'll have to > > downscale every favicon, every time they're displayed). > > no, we would scale to 32px only icons that are larger than filesize limit, and > only those would be downscaled. As i said most of the favicons on web are > 16x16. I think we're talking past each other... this code only affects large favicons, yes. But if we change the default for Firefox, which displays icons as 16x16, then we'll be "optimizing" large favicons to a size that means they will *always* need to be rescaled when displayed. The size that we optimize to should match the size that the app will display them as, to avoid always having to rescale, and I don't think that's achievable across apps without a pref. Fennec will of course have to rescale normal 16x16 favicons when it displays them in the majority case, but that doesn't mean that we shouldn't optimize the large favicon case. > optimizeFilesizeLimit > optimizeToDimension I'll go with these... > Removing useless code is a win, avoid 1 xpcom crossing is a win, being more > dynamic and less error-prone from external implementers is a win. One extra method call is not "useless code", and isn't worth optimizing at the cost of a much harder to understand pref (explaining the implications of the dimension pref gets more complicated when it affects two separate things). I think it would actually make things more error prone.

Comment on attachment 407835 [details] [diff] [review] updated patch >diff --git a/toolkit/components/places/src/nsFaviconService.cpp b/toolkit/components/places/src/nsFaviconService.cpp >+// An uncompressed 16x16 RGBA image is 16*16*4 bytes, so almost all sensible >+// 16x16 icons will be smaller than that. >+#define OPTIMIZED_FAVICON_DIMENSION 16 >+#define OPTIMIZED_FAVICON_FILESIZE 16 * 16 * 4 just a thought, it's 16px*16px*4bytesPerPx, we don't consider the image header (well neither the compression), maybe we are too agressive in Fx and we are optimizing too many icons. followup though, but i'd be ok with increasing this a bit. >+ , mFaviconOptimizeFilesizeLimit(OPTIMIZED_FAVICON_FILESIZE) Drop favicon prefix, this is favicon service and we only handle icon files, so which other kind of file could that be? >+ if (aDataLen > (PRUint32) mFaviconOptimizeFilesizeLimit) { why are the properties not unsigned? can they ever be begative? Is this to allow setting negative values in prefs so disabling any constraint? if so it should be documented in a comment in the .h >diff --git a/toolkit/components/places/src/nsFaviconService.h b/toolkit/components/places/src/nsFaviconService.h >- static nsresult OptimizeFaviconImage(const PRUint8* aData, PRUint32 aDataLen, >- const nsACString& aMimeType, >- nsACString& aNewData, nsACString& aNewMimeType); >+ nsresult OptimizeFaviconImage(const PRUint8* aData, PRUint32 aDataLen, >+ const nsACString& aMimeType, >+ nsACString& aNewData, nsACString& aNewMimeType); NS_HIDDEN_(nsresult) i think >@@ -141,16 +141,25 @@ private: > >+ // Pref-based values for maximum and optimal favicon sizes >+ >+ // If favicon is bigger than this size we will try to optimize it into a >+ // png (whose size is determined by mFaviconDimensions). >+ PRInt32 mFaviconOptimizeFilesizeLimit; // bytes >+ >+ // The target dimension for favicons we try to optimize >+ PRInt32 mOptimizedIconDimension; // pixels end comments with period please. Can we merge the unit size in the comment? "The target dimension in pixels for" "If favicon is bigger than this size in bytes, we will" r=me code wise with the above fixed or answered You did not satisfy my concerns though, i still think one pref is more than clear for any implementer: "please try to optimize icons to this size", rather then "please try to optimize icons bigger than this filesize to this size, i know maybe you won't satisfy the requested filesize though, but i won't bother and if i put wrong value for the filesize, forgive me". Indeed the defines in the header of the file are damn pretty clear and targeted. Seeing what we are doing for history, e.g. remove any time based pref and try to retain what we can without basing ourselves on tens of prefs that nobody knows how to setup correctly, i'd expect us to follow the same direction all around unless there are well funded use-cases (and here i don't see a well funded use-case since we can calculate the size of an uncompressed RGBA image that is an upper limit. The pixels dimension is a well funded use-case).

oh and instead of "places.favicon." i'd prefer places.favicons. unless we already have any pref using the favicon. branch

> why are the properties not unsigned? can they ever be begative? > Is this to allow setting negative values in prefs so disabling any constraint? > if so it should be documented in a comment in the .h They were unsigned so that they can be passed directly to GetIntPref. I didn't intentionally mean to allow negative values as "unlimited", but I suppose that's the effect. I don't really think this needs to be documented, though, especially with the changes in this patch. > You did not satisfy my concerns though Fine, you win! :) How does this patch look? (I addressed all of the other comments, minus the NS_HIDDEN_(nsresult) one since Shawn points out on IRC that it's the default behavior.)

(In reply to comment #24) > (I addressed all of the other comments, minus the NS_HIDDEN_(nsresult) one > since Shawn points out on IRC that it's the default behavior.) hum, Shawn started pointing out we should do that, so now i'm pretty confused :)

Comment on attachment 407896 [details] [diff] [review] only one pref >diff --git a/toolkit/components/places/src/nsFaviconService.cpp b/toolkit/components/places/src/nsFaviconService.cpp > // If the page provided a large image for the favicon (eg, a highres image > // or a multiresolution .ico file), we don't want to store more data than > // needed. >- if (aDataLen > OPTIMIZED_FAVICON_SIZE) { >+ if (aDataLen > MAX_ICON_FILESIZE(mOptimizedIconDimension)) { > rv = OptimizeFaviconImage(aData, aDataLen, aMimeType, newData, newMimeType); > if (NS_SUCCEEDED(rv) && newData.Length() < aDataLen) { > data = reinterpret_cast<PRUint8*>(const_cast<char*>(newData.get())), > dataLen = newData.Length(); > mimeType = &newMimeType; > } > else if (aDataLen > MAX_FAVICON_SIZE) { you should change this as well and any other appearance of this define much cleaner, love it, thanks!

ops(In reply to comment #26) > > else if (aDataLen > MAX_FAVICON_SIZE) { > > you should change this as well and any other appearance of this define nevermind i misread that as OPTIMIZED_FAVICON_SIZE :\