913746 - Make the Favicons bundled for search engines avilable to the Favicon cache.

Status: RESOLVED DUPLICATE of bug 961600

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Chris Kitching [:ckitching]]

Followup to Bug 888326.

Comment 1

[Chris Kitching [:ckitching]]

Attachment: Add the Favicons bundled with the search system to the cache.

Here's a patch - adds the search engine icons to the cache when they are sent up from Gecko. Saves a bit of traffic and gives us high resolution Favicons right away.

Comment 2

[Chris Kitching [:ckitching]]

Attachment: V2 - Use new FaviconView API - Add the Favicons bundled with the search system to the cache.

Unbitrotting.

Comment 3

[:Margaret Leibovic]

Comment on attachment 802109 [details] [diff] [review]
V2 - Use new FaviconView API - Add the Favicons bundled with the search system to the cache.

Review of attachment 802109 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/home/BrowserSearch.java
@@ +405,5 @@
>                  final String iconURI = engineJSON.getString("iconURI");
>                  final Bitmap icon = BitmapUtils.getBitmapFromDataURI(iconURI);
>  
> +                // Pilfer the Favicon and put it in the memcache for later use.
> +                final String searchURL = engineJSON.getString("searchForm");

I had to look up what the searchForm is and why it would make sense to use it as a key for the favicon cache, so if we go with this patch, we should add a comment explaining what searchForm is and why we're using it.

However, I'm hesitant to make this change because it assumes the icon we're shipping in the search plugin is the same as the icon used on the website. While true in practice, our search providers can update their favicons faster than we can (this has happened for both Google and Twitter in recent history), and it feels wrong to always load an icon bundled in our app instead of the one hosted on their website.

I need to think about this more.

Comment 4

[Chris Kitching [:ckitching]]

(In reply to :Margaret Leibovic from comment #3)
> I had to look up what the searchForm is and why it would make sense to use
> it as a key for the favicon cache, so if we go with this patch, we should
> add a comment explaining what searchForm is and why we're using it.
> 
> However, I'm hesitant to make this change because it assumes the icon we're
> shipping in the search plugin is the same as the icon used on the website.
> While true in practice, our search providers can update their favicons
> faster than we can (this has happened for both Google and Twitter in recent
> history), and it feels wrong to always load an icon bundled in our app
> instead of the one hosted on their website.
> 
> I need to think about this more.

True. I also discovered this morning that, in the case of Amazon.com at least, the bundled icon is lower resolution than the icon they serve us.

What might make maximal sense is for us to stop bundling search engine icons in this way (Which mandates sending them over the bridge in base64 format anyway).
Instead, how about we bundle icons as favicons in the local favicon database (In the same format currently used by the caching system when it stores a downloaded favicon to the database for later reuse).
The advantages here are threefold:

1) No more need to send base64-encoded bitmaps around to get the icons for search engines - instead send a favicon URL value (Which is used as the key to both the in-memory favicon cache and the on-daatabse one).
2) We can update the stored icon when we visit the page. (For instance, Amazon.com's bundled icon is lower quality than the one they serve up). This also means when a site changes its logo we won't be stuck with the old version until we can ship a new version.
3) No need for the search preferences code to do special magic to get the icon - it can just request the icon for the appropriate URL from the underlying favicon system and the rest will be worked out. Heck, we could even just not ship Favicons at all and download them on demand.

Comment 5

[Chris Kitching [:ckitching]]

Also - This bug was created before I came across and fixed Bug 748100 - at that time, the bundled icons for the search engines were much better than the ones that were being served up by the Favicon system. (It turns out that's just because we were decoding the tiny icons when large ones were available - not that the icons being served from the sites were particularly small).
With the patch for Bug 748100 applied, all the user-visible gains I was seeing from using this patch are already present. The additional gains of this patch are now solely neatness, plus a slight performance gain (No need to download the Favicons for these bundled sites or send them base64-encoded through the bridge when you open the address bar or settings menu. (The former might actually be worthwhile)).
I never did profile the performance of address-bar-opening. I suspect this base64-decoding is significant (Unless Shilpan applied his solution to sending blobs more sensibly to this alrerady)
While it'd be nice to have one truly unified icon management system, by the sounds of it the rules about bundled icons are strange and scary, so the technically unncessary special-casing of these icons seems to be made essential by that particular weirdness.
Oh well. Not really a big deal. Just makes the solution to Bug 895423 slightly less trivial than it could be.

Comment 6

[:Margaret Leibovic]

Comment on attachment 802109 [details] [diff] [review]
V2 - Use new FaviconView API - Add the Favicons bundled with the search system to the cache.

Review of attachment 802109 [details] [diff] [review]:
-----------------------------------------------------------------

After our discussion on IRC yesterday, and thinking about this more, I don't think we should do this.

Let's just focus on the cache improvements right now.