I mentioned this in bug 884848 - in Favicons.java, we're currently caching both scaled and unscaled icons, and the code that caches/uses the cached icons is kinda confusing to follow right now (e.g. cached icons are used in AllPagesTab, but not the other aweseomescreen tabs). As part of the new about:home work, I think we should take the opportunity to clean things up, especially since we're using favicons in different places now, like in the bookmarks thumbnails. It seems like most of our UI wants 32x32 icons, so I think it could make sense to store those scaled images in the cache, but then we should make sure we're not trying to re-scale them when pulling them out of the cache, and we should also make sure we're not using the cache for cases where we want the original icon (like bug 884848).

Re-summarizing this bug to indicate that what really needs to happen is that someone needs to go through and audit all the places we're scaling/caching favicons. It would be smart to do this after the new about home lands, but this doesn't need to be a strict dependency.

Shall I take this one? It seems related to what I need to do for Bug 895423, and cleaning up this Favicon code seems not too difficult. I did some preliminary work earlier in this direction - perhaps what would be nice to have is a cache to which you can say "I want a Favicon for this URL and I want it to be this big". That way, we abstract the problem of sourcing an appropriately scaled image away from the UI code, while retaining good cache performance in the face of potentially requesting different sizes of Favicon in different places. The strategy for handling such requests for Favicons can be similar to that employed by my modified FaviconView in Bug 895423. Namely, take an unscaled, cache Favicon, scale it by up to a factor of 2 in the direction of the target size (Or some other value of maximum scale chosen to maintain quality), and cache and return a scaled icon as close as possible to the desired size, given this constraint. This has the added bonus that when sites (eg. Reddit) serve up very large Favicons we get to take advantage of this in FaviconView (and anywhere else we subsequently want to display largeish Favicons. Such as in the search engine configuration prompt.). The existing implementation, in the face of large Favicons, bins much of the extra information right away, caches the result, then displays the downscaled version - not ideal.

Sure, feel free to take this from me if you have the time. Caching different icon sizes sounds like a good approach, since it would be costly to need to scale favicons as we scroll through a ListView. I'm curious to know all the places we use favicons, and which need to be scaled or not. On quick glance, it seems like we only actually want a small size for the icon in the toolbar, and an equally large size for about:home and the settings page. Is there any other place we use favicons? If not, I would say we should consider only caching the size optimized for FaviconView, then scale it down if we need a smaller icon for the toolbar, since that gets updated pretty infrequently. Cc'ing bnicholson, since he's also dug into this code before and might have some ideas.

(In reply to :Margaret Leibovic from comment #3) > I'm curious to know all the places we use favicons, and which need to be > scaled or not. On quick glance, it seems like we only actually want a small > size for the icon in the toolbar, and an equally large size for about:home > and the settings page. Is there any other place we use favicons? If not, I > would say we should consider only caching the size optimized for > FaviconView, then scale it down if we need a smaller icon for the toolbar, > since that gets updated pretty infrequently. So far as I can tell, "A size optimised for FaviconView" isn't a thing, since FaviconViews are not of fixed size. On different devices or in different contexts they may be displayed at different sizes. It may well be the case that, for a particular device, all FaviconViews are the same size, but I think this size does vary between devices. As a result, I don't think there's a number we can hardcode in as "The size a FaviconView wants". This is sort of the beauty of the LRU cache system keyed by both size and URL - whichever size of Favicon is wanted will stay in the cache.

Looks like the best way of dealing with Bug 895423 is to do the suggested Favicon caching revamp.

Nifty screenshot showing the difference this patch series makes to the quality of the icons you end up with on your home screen. Less pixelation - hurrah!

(In reply to Chris Kitching [:ckitching] from comment #6) > Created attachment 800002 [details] > Top row: With patch. Second row: Without patch. > > Nifty screenshot showing the difference this patch series makes to the > quality of the icons you end up with on your home screen. Less pixelation - > hurrah! Looking good! Feel free to upload a WIP patch for feedback.

The benefits of this move will become more apparent with the subsequent patches.

Despaghettification.

Even more soon-to-become-noninsane refactoring.

The counter that was used before was not thread safe - jobs with the same id could have been created. (Vaguely related fun fact: In Java, assignment to along isn't atomic.)

Generalising the FaviconView. Motivation to become apparent shortly..

Add a more intelligent caching system. See Javadoc comments for info.

Might as well do this, too...

Unsafely closed resources. My favourite.

Comment on attachment 800455 [details] [diff] [review] Part 6: Add more intelligent caching to the Favicons system. Looks like the original implementation was caching on page URLs - we don't want this because it creates poor cache behaviour. But it turns out we can have more than one Favicon per domain (I had assumed we could now) - so what I have here (Caching on domain) isn't quite right, either. It's not completely obvious how to implement caching on Favicon URLs from all of the callers.

Look what I found: https://bugzilla.mozilla.org/show_bug.cgi?id=834536 Seems this needs fixing at the same time.

Comment on attachment 800448 [details] [diff] [review] Part 1: Move the favicons class into its own package and, while we're at it, get rid of unused imports in the touched files. Review of attachment 800448 [details] [diff] [review]: ----------------------------------------------------------------- Nice.

Comment on attachment 800449 [details] [diff] [review] Part 2: Refactoring Favicons to be static, not a singleton - the singleton instance was staticly assigned and never cleared. Review of attachment 800449 [details] [diff] [review]: ----------------------------------------------------------------- This looks like a great improvement. ::: mobile/android/base/favicons/Favicons.java @@ +59,5 @@ > + return image.getRowBytes() * image.getHeight(); > + } > + }; > + private static final LruCache<String, Long> sFailedCache = new LruCache<String, Long>(64); > + private static final LruCache<String, Integer> sColorCache = new LruCache<String, Integer>(256); Please add some documentation above these fields to let us know what they're for (I see that in the current code there's some comments where these are assigned). @@ +189,5 @@ > cancelFaviconLoad(taskId); > } > } > + if (sHttpClient != null) > + sHttpClient.close(); I see you addressed my comment from bug 887492. Thanks :) @@ +319,5 @@ > contentStream.close(); > + } catch (IOException e) { > + Log.e(LOGTAG, "IOException reading favicon:", e); > + } catch (URISyntaxException e) { > + Log.e(LOGTAG, "URISyntaxException reading favicon:", e); Nice improvement here to be more explicit about the exception type.

Adding the requested comments.

Comment on attachment 801053 [details] [diff] [review] Part 2: V2 - Refactoring Favicons to be static, not a singleton - the singleton instance was staticly assigned and never cleared. Review of attachment 801053 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/favicons/Favicons.java @@ +182,5 @@ > } > return cancelled; > } > > + public static void close() { I just realized that it doesn't look like this method is called from anywhere... is that true? If so, we should probably find a place to call this, probably somewhere during shutdown. We could do some code archeology to see where it was originally introduced, and perhaps see where a call to close() might have been removed.

(In reply to :Margaret Leibovic from comment #21) > I just realized that it doesn't look like this method is called from > anywhere... is that true? If so, we should probably find a place to call > this, probably somewhere during shutdown. We could do some code archeology > to see where it was originally introduced, and perhaps see where a call to > close() might have been removed. Eep! Well spotted - IDE didn't highlight that one as unused - apparently too complicated to work that out. We could put this in shutdown - it's probably not actually that harmful (Although sort of dumb) to just not call it, since resources are implicitly closed for us on shutdown. We're never going to want to close the Favicon database in this way at while we're running (Plausibly we might want to purge the cache contents to save memory, but never close()). Assuming our ancillary threads are marked as daemons, and we can ensure database consistency, that is... Anyway - it looks like GeckoApp.onDestroy is an appropriate place to put a call to this. Here's a patch doing so. Anywhere else seem more sensible? It seems that this call breaks the Favicon system in an expensive to recover from way - I'm pretty sure we never want to do this except when we're finally quitting.

Comment on attachment 800450 [details] [diff] [review] Part 3: Factor the inner classes of Favicon into their own files. Review of attachment 800450 [details] [diff] [review]: ----------------------------------------------------------------- Looking good, I just have a few concerns. ::: mobile/android/base/BrowserApp.java @@ +1339,5 @@ > /* Favicon methods */ > private void loadFavicon(final Tab tab) { > maybeCancelFaviconLoad(tab); > > + int flags = LoadFaviconTask.FLAG_SCALE | ( (tab.isPrivate() || tab.getErrorType() != Tab.ErrorType.NONE) ? 0 : LoadFaviconTask.FLAG_PERSIST); Not your code, but can we clean up the logic here? This is currently hard to parse. Maybe something like: int flags = LoadFaviconTask.FLAG_SCALE; if (!tab.isPrivate() && tab.getErrorType() == Tab.ErrorType.NONE) { flags = flags | LoadFaviconTask.FLAG_PERSIST; } And sprinkle some useful comments in there :) ::: mobile/android/base/favicons/Favicons.java @@ +154,5 @@ > cancelFaviconLoad(taskId); > } > } > + if (LoadFaviconTask.sHttpClient != null) { > + LoadFaviconTask.sHttpClient.close(); I think it would be clearer to keep the code that initalized and closes sHttpClient in the same class. Perhaps it would be better to make a static LoadFaviconTask.close() method that takes care of closing sHttpClient, then sHttpClient could be a private member of LoadFaviconTask. ::: mobile/android/base/favicons/LoadFaviconTask.java @@ +36,5 @@ > + > + public static final int FLAG_PERSIST = 1; > + public static final int FLAG_SCALE = 2; > + > + private long mNextFaviconLoadId; Given how mNextFaviconLoadId is used, I don't think we can move it into LoadFaviconTask. It looks like it's used to give each LoadFaviconTask a unique increasing id. Perhaps you can just change the LoadFaviconTask constructor to take an id, and we can keep that id logic in Favicons. @@ +215,5 @@ > + } > + > + public long getId() { > + return mId; > + } If we do that, we can probably get rid of this getId() method.

Comment on attachment 800456 [details] [diff] [review] Part 7: We bundle a bunch of nice icons for search engines - let's run these through the cache (Solves 819895) Review of attachment 800456 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/BrowserSearch.java @@ +406,5 @@ > final Bitmap icon = BitmapUtils.getBitmapFromDataURI(iconURI); > > + // Pilfer the Favicon and put it in the memcache for later use. > + final String searchURL = engineJSON.getString("searchForm"); > + Favicons.putFaviconInMemCache(searchURL, icon); We discussed this yesterday in person... we should be using the favicon's URL as the key in the cache, so this doesn't look quite right. I know this patch is relatively small, but there is a lot already going on in this bug, so I think this issue should be split out into a follow-up bug. That we we can just focus on landing the big changes first. Clearing the feedback flag for now, let's work on this in a separate bug.

(In reply to :Margaret Leibovic from comment #23) > ::: mobile/android/base/favicons/Favicons.java > @@ +154,5 @@ > > cancelFaviconLoad(taskId); > > } > > } > > + if (LoadFaviconTask.sHttpClient != null) { > > + LoadFaviconTask.sHttpClient.close(); > > I think it would be clearer to keep the code that initalized and closes > sHttpClient in the same class. Perhaps it would be better to make a static > LoadFaviconTask.close() method that takes care of closing sHttpClient, then > sHttpClient could be a private member of LoadFaviconTask. Makes sense. Can eliminate the getter method, too. > Given how mNextFaviconLoadId is used, I don't think we can move it into > LoadFaviconTask. It looks like it's used to give each LoadFaviconTask a > unique increasing id. Perhaps you can just change the LoadFaviconTask > constructor to take an id, and we can keep that id logic in Favicons. Actually, this patch is something of an intermediate step - the use of mNextFaviconLoadId is flawed in general - Part 4 is all about addressing the problems there. If you still don't like it after Part 4 then we'll talk :P. > @@ +215,5 @@ > > + } > > + > > + public long getId() { > > + return mId; > > + } > > If we do that, we can probably get rid of this getId() method. It is indeed never used. I kept it around as a sort of "It might be handy later" thing. Can chuck it if you like. > ::: mobile/android/base/BrowserApp.java > @@ +1339,5 @@ > > /* Favicon methods */ > > private void loadFavicon(final Tab tab) { > > maybeCancelFaviconLoad(tab); > > > > + int flags = LoadFaviconTask.FLAG_SCALE | ( (tab.isPrivate() || tab.getErrorType() != Tab.ErrorType.NONE) ? 0 : LoadFaviconTask.FLAG_PERSIST); Good spot - this definetly needs some documenting - although a later patch in this series removes FLAG_SCALE - I'll do the cleanup at that point. (Plus removing that flag simplifies things a fair bit anyway)

(In reply to :Margaret Leibovic from comment #24) > Comment on attachment 800456 [details] [diff] [review] > Part 7: We bundle a bunch of nice icons for search engines - let's run these > through the cache (Solves 819895) > > Review of attachment 800456 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/home/BrowserSearch.java > @@ +406,5 @@ > > final Bitmap icon = BitmapUtils.getBitmapFromDataURI(iconURI); > > > > + // Pilfer the Favicon and put it in the memcache for later use. > > + final String searchURL = engineJSON.getString("searchForm"); > > + Favicons.putFaviconInMemCache(searchURL, icon); > > We discussed this yesterday in person... we should be using the favicon's > URL as the key in the cache, so this doesn't look quite right. > > I know this patch is relatively small, but there is a lot already going on > in this bug, so I think this issue should be split out into a follow-up bug. > That we we can just focus on landing the big changes first. Clearing the > feedback flag for now, let's work on this in a separate bug. Bug 913746. The use-FaviconURLs-not-pageURLs-in-the-cache thing is not completely trivial. If you check out where stuff is calling into the cache, often they don't HAVE a Favicon URL available. I'm not sure how to solve this. Query the history database for the Favicon URL and hope for the best? This returns null surprisingly often. Any suggestions, or is this really a case of refactoring ALL the things? Also, the problem mentioned in Bug 905685 adds a further layer of complication. The new about:home's use of Favicons is both extensive and quite strange. It begins to smell like the true solution to this problem is to solve Bug 905685, and, while in the process of completely changing how about:home uses Favicons, have it do use the right sort of URLs. Perhaps a nonterrible plan is to keep the existing very poor pageURL behvaiour for now and deal with Bug 834536 as a followup. (And add commenting and suchlike to the new cache to make it clear that it expects Favicon URLs - we're just abusing it slightly in the meantime)

Thanks for the feedback so far. Here is a revised and now "complete" version of the patch series for your perusal. This set currently fixes Bug 887492, Bug 873243 (All instances bar address bar, which is subsequently being special-cased in Bug 792240), Bug 834536, Bug 753356, Bug 782823 and all of Bug 813810 that isn't covered by Bug 748100. As well as the original problem you brought up in this bug, that is. Why have I not submitted individual patches for each of these, you may be asking. The answer is that most of the above are solved not by changing one thing about the Favicon caching system, but by redoing the Favicon caching system in a better way which has the handy side effect of making the problem go away. Think of this not as the concetention of a large number of fixes, but as a restructuring that makes the original problems cease to exist (In several cases, anyway) As it happens, it was sometime after I'd accidentially fixed a number of these bugs while writing a refactoring patch that was intended for Bug 895423 that I queried the bug database for other Favicon-related problems and decided to address them all. I apologise profusely for getting so spectacularly far from my original objective to enhance the customisation preferences system. In what is liable to come as bad news, this series is a dependency of Bug 895423, which is itself tracking 26. Expedience appreciated.

The usefulness of this becomes more apparent when you consider Bug 895423 (And the rest of this series).

A large, unpleasantly difficult to seperate out patch adding the bulk of the new caching system.

Note the now-nonexistence of a Part 8. See: Bug 913746.

(In reply to Chris Kitching [:ckitching] from comment #26) > The use-FaviconURLs-not-pageURLs-in-the-cache thing is not completely > trivial. If you check out where stuff is calling into the cache, often they > don't HAVE a Favicon URL available. I'm not sure how to solve this. Query > the history database for the Favicon URL and hope for the best? This returns > null surprisingly often. > Any suggestions, or is this really a case of refactoring ALL the things? Disregard this comment - I since worked out how to do it (Although of course feel free to comment on that - just pointing out that, contrary to my earlier thoughts, I have no implemented the change of cache key and associated refactoring. Actually wasn't too bad. > > Also, the problem mentioned in Bug 905685 adds a further layer of > complication. The new about:home's use of Favicons is both extensive and > quite strange. It begins to smell like the true solution to this problem is > to solve Bug 905685, and, while in the process of completely changing how > about:home uses Favicons, have it do use the right sort of URLs. This is essentially 0400 nonsense. > Perhaps a nonterrible plan is to keep the existing very poor pageURL > behvaiour for now and deal with Bug 834536 as a followup. (And add > commenting and suchlike to the new cache to make it clear that it expects > Favicon URLs - we're just abusing it slightly in the meantime) Let's not do that.

(In reply to Chris Kitching [:ckitching] from comment #30) > Created attachment 801203 [details] [diff] [review] > Part 6: V2 - Add more intelligent caching to the Favicons system. > > A large, unpleasantly difficult to seperate out patch adding the bulk of the > new caching system. Is this patch a requirement for the changes you need to make for bug 895423? I ask because this is a pretty big patch, and I don't want us to be blocked on landing it if we don't need to be. Looking through it quickly, it seems like a sensible approach, but I'll need some time to go through it in detail (and maybe get some feedback from bnicholson or wesj as well).

Comment on attachment 801200 [details] [diff] [review] Part 3: V2 - Factor the inner classes of Favicon into their own files. Review of attachment 801200 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, but I have some questions I'd like you to consider before landing this. ::: mobile/android/base/favicons/Favicons.java @@ +31,5 @@ > > private static int sFaviconSmallSize = -1; > private static int sFaviconLargeSize = -1; > > + protected static Context sContext; Why protected not private? @@ +36,2 @@ > > + static final Map<Long,LoadFaviconTask> sLoadTasks = Collections.synchronizedMap(new HashMap<Long, LoadFaviconTask>()); What do you think of making this private, then creating a removeLoadTask(int id) method for LoadFaviconTask to call? I like making member variables private when possible, to make it easier to keep track of where they're used. @@ +162,5 @@ > long taskId = iter.next(); > cancelFaviconLoad(taskId); > } > } > + LoadFaviconTask.closeHTTPClient(); Nice, I like this solution. ::: mobile/android/base/favicons/LoadFaviconTask.java @@ +43,5 @@ > + private String mFaviconUrl; > + private OnFaviconLoadedListener mListener; > + private int mFlags; > + > + static AndroidHttpClient sHttpClient = AndroidHttpClient.newInstance(GeckoAppShell.getGeckoInterface().getDefaultUAString()); We previously delayed initializing this until it was needed. Is there a reason we don't need to do that? @@ +51,5 @@ > + OnFaviconLoadedListener listener) { > + super(backgroundThreadHandler); > + > + synchronized(this) { > + mId = ++mNextFaviconLoadId; I did peek ahead at your next patches, so I'll let you get away with this here, since it's going to be fixed later in the series. @@ +199,5 @@ > + // Note that we don't call the listener callback if the > + // favicon load is cancelled. > + } > + > + public long getId() { This can just be package access. @@ +203,5 @@ > + public long getId() { > + return mId; > + } > + > + public static void closeHTTPClient() { Same here. Actually, this whole class can just be package level access, since it's only used in Favicons.

I realized I never addressed this... (In reply to Chris Kitching [:ckitching] from comment #22) > Created attachment 801063 [details] [diff] [review] > Part 2: V3 - Refactoring Favicons to be static, not a singleton - the > singleton instance was staticly assigned and never cleared. > > (In reply to :Margaret Leibovic from comment #21) > > I just realized that it doesn't look like this method is called from > > anywhere... is that true? If so, we should probably find a place to call > > this, probably somewhere during shutdown. We could do some code archeology > > to see where it was originally introduced, and perhaps see where a call to > > close() might have been removed. > > Eep! Well spotted - IDE didn't highlight that one as unused - apparently too > complicated to work that out. > > We could put this in shutdown - it's probably not actually that harmful > (Although sort of dumb) to just not call it, since resources are implicitly > closed for us on shutdown. We're never going to want to close the Favicon > database in this way at while we're running (Plausibly we might want to > purge the cache contents to save memory, but never close()). > Assuming our ancillary threads are marked as daemons, and we can ensure > database consistency, that is... > > Anyway - it looks like GeckoApp.onDestroy is an appropriate place to put a > call to this. Here's a patch doing so. Anywhere else seem more sensible? It > seems that this call breaks the Favicon system in an expensive to recover > from way - I'm pretty sure we never want to do this except when we're > finally quitting. Looks like that's indeed how it was originally implemented: http://hg.mozilla.org/mozilla-central/rev/f069ffc41471#l2.171 I really wish there was an easy way to find blame for removed lines. Lucas, do you remember ever seeing a patch to remove this call to Favicons.close()? I'm wondering if there was a reason for this removal (meaning we could just get rid of the Favicons.close() method altogether) or if it makes sense to go ahead with this patch Chris wrote to replace it.

Comment on attachment 801201 [details] [diff] [review] Part 4: V2 - LoadFaviconTask's use mNextFaviconLoadId is now thread safe. Review of attachment 801201 [details] [diff] [review]: ----------------------------------------------------------------- Nice catch here.

Comment on attachment 801202 [details] [diff] [review] Part 5: V2 - Make the FaviconView able to resize strangely sized inputs to more appropriate sizes, if enabled. Review of attachment 801202 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good, I just have one main concern about the scaling logic. ::: mobile/android/base/widget/FaviconView.java @@ +27,5 @@ > + // Reference to the unscaled bitmap, if any, to prevent repeated assignments of the same bitmap > + // to the view from causing repeated rescalings (Some of the callers do this) > + private Bitmap mUnscaledbitmap; > + > + private String mIconKey; Add a comment about what this is. @@ +74,5 @@ > + } > + > + if (mScalingExpected && mActualWidth != mIconBitmap.getWidth()) { > + scaleBitmap(); > + mScalingExpected = false; Add a comment explaining why we set this to false. I assume it's so that we only scale the image once, right? @@ +134,4 @@ > * @param bitmap favicon image > * @param key string used as a key to cache the dominant color of this image > */ > + public void updateImage(Bitmap bitmap, String key, boolean aAllowScaling) { Nit: allowScaling (this code doesn't current use the "a" prefix for parameters, and I think we've been moving away from doing that in general). Also, add a @param comment up above explaining this new parameter. @@ +142,5 @@ > + mUnscaledbitmap = bitmap; > + mIconBitmap = bitmap; > + mIconKey = key; > + mScalingExpected = aAllowScaling; > + // Possibly update the display. Nit: Add a blank line above this comment for better readability. @@ +150,2 @@ > public void updateImage(Bitmap bitmap, String key) { > + updateImage(bitmap, key, false); This makes me think of: http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html I only see you calling this 3-parameter updateImage here, so can't we just initialize mScalingExpected to false? Although this raises another issue that we've never actually calling scaleBitmap() in formatImage. Are you planning to use your new updateImage method in a future patch? I don't see it in the other patches you uploaded to this bug. If we don't need this code path at the moment, we should just get rid of it. And if we do need it, I would like for us to think if there's a better way to do this than a boolean paramter.

Comment on attachment 801204 [details] [diff] [review] Part 6: V2 - Prevent a corrupted Favicon from causing us possibly to leak an unclosed InputStream. Review of attachment 801204 [details] [diff] [review]: ----------------------------------------------------------------- Nice catch. Also nice small patch :)

Comment on attachment 801203 [details] [diff] [review] Part 6: V2 - Add more intelligent caching to the Favicons system. Let's also ask bnicholson to look at this, since it's a pretty complex patch.

(In reply to :Margaret Leibovic from comment #33) > Is this patch a requirement for the changes you need to make for bug 895423? > I ask because this is a pretty big patch, and I don't want us to be blocked > on landing it if we don't need to be. The patch for Bug 895423 can be landed provided parts 1-5 of this patch are in place, but will only be partially functional. Search engine icons for non-default engines will remain unreasonably small (Because the smallness of such icons is caused by either unwise scaling in the old caching system or Bug 748100 (Which itself depends on the entirety of this patch series)) The result would be: https://www.dropbox.com/s/gu56iv6bhr3mqje/PrefWithoutNewCache.png Instead of: https://www.dropbox.com/s/dmdapt411l5agr4/PrefAfter.png It is for this reason that I did all this Favicon stuff in the first place, incidentially. > Looking through it quickly, it seems like a sensible approach, but I'll need > some time to go through it in detail (and maybe get some feedback from > bnicholson or wesj as well). Aye, it is quite a hefty one - fixes a consideable number of problems, though. It would be extremely excellent if it could get reviewed - the UI improvement of this in conjunction with Bug 748100 is nontrivial. (I guess I already excitedly showed you: https://www.dropbox.com/s/dd8aj12yektrah3/NvidiaFix.png Vs: https://www.dropbox.com/s/h2u6wh1xtspdggg/NightlyHistory.png ) I understand that the amount of work involved is also nontrivial - let me know if there's anything I can do to help out on that front.

Comment on attachment 801203 [details] [diff] [review] Part 6: V2 - Add more intelligent caching to the Favicons system. Splitting bugs, as requested. Moved to Bug 914296.

(In reply to :Margaret Leibovic from comment #34) > This looks good, but I have some questions I'd like you to consider before > landing this. > > ::: mobile/android/base/favicons/Favicons.java > @@ +31,5 @@ > > > > private static int sFaviconSmallSize = -1; > > private static int sFaviconLargeSize = -1; > > > > + protected static Context sContext; > > Why protected not private? LoadFaviconTask uses sContext to access the ContentResolver to talk to the database. > ::: mobile/android/base/favicons/LoadFaviconTask.java > @@ +43,5 @@ > > + private String mFaviconUrl; > > + private OnFaviconLoadedListener mListener; > > + private int mFlags; > > + > > + static AndroidHttpClient sHttpClient = AndroidHttpClient.newInstance(GeckoAppShell.getGeckoInterface().getDefaultUAString()); > > We previously delayed initializing this until it was needed. Is there a > reason we don't need to do that? My thinking was that the cost is trivial, and it removes a chunk of irritating boilerplate (The whole null check and create a new instance if null business is a pain)

Unbitrotting after changes to previous patch.

(In reply to :Margaret Leibovic from comment #37) > @@ +150,2 @@ > > public void updateImage(Bitmap bitmap, String key) { > > + updateImage(bitmap, key, false); > > This makes me think of: > http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html > > I only see you calling this 3-parameter updateImage here, so can't we just > initialize mScalingExpected to false? Although this raises another issue > that we've never actually calling scaleBitmap() in formatImage. > > Are you planning to use your new updateImage method in a future patch? I > don't see it in the other patches you uploaded to this bug. If we don't need > this code path at the moment, we should just get rid of it. And if we do > need it, I would like for us to think if there's a better way to do this > than a boolean paramter. Actually, the 3-parameter version is the "new" one. The two-parameter version retains the signature of the original FaviconView's updateImage function, and has comparable semantics (No scaling, just display). Currently, it is used by home/SearchEngineRow.java:184 and home/TwoLinePageRow.java:112. The thinking behind this is that, generally, we don't want the scaling logic to be used. It does have valid use-cases, but we should prefer the Favicon caching system's scaling be used (It caches the results.) It seemed bad practice to have FaviconView talk to the cache directly, and there are times (eg. Search preferences screen) where we want to display in a FaviconView things we've obtained in other ways. Plus, at this point in the patch series, the new Favicon cache doesn't actually exist. The boolean defaulting to false was simply to ensure that the behaviour of the FaviconView doesn't change for places using it the old way. Cluttering up their calls with an explicit boolean flag seemed untidy. Open to suggestions on how to improve it - add a new updateImageWithScaling method? Rename the two-arg version to "updateImageWithoutScaling" or similar?

(In reply to Chris Kitching [:ckitching] from comment #44) > (In reply to :Margaret Leibovic from comment #37) > > @@ +150,2 @@ > > > public void updateImage(Bitmap bitmap, String key) { > > > + updateImage(bitmap, key, false); > > > > This makes me think of: > > http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html > > > > I only see you calling this 3-parameter updateImage here, so can't we just > > initialize mScalingExpected to false? Although this raises another issue > > that we've never actually calling scaleBitmap() in formatImage. > > > > Are you planning to use your new updateImage method in a future patch? I > > don't see it in the other patches you uploaded to this bug. If we don't need > > this code path at the moment, we should just get rid of it. And if we do > > need it, I would like for us to think if there's a better way to do this > > than a boolean paramter. > > Actually, the 3-parameter version is the "new" one. The two-parameter > version retains the signature of the original FaviconView's updateImage > function, and has comparable semantics (No scaling, just display). > Currently, it is used by home/SearchEngineRow.java:184 and > home/TwoLinePageRow.java:112. Where is this new version used though? If it's not used, we shouldn't include it until it needs to be used somewhere.

(In reply to :Margaret Leibovic from comment #45) > Where is this new version used though? If it's not used, we shouldn't > include it until it needs to be used somewhere. The new version is used in both the former part 6 of this bug and in the patch for Bug 895423. The idea of this patch was to solve the general problem of making the Favicon view more clever. I can move this change into one of the other patches if you insist, but it'll create a dependency between them (The one not containing this change will depend on the one containing this change). As it stands, they can be processed concurrently, or in either order.

(In reply to :Margaret Leibovic from comment #34) > Actually, this whole class can just be package level access, since it's only > used in Favicons. My mind appears to have taken a minor holiday and omitted to address this - this is not correct - the flag fields are referenced from callers in other packages, so in fact the current version of the patch won't compile for this reason - should the flags be shifted into Favicons.java, or the package made public again?

(In reply to Chris Kitching [:ckitching] from comment #47) > My mind appears to have taken a minor holiday and omitted to address this - > this is not correct - the flag fields are referenced from callers in other > packages, so in fact the current version of the patch won't compile for this > reason - should the flags be shifted into Favicons.java, or the package made > public again? I just discovered static imports. Problem solved.

Unbitrot.

Unbitrot.

More unbitrot.

Even more unbitrotting.

Unbitrot, and eliminated the evil boolean flag. There are now two methods, "updateImage" and "updateAndScaleImage". As of this patch, updateAndScaleImage is not actually used - as I mentioned before, moving this into either Bug 914296 or Bug 895423 (The users of this method) is discouraged, as it would create a dependancy between these two bugs.

Correcting the commit message.

(In reply to :Margaret Leibovic from comment #35) > I realized I never addressed this... > > (In reply to Chris Kitching [:ckitching] from comment #22) > > Created attachment 801063 [details] [diff] [review] > > Part 2: V3 - Refactoring Favicons to be static, not a singleton - the > > singleton instance was staticly assigned and never cleared. > > > > (In reply to :Margaret Leibovic from comment #21) > > > I just realized that it doesn't look like this method is called from > > > anywhere... is that true? If so, we should probably find a place to call > > > this, probably somewhere during shutdown. We could do some code archeology > > > to see where it was originally introduced, and perhaps see where a call to > > > close() might have been removed. > > > > Eep! Well spotted - IDE didn't highlight that one as unused - apparently too > > complicated to work that out. > > > > We could put this in shutdown - it's probably not actually that harmful > > (Although sort of dumb) to just not call it, since resources are implicitly > > closed for us on shutdown. We're never going to want to close the Favicon > > database in this way at while we're running (Plausibly we might want to > > purge the cache contents to save memory, but never close()). > > Assuming our ancillary threads are marked as daemons, and we can ensure > > database consistency, that is... > > > > Anyway - it looks like GeckoApp.onDestroy is an appropriate place to put a > > call to this. Here's a patch doing so. Anywhere else seem more sensible? It > > seems that this call breaks the Favicon system in an expensive to recover > > from way - I'm pretty sure we never want to do this except when we're > > finally quitting. > > Looks like that's indeed how it was originally implemented: > http://hg.mozilla.org/mozilla-central/rev/f069ffc41471#l2.171 > > I really wish there was an easy way to find blame for removed lines. > > Lucas, do you remember ever seeing a patch to remove this call to > Favicons.close()? I'm wondering if there was a reason for this removal > (meaning we could just get rid of the Favicons.close() method altogether) or > if it makes sense to go ahead with this patch Chris wrote to replace it. Hard to tell why this call was removed. The original intent of the close() back when I originally wrote this code was to close the db connection is held for the favicon-specific database that we don't use anymore. Then it morphed into a method to cancel all pending favicon loading tasks. My (wild) guess is that this call was removed when we got rid of the favicon-specific database (bnicholson maybe?). But then someone (probably me) added more code to the close() method assuming that it was still being called. So, yes, I'd restore this call in onDestroy().

Comment on attachment 802066 [details] [diff] [review] Part 3: V4 - Factor the inner classes of Favicon into their own files. Review of attachment 802066 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/favicons/Favicons.java @@ +201,5 @@ > sFaviconLargeSize = Math.round(sContext.getResources().getDimension(R.dimen.favicon_size_large)); > } > } > > + public static void removeLoadTask(long aTaskId) { Nit: Just call this parameter taskId.

Comment on attachment 802074 [details] [diff] [review] Part 5: V3 - Make the FaviconView able to resize strangely sized inputs to more appropriate sizes, if enabled. Review of attachment 802074 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for addressing my feedback. Lots of nice comments in this patch, too!

Addressing nit.

Applying the requested naming convention change.

Applying the requested naming convention change.

Applying the requested naming convention change.

Applying the requested naming convention change.

Some cleanup, and sorting out the naming convention again. I'm getting oranges on try when I push the entire favicons ensemble. I believe this is caused by the patch in Bug 914296 - will have more info soon. In any case, don't checkin-needed-ify this yet - just in case it turns out to be a problem in here.

Rebasing.

Awaiting try run return to mark for checkin, then. Yay!

https://tbpl.mozilla.org/?tree=Try&rev=55921c95ee54 Victory.

https://hg.mozilla.org/integration/fx-team/rev/8e02a09e5ec5 https://hg.mozilla.org/integration/fx-team/rev/26e40024ee85 https://hg.mozilla.org/integration/fx-team/rev/856365a2e58b https://hg.mozilla.org/integration/fx-team/rev/84992b91dc39 https://hg.mozilla.org/integration/fx-team/rev/1fe12c9597f4 https://hg.mozilla.org/integration/fx-team/rev/0b9155aa5c76

https://hg.mozilla.org/mozilla-central/rev/8e02a09e5ec5 https://hg.mozilla.org/mozilla-central/rev/26e40024ee85 https://hg.mozilla.org/mozilla-central/rev/856365a2e58b https://hg.mozilla.org/mozilla-central/rev/84992b91dc39 https://hg.mozilla.org/mozilla-central/rev/1fe12c9597f4 https://hg.mozilla.org/mozilla-central/rev/0b9155aa5c76