Added last_access_time to DownloadItem and History DB

Added last_access_time to DownloadItem and History DB

Added the last access time for a download item. If the item has not
been accessed yet, it should default to 0. This involves adding a
column to the History DB and migrating the older versions to have
the same default value for this field.

Download home and DownloadSnackbarController currently open the
download item directly. Since they use the filename to open the
file directly, the native DownloadItem::Open() is not called from
android. So a public method UpdateLastAccessTime was added to
DownloadItem and hence DownloadServiceManager to directly update
last access time.

Probably we should change all the download open paths to use native
DownloadItem::Open in future.

BUG=689981
Review-Url: https://codereview.chromium.org/2705283003
Cr-Commit-Position: refs/heads/master@{#454548}
Committed: https://chromium.googlesource.com/chromium/src/+/60eb97fc48ad3a8df63c8b17aacb30985af52dd1

[shaktisahu, 2017-02-21 21:09:10]

shaktisahu@chromium.org changed reviewers:
+ dtrainor@chromium.org

[shaktisahu, 2017-02-21 21:09:11]

dtrainor@ - PTAL

[shaktisahu, 2017-02-21 22:12:27]

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

[commit-bot: I haz the power, 2017-02-21 22:13:34]

Dry run: 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, 2017-02-21 22:25:58]

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

[commit-bot: I haz the power, 2017-02-21 22:25:59]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[shaktisahu, 2017-02-21 23:01:48]

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

[commit-bot: I haz the power, 2017-02-21 23:04:53]

Dry run: 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, 2017-02-21 23:27:22]

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

[commit-bot: I haz the power, 2017-02-21 23:27:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shaktisahu, 2017-02-21 23:51:30]

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

[commit-bot: I haz the power, 2017-02-21 23:52:41]

Dry run: 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, 2017-02-22 00:30:35]

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

[commit-bot: I haz the power, 2017-02-22 00:30:36]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[shaktisahu, 2017-02-22 00:36:15]

Patchset #2 (id:20001) has been deleted

[shaktisahu, 2017-02-22 00:36:28]

Patchset #2 (id:40001) has been deleted

[shaktisahu, 2017-02-22 00:36:36]

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

[commit-bot: I haz the power, 2017-02-22 00:38:32]

Dry run: 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, 2017-02-22 01:08:28]

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

[commit-bot: I haz the power, 2017-02-22 01:08:28]

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

[shaktisahu, 2017-02-22 01:27:01]

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

[commit-bot: I haz the power, 2017-02-22 01:28:21]

Dry run: 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, 2017-02-22 01:51:33]

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

[commit-bot: I haz the power, 2017-02-22 01:51:34]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[shaktisahu, 2017-02-22 02:58:47]

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

[commit-bot: I haz the power, 2017-02-22 03:00:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shaktisahu, 2017-02-22 03:18:04]

Patchset #2 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-02-22 03:52:52]

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

[commit-bot: I haz the power, 2017-02-22 03:52:53]

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

[shaktisahu, 2017-02-22 04:57:25]

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

[commit-bot: I haz the power, 2017-02-22 04:58:09]

Dry run: 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, 2017-02-22 05:44:18]

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

[commit-bot: I haz the power, 2017-02-22 05:44:19]

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

[David Trainor- moved to gerrit, 2017-02-22 05:56:03]

looking good.  a few nits.

https://codereview.chromium.org/2705283003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(right):

https://codereview.chromium.org/2705283003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1072:
updateLastAccessTime(downloadInfo.getDownloadGuid());
Should this be in the postExecute of the broader call?  Is it here because we
don't have the download guid?

https://codereview.chromium.org/2705283003/diff/120001/chrome/browser/android...
File chrome/browser/android/download/download_manager_service.cc (right):

https://codereview.chromium.org/2705283003/diff/120001/chrome/browser/android...
chrome/browser/android/download/download_manager_service.cc:247:
content::DownloadManager* manager = GetDownloadManager(false);
Can we assume always not off the record?

https://codereview.chromium.org/2705283003/diff/120001/components/history/cor...
File components/history/core/browser/history_backend_db_unittest.cc (right):

https://codereview.chromium.org/2705283003/diff/120001/components/history/cor...
components/history/core/browser/history_backend_db_unittest.cc:799: base::Time
last_access_time2(start_time + base::TimeDelta::FromHours(5));
start_time2?

https://codereview.chromium.org/2705283003/diff/120001/content/browser/downlo...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2705283003/diff/120001/content/browser/downlo...
content/browser/download/download_item_impl.cc:215:
last_access_time_(base::Time()),
Don't need this

https://codereview.chromium.org/2705283003/diff/120001/content/browser/downlo...
content/browser/download/download_item_impl.cc:254:
last_access_time_(base::Time()),
Don't need this

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

https://codereview.chromium.org/2705283003/diff/120001/content/public/browser...
content/public/browser/download_item.h:411: virtual void UpdateLastAccessTime()
= 0;
Should this just be SetLastAccessTime(Time)?  That would allow us to eventually
read the file data from disk to see the access time for externally opened files.

[shaktisahu, 2017-02-22 06:45:24]

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

[commit-bot: I haz the power, 2017-02-22 06:46:40]

Dry run: 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, 2017-02-22 08:24:59]

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

[commit-bot: I haz the power, 2017-02-22 08:25:00]

Dry run: This issue passed the CQ dry run.

[shaktisahu, 2017-02-23 06:55:51]

https://codereview.chromium.org/2705283003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(right):

https://codereview.chromium.org/2705283003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1072:
updateLastAccessTime(downloadInfo.getDownloadGuid());
On 2017/02/22 05:56:03, David Trainor wrote:
> Should this be in the postExecute of the broader call?  Is it here because we
> don't have the download guid?

Yes. I didn't find the download guid for all the callers.

https://codereview.chromium.org/2705283003/diff/120001/chrome/browser/android...
File chrome/browser/android/download/download_manager_service.cc (right):

https://codereview.chromium.org/2705283003/diff/120001/chrome/browser/android...
chrome/browser/android/download/download_manager_service.cc:247:
content::DownloadManager* manager = GetDownloadManager(false);
On 2017/02/22 05:56:03, David Trainor wrote:
> Can we assume always not off the record?

Oops. I guess I have to pass the is_off_the_record from Java.

https://codereview.chromium.org/2705283003/diff/120001/components/history/cor...
File components/history/core/browser/history_backend_db_unittest.cc (right):

https://codereview.chromium.org/2705283003/diff/120001/components/history/cor...
components/history/core/browser/history_backend_db_unittest.cc:799: base::Time
last_access_time2(start_time + base::TimeDelta::FromHours(5));
On 2017/02/22 05:56:03, David Trainor wrote:
> start_time2?

Done.

https://codereview.chromium.org/2705283003/diff/120001/content/browser/downlo...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2705283003/diff/120001/content/browser/downlo...
content/browser/download/download_item_impl.cc:215:
last_access_time_(base::Time()),
On 2017/02/22 05:56:03, David Trainor wrote:
> Don't need this

Done.

https://codereview.chromium.org/2705283003/diff/120001/content/browser/downlo...
content/browser/download/download_item_impl.cc:254:
last_access_time_(base::Time()),
On 2017/02/22 05:56:03, David Trainor wrote:
> Don't need this

Done.

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

https://codereview.chromium.org/2705283003/diff/120001/content/public/browser...
content/public/browser/download_item.h:411: virtual void UpdateLastAccessTime()
= 0;
On 2017/02/22 05:56:03, David Trainor wrote:
> Should this just be SetLastAccessTime(Time)?  That would allow us to
eventually
> read the file data from disk to see the access time for externally opened
files.

Good idea. Done.

[David Trainor- moved to gerrit, 2017-02-23 21:21:21]

lgtm!

https://codereview.chromium.org/2705283003/diff/160001/chrome/browser/android...
File chrome/browser/android/download/download_manager_service.cc (right):

https://codereview.chromium.org/2705283003/diff/160001/chrome/browser/android...
chrome/browser/android/download/download_manager_service.cc:246: jlong
last_access_time,
We might not need this from Java, but I like that the native method takes the
time.  I'm fine either way though.

[shaktisahu, 2017-02-23 23:18:07]

cpu@ - PTAL

https://codereview.chromium.org/2705283003/diff/160001/chrome/browser/android...
File chrome/browser/android/download/download_manager_service.cc (right):

https://codereview.chromium.org/2705283003/diff/160001/chrome/browser/android...
chrome/browser/android/download/download_manager_service.cc:246: jlong
last_access_time,
On 2017/02/23 21:21:20, David Trainor wrote:
> We might not need this from Java, but I like that the native method takes the
> time.  I'm fine either way though.

Done. Removed from Java.

[shaktisahu, 2017-02-24 00:54:26]

shaktisahu@chromium.org changed reviewers:
+ cpu@chromium.org

[shaktisahu, 2017-02-24 00:54:27]

cpu@ - PTAL

[David Trainor- moved to gerrit, 2017-02-25 04:14:28]

Shakti can we add (here or to a future CL) a way to query this from the Java
download item?  e.g. push it up from native -> Java with the rest of the
download metadata?

[cpu_(ooo_6.6-7.5), 2017-02-25 22:08:49]

Hi, what in particular you want me to review? maybe content/public/browser? if
so looks good.

I am not an expert in any of the directories. I see you have David so you are
covered in downloads. Might I suggest sdefresne@ for comp/history?

[shaktisahu, 2017-02-25 22:29:00]

On 2017/02/25 22:08:49, cpu wrote:
> Hi, what in particular you want me to review? maybe content/public/browser? if
> so looks good.
> 
> I am not an expert in any of the directories. I see you have David so you are
> covered in downloads. Might I suggest sdefresne@ for comp/history?

Thank you. 
I basically needed review for downloads and components/history.
This kind of CL touches too many directories, so I thought of adding one
reviewer who can review it all :). And the tool showed your name as someone who
can give lgtm for all the remaining files.
But I will first add sdefresne@ for comp/history for a more detailed review.

[shaktisahu, 2017-02-25 22:46:30]

Description was changed from

==========
Added last_access_time to DownloadItem and History DB

Added the last access time for a download item. If the item has not
been accessed yet, it should default to 0. This involves adding a
column to the History DB and migrating the older versions to have
the same default value for this field.

Download home and DownloadSnackbarController currently open the
download item directly. Since they use the filename to open the
file directly, the native DownloadItem::Open() is not called from
android. So a public method UpdateLastAccessTime was added to
DownloadItem and hence DownloadServiceManager to directly update
last access time.

Probably we should change all the download open paths to use native
DownloadItem::Open in future.

BUG=689981
==========

to

==========
Added last_access_time to DownloadItem and History DB

Added the last access time for a download item. If the item has not
been accessed yet, it should default to 0. This involves adding a
column to the History DB and migrating the older versions to have
the same default value for this field.

Download home and DownloadSnackbarController currently open the
download item directly. Since they use the filename to open the
file directly, the native DownloadItem::Open() is not called from
android. So a public method UpdateLastAccessTime was added to
DownloadItem and hence DownloadServiceManager to directly update
last access time.

Probably we should change all the download open paths to use native
DownloadItem::Open in future.

BUG=689981
==========

[shaktisahu, 2017-02-25 22:46:31]

shaktisahu@chromium.org changed reviewers:
+ sdefresne@chromium.org

[shaktisahu, 2017-02-26 22:16:00]

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

[commit-bot: I haz the power, 2017-02-26 22:16:12]

Dry run: 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, 2017-02-26 23:03:23]

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

[commit-bot: I haz the power, 2017-02-26 23:03:24]

Dry run: This issue passed the CQ dry run.

[shaktisahu, 2017-02-28 05:50:18]

sdefresne@ - PTAL

[sdefresne, 2017-02-28 18:09:07]

https://codereview.chromium.org/2705283003/diff/200001/components/history/cor...
File components/history/core/browser/download_row.h (right):

https://codereview.chromium.org/2705283003/diff/200001/components/history/cor...
components/history/core/browser/download_row.h:49: const base::Time&
last_access,
base/time.h has a comment saying that all time classes should be passed by
value, can you change here and everywhere else?

// All time classes are copyable, assignable, and occupy 64-bits per instance.
// As a result, prefer passing them by value:
//   void MyFunction(TimeDelta arg);
// If circumstances require, you may also pass by const reference:
//   void MyFunction(const TimeDelta& arg);  // Not preferred.

https://cs.chromium.org/chromium/src/base/time/time.h?q=base::Time&sq=package...

[shaktisahu, 2017-02-28 21:07:54]

sdefresne@ - PTAL

https://codereview.chromium.org/2705283003/diff/200001/components/history/cor...
File components/history/core/browser/download_row.h (right):

https://codereview.chromium.org/2705283003/diff/200001/components/history/cor...
components/history/core/browser/download_row.h:49: const base::Time&
last_access,
On 2017/02/28 18:09:07, sdefresne wrote:
> base/time.h has a comment saying that all time classes should be passed by
> value, can you change here and everywhere else?
> 
> // All time classes are copyable, assignable, and occupy 64-bits per instance.
> // As a result, prefer passing them by value:
> //   void MyFunction(TimeDelta arg);
> // If circumstances require, you may also pass by const reference:
> //   void MyFunction(const TimeDelta& arg);  // Not preferred.
> 
>
https://cs.chromium.org/chromium/src/base/time/time.h?q=base::Time&sq=package...

Done. Thanks!

[sdefresne, 2017-02-28 21:48:12]

lgtm

[shaktisahu, 2017-02-28 23:04:19]

On 2017/02/28 21:48:12, sdefresne wrote:
> lgtm

cpu@ - Can you please take a look at content/public/ and chrome/browser
(remaining files)? It should be pretty straight forward change. Thanks!

[cpu_(ooo_6.6-7.5), 2017-03-03 04:48:32]

lgtm

[shaktisahu, 2017-03-03 05:13:51]

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

[commit-bot: I haz the power, 2017-03-03 05:14:05]

Dry run: 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, 2017-03-03 05:16:23]

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

[commit-bot: I haz the power, 2017-03-03 05:16:25]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[shaktisahu, 2017-03-03 08:01:28]

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

[commit-bot: I haz the power, 2017-03-03 08:01:53]

Dry run: 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, 2017-03-03 08:45:59]

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

[commit-bot: I haz the power, 2017-03-03 08:46:00]

Dry run: This issue passed the CQ dry run.

[shaktisahu, 2017-03-03 08:48:58]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2017-03-03 08:48:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, cpu@chromium.org, sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2705283003/#ps240001
(title: "rebase origin/master")

[commit-bot: I haz the power, 2017-03-03 08:49:13]

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, 2017-03-03 08:53:30]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1488530938533470,
"parent_rev": "f72ffad58839a50563b41ba70adf941b2633f1c1", "commit_rev":
"60eb97fc48ad3a8df63c8b17aacb30985af52dd1"}

[commit-bot: I haz the power, 2017-03-03 08:54:12]

Description was changed from

==========
Added last_access_time to DownloadItem and History DB

Added the last access time for a download item. If the item has not
been accessed yet, it should default to 0. This involves adding a
column to the History DB and migrating the older versions to have
the same default value for this field.

Download home and DownloadSnackbarController currently open the
download item directly. Since they use the filename to open the
file directly, the native DownloadItem::Open() is not called from
android. So a public method UpdateLastAccessTime was added to
DownloadItem and hence DownloadServiceManager to directly update
last access time.

Probably we should change all the download open paths to use native
DownloadItem::Open in future.

BUG=689981
==========

to

==========
Added last_access_time to DownloadItem and History DB

Added the last access time for a download item. If the item has not
been accessed yet, it should default to 0. This involves adding a
column to the History DB and migrating the older versions to have
the same default value for this field.

Download home and DownloadSnackbarController currently open the
download item directly. Since they use the filename to open the
file directly, the native DownloadItem::Open() is not called from
android. So a public method UpdateLastAccessTime was added to
DownloadItem and hence DownloadServiceManager to directly update
last access time.

Probably we should change all the download open paths to use native
DownloadItem::Open in future.

BUG=689981

Review-Url: https://codereview.chromium.org/2705283003
Cr-Commit-Position: refs/heads/master@{#454548}
Committed:
https://chromium.googlesource.com/chromium/src/+/60eb97fc48ad3a8df63c8b17aacb...
==========

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

Committed patchset #10 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/60eb97fc48ad3a8df63c8b17aacb...