1057086 - Improve type safety in UiAsyncTask

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Kitching [:ckitching]]

Pulling the cleanup patch out of Bug 1056128.

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

Comment 1

[Chris Kitching [:ckitching]]

Attachment: Refactor UIAsyncTask

One patch, reviewed in Bug 1056128 by nalexander.

Comment 2

[Chris Kitching [:ckitching]]

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

Comment 3

[Lucas Rocha (:lucasr)]

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

Comment 4

[Chris Kitching [:ckitching]]

https://hg.mozilla.org/integration/fx-team/rev/bc145686164e

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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