898501 - HomeFragment should handle FaviconsLoader

Status: RESOLVED FIXED

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

Description

[Sriram Ramasubramanian [:sriram]]

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.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 1: HomeCursorLoaderCallbacks

This makes the HomeFragment handle the basic FaviconsLoader. It has an LOADER_ID of 100, so that it doesn't collide with others.

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 2: MostRecentPage

Comment 3

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 3: ReadingListPage

OCD. :D

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 4: BookmarksPage

Comment 5

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 5: BrowserSearch

Comment 6

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 6: MostVisitedPage

Comment 7

[Sriram Ramasubramanian [:sriram]]

Attachment: 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.

Comment 8

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 8: LastTabsPage

OCD :D

Comment 9

[Lucas Rocha (:lucasr)]

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.

Comment 10

[Lucas Rocha (:lucasr)]

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.

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 781994 [details] [diff] [review]
Part 3: ReadingListPage

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

No FaviconsLoader goodness here?

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 781996 [details] [diff] [review]
Part 4: BookmarksPage

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

Cool.

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 781999 [details] [diff] [review]
Part 6: MostVisitedPage

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

Nice.

Comment 14

[Lucas Rocha (:lucasr)]

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?

Comment 15

[Lucas Rocha (:lucasr)]

Comment on attachment 782003 [details] [diff] [review]
Part 8: LastTabsPage

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

Cool.

Comment 16

[Sriram Ramasubramanian [:sriram]]

(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.

Comment 17

[Sriram Ramasubramanian [:sriram]]

(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.

Comment 18

[Sriram Ramasubramanian [:sriram]]

(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?

Comment 19

[Lucas Rocha (:lucasr)]

(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.

Comment 20

[Lucas Rocha (:lucasr)]

(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.

Comment 21

[Sriram Ramasubramanian [:sriram]]

Everything except part 2 and part 6 as they are dependent on bug 895837.
https://hg.mozilla.org/projects/fig/rev/0a243d3f33c2
https://hg.mozilla.org/projects/fig/rev/0398421459d7
https://hg.mozilla.org/projects/fig/rev/f2d68c93e066
https://hg.mozilla.org/projects/fig/rev/4fa359c591e5
https://hg.mozilla.org/projects/fig/rev/a9f2a85bf1eb
https://hg.mozilla.org/projects/fig/rev/1fc9bcc1d4ab

Comment 22

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 9: Moving HomeCursor..acks out

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.

Comment 23

[Lucas Rocha (:lucasr)]

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.

Comment 24

[Sriram Ramasubramanian [:sriram]]

Part 2 and Part 6:
http://hg.mozilla.org/projects/fig/rev/e169d1c5043c
http://hg.mozilla.org/projects/fig/rev/87e218e88505

Comment 25

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 10: Last few bits

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.

Comment 26

[Lucas Rocha (:lucasr)]

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?

Comment 27

[Sriram Ramasubramanian [:sriram]]

Merged 9 & 10 and pushed:
http://hg.mozilla.org/projects/fig/rev/41e8b95f5667

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0a243d3f33c2
https://hg.mozilla.org/mozilla-central/rev/0398421459d7
https://hg.mozilla.org/mozilla-central/rev/f2d68c93e066
https://hg.mozilla.org/mozilla-central/rev/4fa359c591e5
https://hg.mozilla.org/mozilla-central/rev/a9f2a85bf1eb
https://hg.mozilla.org/mozilla-central/rev/1fc9bcc1d4ab
https://hg.mozilla.org/mozilla-central/rev/e169d1c5043c
https://hg.mozilla.org/mozilla-central/rev/87e218e88505
https://hg.mozilla.org/mozilla-central/rev/41e8b95f5667