deprecate notificationId for downloads

deprecate notificationId for downloads

The downloadId is going to be deprecated, and it is replaced by downloadGuid.
Android currently use the downloadId as notificationId.
This should also be deprecated.
This change generates a new notificationId for each newly added
download.
And a SharedPref entry is used to record the next notificationId to be generated

BUG=613014
Committed: https://crrev.com/b935b5d8c1675d06e6733483ba5ff56ed22906ae
Cr-Commit-Position: refs/heads/master@{#396254}

[qinmin, 2016-05-19 00:09:18]

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

[qinmin, 2016-05-19 00:09:19]

PTAL

[Ted C, 2016-05-20 00:02:13]

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

https://codereview.chromium.org/1993873003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:56:
static final String NOTIFICATION_NAMESPACE = "DownloadNotificationService";
rename DEPRECATED?

https://codereview.chromium.org/1993873003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:58:
private static final String NEXT_DOWNLOAD_NOTIFICATION_ID =
"NextDownloadNotificationId";
Instead of Next, I would use V2

https://codereview.chromium.org/1993873003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:60:
private static final int STARTING_NOTIFICATION_ID = 1000000;
any reason we don't use a different notification namespace?  couldn't we just
clear all notifications in the previous namespace and then add new ones w/o
having to worry about the id collision possibilities?

https://codereview.chromium.org/1993873003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:106:
NEXT_DOWNLOAD_NOTIFICATION_ID, STARTING_NOTIFICATION_ID);
+4 indent

[qinmin, 2016-05-23 23:03:14]

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

https://codereview.chromium.org/1993873003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:56:
static final String NOTIFICATION_NAMESPACE = "DownloadNotificationService";
On 2016/05/20 00:02:13, Ted C wrote:
> rename DEPRECATED?

There is no need to deprecate it. See reasons below

https://codereview.chromium.org/1993873003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:58:
private static final String NEXT_DOWNLOAD_NOTIFICATION_ID =
"NextDownloadNotificationId";
On 2016/05/20 00:02:13, Ted C wrote:
> Instead of Next, I would use V2

Since I am keeping the namespace, V2 is probably not very suitable.

https://codereview.chromium.org/1993873003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:60:
private static final int STARTING_NOTIFICATION_ID = 1000000;
On 2016/05/20 00:02:13, Ted C wrote:
> any reason we don't use a different notification namespace?  couldn't we just
> clear all notifications in the previous namespace and then add new ones w/o
> having to worry about the id collision possibilities?

If we clear all existing notifications, we have to recreate these notifications
with new namespace so that user won't lose paused downloads.
However,recreate notification requires us to read the intent from the existing
notification. That requires API 18, and BIND_NOTIFICATION_LISTENER_SERVICE
permission, which is not desirable.

https://codereview.chromium.org/1993873003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:106:
NEXT_DOWNLOAD_NOTIFICATION_ID, STARTING_NOTIFICATION_ID);
On 2016/05/20 00:02:13, Ted C wrote:
> +4 indent

Done.

[Ted C, 2016-05-23 23:15:23]

lgtm

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

https://codereview.chromium.org/1993873003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:58:
private static final String NEXT_DOWNLOAD_NOTIFICATION_ID =
"NextDownloadNotificationId";
On 2016/05/23 23:03:13, qinmin wrote:
> On 2016/05/20 00:02:13, Ted C wrote:
> > Instead of Next, I would use V2
> 
> Since I am keeping the namespace, V2 is probably not very suitable.

got it...I was thinking it was a namespace...do'h

[qinmin, 2016-05-25 23:32:15]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-05-25 23:32:15]

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/1993873003/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-25 23:32:46]

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

[commit-bot: I haz the power, 2016-05-26 00:29:57]

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

[commit-bot: I haz the power, 2016-05-26 00:29:58]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[qinmin, 2016-05-26 18:27:49]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-05-26 18:27:50]

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/1993873003/#ps60001
(title: "fix tests")

[commit-bot: I haz the power, 2016-05-26 18:28:21]

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

[commit-bot: I haz the power, 2016-05-26 19:33:15]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-26 19:35:13]

Description was changed from

==========
deprecate notificationId for downloads

The downloadId is going to be deprecated, and it is replaced by downloadGuid.
Android currently use the downloadId as notificationId.
This should also be deprecated.
This change generates a new notificationId for each newly added
download.
And a SharedPref entry is used to record the next notificationId to be generated

BUG=613014
==========

to

==========
deprecate notificationId for downloads

The downloadId is going to be deprecated, and it is replaced by downloadGuid.
Android currently use the downloadId as notificationId.
This should also be deprecated.
This change generates a new notificationId for each newly added
download.
And a SharedPref entry is used to record the next notificationId to be generated

BUG=613014

Committed: https://crrev.com/b935b5d8c1675d06e6733483ba5ff56ed22906ae
Cr-Commit-Position: refs/heads/master@{#396254}
==========

[commit-bot: I haz the power, 2016-05-26 19:35:14]

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