Closed Bug 893990 Opened 9 years ago Closed 8 years ago

BookmarksPage should use FaviconsLoader

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

(2 files)

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).
Attached patch PatchSplinter Review
Uses the same code from VisitedPage.
Attachment #775867 - Flags: review?(margaret.leibovic)
Assignee: nobody → sriram
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.
Attachment #775867 - Flags: review?(margaret.leibovic) → review+
(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
(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.
Attached patch PatchSplinter Review
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.
Attachment #776709 - Flags: review?(margaret.leibovic)
Attachment #776709 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/cc50c0290cf4
https://hg.mozilla.org/mozilla-central/rev/782aeb850fcc
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.