refactor DownloadManagerService to eliminate variable access on 2 different threads

refactor DownloadManagerService to eliminate variable access on 2 different threads

When a download progress changes, Chrome posts an async
task to update the notification.
Both the async task and the UI thread reads the same variables.
As a result, these variables are declared volatile or using thread safe types.
This change eliminates such variable sharing by copying the reference first,
and then pass the copied data to the async task.

Committed: https://crrev.com/4f6d799d1e1355731776cbdd80ad03d6c0a9d237
Cr-Commit-Position: refs/heads/master@{#406618}

[qinmin, 2016-07-14 20:43:22]

qinmin@chromium.org changed reviewers:
+ yfriedman@chromium.org

[qinmin, 2016-07-14 20:43:22]

PTAL

[qinmin, 2016-07-18 18:19:24]

qinmin@chromium.org changed reviewers:
+ tedchoc@chromium.org

[qinmin, 2016-07-18 18:19:25]

+tedchoc

[qinmin, 2016-07-18 18:22:29]

Description was changed from

==========
refactor DownloadManagerService to eliminate variable access on 2 different
threads

When a download progress changes, Chrome posts an async
task to update the notification.
And the async task and the UI thread reads the same variables.
This change eliminates such variable sharing by copying the data first,
and then pass the copied data to the async task.
==========

to

==========
refactor DownloadManagerService to eliminate variable access on 2 different
threads

When a download progress changes, Chrome posts an async
task to update the notification.
Both the async task and the UI thread reads the same variables.
As a result, these variables are declared volatile or using thread safe types. 
This change eliminates such variable sharing by copying the reference first,
and then pass the copied data to the async task.
==========

[Yaron, 2016-07-19 14:55:19]

https://codereview.chromium.org/2151063002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadManagerServiceTest.java
(right):

https://codereview.chromium.org/2151063002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadManagerServiceTest.java:365:
public void testOnlyOneProgressForFastUpdates() throws InterruptedException {
name needs to be updated. Is the previous test case no longer valid?

[qinmin, 2016-07-19 17:47:27]

https://codereview.chromium.org/2151063002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadManagerServiceTest.java
(right):

https://codereview.chromium.org/2151063002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadManagerServiceTest.java:365:
public void testOnlyOneProgressForFastUpdates() throws InterruptedException {
On 2016/07/19 14:55:19, Yaron wrote:
> name needs to be updated. Is the previous test case no longer valid?

Done. The previous test case tests a higher update frequency compared to
arriving updates. While this tests tests a lower update frequency. Updated the
test name of the previous test case to reflect the difference.

[Yaron, 2016-07-19 19:31:22]

https://codereview.chromium.org/2151063002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadManagerServiceTest.java
(right):

https://codereview.chromium.org/2151063002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadManagerServiceTest.java:365:
public void testOnlyOneProgressForFastUpdates() throws InterruptedException {
On 2016/07/19 17:47:27, qinmin wrote:
> On 2016/07/19 14:55:19, Yaron wrote:
> > name needs to be updated. Is the previous test case no longer valid?
> 
> Done. The previous test case tests a higher update frequency compared to
> arriving updates. While this tests tests a lower update frequency. Updated the
> test name of the previous test case to reflect the difference.

Err, i meant the case of 10, 30, 30 (as in what this test was measuring before).
Why did we previously get 1 update but now 2?

[qinmin, 2016-07-19 19:47:31]

https://codereview.chromium.org/2151063002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadManagerServiceTest.java
(right):

https://codereview.chromium.org/2151063002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadManagerServiceTest.java:365:
public void testOnlyOneProgressForFastUpdates() throws InterruptedException {
On 2016/07/19 19:31:22, Yaron wrote:
> On 2016/07/19 17:47:27, qinmin wrote:
> > On 2016/07/19 14:55:19, Yaron wrote:
> > > name needs to be updated. Is the previous test case no longer valid?
> > 
> > Done. The previous test case tests a higher update frequency compared to
> > arriving updates. While this tests tests a lower update frequency. Updated
the
> > test name of the previous test case to reflect the difference.
> 
> Err, i meant the case of 10, 30, 30 (as in what this test was measuring
before).
> Why did we previously get 1 update but now 2?

The progress, either 10, 20,30 or 10,30,30 doesn't matter the test outcome. 
Previously we get 1 update because when a progress update comes, we fire a
delayed task to update the notification. When the delayed task runs, the shared
variable is updated 3 times  so only the last one matters.
With this patch, whenever an update arrives, Chrome will immediately update the
notification with the new progress information. And then fire a delayed task to
schedule the next update. When the delayed task runs, the progress information
is updated twice (by 2nd and 3rd progress update), so it will update the
notification again.

So with this patch, the notification will be updated more promptly at the
beginning.

[Yaron, 2016-07-20 01:04:01]

lgtm

[qinmin, 2016-07-20 16:29:17]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2016-07-20 16:29:32]

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, 2016-07-20 18:06:15]

Description was changed from

==========
refactor DownloadManagerService to eliminate variable access on 2 different
threads

When a download progress changes, Chrome posts an async
task to update the notification.
Both the async task and the UI thread reads the same variables.
As a result, these variables are declared volatile or using thread safe types. 
This change eliminates such variable sharing by copying the reference first,
and then pass the copied data to the async task.
==========

to

==========
refactor DownloadManagerService to eliminate variable access on 2 different
threads

When a download progress changes, Chrome posts an async
task to update the notification.
Both the async task and the UI thread reads the same variables.
As a result, these variables are declared volatile or using thread safe types. 
This change eliminates such variable sharing by copying the reference first,
and then pass the copied data to the async task.
==========

[commit-bot: I haz the power, 2016-07-20 18:06:15]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-20 18:06:29]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-20 18:07:41]

Description was changed from

==========
refactor DownloadManagerService to eliminate variable access on 2 different
threads

When a download progress changes, Chrome posts an async
task to update the notification.
Both the async task and the UI thread reads the same variables.
As a result, these variables are declared volatile or using thread safe types. 
This change eliminates such variable sharing by copying the reference first,
and then pass the copied data to the async task.
==========

to

==========
refactor DownloadManagerService to eliminate variable access on 2 different
threads

When a download progress changes, Chrome posts an async
task to update the notification.
Both the async task and the UI thread reads the same variables.
As a result, these variables are declared volatile or using thread safe types.
This change eliminates such variable sharing by copying the reference first,
and then pass the copied data to the async task.

Committed: https://crrev.com/4f6d799d1e1355731776cbdd80ad03d6c0a9d237
Cr-Commit-Position: refs/heads/master@{#406618}
==========

[commit-bot: I haz the power, 2016-07-20 18:07:42]

Patchset 2 (id:??) landed as
https://crrev.com/4f6d799d1e1355731776cbdd80ad03d6c0a9d237
Cr-Commit-Position: refs/heads/master@{#406618}