891105 - Cancel LoadThumbnailsTask when not showing the about:home

Status: RESOLVED FIXED

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

Description

[Sriram Ramasubramanian [:sriram]]

Cancel the LoadThumbnailsTask, when the user moved out of about:home.

Comment 1

[Sriram Ramasubramanian [:sriram]]

I'm thinking of using Loaders for this.

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Made the AsyncTask a AsyncTaskLoader. I'm not checking for the id on the ThumbnailsLoaderCallback as there is just one loader handled by it.

Comment 3

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

A missed "tab" is fixed in this.

Comment 4

[Lucas Rocha (:lucasr)]

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

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

You're using a cursor in the thumbnails loader that is managed by another Loader. This is not thread-safe. You should probably create a data structure with all the data you need from cursor and use that copy in the thumbnails loader instead.

Comment 5

[Sriram Ramasubramanian [:sriram]]

I had the same doubt on concurrency. However, if I had to cache the cursor data (basically the url), where would I do it? Would I do it when I get the result from the CursorLoader, and pass it to ThumbnailLoader?

Comment 6

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #5)
> I had the same doubt on concurrency. However, if I had to cache the cursor
> data (basically the url), where would I do it? Would I do it when I get the
> result from the CursorLoader, and pass it to ThumbnailLoader?

Have a look at how FaviconsLoader does it. The thumbnails loader should be similar.

Comment 7

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 1: Thumbnail Loader

This spins a list from the cursor, passes that as an argument to load the thumbnails. This is pretty close to FaviconsLoader (but for the static methods).

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 774351 [details] [diff] [review]
Part 1: Thumbnail Loader

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

Great.

::: mobile/android/base/home/BookmarksPage.java
@@ +52,5 @@
>      // Key for bookmarks folder id.
>      private static final String BOOKMARKS_FOLDER_KEY = "folder_id";
>  
> +    // Key for thumbnail urls.
> +    private static final String THUMBNAILS_URL_KEY = "urls";

THUMBNAILS_URL_KEY -> THUMBNAILS_URLS_KEY

Comment 9

[Sriram Ramasubramanian [:sriram]]

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

Comment 10

[Ed Morley [:emorley]]

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