HistoryPage should use FaviconsLoader

RESOLVED FIXED in Firefox 26

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sriram, Assigned: sriram)

Tracking

unspecified
Firefox 26
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: abouthome-hackathon, fixed-fig)

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
The list is stuttering for bigger favicons. Let's better use FaviconsLoader here too.
(Assignee)

Updated

5 years ago
Blocks: 862793
Whiteboard: abouthome-hackathon
(Assignee)

Comment 1

5 years ago
Created attachment 781522 [details] [diff] [review]
Patch

Smoooooth as butter.
Attachment #781522 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 2

5 years ago
Created attachment 781543 [details] [diff] [review]
Patch

Missed a qrefresh after a small LocalBrowserDB change.
Attachment #781522 - Attachment is obsolete: true
Attachment #781522 - Flags: review?(lucasr.at.mozilla)
Attachment #781543 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 3

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

Comment 4

5 years ago
Created attachment 781550 [details] [diff] [review]
WIP: HomeCursorLoaderCallbacks

Better patch.
Attachment #781544 - Attachment is obsolete: true
Attachment #781544 - Flags: feedback?(lucasr.at.mozilla)
Attachment #781550 - Flags: feedback?(lucasr.at.mozilla)
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.
Attachment #781543 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 781550 [details] [diff] [review]
WIP: HomeCursorLoaderCallbacks

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

Looks like a nice refactoring.
Attachment #781550 - Flags: feedback?(lucasr.at.mozilla) → feedback+
(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 ;-)

Updated

5 years ago
Assignee: nobody → sriram
(Assignee)

Comment 8

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

The other patch is handled in bug 898501.
Whiteboard: abouthome-hackathon → abouthome-hackathon, fixed-fig

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/008632d54607
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.