898276 - HistoryPage should use FaviconsLoader

Status: RESOLVED FIXED

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

Description

[Sriram Ramasubramanian [:sriram]]

The list is stuttering for bigger favicons. Let's better use FaviconsLoader here too.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Smoooooth as butter.

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Missed a qrefresh after a small LocalBrowserDB change.

Comment 3

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP: HomeCursorLoaderCallbacks

Every fragment uses FaviconsLoader to make the list view scroll fast. And every fragment uses a FAVICON_LOADER_ID and does the same set of operations.
1. Start the FaviconLoader once the BrowserDB returns a cursor.
2. Refresh the adapter when the favicons are loaded in memory.

Why not we move it into HomeFragment and all CursorLoaderCallbacks implement the base one? The base one will handle this case. The other callbacks will handle loader ids for their fragment.

Also, if you like this approach, I would like to make two changes:
1. Actually HomeCursorLoaderCallbacks can have a loadFavicons() method, that the inherited classes can call -- thought of this after uploading the patch. This will clean up some crazy switch-cases.
2. Please please please let me change FAVICON_LOADER_ID to LOADER_ID_FAVICON. And same way for other loader ids too. I'll post separate patch for each fragment ;) ;)

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP: HomeCursorLoaderCallbacks

Better patch.

Comment 5

[Lucas Rocha (:lucasr)]

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

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

Nice.

::: mobile/android/base/home/HistoryPage.java
@@ +336,5 @@
>          public Loader<Cursor> onCreateLoader(int id, Bundle args) {
> +            switch(id) {
> +                case HISTORY_LOADER_ID:
> +                    return new HistoryCursorLoader(getActivity());
> +            

nit: remove trailing space.

@@ +353,5 @@
> +                    mAdapter.swapCursor(c);
> +                    FaviconsLoader.restartFromCursor(getLoaderManager(), FAVICONS_LOADER_ID,
> +                            mCursorLoaderCallbacks, c);
> +                    break;
> +            

nit: remove trailing space.

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 781550 [details] [diff] [review]
WIP: HomeCursorLoaderCallbacks

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

Looks like a nice refactoring.

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #3)
> Also, if you like this approach, I would like to make two changes:
> 1. Actually HomeCursorLoaderCallbacks can have a loadFavicons() method, that
> the inherited classes can call -- thought of this after uploading the patch.
> This will clean up some crazy switch-cases.

It seems this one is already in the patch? Anyway, looks good.

> 2. Please please please let me change FAVICON_LOADER_ID to
> LOADER_ID_FAVICON. And same way for other loader ids too. I'll post separate
> patch for each fragment ;) ;)

I'm fine with renaming the loader id constants. The only thing I care about is that they are consistent ;-)

Comment 8

[Sriram Ramasubramanian [:sriram]]

Rebased and pushed:
http://hg.mozilla.org/projects/fig/rev/008632d54607

The other patch is handled in bug 898501.

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/008632d54607