Move SharedPreference for download into DownloadNotificationService

Move SharedPreference for download into DownloadNotificationService

Previously we use SharedPreference to track ongoing downloads when Chrome
gets killed.
So when chrome restarts, we can use the SharedPreferences to clean up
notifications.
With DownloadNotificationService, it should handle all notifications
along with those SharedPreferences.
And since we no longer clear up notification on restart, we can simply
remove the SharedPreference after download is paused.

BUG=

Committed: https://crrev.com/2b978d9fdb485b980c274064a5506a9c0bbc08f3
Cr-Commit-Position: refs/heads/master@{#375696}

[qinmin, 2016-02-12 23:54:42]

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

[qinmin, 2016-02-12 23:54:43]

PTAL

[Ted C, 2016-02-16 17:50:08]

lgtm w/ a few little comments

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

https://codereview.chromium.org/1696963002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:290:
// Remove old DOWNLOAD_NOTIFICATION_IDS SharedPrefs.
Should we keep this todo?

Is DOWNLOAD_NOTIFICATION_IDS still unused?  Maybe we should rename it to
DEPRECATED_DOWNLOAD_NOTIFCATION_IDS or
DOWNLOAD_NOTIFICATION_IDS_PRE_M<Milestone> and then we could have a comment on
the variable instead of in the code?

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

https://codereview.chromium.org/1696963002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:284:
for (PendingNotification notification : notifications) {
since you're here, I would change this to int i = 0; i < ...; i++ to avoid the
allocation of a iterator (not a huge deal, but we should avoid this syntax for
lists where it is not necessary)

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

https://codereview.chromium.org/1696963002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadNotificationServiceTest.java:77:
.getNotificationString());
these should be indented 8 from the start of the line, not 8 from the (

same below

[qinmin, 2016-02-16 22:00:16]

https://chromiumcodereview.appspot.com/1696963002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(left):

https://chromiumcodereview.appspot.com/1696963002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:290:
// Remove old DOWNLOAD_NOTIFICATION_IDS SharedPrefs.
On 2016/02/16 17:50:08, Ted C wrote:
> Should we keep this todo?
> 
> Is DOWNLOAD_NOTIFICATION_IDS still unused?  Maybe we should rename it to
> DEPRECATED_DOWNLOAD_NOTIFCATION_IDS or
> DOWNLOAD_NOTIFICATION_IDS_PRE_M<Milestone> and then we could have a comment on
> the variable instead of in the code?

Done. DOWNLOAD_NOTIFICATION_IDS were used in M48 and below.  Renamed the
variable to DEPRECATED_DOWNLOAD_NOTIFCATION_IDS

https://chromiumcodereview.appspot.com/1696963002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
(right):

https://chromiumcodereview.appspot.com/1696963002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:284:
for (PendingNotification notification : notifications) {
On 2016/02/16 17:50:08, Ted C wrote:
> since you're here, I would change this to int i = 0; i < ...; i++ to avoid the
> allocation of a iterator (not a huge deal, but we should avoid this syntax for
> lists where it is not necessary)

Done.

https://chromiumcodereview.appspot.com/1696963002/diff/1/chrome/android/javat...
File
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadNotificationServiceTest.java
(right):

https://chromiumcodereview.appspot.com/1696963002/diff/1/chrome/android/javat...
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadNotificationServiceTest.java:77:
.getNotificationString());
On 2016/02/16 17:50:08, Ted C wrote:
> these should be indented 8 from the start of the line, not 8 from the (
> 
> same below

Done. Another formatting issue by "git cl format"

[qinmin, 2016-02-16 22:09:11]

Patchset #2 (id:20001) has been deleted

[qinmin, 2016-02-16 22:09:40]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-02-16 22:09:40]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1696963002/#ps40001
(title: "nits")

[commit-bot: I haz the power, 2016-02-16 22:13:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1696963002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1696963002/40001

[commit-bot: I haz the power, 2016-02-16 23:00:59]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-02-16 23:01:51]

Description was changed from

==========
Move SharedPreference for download into DownloadNotificationService

Previously we use SharedPreference to track ongoing downloads when Chrome
gets killed.
So when chrome restarts, we can use the SharedPreferences to clean up
notifications.
With DownloadNotificationService, it should handle all notifications
along with those SharedPreferences.
And since we no longer clear up notification on restart, we can simply
remove the SharedPreference after download is paused.

BUG=
==========

to

==========
Move SharedPreference for download into DownloadNotificationService

Previously we use SharedPreference to track ongoing downloads when Chrome
gets killed.
So when chrome restarts, we can use the SharedPreferences to clean up
notifications.
With DownloadNotificationService, it should handle all notifications
along with those SharedPreferences.
And since we no longer clear up notification on restart, we can simply
remove the SharedPreference after download is paused.

BUG=

Committed: https://crrev.com/2b978d9fdb485b980c274064a5506a9c0bbc08f3
Cr-Commit-Position: refs/heads/master@{#375696}
==========

[commit-bot: I haz the power, 2016-02-16 23:01:52]

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