Closed Bug 961499 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

29 Branch
All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 + fixed
firefox30 --- verified
fennec 29+ ---

People

(Reporter: aaronmt, Assigned: rnewman)

References

Details

(Keywords: regression, reproducible, Whiteboard: [qa+])

Attachments

(2 files)

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)
Blocks: ICODecoder
Also reproducible with any of the listings in bookmarks, history, reading-list, etc.
Related to Bug 958399?
tracking-fennec: ? → 29+
Version: Trunk → Firefox 29
Still reproducible 01/31 Nightly
We should figure out what caused this.
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.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
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.
(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?
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.
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 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
Attachment #8370458 - Flags: review?(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!
Attachment #8370458 - Flags: review?(bnicholson) → review+
https://hg.mozilla.org/services/services-central/rev/90fe71702839

Thanks for the quick review, Brian!
Whiteboard: [fixed in services][qa+]
https://hg.mozilla.org/mozilla-central/rev/90fe71702839
Flags: needinfo?(rnewman)
Whiteboard: [fixed in services][qa+] → [qa+]
Target Milestone: --- → Firefox 30
Flags: needinfo?(rnewman)
Hardware: ARM → All
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.
Attachment #8370458 - Flags: approval-mozilla-aurora?
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.
Attachment #8370458 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.