Closed Bug 887985 Opened 9 years ago Closed 8 years ago

TopBookmarksView should use cursor loaders

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: fixed-fig)

Attachments

(1 file, 1 obsolete file)

TopBookmarksView should use cursor loaders.
Blocks: 882365
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #768576 - Flags: review?(lucasr.at.mozilla)
Attached patch PatchSplinter Review
Minor correction to avoid NPE while loading a page.
Attachment #768576 - Attachment is obsolete: true
Attachment #768576 - Flags: review?(lucasr.at.mozilla)
Attachment #768585 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #768585 - Flags: review?(lucasr.at.mozilla) → review+
http://hg.mozilla.org/projects/fig/rev/d51cb8a80120
Assignee: nobody → sriram
Whiteboard: fixed-fig
https://hg.mozilla.org/mozilla-central/rev/d51cb8a80120
Status: NEW → RESOLVED
Closed: 8 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.