[Downloads] Introduce GUIDs for downloads.

[Downloads] Introduce GUIDs for downloads.

Chrome on Android and extensions attempt to keep references to downloads
across browser sessions using download IDs. This isn't safe since a
subsequent browser session could reuse a download ID to refer to a
different download.

In order to provide a weak cross-session reference, this CL introduces
GUIDs. At worst, attempting to lookup a download via a GUID will fail
rather than end up referring to a different download. This assumes that
there will not be any conflicts among the GUIDs, which is apparently a
safe assumption to make.

This CL includes the following changes:

* Introduce a GetGuid() method to DownloadItem and its subclasses.

* Allow DownloadItemImpl to create a new GUID for itself for new
  downloads using base::GenerateGuid().

* Store and retrieve GUIDs from downloads history for persisted
  downloads.

* Generate GUIDs for downloads in the history DB which are being
  migrated due to a browser upgrade.

* Since we are revving the history DB schema, also adds the fields for
  HTTP method and hash fields.

  Note that in order to keep the size of the CL sane, code for
  incorporating HTTP method and hash into the downloads system is not
  included in this CL. The latter is
  https://codereview.chromium.org/1751603002 .

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

[asanka, 2016-03-11 20:15:05]

Description was changed from

==========
[Downloads] Introduce GUIDs for downloads.

Chrome on Android and extensions attempt to keep references to downloads
across browser sessions using download IDs. This isn't safe since a
subsequent browser session could reuse a download ID to refer to a
different download.

In order to provide a weak cross-session reference, this CL introduces
GUIDs. At worse, attempting to lookup a download via a GUID will fail
rather than end up referring to a different download. This assumes that
there will not be any conflicts among the GUIDs, which is apparently a
safe assumption to make.

This CL includes the following changes:

* Introduce a GetGuid() method to DownloadItem and its subclasses.

* Allow DownloadItemImpl to create a new GUID for itself for new
  downloads.

* Store and retrieve GUIDs from downloads history for persisted
  downloads.

* Generate GUIDs for downloads in the history DB which are being
  migrated due to a browser upgrade.

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

to

==========
[Downloads] Introduce GUIDs for downloads.

Chrome on Android and extensions attempt to keep references to downloads
across browser sessions using download IDs. This isn't safe since a
subsequent browser session could reuse a download ID to refer to a
different download.

In order to provide a weak cross-session reference, this CL introduces
GUIDs. At worse, attempting to lookup a download via a GUID will fail
rather than end up referring to a different download. This assumes that
there will not be any conflicts among the GUIDs, which is apparently a
safe assumption to make.

This CL includes the following changes:

* Introduce a GetGuid() method to DownloadItem and its subclasses.

* Allow DownloadItemImpl to create a new GUID for itself for new
  downloads.

* Store and retrieve GUIDs from downloads history for persisted
  downloads.

* Generate GUIDs for downloads in the history DB which are being
  migrated due to a browser upgrade.

* Since we are revving the history DB schema, also adds the fields for
  HTTP method and hash fields.

  Note that in order to keep the size of the CL sane, code for
  incorporating HTTP method and hash into the downloads system is not
  included in this CL. The latter is
  https://codereview.chromium.org/1751603002 .

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

[asanka, 2016-03-11 20:24:04]

asanka@chromium.org changed reviewers:
+ jam@chromium.org, sky@chromium.org, svaldez@chromium.org

[asanka, 2016-03-11 20:25:13]

asanka@chromium.org changed reviewers:
+ asvitkine@chromium.org

[asanka, 2016-03-11 20:27:42]

svaldez:
  Everything!

asvitkine:
  tools/metrics/histograms/histograms.xml

jam:
  content/public/*

sky:
  chrome/browser/android/download/download_manager_service_unittest.cc
  chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc
 
chrome/browser/safe_browsing/incident_reporting/last_download_finder_unittest.cc
  components/history/core/*

[asanka, 2016-03-11 20:30:06]

Description was changed from

==========
[Downloads] Introduce GUIDs for downloads.

Chrome on Android and extensions attempt to keep references to downloads
across browser sessions using download IDs. This isn't safe since a
subsequent browser session could reuse a download ID to refer to a
different download.

In order to provide a weak cross-session reference, this CL introduces
GUIDs. At worse, attempting to lookup a download via a GUID will fail
rather than end up referring to a different download. This assumes that
there will not be any conflicts among the GUIDs, which is apparently a
safe assumption to make.

This CL includes the following changes:

* Introduce a GetGuid() method to DownloadItem and its subclasses.

* Allow DownloadItemImpl to create a new GUID for itself for new
  downloads.

* Store and retrieve GUIDs from downloads history for persisted
  downloads.

* Generate GUIDs for downloads in the history DB which are being
  migrated due to a browser upgrade.

* Since we are revving the history DB schema, also adds the fields for
  HTTP method and hash fields.

  Note that in order to keep the size of the CL sane, code for
  incorporating HTTP method and hash into the downloads system is not
  included in this CL. The latter is
  https://codereview.chromium.org/1751603002 .

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

to

==========
[Downloads] Introduce GUIDs for downloads.

Chrome on Android and extensions attempt to keep references to downloads
across browser sessions using download IDs. This isn't safe since a
subsequent browser session could reuse a download ID to refer to a
different download.

In order to provide a weak cross-session reference, this CL introduces
GUIDs. At worst, attempting to lookup a download via a GUID will fail
rather than end up referring to a different download. This assumes that
there will not be any conflicts among the GUIDs, which is apparently a
safe assumption to make.

This CL includes the following changes:

* Introduce a GetGuid() method to DownloadItem and its subclasses.

* Allow DownloadItemImpl to create a new GUID for itself for new
  downloads using base::GenerateGuid().

* Store and retrieve GUIDs from downloads history for persisted
  downloads.

* Generate GUIDs for downloads in the history DB which are being
  migrated due to a browser upgrade.

* Since we are revving the history DB schema, also adds the fields for
  HTTP method and hash fields.

  Note that in order to keep the size of the CL sane, code for
  incorporating HTTP method and hash into the downloads system is not
  included in this CL. The latter is
  https://codereview.chromium.org/1751603002 .

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

[sky, 2016-03-11 20:49:16]

LGTM

https://codereview.chromium.org/1781983002/diff/80001/chrome/browser/android/...
File chrome/browser/android/download/download_manager_service_unittest.cc
(right):

https://codereview.chromium.org/1781983002/diff/80001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service_unittest.cc:39:
ON_CALL(manager_, GetDownload(_))
UGH! gmock, UGH!

[Alexei Svitkine (slow), 2016-03-14 15:38:41]

lgtm

https://codereview.chromium.org/1781983002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1781983002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:9410: +  <summary>Time taken to create a
single download in the  history DB.</summary>
Nit: Remove extra space before history

https://codereview.chromium.org/1781983002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:9420: +  <summary>Time taken to update a
single download in the  history DB.</summary>
Ditto.

[jam, 2016-03-14 16:37:01]

On 2016/03/11 20:27:42, asanka wrote:

> jam:
>   content/public/*
lgtm

[svaldez, 2016-03-14 16:59:51]

lgtm.

https://codereview.chromium.org/1781983002/diff/80001/components/history/core...
File components/history/core/browser/download_row.cc (right):

https://codereview.chromium.org/1781983002/diff/80001/components/history/core...
components/history/core/browser/download_row.cc:72: bool
DownloadRow::operator==(const DownloadRow& rhs) const {
Are the // for formatting?

https://codereview.chromium.org/1781983002/diff/80001/content/public/browser/...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/1781983002/diff/80001/content/public/browser/...
content/public/browser/download_item.h:167: virtual DownloadState GetState()
const = 0;
Consider adding a comment here.

[asanka, 2016-03-14 19:17:54]

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

[commit-bot: I haz the power, 2016-03-14 19:18:23]

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

[asanka, 2016-03-14 19:18:33]

https://codereview.chromium.org/1781983002/diff/80001/chrome/browser/android/...
File chrome/browser/android/download/download_manager_service_unittest.cc
(right):

https://codereview.chromium.org/1781983002/diff/80001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service_unittest.cc:39:
ON_CALL(manager_, GetDownload(_))
On 2016/03/11 at 20:49:16, sky wrote:
> UGH! gmock, UGH!

Should've warned about the accidental gmock exposure. :-)

https://codereview.chromium.org/1781983002/diff/80001/components/history/core...
File components/history/core/browser/download_row.cc (right):

https://codereview.chromium.org/1781983002/diff/80001/components/history/core...
components/history/core/browser/download_row.cc:72: bool
DownloadRow::operator==(const DownloadRow& rhs) const {
On 2016/03/14 at 16:59:50, svaldez wrote:
> Are the // for formatting?

Yeah. It was pretty unreadable when all the clauses were binpacked. For the
moment I locally futzed the .clang-format file and reformatted this section
without the comment markers which yielded what I wanted. But it'll go away if
someone 'git cl formats' this method.

https://codereview.chromium.org/1781983002/diff/80001/content/public/browser/...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/1781983002/diff/80001/content/public/browser/...
content/public/browser/download_item.h:167: virtual DownloadState GetState()
const = 0;
On 2016/03/14 at 16:59:50, svaldez wrote:
> Consider adding a comment here.

Done.

https://codereview.chromium.org/1781983002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1781983002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:9410: +  <summary>Time taken to create a
single download in the  history DB.</summary>
On 2016/03/14 at 15:38:40, Alexei Svitkine (very slow) wrote:
> Nit: Remove extra space before history

Done.

https://codereview.chromium.org/1781983002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:9420: +  <summary>Time taken to update a
single download in the  history DB.</summary>
On 2016/03/14 at 15:38:41, Alexei Svitkine (very slow) wrote:
> Ditto.

Done.

[commit-bot: I haz the power, 2016-03-14 20:08:54]

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

[commit-bot: I haz the power, 2016-03-14 20:08:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[asanka, 2016-03-14 20:35:02]

The CQ bit was checked by asanka@chromium.org

[asanka, 2016-03-14 20:35:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, svaldez@chromium.org, jam@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1781983002/#ps100001
(title: "Address comments")

[commit-bot: I haz the power, 2016-03-14 20:35:32]

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

[commit-bot: I haz the power, 2016-03-14 21:23:32]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-14 21:25:05]

Description was changed from

==========
[Downloads] Introduce GUIDs for downloads.

Chrome on Android and extensions attempt to keep references to downloads
across browser sessions using download IDs. This isn't safe since a
subsequent browser session could reuse a download ID to refer to a
different download.

In order to provide a weak cross-session reference, this CL introduces
GUIDs. At worst, attempting to lookup a download via a GUID will fail
rather than end up referring to a different download. This assumes that
there will not be any conflicts among the GUIDs, which is apparently a
safe assumption to make.

This CL includes the following changes:

* Introduce a GetGuid() method to DownloadItem and its subclasses.

* Allow DownloadItemImpl to create a new GUID for itself for new
  downloads using base::GenerateGuid().

* Store and retrieve GUIDs from downloads history for persisted
  downloads.

* Generate GUIDs for downloads in the history DB which are being
  migrated due to a browser upgrade.

* Since we are revving the history DB schema, also adds the fields for
  HTTP method and hash fields.

  Note that in order to keep the size of the CL sane, code for
  incorporating HTTP method and hash into the downloads system is not
  included in this CL. The latter is
  https://codereview.chromium.org/1751603002 .

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

to

==========
[Downloads] Introduce GUIDs for downloads.

Chrome on Android and extensions attempt to keep references to downloads
across browser sessions using download IDs. This isn't safe since a
subsequent browser session could reuse a download ID to refer to a
different download.

In order to provide a weak cross-session reference, this CL introduces
GUIDs. At worst, attempting to lookup a download via a GUID will fail
rather than end up referring to a different download. This assumes that
there will not be any conflicts among the GUIDs, which is apparently a
safe assumption to make.

This CL includes the following changes:

* Introduce a GetGuid() method to DownloadItem and its subclasses.

* Allow DownloadItemImpl to create a new GUID for itself for new
  downloads using base::GenerateGuid().

* Store and retrieve GUIDs from downloads history for persisted
  downloads.

* Generate GUIDs for downloads in the history DB which are being
  migrated due to a browser upgrade.

* Since we are revving the history DB schema, also adds the fields for
  HTTP method and hash fields.

  Note that in order to keep the size of the CL sane, code for
  incorporating HTTP method and hash into the downloads system is not
  included in this CL. The latter is
  https://codereview.chromium.org/1751603002 .

BUG=593020

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

[commit-bot: I haz the power, 2016-03-14 21:25:06]

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