Closed Bug 898276 Opened 11 years ago Closed 11 years ago

HistoryPage 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: abouthome-hackathon, fixed-fig)

Attachments

(2 files, 2 obsolete files)

The list is stuttering for bigger favicons. Let's better use FaviconsLoader here too.
Whiteboard: abouthome-hackathon
Attached patch Patch (obsolete) — Splinter Review
Smoooooth as butter.
Attachment #781522 - Flags: review?(lucasr.at.mozilla)
Attached 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)
Attached patch WIP: HomeCursorLoaderCallbacks (obsolete) — Splinter Review
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)
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
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
Closed: 11 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.