remove unnecessary thread hopping when updating Download notifications

remove unnecessary thread hopping when updating Download notifications

We create an async task to call DownloadManagerService.updateAllNotifications() when updating notifications.
However, later we post a task back to the Ui thread in SystemDownloadNotifier
We create the Async task just because of the strict mode:
addCompletedDownload() and canResolveDownloadItem() needs to be on a non UI thread.
This CL moves the updateAllNotifications() back to the UI thread.
only addCompletedDownload() and canResolveDownloadItem() are left in the async task.

BUG=693273
Review-Url: https://codereview.chromium.org/2701613005
Cr-Commit-Position: refs/heads/master@{#451524}
Committed: https://chromium.googlesource.com/chromium/src/+/5939cf034a8d276d3f08b16a85503905a4c0580a

[qinmin, 2017-02-17 00:16:21]

qinmin@chromium.org changed reviewers:
+ dfalcantara@chromium.org, dtrainor@chromium.org, shaktisahu@chromium.org

[qinmin, 2017-02-17 00:16:22]

PTAL

[gone, 2017-02-17 19:31:34]

lgtm

https://codereview.chromium.org/2701613005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(left):

https://codereview.chromium.org/2701613005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:561:
assert !ThreadUtils.runningOnUiThread();
If this thing should only be called from the UI thread now, the assert should be
kept and the condition flipped.

https://codereview.chromium.org/2701613005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(right):

https://codereview.chromium.org/2701613005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:568:
updateSuccessful = updateDownloadSuccessNotification(progress);
You don't really know if the update was successful at this point; it's called
from the background thread.  Maybe rename the boolean to something else?

https://codereview.chromium.org/2701613005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:643:
Log.e(TAG, "reaching thread limit, reschedule notification update later.");
"thread limit reached"

https://codereview.chromium.org/2701613005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/SystemDownloadNotifier.java
(left):

https://codereview.chromium.org/2701613005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/SystemDownloadNotifier.java:230:
* Helper method to schedule download notification updates, can be called on any
thread.
Assert that you're on the right thread

[qinmin, 2017-02-17 21:34:37]

https://codereview.chromium.org/2701613005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(left):

https://codereview.chromium.org/2701613005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:561:
assert !ThreadUtils.runningOnUiThread();
On 2017/02/17 19:31:33, dfalcantara (load balance plz) wrote:
> If this thing should only be called from the UI thread now, the assert should
be
> kept and the condition flipped.

Done.

https://codereview.chromium.org/2701613005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(right):

https://codereview.chromium.org/2701613005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:568:
updateSuccessful = updateDownloadSuccessNotification(progress);
On 2017/02/17 19:31:33, dfalcantara (load balance plz) wrote:
> You don't really know if the update was successful at this point; it's called
> from the background thread.  Maybe rename the boolean to something else?

Done.

https://codereview.chromium.org/2701613005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:643:
Log.e(TAG, "reaching thread limit, reschedule notification update later.");
On 2017/02/17 19:31:34, dfalcantara (load balance plz) wrote:
> "thread limit reached"

Done.

https://codereview.chromium.org/2701613005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/SystemDownloadNotifier.java
(left):

https://codereview.chromium.org/2701613005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/SystemDownloadNotifier.java:230:
* Helper method to schedule download notification updates, can be called on any
thread.
On 2017/02/17 19:31:34, dfalcantara (load balance plz) wrote:
> Assert that you're on the right thread

Done.

[qinmin, 2017-02-17 21:34:45]

The CQ bit was checked by qinmin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 21:35:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 23:39:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 23:39:49]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[qinmin, 2017-02-19 18:01:25]

Patchset #2 (id:20001) has been deleted

[qinmin, 2017-02-19 18:01:48]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2017-02-19 18:01:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2701613005/#ps40001
(title: "addressing comments")

[commit-bot: I haz the power, 2017-02-19 18:01:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-19 18:38:32]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487527308307110,
"parent_rev": "3e3774f369e1e642d06e895276b08814b279c20c", "commit_rev":
"5939cf034a8d276d3f08b16a85503905a4c0580a"}

[commit-bot: I haz the power, 2017-02-19 18:39:12]

Description was changed from

==========
remove unnecessary thread hopping when updating Download notifications

We create an async task to call DownloadManagerService.updateAllNotifications()
when updating notifications.
However, later we post a task back to the Ui thread in SystemDownloadNotifier
We create the Async task just because of the strict mode:
addCompletedDownload() and canResolveDownloadItem() needs to be on a non UI
thread.
This CL moves the updateAllNotifications() back to the UI thread.
only addCompletedDownload() and canResolveDownloadItem() are left in the async
task.

BUG=693273
==========

to

==========
remove unnecessary thread hopping when updating Download notifications

We create an async task to call DownloadManagerService.updateAllNotifications()
when updating notifications.
However, later we post a task back to the Ui thread in SystemDownloadNotifier
We create the Async task just because of the strict mode:
addCompletedDownload() and canResolveDownloadItem() needs to be on a non UI
thread.
This CL moves the updateAllNotifications() back to the UI thread.
only addCompletedDownload() and canResolveDownloadItem() are left in the async
task.

BUG=693273

Review-Url: https://codereview.chromium.org/2701613005
Cr-Commit-Position: refs/heads/master@{#451524}
Committed:
https://chromium.googlesource.com/chromium/src/+/5939cf034a8d276d3f08b16a8550...
==========

[commit-bot: I haz the power, 2017-02-19 18:39:13]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/5939cf034a8d276d3f08b16a8550...