Closed Bug 1057086 Opened 10 years ago Closed 10 years ago

Improve type safety in UiAsyncTask

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: ckitching, Assigned: ckitching)

References

Details

Attachments

(1 file, 1 obsolete file)

Pulling the cleanup patch out of Bug 1056128. Already got an r+ over there, so this should be fairly quick...
Attached patch Refactor UIAsyncTask (obsolete) — Splinter Review
One patch, reviewed in Bug 1056128 by nalexander.
Attachment #8476997 - Flags: review+
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+
Blocks: 1056128
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: