961499 - Regression: Home-Screen shortcuts added from top-site thumbnails merely show the common bitmap colour and not an actual-site favicon

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: Screen Shot 2014-01-19 at 12.52.49 PM.png

This looks terrible.

Currently, added shortcuts don't actually use a site-favicon, they use a common colour bitmap.

Steps:

i) http://microsoft.com
ii) Visit Top-Site and add it to one of the available positions
iii) Long-tap and add to home-screen

See screenshot.

Expected: Same nice looking Microsoft favicon as if I long-tapped and added to home-screen from the URL bar.

Actual: Bitmap common colour icon.

--
Nightly (01/19)
LG Nexus 4 (Android 4.4.2)

Comment 1

[Aaron Train [:aaronmt]]

Also reproducible with any of the listings in bookmarks, history, reading-list, etc.

Comment 2

[Richard Newman [:rnewman]]

Related to Bug 958399?

Comment 3

[Aaron Train [:aaronmt]]

Still reproducible 01/31 Nightly

Comment 4

[:Margaret Leibovic]

We should figure out what caused this.

Comment 5

[Aaron Train [:aaronmt]]

Bug 748100

Comment 6

[Richard Newman [:rnewman]]

This is perhaps the problem:

    static void createShortcut(String aTitle, String aURI, String aUniqueURI, String aIconData, String aType) {
        createShortcut(aTitle, aURI, aUniqueURI, BitmapUtils.getBitmapFromDataURI(aIconData), aType);
    }

I found this by simply grepping for BitmapUtils and walking through the results.

*Nothing should ever be manually creating a favicon by calling BitmapUtils*. That's because *that string is not a PNG*.

Replace that call with "ask the icon decoder for the right favicon", or (better) reword this method to drop that argument and ask the icon cache for the icon for this page, and things will probably magically begin to work.

Comment 7

[Richard Newman [:rnewman]]

Here's the error.

    public static void getLargestFaviconForPage(String url, OnFaviconLoadedListener onFaviconLoadedListener) {
        Log.i(LOGTAG, "Fetching largest favicon for " + url);
        loadUncachedFavicon(url, null, 0, -1, onFaviconLoadedListener);
    }


Firstly, this should use GeckoAppShell.getPreferredIconSize.

Secondly, -1 here is resulting in this:



02-04 15:56:59.597 I/GeckoFavicons( 7153): Getting sized: http://www.microsoft.com/favicon.ico?v2, -1
02-04 15:56:59.597 I/FaviconCache( 7153): getFaviconForDimensions: http://www.microsoft.com/favicon.ico?v2, -1
02-04 15:56:59.597 I/LoadFaviconTask( 7153): android.graphics.Bitmap@2135b8b0, -1
02-04 15:56:59.597 I/LoadFaviconTask( 7153): processResult: 1x1


I found and fixed a bunch of other bugs along the way, so this'll be a nice little bundle.

Comment 8

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #7)
> Firstly, this should use GeckoAppShell.getPreferredIconSize.

It certainly shouldn't use -1 - but are you sure getPreferredIconSize is right?

getPreferredIconSize returns the largest size we care about for use in the UI in Fennec (I think?). That seems like it's not the same thing as the size that you want for a desktop shortcut (Which surely depends on the launcher you're using and various other things, and could essentially be *anything*).

So - isn't the correct behaviour here to pass Integer.MAX_VALUE, give Android the biggest image we possibly can, and have their icon-creation logic sort out the mess?

Comment 9

[Wesley Johnston (:wesj)]

getPreferredIconSize gets the launcher icon size (when its available):

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#855

Feel free to rename it if you want.

Comment 10

[Richard Newman [:rnewman]]

Attachment: Return correct size favicons for Top Sites' add to home screen functionality. v1

This patch does the following:

* Moves the data URI -> Bitmap logic from SearchEnginePreference to FaviconDecoder.getMostSuitableBitmapFromDataURI.
* Cleans up GeckoAppShell.createShortcut to allow for bitmap decoding to be done on the background thread.
* Changes getLargestFaviconForPage into getPreferredSizeFaviconForPage, using GeckoAppShell's getPreferredIconSize method to decide on size.
* Fixes the webapp EventListener to parse data: URIs correctly, to ask for the preferred icon size.
* Parses icon data: URIs correctly in all createShortcut call paths.
* Fixes FaviconCache's "-1" logic for picking the largest primary. (Tested before applying the previous change, naturally.)
* Fixes the docs for nsIShellService.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8370458 [details] [diff] [review]
Return correct size favicons for Top Sites' add to home screen functionality. v1

Feel free to punt to someone else if you think they're a better sucker/reviewer. :P

Comment 12

[Brian Nicholson (:bnicholson)]

Comment on attachment 8370458 [details] [diff] [review]
Return correct size favicons for Top Sites' add to home screen functionality. v1

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

Looks good to me!

Comment 13

[Richard Newman [:rnewman]]

https://hg.mozilla.org/services/services-central/rev/90fe71702839

Thanks for the quick review, Brian!

Comment 14

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-central/rev/90fe71702839

Comment 15

[Richard Newman [:rnewman]]

Comment on attachment 8370458 [details] [diff] [review]
Return correct size favicons for Top Sites' add to home screen functionality. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
  Favicon cache work.

User impact if declined: 
  Adding a site to the home screen from Top Sites will result in a blank single-color favicon. Also fixes other bugs around passing favicons from Gecko to Java, which might affect web apps.

Testing completed (on m-c, etc.): 
  Tested locally. Landed on m-c yesterday.

Risk to taking this patch (and alternatives if risky): 
  Should be slim -- slim enough that I'd consider porting part of this fix to Beta.

String or IDL/UUID changes made by this patch:
  Makes a comment-only change to mobile/android/components/build/nsIShellService.idl.

Comment 16

[Ioana Chiorean]

Build:30.0a1 02/06 
I am not able to reproduce this with latest nightly. Marking the flag as verified.

I would love to see this is Aurora too as it looks pretty bad and we just entered the train.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/606cf23cae6d