Closed Bug 885519 Opened 8 years ago Closed 8 years ago

Show favicons if there is no thumbnail available for TopBookmarksView

Categories

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

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: sriram, Assigned: sriram)

References

Details

(Whiteboard: fixed-fig)

Attachments

(1 file)

We might not have thumbnails for few views in TopBookmarksView. Better show favicons in such cases.
Blocks: 882365
Attached patch PatchSplinter Review
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.
Attachment #766160 - Flags: review?(lucasr.at.mozilla)
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 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?
Attachment #766160 - Flags: review?(lucasr.at.mozilla) → review+
(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
Assignee: nobody → sriram
Whiteboard: fixed-fig
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
Duplicate of this bug: 837392
https://hg.mozilla.org/mozilla-central/rev/3d60ab3e61b6
https://hg.mozilla.org/mozilla-central/rev/ce84e7c8901c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.