HistoryPage should use FaviconsLoader

RESOLVED FIXED in Firefox 26

Status

()

RESOLVED FIXED
6 years ago
6 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)

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

Updated

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

Comment 1

6 years ago
Posted patch Patch (obsolete) — Splinter Review
Smoooooth as butter.
Attachment #781522 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 2

6 years ago
Posted patch PatchSplinter Review
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

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

6 years ago
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 ;-)
Assignee: nobody → sriram
(Assignee)

Comment 8

6 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
https://hg.mozilla.org/mozilla-central/rev/008632d54607
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.