Closed
Bug 1057086
Opened 10 years ago
Closed 10 years ago
Improve type safety in UiAsyncTask
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 34
People
(Reporter: ckitching, Assigned: ckitching)
References
Details
Attachments
(1 file, 1 obsolete file)
30.58 KB,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
Pulling the cleanup patch out of Bug 1056128.
Already got an r+ over there, so this should be fairly quick...
Assignee | ||
Comment 1•10 years ago
|
||
One patch, reviewed in Bug 1056128 by nalexander.
Attachment #8476997 -
Flags: review+
Assignee | ||
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
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 | ||
Comment 4•10 years ago
|
||
Comment 5•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•