885519 - Show favicons if there is no thumbnail available for TopBookmarksView

Status: RESOLVED FIXED

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

Description

[Sriram Ramasubramanian [:sriram]]

We might not have thumbnails for few views in TopBookmarksView. Better show favicons in such cases.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This queries the db for thumbnails. It populates the map with thumbnails.
And then from the list of urls, if there was no entry added previously in the map, it tries to look for favicon. If one's available, that's added. Since favicons can even be 256x256, we cannot identify whether the bitmap is a thumbnail or favicon while updating. Hence changing the map to hold this value through a class Thumbnail.

Comment 2

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 766160 [details] [diff] [review]
Patch

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

::: mobile/android/base/home/TopBookmarksView.java
@@ +365,5 @@
>  
> +            final Map<String, Thumbnail> thumbnails = new HashMap<String, Thumbnail>();
> +
> +            // Query the DB for thumbnails.
> +            final ContentResolver cr = getContext().getContentResolver();

I might not need this statement. I'll remove it.

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 766160 [details] [diff] [review]
Patch

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

Nice.

::: mobile/android/base/home/TopBookmarksView.java
@@ +387,5 @@
>                      cursor.close();
>                  }
>              }
>  
> +            // Query the DB for Favicons.

Query the DB for favicons for the urls without thumbnails

@@ +392,5 @@
> +            for (String url : urls) {
> +                if (!thumbnails.containsKey(url)) {
> +                    final Bitmap bitmap = BrowserDB.getFaviconForUrl(cr, url);
> +                    if (bitmap != null) {
> +                        thumbnails.put(url, new Thumbnail(bitmap, true));

This should be false, no?

Comment 4

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #3)
> Comment on attachment 766160 [details] [diff] [review]
> Patch
> 
> Review of attachment 766160 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice.
> @@ +392,5 @@
> > +            for (String url : urls) {
> > +                if (!thumbnails.containsKey(url)) {
> > +                    final Bitmap bitmap = BrowserDB.getFaviconForUrl(cr, url);
> > +                    if (bitmap != null) {
> > +                        thumbnails.put(url, new Thumbnail(bitmap, true));
> 
> This should be false, no?

Unfortunately, if there's no favicon, we return null from DB.

http://hg.mozilla.org/projects/fig/rev/3d60ab3e61b6

Comment 5

[Sriram Ramasubramanian [:sriram]]

Oops. I thought the comment was for bitmap. Didn't realize the mistake in setting true for both. Changed it: http://hg.mozilla.org/projects/fig/rev/ce84e7c8901c

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3d60ab3e61b6
https://hg.mozilla.org/mozilla-central/rev/ce84e7c8901c