893990 - BookmarksPage should use FaviconsLoader

Status: RESOLVED FIXED

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

Description

[Sriram Ramasubramanian [:sriram]]

Currently the ListView in BookmarksPage uses the Favicon data from the cursor. This takes quite a while to parse and show, if the image is bigger. We also have FaviconsLoader that stores the icons in a memory cache. It's better to use that with the BookmarksPage (it has the advantage that favicons will already be in memory, if some other page had requested it).

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Uses the same code from VisitedPage.

Comment 2

[:Margaret Leibovic]

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

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

This looks good to me, but I think we'll need a follow-up bug for ReadingListPage, since that uses getBookmarksInFolder to get its entries... However, when I just tested this now it worked. Looking at TwoLinePageRow.updateFromCursor, I was wondering if that could be just because it's getting the icon from the cache, but I added some logging to check and found that it's not. Do you know how this could be working?

As an aside, I think it would be worthwhile to make a wiki page documenting how we use CursorLoaders and how all this homepage logic is architected, if only to convince ourselves it's well designed :)

::: mobile/android/base/db/LocalBrowserDB.java
@@ +69,5 @@
>                             Bookmarks.URL,
>                             Bookmarks.TITLE,
>                             Bookmarks.TYPE,
>                             Bookmarks.PARENT,
> +                           Bookmarks.KEYWORD }; 

Trailing whitespace.

Also, it doesn't look like we use the KEYWORD column anywhere. It could be a good follow-up patch to clean that up.

::: mobile/android/base/home/BookmarksPage.java
@@ +46,5 @@
>      // Cursor loader ID for grid of bookmarks.
>      private static final int TOP_BOOKMARKS_LOADER_ID = 1;
>  
> +    // Loader ID for favicons.
> +    private static final int FAVICONS_LOADER_ID = 2; 

Trailing whitespace.

@@ +235,5 @@
>              switch(loaderId) {
>                  case BOOKMARKS_LIST_LOADER_ID: {
>                      mListAdapter.swapCursor(c);
> +                    FaviconsLoader.restartFromCursor(getLoaderManager(), FAVICONS_LOADER_ID,
> +                            mLoaderCallbacks, c);

In VisitedPage this variable is called mCursorLoaderCallbacks. Maybe it would be a nice follow-up to make these names consistent.

Comment 3

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/projects/fig/rev/cc50c0290cf4

Comment 4

[Sriram Ramasubramanian [:sriram]]

(In reply to :Margaret Leibovic from comment #2)
> Comment on attachment 775867 [details] [diff] [review]
> Patch
> 
> Review of attachment 775867 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me, but I think we'll need a follow-up bug for
> ReadingListPage, since that uses getBookmarksInFolder to get its entries...
> However, when I just tested this now it worked. Looking at
> TwoLinePageRow.updateFromCursor, I was wondering if that could be just
> because it's getting the icon from the cache, but I added some logging to
> check and found that it's not. Do you know how this could be working?

That's because we currently load all pages in about:home. This causes visited page to load -- which loads the favicons to the memory cache. So when reading list is shown, it tries to get it from memory.

> 
> As an aside, I think it would be worthwhile to make a wiki page documenting
> how we use CursorLoaders and how all this homepage logic is architected, if
> only to convince ourselves it's well designed :)

I wish I could write so well to write wiki pages ;) That's what is stopping me from documenting such nice stuff.

> 
> ::: mobile/android/base/db/LocalBrowserDB.java
> @@ +69,5 @@
> >                             Bookmarks.URL,
> >                             Bookmarks.TITLE,
> >                             Bookmarks.TYPE,
> >                             Bookmarks.PARENT,
> > +                           Bookmarks.KEYWORD }; 
> 
> Trailing whitespace.
> 
> Also, it doesn't look like we use the KEYWORD column anywhere. It could be a
> good follow-up patch to clean that up.

I can write a followup for this.

> 
> @@ +235,5 @@
> >              switch(loaderId) {
> >                  case BOOKMARKS_LIST_LOADER_ID: {
> >                      mListAdapter.swapCursor(c);
> > +                    FaviconsLoader.restartFromCursor(getLoaderManager(), FAVICONS_LOADER_ID,
> > +                            mLoaderCallbacks, c);
> 
> In VisitedPage this variable is called mCursorLoaderCallbacks. Maybe it
> would be a nice follow-up to make these names consistent.

I found the name to be super long and started using mLoaderCallbacks. May be we should move to this shorter name as anyways the loaders are usually based on cursors :P

Comment 5

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #4)
> (In reply to :Margaret Leibovic from comment #2)
> > Comment on attachment 775867 [details] [diff] [review]
> > Patch
> > 
> > Review of attachment 775867 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks good to me, but I think we'll need a follow-up bug for
> > ReadingListPage, since that uses getBookmarksInFolder to get its entries...
> > However, when I just tested this now it worked. Looking at
> > TwoLinePageRow.updateFromCursor, I was wondering if that could be just
> > because it's getting the icon from the cache, but I added some logging to
> > check and found that it's not. Do you know how this could be working?
> 
> That's because we currently load all pages in about:home. This causes
> visited page to load -- which loads the favicons to the memory cache. So
> when reading list is shown, it tries to get it from memory.

Yeah, I was confused here, my logging does indicate that we're getting the favicon from the cache, and that's why it's working. So we should file this follow-up bug to fix the case where the favicon isn't in the cache, since this is depending on the fact that the icon loaded on the visited page (I would certainly hope we aren't actually loading icons for *all* pages whenever about:home is opened, I think it's just working here because I only have a few entries in history).

> > As an aside, I think it would be worthwhile to make a wiki page documenting
> > how we use CursorLoaders and how all this homepage logic is architected, if
> > only to convince ourselves it's well designed :)
> 
> I wish I could write so well to write wiki pages ;) That's what is stopping
> me from documenting such nice stuff.

That's a bad excuse!

> > @@ +235,5 @@
> > >              switch(loaderId) {
> > >                  case BOOKMARKS_LIST_LOADER_ID: {
> > >                      mListAdapter.swapCursor(c);
> > > +                    FaviconsLoader.restartFromCursor(getLoaderManager(), FAVICONS_LOADER_ID,
> > > +                            mLoaderCallbacks, c);
> > 
> > In VisitedPage this variable is called mCursorLoaderCallbacks. Maybe it
> > would be a nice follow-up to make these names consistent.
> 
> I found the name to be super long and started using mLoaderCallbacks. May be
> we should move to this shorter name as anyways the loaders are usually based
> on cursors :P

Yeah, I think mLoaderCallbacks is nicer. We could update VisitedPage and ReadingListPage to follow suit.

Comment 6

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Aah! I removed those spaces in my previous patch. But it didn't qrefresh properly. That's fixed in this. And I've removed the column too.

Comment 7

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/projects/fig/rev/782aeb850fcc

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cc50c0290cf4
https://hg.mozilla.org/mozilla-central/rev/782aeb850fcc