897772 - [fig] "Add to Home Screen" shortcut doesn't have an icon

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[:Margaret Leibovic]

In HomeContextMenuInfo, we're assuming the cursor always has a favicon, and using that to create the homescreen shortcut. However, the cursor doesn't always have a favicon.

Comment 1

[:Margaret Leibovic]

Attachment: patch

Now that we use FaviconsLoader to lazily load the favicons for all the HomeListViews, we never have a Combined.FAVICONS column in the cursor for the list. Luckily, because FaviconsLoader caches the images, we can just pull them out of the cache (since this is a context menu, we know that the image must be loaded for the item the user is long tapping on).

Comment 2

[Wesley Johnston (:wesj)]

Comment on attachment 789286 [details] [diff] [review]
patch

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

::: mobile/android/base/home/HomeFragment.java
@@ +112,5 @@
>                      break;
>                  }
>  
> +                // The favicon should be in the cache because we're using a FavionsLoader to load the icon.
> +                Bitmap bitmap = Favicons.getInstance().getFaviconFromMemCache(info.url);

Is there a reason we wouldn't want to call Favicons.loadFavicon() and pass in the callback here instead?

Comment 3

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #2)
> Comment on attachment 789286 [details] [diff] [review]
> patch
> 
> Review of attachment 789286 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomeFragment.java
> @@ +112,5 @@
> >                      break;
> >                  }
> >  
> > +                // The favicon should be in the cache because we're using a FavionsLoader to load the icon.
> > +                Bitmap bitmap = Favicons.getInstance().getFaviconFromMemCache(info.url);
> 
> Is there a reason we wouldn't want to call Favicons.loadFavicon() and pass
> in the callback here instead?

To do that, we'd need to get the favicon URL (right now we only have the page URL). Favicons does have a method to do that, but it has to query the DB. If we're confident that the image will be in the cache, I think it would be smarter to save ourselves from this extra work.

The cache is 1MB:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Favicons.java#69

So we would need to have loaded over 1MB worth of favicons since the icon in the menu item was last loaded. Since the icon will be drawn in the row that triggers the context menu, I am pretty confident it will be in the cache. Do you see a case where it might not be?

Comment 4

[:Margaret Leibovic]

lucar's comment about the cache in bug 905685 makes me worried that this approach isn't actually as robust as I had hoped. However, maybe it will be once that bug is fixed.

Comment 5

[:Margaret Leibovic]

Updating summary, since this always happens now.

Comment 6

[:Margaret Leibovic]

Attachment: alternate patch

This actually goes through the loadFavicon dance to get the favicon. I decided to create a AddToLauncherTask class, to match the pattern we're already using for RemoveBookmarkTask and RemoveHistoryTask.

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 791483 [details] [diff] [review]
alternate patch

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

::: mobile/android/base/home/HomeFragment.java
@@ -112,5 @@
> -            // FIXME: bug 897772
> -            Bitmap bitmap = null;
> -            if (info.favicon != null) {
> -                bitmap = BitmapUtils.decodeByteArray(info.favicon);
> -            }

I wonder if its worth leaving this in and fast pathing if we can? i.e.

if (info.favicon == null) {
  new AddToLauncherTask().execute();
  return true;
}

Comment 8

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #7)
> Comment on attachment 791483 [details] [diff] [review]
> alternate patch
> 
> Review of attachment 791483 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomeFragment.java
> @@ -112,5 @@
> > -            // FIXME: bug 897772
> > -            Bitmap bitmap = null;
> > -            if (info.favicon != null) {
> > -                bitmap = BitmapUtils.decodeByteArray(info.favicon);
> > -            }
> 
> I wonder if its worth leaving this in and fast pathing if we can? i.e.
> 
> if (info.favicon == null) {
>   new AddToLauncherTask().execute();
>   return true;
> }

I'm not sure what you're getting at here... info.favicon is always null now, since the cursors we're using never have favicons stored in them, so there's not really any point to us using that for a check.

Comment 9

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/26b37a40d65f

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/26b37a40d65f