Switch to use download GUID to indentify download items

Switch to use download GUID to indentify download items

Download GUID is introduced by
http://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1
Unlike download Id, the GUID can live across browser sessions and will
not be reused.
This change switches the java class to also use the
GUID.
Chrome will temporarily use the old download Id as the notification Id.
This is because the android notification requires an int to identify it,
rather than a string.
In the future, we will generate the notification Id from java side.

The download Ids generated by the Android DownloadManager are not
affected by this CL.
However, they are only used in OMA downloads and when we call
addCompletedDownload().

BUG=593020
Committed: https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5
Cr-Commit-Position: refs/heads/master@{#383443}

[qinmin, 2016-03-18 22:30:29]

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

[qinmin, 2016-03-18 22:30:30]

PTAL

[Ted C, 2016-03-22 00:20:06]

ufft...this makes somewhat hard to read code harder to read.

Can we isolate the ids into either a class or always encode them to strings and
isolate the boundary where we need ints to a much smaller subset of calls.

At any given point, I'm not sure what should be there or not.  Maybe they both
should be, but it is definitely not directly obvious.

Happy to discuss this a bit more in person though as maybe at first glance it
seems much, but maybe this is what needs to be done.

[qinmin, 2016-03-23 23:29:21]

On 2016/03/22 00:20:06, Ted C wrote:
> ufft...this makes somewhat hard to read code harder to read.
> 
> Can we isolate the ids into either a class or always encode them to strings
and
> isolate the boundary where we need ints to a much smaller subset of calls.
> 
> At any given point, I'm not sure what should be there or not.  Maybe they both
> should be, but it is definitely not directly obvious.
> 
> Happy to discuss this a bit more in person though as maybe at first glance it
> seems much, but maybe this is what needs to be done.

The download Id will finally go away if we move all download to chrome network
stack (some OMA download may still need it, but only after the descriptor is
parsed)
A better refactoring would be move all DownloadId related classes/methods to
DownloadManagerDelegate. But that require some effort to do.
I added some TODOs in the CL. But given the time, can we move forward with this
change first, and get back to the TODOs later?
Some minor changes:
1. moved PendingNotification from DownloadNotificationService to a standalone
file: DownloadSharedPreferenceEntry.
2. For completed download, added a wrapper class CompletedDownload.
   CompletedDownload class now represents the download items that has been added
to the Android DownloadManager, and has a valid download Id associated with
them.

[qinmin, 2016-03-23 23:31:16]

Patchset #2 (id:20001) has been deleted

[qinmin, 2016-03-24 07:34:47]

On 2016/03/23 23:29:21, qinmin wrote:
> On 2016/03/22 00:20:06, Ted C wrote:
> > ufft...this makes somewhat hard to read code harder to read.
> > 
> > Can we isolate the ids into either a class or always encode them to strings
> and
> > isolate the boundary where we need ints to a much smaller subset of calls.
> > 
> > At any given point, I'm not sure what should be there or not.  Maybe they
both
> > should be, but it is definitely not directly obvious.
> > 
> > Happy to discuss this a bit more in person though as maybe at first glance
it
> > seems much, but maybe this is what needs to be done.
> 
> The download Id will finally go away if we move all download to chrome network
> stack (some OMA download may still need it, but only after the descriptor is
> parsed)
> A better refactoring would be move all DownloadId related classes/methods to
> DownloadManagerDelegate. But that require some effort to do.
> I added some TODOs in the CL. But given the time, can we move forward with
this
> change first, and get back to the TODOs later?
> Some minor changes:
> 1. moved PendingNotification from DownloadNotificationService to a standalone
> file: DownloadSharedPreferenceEntry.
> 2. For completed download, added a wrapper class CompletedDownload.
>    CompletedDownload class now represents the download items that has been
added
> to the Android DownloadManager, and has a valid download Id associated with
> them.

Packaging DownloadInfo into a DownloadItem object. The DownloadItem can
represent download that goes through both Android DownloadManager and Chrome's
network stack.
However, this is just a cosmetic refactoring.
I am planning to move all the Android DownloadManager related stuff into
DownloadManagerDelegate. But the change will be much bigger than this one. So I
will keep that a separate CL.

[Ted C, 2016-03-24 20:52:21]

https://codereview.chromium.org/1809203006/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadSharedPreferenceEntry.java
(right):

https://codereview.chromium.org/1809203006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadSharedPreferenceEntry.java:39:
static DownloadSharedPreferenceEntry parseFromString(String sharedPrefString) {
all previous shared preferences wouldn't have the GUID (nor the metered network
bit) would they?

Do you need these to be able to parse between upgrades?  Do you clear out
invalid ones?

If we need these to be stable between versions, then I think you should add a
version bit to the front.

If they don't need to be stable between upgrades, can you add a comment
specifically stating that so
we can avoid having me ask this everytime :-)

https://codereview.chromium.org/1809203006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadSharedPreferenceEntry.java:69:
* xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx.
is there any verification done for GUIDs in native?  if so, could we share them?

[qinmin, 2016-03-24 22:56:07]

https://codereview.chromium.org/1809203006/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadSharedPreferenceEntry.java
(right):

https://codereview.chromium.org/1809203006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadSharedPreferenceEntry.java:39:
static DownloadSharedPreferenceEntry parseFromString(String sharedPrefString) {
On 2016/03/24 20:52:21, Ted C wrote:
> all previous shared preferences wouldn't have the GUID (nor the metered
network
> bit) would they?
> 
> Do you need these to be able to parse between upgrades?  Do you clear out
> invalid ones?
> 
> If we need these to be stable between versions, then I think you should add a
> version bit to the front.
> 
> If they don't need to be stable between upgrades, can you add a comment
> specifically stating that so
> we can avoid having me ask this everytime :-)

Yes, a version bit would be a good idea. Although right now we should treat all
previous SharedPreferences as invalid (and this feature is still not hitting
beta), but things might change later on.

https://codereview.chromium.org/1809203006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadSharedPreferenceEntry.java:69:
* xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx.
On 2016/03/24 20:52:21, Ted C wrote:
> is there any verification done for GUIDs in native?  if so, could we share
them?

Done. added GuidUtils.java to call native function in guid.cc

[qinmin, 2016-03-24 23:04:54]

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

[qinmin, 2016-03-24 23:04:55]

+yfriedman for base/android/ OWNER

[Ted C, 2016-03-25 00:18:02]

lgtm

https://codereview.chromium.org/1809203006/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadSharedPreferenceEntry.java
(right):

https://codereview.chromium.org/1809203006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadSharedPreferenceEntry.java:39:
static DownloadSharedPreferenceEntry parseFromString(String sharedPrefString) {
On 2016/03/24 22:56:07, qinmin wrote:
> On 2016/03/24 20:52:21, Ted C wrote:
> > all previous shared preferences wouldn't have the GUID (nor the metered
> network
> > bit) would they?
> > 
> > Do you need these to be able to parse between upgrades?  Do you clear out
> > invalid ones?
> > 
> > If we need these to be stable between versions, then I think you should add
a
> > version bit to the front.
> > 
> > If they don't need to be stable between upgrades, can you add a comment
> > specifically stating that so
> > we can avoid having me ask this everytime :-)
> 
> Yes, a version bit would be a good idea. Although right now we should treat
all
> previous SharedPreferences as invalid (and this feature is still not hitting
> beta), but things might change later on.

Let's add a TODO then

[Ted C, 2016-03-25 00:18:34]

https://codereview.chromium.org/1809203006/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadSharedPreferenceEntry.java
(right):

https://codereview.chromium.org/1809203006/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadSharedPreferenceEntry.java:18:
private static final int VERSION = 1;
oh you already did it...nevermind :-)  even better!

[qinmin, 2016-03-25 14:35:29]

qinmin@chromium.org changed reviewers:
+ mark@chromium.org, torne@chromium.org
- yfriedman@chromium.org

[qinmin, 2016-03-25 14:35:29]

+torne for base/android OWNER
+mark for base/OWNER

[Mark Mentovai, 2016-03-25 14:41:17]

LGTM in base. I didn’t look in base/android or at anything else.

[asanka, 2016-03-25 15:23:37]

On 2016/03/25 at 14:41:17, mark wrote:
> LGTM in base. I didn’t look in base/android or at anything else.

Not an objection, but base/guid* is RFC 4122 compliant. Any compliant validator
should work including java.util.UUID.

[Ted C, 2016-03-25 16:20:28]

On 2016/03/25 15:23:37, asanka wrote:
> On 2016/03/25 at 14:41:17, mark wrote:
> > LGTM in base. I didn’t look in base/android or at anything else.
> 
> Not an objection, but base/guid* is RFC 4122 compliant. Any compliant
validator
> should work including java.util.UUID.

If we can use UUID#fromString for validation, then let us do that.

[qinmin, 2016-03-25 16:29:21]

On 2016/03/25 16:20:28, Ted C wrote:
> On 2016/03/25 15:23:37, asanka wrote:
> > On 2016/03/25 at 14:41:17, mark wrote:
> > > LGTM in base. I didn’t look in base/android or at anything else.
> > 
> > Not an objection, but base/guid* is RFC 4122 compliant. Any compliant
> validator
> > should work including java.util.UUID.
> 
> If we can use UUID#fromString for validation, then let us do that.

No, we should not use UUID class. UUID.fromString() is not very strict on
checking the validity of the UUID as it didn't check the length of the string:
http://stackoverflow.com/questions/9768510/java-util-uuid-fromstring-not-chec...

In out test, I do use java.util.UUID to generate the GUIDs for simplicity

[qinmin, 2016-03-25 16:44:59]

On 2016/03/25 16:29:21, qinmin wrote:
> On 2016/03/25 16:20:28, Ted C wrote:
> > On 2016/03/25 15:23:37, asanka wrote:
> > > On 2016/03/25 at 14:41:17, mark wrote:
> > > > LGTM in base. I didn’t look in base/android or at anything else.
> > > 
> > > Not an objection, but base/guid* is RFC 4122 compliant. Any compliant
> > validator
> > > should work including java.util.UUID.
> > 
> > If we can use UUID#fromString for validation, then let us do that.
> 
> No, we should not use UUID class. UUID.fromString() is not very strict on
> checking the validity of the UUID as it didn't check the length of the string:
>
http://stackoverflow.com/questions/9768510/java-util-uuid-fromstring-not-chec...
> 
> In out test, I do use java.util.UUID to generate the GUIDs for simplicity

Ok, I found a way to do this with UUID. We can convert the String to UUID using
UUID.fromString(), and then convert it back to string using UUID#toString() and
do a case-insensitive comparison with the original string

[qinmin, 2016-03-25 22:54:12]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-03-25 22:54:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, mark@chromium.org
Link to the patchset: https://codereview.chromium.org/1809203006/#ps100001
(title: "rebase and move isValidGUID check into java")

[commit-bot: I haz the power, 2016-03-25 22:54:33]

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

[commit-bot: I haz the power, 2016-03-25 23:10:01]

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

[commit-bot: I haz the power, 2016-03-25 23:10:02]

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

[qinmin, 2016-03-25 23:43:10]

Patchset #5 (id:100001) has been deleted

[qinmin, 2016-03-26 00:21:00]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-03-26 00:21:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, mark@chromium.org
Link to the patchset: https://codereview.chromium.org/1809203006/#ps120001
(title: "rebase and move GUID check into java")

[commit-bot: I haz the power, 2016-03-26 00:21:19]

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

[commit-bot: I haz the power, 2016-03-26 01:28:27]

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

[commit-bot: I haz the power, 2016-03-26 01:28:28]

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-03-26 02:44:42]

Patchset #6 (id:140001) has been deleted

[qinmin, 2016-03-26 03:36:18]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-03-26 03:36:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, mark@chromium.org
Link to the patchset: https://codereview.chromium.org/1809203006/#ps160001
(title: "fix merge error")

[commit-bot: I haz the power, 2016-03-26 03:36:26]

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

[commit-bot: I haz the power, 2016-03-26 04:13:50]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2016-03-26 04:15:06]

Description was changed from

==========
Switch to use download GUID to indentify download items

Download GUID is introduced by
http://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1
Unlike download Id, the GUID can live across browser sessions and will
not be reused.
This change switches the java class to also use the
GUID.
Chrome will temporarily use the old download Id as the notification Id.
This is because the android notification requires an int to identify it,
rather than a string.
In the future, we will generate the notification Id from java side.

The download Ids generated by the Android DownloadManager are not
affected by this CL.
However, they are only used in OMA downloads and when we call
addCompletedDownload().

BUG=593020
==========

to

==========
Switch to use download GUID to indentify download items

Download GUID is introduced by
http://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1
Unlike download Id, the GUID can live across browser sessions and will
not be reused.
This change switches the java class to also use the
GUID.
Chrome will temporarily use the old download Id as the notification Id.
This is because the android notification requires an int to identify it,
rather than a string.
In the future, we will generate the notification Id from java side.

The download Ids generated by the Android DownloadManager are not
affected by this CL.
However, they are only used in OMA downloads and when we call
addCompletedDownload().

BUG=593020

Committed: https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5
Cr-Commit-Position: refs/heads/master@{#383443}
==========

[commit-bot: I haz the power, 2016-03-26 04:15:08]

Patchset 6 (id:??) landed as
https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5
Cr-Commit-Position: refs/heads/master@{#383443}