Improve type safety in UiAsyncTask

RESOLVED FIXED in Firefox 34

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ckitching, Assigned: ckitching)

Tracking

unspecified
Firefox 34
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Pulling the cleanup patch out of Bug 1056128.

Already got an r+ over there, so this should be fairly quick...
(Assignee)

Comment 1

4 years ago
Created attachment 8476997 [details] [diff] [review]
Refactor UIAsyncTask

One patch, reviewed in Bug 1056128 by nalexander.
Attachment #8476997 - Flags: review+
(Assignee)

Comment 2

4 years ago
Created attachment 8477090 [details] [diff] [review]
V2: Cleanup UIAsyncTask

Getting additional review from lucas was suggested.

Lucas: I suggest you skim Bug 1056128 for context, particularly comment 8.


I guess the main question for you is: Do we care that much about matching Android's API that we're willing to either have a poor API ourselves or disable a bunch of useful warnings?
... Or do we favour a nice API, even if it isn't identical to Android's.
Attachment #8476997 - Attachment is obsolete: true
Attachment #8477090 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8477090 [details] [diff] [review]
V2: Cleanup UIAsyncTask

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

Looks good to me. I'm biased here because I've never been a fan of the Params bit in Android's AsyncTask.

We don't have to 100% compatible with AsyncTask. IIRC, we've only made it compatible to make it easier to port existing code.

::: mobile/android/base/BrowserApp.java
@@ +2797,5 @@
>          if (!Intent.ACTION_MAIN.equals(action)) {
>              return;
>          }
>  
> +        (new UIAsyncTask.WithoutParams<Boolean>(ThreadUtils.getBackgroundHandler()) {

I wonder why we've never added a constructor that takes no background handler and defaults to ThreadUtils.getBackgroundHandler(). Let's optimize for the most common case. Please file a follow-up.

::: mobile/android/base/util/UIAsyncTask.java
@@ +17,5 @@
> + *
> + * We use generics differently to Android's AsyncTask.
> + * Android uses a "Params" type parameter to represent the type of all the parameters to this task.
> + * It then uses arguments of type Params... to permit arbitrarily-many of these to be passed
> + * fluently.

nit: add empty comment line here.

@@ +93,5 @@
>              });
>          }
>      }
>  
> +    protected void execute(final Param params) {

nit: params -> param

@@ +115,5 @@
>  
>      protected void onPreExecute() { }
>      protected void onPostExecute(Result result) { }
>      protected void onCancelled() { }
> +    protected abstract Result doInBackground(Param params);

nit: params -> param
Attachment #8477090 - Flags: review?(lucasr.at.mozilla) → review+
(Assignee)

Updated

4 years ago
Blocks: 1056128
https://hg.mozilla.org/mozilla-central/rev/bc145686164e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
You need to log in before you can comment on or make changes to this bug.