Show favicons if there is no thumbnail available for TopBookmarksView

RESOLVED FIXED in Firefox 26

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: sriram, Assigned: sriram)

Tracking

unspecified
Firefox 26
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-fig)

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
We might not have thumbnails for few views in TopBookmarksView. Better show favicons in such cases.
(Assignee)

Updated

6 years ago
Blocks: 882365
(Assignee)

Comment 1

6 years ago
Created attachment 766160 [details] [diff] [review]
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.
Attachment #766160 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 2

6 years ago
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+
(Assignee)

Comment 4

6 years ago
(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
(Assignee)

Comment 5

6 years ago
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

Updated

6 years ago
Duplicate of this bug: 837392

Comment 7

5 years ago
https://hg.mozilla.org/mozilla-central/rev/3d60ab3e61b6
https://hg.mozilla.org/mozilla-central/rev/ce84e7c8901c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.