HomeFragment should handle FaviconsLoader

RESOLVED FIXED in Firefox 26

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sriram, Assigned: sriram)

Tracking

unspecified
Firefox 26
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: abouthome-hackathon, fixed-fig)

Attachments

(10 attachments)

FaviconsLoader is pretty much called by every fragment for making the list scroll so smooth. It's better to have this piece of code in HomeFragment instead of copy-pasting in all different Fragments.
Assignee

Updated

6 years ago
Assignee: nobody → sriram
Whiteboard: abouthome-hackathon
This makes the HomeFragment handle the basic FaviconsLoader. It has an LOADER_ID of 100, so that it doesn't collide with others.
Attachment #781992 - Flags: review?(lucasr.at.mozilla)
Attachment #781993 - Flags: review?(lucasr.at.mozilla)
Assignee

Updated

6 years ago
Attachment #781994 - Attachment description: ReadingListPage → Part 3: ReadingListPage
Assignee

Updated

6 years ago
Attachment #781994 - Flags: review?(lucasr.at.mozilla)
Attachment #781996 - Flags: review?(lucasr.at.mozilla)
Attachment #781998 - Flags: review?(lucasr.at.mozilla)
Attachment #781999 - Flags: review?(lucasr.at.mozilla)
This cannot re-use HomeCursorLoaderCallbacks because
1. This extends DialogFragment and not HomeFragment.
2. HomeCursorLoaderCallbacks cannot be made static as it needs the LoaderManager.

If we want to make this re-use the HomeCurs..acks, we might need to make it static and have a constructor that takes Loader as an argument. In that case, it could be move out to a separate file. I can do this as a separate patch if you would like that approach.
OCD :D
Attachment #782003 - Flags: review?(lucasr.at.mozilla)
Assignee

Updated

6 years ago
Attachment #782002 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 781992 [details] [diff] [review]
Part 1: HomeCursorLoaderCallbacks

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

::: mobile/android/base/home/HomeFragment.java
@@ +231,5 @@
> +        }
> +
> +        @Override
> +        public void onLoaderReset(Loader<Cursor> loader) {
> +            // Do nothing.

Do thing by default.
Attachment #781992 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 781993 [details] [diff] [review]
Part 2: MostRecentPage

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

Nice.

::: mobile/android/base/home/MostRecentPage.java
@@ +371,1 @@
>                      mAdapter.swapCursor(null);

Get rid of this tab here while at it.
Attachment #781993 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 781994 [details] [diff] [review]
Part 3: ReadingListPage

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

No FaviconsLoader goodness here?
Attachment #781994 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 781996 [details] [diff] [review]
Part 4: BookmarksPage

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

Cool.
Attachment #781996 - Flags: review?(lucasr.at.mozilla) → review+
Attachment #781998 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 781999 [details] [diff] [review]
Part 6: MostVisitedPage

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

Nice.
Attachment #781999 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 782002 [details] [diff] [review]
Part 7: PinBookmarkDialog

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

::: mobile/android/base/home/PinBookmarkDialog.java
@@ +197,3 @@
>                  mAdapter.swapCursor(c);
>  
> +                FaviconsLoader.restartFromCursor(getLoaderManager(), LOADER_ID_FAVICONS,

Why can't PinBookmarkDialog use HomeCursorLoaderCallbacks too?
Attachment #782002 - Flags: review?(lucasr.at.mozilla) → review-
Comment on attachment 782003 [details] [diff] [review]
Part 8: LastTabsPage

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

Cool.
Attachment #782003 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Lucas Rocha (:lucasr) from comment #9)
> Comment on attachment 781992 [details] [diff] [review]
> Part 1: HomeCursorLoaderCallbacks
> 
> Review of attachment 781992 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomeFragment.java
> @@ +231,5 @@
> > +        }
> > +
> > +        @Override
> > +        public void onLoaderReset(Loader<Cursor> loader) {
> > +            // Do nothing.
> 
> Do thing by default.

I don't understand this comment.
(In reply to Lucas Rocha (:lucasr) from comment #14)
> Comment on attachment 782002 [details] [diff] [review]
> Part 7: PinBookmarkDialog
> 
> Review of attachment 782002 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/PinBookmarkDialog.java
> @@ +197,3 @@
> >                  mAdapter.swapCursor(c);
> >  
> > +                FaviconsLoader.restartFromCursor(getLoaderManager(), LOADER_ID_FAVICONS,
> 
> Why can't PinBookmarkDialog use HomeCursorLoaderCallbacks too?

I've mentioned the reason in comment 7.
(In reply to Sriram Ramasubramanian [:sriram] from comment #17)
> (In reply to Lucas Rocha (:lucasr) from comment #14)
> > Comment on attachment 782002 [details] [diff] [review]
> > Part 7: PinBookmarkDialog
> > 
> > Review of attachment 782002 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/base/home/PinBookmarkDialog.java
> > @@ +197,3 @@
> > >                  mAdapter.swapCursor(c);
> > >  
> > > +                FaviconsLoader.restartFromCursor(getLoaderManager(), LOADER_ID_FAVICONS,
> > 
> > Why can't PinBookmarkDialog use HomeCursorLoaderCallbacks too?
> 
> I've mentioned the reason in comment 7.

Also, I'm changing the variable names to be "LOADER_ID_*". So still this patch is needed right?
(In reply to Sriram Ramasubramanian [:sriram] from comment #16)
> (In reply to Lucas Rocha (:lucasr) from comment #9)
> > Comment on attachment 781992 [details] [diff] [review]
> > Part 1: HomeCursorLoaderCallbacks
> > 
> > Review of attachment 781992 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/base/home/HomeFragment.java
> > @@ +231,5 @@
> > > +        }
> > > +
> > > +        @Override
> > > +        public void onLoaderReset(Loader<Cursor> loader) {
> > > +            // Do nothing.
> > 
> > Do thing by default.
> 
> I don't understand this comment.

hehe, that was a thinko. I meant the comment could be "Do nothing by default" for clarity.
(In reply to Sriram Ramasubramanian [:sriram] from comment #7)
> Created attachment 782002 [details] [diff] [review]
> Part 7: PinBookmarkDialog
> 
> This cannot re-use HomeCursorLoaderCallbacks because
> 1. This extends DialogFragment and not HomeFragment.
> 2. HomeCursorLoaderCallbacks cannot be made static as it needs the
> LoaderManager.
> 
> If we want to make this re-use the HomeCurs..acks, we might need to make it
> static and have a constructor that takes Loader as an argument. In that
> case, it could be move out to a separate file. I can do this as a separate
> patch if you would like that approach.

Yep, please file a follow-up.
Attachment #782002 - Flags: review- → review+
This moves the HomeCursorLoaderCallbacks to a separate file. The constructor takes two arguments -- Context and LoaderManager. I hope neither of them changes. If they do, we might need to re-think about this approach. This also updates PinBookmarkDialog to use the HomeCurs..acks.
Attachment #782808 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 782808 [details] [diff] [review]
Part 9: Moving HomeCursor..acks out

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

Cool.

::: mobile/android/base/home/HomeCursorLoaderCallbacks.java
@@ +20,5 @@
> +    // Cursor loader ID for favicons query
> +    private static final int LOADER_ID_FAVICONS = 100;
> +
> +    // Callback for favicons loaded in memory.
> +    public abstract void onFaviconsLoaded();

nit: move this method at the bottom of the class. Having it mixed with the attributes makes it a bit less visible.
Attachment #782808 - Flags: review?(lucasr.at.mozilla) → review+
This is for the Part 2 and Part 6. When I worked on Part 9, I couldn't apply those two patches as their dependent patch was backed out. This completes the big loop. I can fold this with Part 9 and land it.
Attachment #783978 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 783978 [details] [diff] [review]
Part 10: Last few bits

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

I guess you will merge these changes to their respective original patches?
Attachment #783978 - Flags: review?(lucasr.at.mozilla) → review+
Merged 9 & 10 and pushed:
http://hg.mozilla.org/projects/fig/rev/41e8b95f5667
Whiteboard: abouthome-hackathon → abouthome-hackathon, fixed-fig
You need to log in before you can comment on or make changes to this bug.