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
|
||
https://hg.mozilla.org/integration/fx-team/rev/bc145686164e
Comment 5•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bc145686164e
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Updated•3 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
•