887985 - TopBookmarksView should use cursor loaders

Status: RESOLVED FIXED

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

Description

[Sriram Ramasubramanian [:sriram]]

TopBookmarksView should use cursor loaders.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This moves the AsyncTask logic to the cursor loaders.

Note: I've removed the cursor closing logic in onDetachedFromWindow() on hopes that Loader will do it. I can add that back if needed.

Doubt: When I rotate the phone, the mAdapter is null and the swapCursor() call was throwing an NPE in onAttachedToWindow(). (Hence I do a null check now). Why does that happen? Loader is loading before the view attaches to the window? But we init the loader only when activity is created (where all its views are instantiated). Should I just move the "new TopBookmarksAdapter()" to the constructor? Also, this doesnt happen with the BookmarksListView.

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Minor correction to avoid NPE while loading a page.

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 768585 [details] [diff] [review]
Patch

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

Looks good (with the suggested changes)

::: mobile/android/base/home/BookmarksPage.java
@@ +32,5 @@
>      // Cursor loader ID for list of bookmarks.
>      private static final int LOADER_ID_BOOKMARKS_LIST = 0;
>  
> +    // Cursor loader ID for grid of bookmarks.
> +    private static final int LOADER_ID_TOP_BOOKMARKS = 1;

TOP_BOOKMARKS_LOADER_ID for consistency with the other pages.

::: mobile/android/base/home/TopBookmarksView.java
@@ +209,5 @@
> +     * Refreshes the grid with the given cursor.
> +     *
> +     * @param cursor The cursor to use with the adapter.
> +     */
> +    public void refreshGridWithCursor(Cursor cursor) {

Rename it to refreshFromCursor() for simplicity and to be consistent with, say, TwoLinePageRow?

Comment 4

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/projects/fig/rev/d51cb8a80120

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d51cb8a80120