Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2813153002:
getInstalledRelatedApps: Avoid blocking the UI thread doing computation. (Closed)
|
Created:
3 years, 8 months ago by Matt Giuca Modified:
3 years, 8 months ago Reviewers:
Ted C CC:
chromium-reviews, jam, darin-cc_chromium.org, agrieve+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptiongetInstalledRelatedApps: Avoid blocking the UI thread doing computation.
Performs the computation on a background thread, so the UI is responsive
while a result from this API is pending.
BUG=710745
Review-Url: https://codereview.chromium.org/2813153002
Cr-Commit-Position: refs/heads/master@{#464243}
Committed: https://chromium.googlesource.com/chromium/src/+/ad0f03580fa8c308ee7f63f62489f7c322df1ac9
Patch Set 1 #Patch Set 2 : Avoid reflowing comment. #
Total comments: 8
Patch Set 3 : Respond to review. #
Dependent Patchsets: Messages
Total messages: 18 (9 generated)
mgiuca@chromium.org changed reviewers: + tedchoc@chromium.org
Hi Ted, Spinning this off from https://codereview.chromium.org/2802603002 since it's actually unrelated to the timing delay issue there. (Since I realised that we actually don't want to do the computation on the UI thread either.) This is based on your advice there except postDelayed() is not needed since we just want to do the computation in the background thread, then return.
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/ comments https://codereview.chromium.org/2813153002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java (right): https://codereview.chromium.org/2813153002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:92: } .execute Should go on this line...hopefully clang-format allows that. https://codereview.chromium.org/2813153002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:93: .execute(); When using an AsyncTask, we should always use executeOnExecutor. The default executor has changed throughout the history of Android, so you want to be explicit (I suspect you'll want to use https://developer.android.com/reference/android/os/AsyncTask.html#THREAD_POOL... https://codereview.chromium.org/2813153002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:112: RelatedApplication[] relatedApps, URI frameUrl) { I would add assert !ThreadUtils.runningOnUiThread();
https://codereview.chromium.org/2813153002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java (right): https://codereview.chromium.org/2813153002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:92: } On 2017/04/12 15:47:04, Ted C wrote: > .execute > > Should go on this line...hopefully clang-format allows that. Uh, no, clang-format did this crazy thing in the first place. Should I just overrule it? Or assign it to a variable so I can call it on a new line?
https://codereview.chromium.org/2813153002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java (right): https://codereview.chromium.org/2813153002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:92: } On 2017/04/12 23:14:01, Matt Giuca wrote: > On 2017/04/12 15:47:04, Ted C wrote: > > .execute > > > > Should go on this line...hopefully clang-format allows that. > > Uh, no, clang-format did this crazy thing in the first place. Should I just > overrule it? Or assign it to a variable so I can call it on a new line? I would overrule that...that is very weird and seems like a bug in clang-format. I'll see if I can repro this and file a clang format bug.
https://codereview.chromium.org/2813153002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java (right): https://codereview.chromium.org/2813153002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:92: } On 2017/04/12 23:35:28, Ted C wrote: > On 2017/04/12 23:14:01, Matt Giuca wrote: > > On 2017/04/12 15:47:04, Ted C wrote: > > > .execute > > > > > > Should go on this line...hopefully clang-format allows that. > > > > Uh, no, clang-format did this crazy thing in the first place. Should I just > > overrule it? Or assign it to a variable so I can call it on a new line? > > I would overrule that...that is very weird and seems like a bug in clang-format. > > I'll see if I can repro this and file a clang format bug. Done. https://codereview.chromium.org/2813153002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:93: .execute(); On 2017/04/12 15:47:04, Ted C wrote: > When using an AsyncTask, we should always use executeOnExecutor. The default > executor has changed throughout the history of Android, so you want to be > explicit (I suspect you'll want to use > https://developer.android.com/reference/android/os/AsyncTask.html#THREAD_POOL... Done. Note: This required adding CustomShadowAsyncTask to the Robolectric test, which just changes executeOnExecutor to go back to using execute (which just uses the single thread). https://codereview.chromium.org/2813153002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:112: RelatedApplication[] relatedApps, URI frameUrl) { On 2017/04/12 15:47:04, Ted C wrote: > I would add > > assert !ThreadUtils.runningOnUiThread(); I tried this, but it fails in the tests due to the above (the fact that Robolectric is running all of this on the one thread). So I can't.
The CQ bit was checked by mgiuca@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2813153002/#ps40001 (title: "Respond to review.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/13 01:19:33, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... https://cs.chromium.org/chromium/src/components/variations/android/junit/src/... I think you can and should still use ThreadUtils. We might have to use Answers or other mock constructs, but should be what we use imo.
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492046329035570,
"parent_rev": "ef7d5fbdaadfd3c2ed18d662d2ef754e5d208c76", "commit_rev":
"ad0f03580fa8c308ee7f63f62489f7c322df1ac9"}
Message was sent while issue was closed.
Description was changed from ========== getInstalledRelatedApps: Avoid blocking the UI thread doing computation. Performs the computation on a background thread, so the UI is responsive while a result from this API is pending. BUG=710745 ========== to ========== getInstalledRelatedApps: Avoid blocking the UI thread doing computation. Performs the computation on a background thread, so the UI is responsive while a result from this API is pending. BUG=710745 Review-Url: https://codereview.chromium.org/2813153002 Cr-Commit-Position: refs/heads/master@{#464243} Committed: https://chromium.googlesource.com/chromium/src/+/ad0f03580fa8c308ee7f63f62489... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/ad0f03580fa8c308ee7f63f62489...
Message was sent while issue was closed.
On 2017/04/13 01:59:53, Ted C wrote: > On 2017/04/13 01:19:33, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > https://cs.chromium.org/chromium/src/components/variations/android/junit/src/... > > I think you can and should still use ThreadUtils. We might have to use Answers > or other mock constructs, but should be what we use imo. Since this landed after your comment, moved this to a separate CL: https://codereview.chromium.org/2810373002 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
