Downloads : Last access time update for NTP, duplicate infobar and notifications

Downloads : Last access time update for NTP and notifications

This CL tries to update the last access time for a download item correctly.
The download items have several clients and the access does not go through
a common code path. One option is to look at the file system's last access
time (atime in Unix) and update it from a non-UI thread task. However, it
was found that the last access time is not updated or at best lazily updated
by most of the POSIX systems in case of a file read. So the other approach
is to update the last access time manually whenever the file is opened
through chrome. NTP, download infobar, duplicate download infobar,
download home and download notification are the entry points where
we fire an intent to open the downloaded file. To find the download
item, we need a GUID and the correct profile which was passed correctly
to the appropriate call sites.

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

[shaktisahu, 2017-03-07 06:31:01]

Description was changed from

==========
Downloads : Last access time update for NTP, duplicate infobar and notifications


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

to

==========
Downloads : Last access time update for NTP, duplicate infobar and notifications

This CL tries to update the last access time for a download item correctly.
The download items have several clients and the access does not go through
a common code path. One option is to look at the file system's last access
time (atime in Unix) and update it from a non-UI thread task. However, I
found that the last access time is not updated or at best lazily updated
by most of the POSIX systems in case of a file read. So the other approach
is to update the last access time manually whenever the file is opened
through chrome. NTP, download infobar, duplicate download infobar,
download home and download notification are the entry points where
we fire an intent to open the downloaded file. To find the download
item, we need a GUID and the correct profile which was passed correctly
to the appropriate call sites.

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

[shaktisahu, 2017-03-07 06:36:04]

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

[shaktisahu, 2017-03-07 06:36:04]

qinmin@ - PTAL.

I haven't fixed the mock* files or any failing test cases yet for the change,
which I will add later in this CL. I tested infobar, duplicate infobar and NTP.
Notifications was not tested as it fails due to an existing bug about correct
thread assertion.

[dgn, 2017-03-07 13:26:52]

drive-by question

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

https://codereview.chromium.org/2737723002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:503:
service.updateLastAccessTime(downloadGuid, isOffTheRecord);
Incognito downloads are still visible in the downloads manager UI in the regular
profile right? Is the profile relevant for downloads in Android then?

In Zine we don't really pay attention to the profile used to requests and always
assume it's going to be the regular one. Trying find if we might have bugs
related to that.

[shaktisahu, 2017-03-08 02:36:05]

Description was changed from

==========
Downloads : Last access time update for NTP, duplicate infobar and notifications

This CL tries to update the last access time for a download item correctly.
The download items have several clients and the access does not go through
a common code path. One option is to look at the file system's last access
time (atime in Unix) and update it from a non-UI thread task. However, I
found that the last access time is not updated or at best lazily updated
by most of the POSIX systems in case of a file read. So the other approach
is to update the last access time manually whenever the file is opened
through chrome. NTP, download infobar, duplicate download infobar,
download home and download notification are the entry points where
we fire an intent to open the downloaded file. To find the download
item, we need a GUID and the correct profile which was passed correctly
to the appropriate call sites.

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

to

==========
Downloads : Last access time update for NTP, duplicate infobar and notifications

This CL tries to update the last access time for a download item correctly.
The download items have several clients and the access does not go through
a common code path. One option is to look at the file system's last access
time (atime in Unix) and update it from a non-UI thread task. However, I
found that the last access time is not updated or at best lazily updated
by most of the POSIX systems in case of a file read. So the other approach
is to update the last access time manually whenever the file is opened
through chrome. NTP, download infobar, duplicate download infobar,
download home and download notification are the entry points where
we fire an intent to open the downloaded file. To find the download
item, we need a GUID and the correct profile which was passed correctly
to the appropriate call sites.

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

[shaktisahu, 2017-03-08 02:36:06]

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

[shaktisahu, 2017-03-08 02:49:17]

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

https://codereview.chromium.org/2737723002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:503:
service.updateLastAccessTime(downloadGuid, isOffTheRecord);
On 2017/03/07 13:26:51, dgn wrote:
> Incognito downloads are still visible in the downloads manager UI in the
regular
> profile right? Is the profile relevant for downloads in Android then?
> 
> In Zine we don't really pay attention to the profile used to requests and
always
> assume it's going to be the regular one. Trying find if we might have bugs
> related to that.

It seems like incognito downloads are not visible in the regular profile. In
fact they seem to get deleted when chrome is restarted. Not sure what is the
expectation here though.

[shaktisahu, 2017-03-08 02:49:28]

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

[commit-bot: I haz the power, 2017-03-08 02:49: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-08 03:04:42]

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

[commit-bot: I haz the power, 2017-03-08 03:04:43]

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

[dgn, 2017-03-08 11:02:08]

dgn@chromium.org changed reviewers:
+ dgn@chromium.org

[dgn, 2017-03-08 11:02:09]

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

https://codereview.chromium.org/2737723002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:503:
service.updateLastAccessTime(downloadGuid, isOffTheRecord);
On 2017/03/08 02:49:17, shaktisahu wrote:
> On 2017/03/07 13:26:51, dgn wrote:
> > Incognito downloads are still visible in the downloads manager UI in the
> regular
> > profile right? Is the profile relevant for downloads in Android then?
> > 
> > In Zine we don't really pay attention to the profile used to requests and
> always
> > assume it's going to be the regular one. Trying find if we might have bugs
> > related to that.
> 
> It seems like incognito downloads are not visible in the regular profile. In
> fact they seem to get deleted when chrome is restarted. Not sure what is the
> expectation here though.

Ah you're right, the downloaded file is only visible in Chrome when opening the
Download Manager from the incognito tab, so it disappears from the history when
the incognito profile is closed. So I suppose for Zine we just have to make sure
we are using the regular profile then. All right, thanks!

[qinmin, 2017-03-09 08:32:57]

https://codereview.chromium.org/2737723002/diff/1/chrome/browser/android/down...
File
chrome/browser/android/download/chrome_duplicate_download_infobar_delegate.cc
(right):

https://codereview.chromium.org/2737723002/diff/1/chrome/browser/android/down...
chrome/browser/android/download/chrome_duplicate_download_infobar_delegate.cc:118:
DCHECK(download_item_);
I am wondering whether you really need to do this for duplicate infobar.
For duplicate infobar, the download hasn't started yet. The file exists on the
sdcard may belong to another download GUID, or it could be downloaded by another
software. 
So it seems wrong to associate this new GUID with that download.

[David Trainor- moved to gerrit, 2017-03-09 17:29:30]

lgtm % min's comment

[shaktisahu, 2017-03-10 02:31:18]

Description was changed from

==========
Downloads : Last access time update for NTP, duplicate infobar and notifications

This CL tries to update the last access time for a download item correctly.
The download items have several clients and the access does not go through
a common code path. One option is to look at the file system's last access
time (atime in Unix) and update it from a non-UI thread task. However, I
found that the last access time is not updated or at best lazily updated
by most of the POSIX systems in case of a file read. So the other approach
is to update the last access time manually whenever the file is opened
through chrome. NTP, download infobar, duplicate download infobar,
download home and download notification are the entry points where
we fire an intent to open the downloaded file. To find the download
item, we need a GUID and the correct profile which was passed correctly
to the appropriate call sites.

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

to

==========
Downloads : Last access time update for NTP and notifications

This CL tries to update the last access time for a download item correctly.
The download items have several clients and the access does not go through
a common code path. One option is to look at the file system's last access
time (atime in Unix) and update it from a non-UI thread task. However, it
was found that the last access time is not updated or at best lazily updated
by most of the POSIX systems in case of a file read. So the other approach
is to update the last access time manually whenever the file is opened
through chrome. NTP, download infobar, duplicate download infobar,
download home and download notification are the entry points where
we fire an intent to open the downloaded file. To find the download
item, we need a GUID and the correct profile which was passed correctly
to the appropriate call sites.

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

[shaktisahu, 2017-03-10 02:36:59]

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

[commit-bot: I haz the power, 2017-03-10 02:37:31]

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

[shaktisahu, 2017-03-10 02:39:59]

qinmin@ - PTAL. I removed the duplicate infobar changes.

https://codereview.chromium.org/2737723002/diff/1/chrome/browser/android/down...
File
chrome/browser/android/download/chrome_duplicate_download_infobar_delegate.cc
(right):

https://codereview.chromium.org/2737723002/diff/1/chrome/browser/android/down...
chrome/browser/android/download/chrome_duplicate_download_infobar_delegate.cc:118:
DCHECK(download_item_);
On 2017/03/09 08:32:57, qinmin wrote:
> I am wondering whether you really need to do this for duplicate infobar.
> For duplicate infobar, the download hasn't started yet. The file exists on the
> sdcard may belong to another download GUID, or it could be downloaded by
another
> software. 
> So it seems wrong to associate this new GUID with that download.

I see. The GUID is of the new created item whereas the file path is one of the
suggested file paths of one of the already downloaded files.
So it would be wrong to associate this GUID since it is not downloaded yet.

However I feel, we should be updating the last access time of the GUID of the
existing download which is being opened. It seems complicated to get that GUID,
so I will skip that in this patch. wdyt?

[commit-bot: I haz the power, 2017-03-10 02:44:43]

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

[commit-bot: I haz the power, 2017-03-10 02:44:45]

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

[shaktisahu, 2017-03-10 03:40:40]

Patchset #3 (id:40001) has been deleted

[shaktisahu, 2017-03-10 03:40:47]

Patchset #3 (id:60001) has been deleted

[shaktisahu, 2017-03-10 03:41:24]

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

[commit-bot: I haz the power, 2017-03-10 03:41:38]

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-10 04:09:48]

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

[commit-bot: I haz the power, 2017-03-10 04:09:49]

Dry run: Try jobs failed on following builders:
  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-03-10 04:47:56]

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

[commit-bot: I haz the power, 2017-03-10 04:48:16]

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-10 05:30:10]

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

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

Dry run: This issue passed the CQ dry run.

[shaktisahu, 2017-03-10 05:32:00]

Patchset #3 (id:80001) has been deleted

[qinmin, 2017-03-10 07:27:27]

lgtm

[shaktisahu, 2017-03-10 07:55:55]

Description was changed from

==========
Downloads : Last access time update for NTP and notifications

This CL tries to update the last access time for a download item correctly.
The download items have several clients and the access does not go through
a common code path. One option is to look at the file system's last access
time (atime in Unix) and update it from a non-UI thread task. However, it
was found that the last access time is not updated or at best lazily updated
by most of the POSIX systems in case of a file read. So the other approach
is to update the last access time manually whenever the file is opened
through chrome. NTP, download infobar, duplicate download infobar,
download home and download notification are the entry points where
we fire an intent to open the downloaded file. To find the download
item, we need a GUID and the correct profile which was passed correctly
to the appropriate call sites.

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

to

==========
Downloads : Last access time update for NTP and notifications

This CL tries to update the last access time for a download item correctly.
The download items have several clients and the access does not go through
a common code path. One option is to look at the file system's last access
time (atime in Unix) and update it from a non-UI thread task. However, it
was found that the last access time is not updated or at best lazily updated
by most of the POSIX systems in case of a file read. So the other approach
is to update the last access time manually whenever the file is opened
through chrome. NTP, download infobar, duplicate download infobar,
download home and download notification are the entry points where
we fire an intent to open the downloaded file. To find the download
item, we need a GUID and the correct profile which was passed correctly
to the appropriate call sites.

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

[shaktisahu, 2017-03-10 07:55:55]

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

[shaktisahu, 2017-03-10 07:58:13]

vitaliii@ - PTAL at components/ntp_snippets/ and chrome/browser/ntp_snippets/

[vitaliii, 2017-03-10 08:15:46]

ntp -> lgtm % nits

Thank you!

https://codereview.chromium.org/2737723002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
(right):

https://codereview.chromium.org/2737723002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:496:
Intent viewIntent = createViewIntentForDownloadItem(getUriForItem(file),
mimeType);
Why not to update last access time before "return true"?

It looks like returning true here means that the file has been (or will be)
opened outside of Chrome.

https://codereview.chromium.org/2737723002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/content_suggestion.h (right):

https://codereview.chromium.org/2737723002/diff/100001/components/ntp_snippet...
components/ntp_snippets/content_suggestion.h:24: DownloadSuggestionExtra(const
DownloadSuggestionExtra&);
Why do you need a copy constructor here?

[shaktisahu, 2017-03-10 08:46:56]

https://codereview.chromium.org/2737723002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
(right):

https://codereview.chromium.org/2737723002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:496:
Intent viewIntent = createViewIntentForDownloadItem(getUriForItem(file),
mimeType);
On 2017/03/10 08:15:46, vitaliii wrote:
> Why not to update last access time before "return true"?
> 
> It looks like returning true here means that the file has been (or will be)
> opened outside of Chrome.

Actually, it presents a choice of apps to the user and user may or may not
select to open the file, which we have no way to know. I am not sure whether to
update the last access time in that case, may be we should.

https://codereview.chromium.org/2737723002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/content_suggestion.h (right):

https://codereview.chromium.org/2737723002/diff/100001/components/ntp_snippet...
components/ntp_snippets/content_suggestion.h:24: DownloadSuggestionExtra(const
DownloadSuggestionExtra&);
On 2017/03/10 08:15:46, vitaliii wrote:
> Why do you need a copy constructor here?

To make the compiler happy. It complained about something like copy constructor
needed for complex structs.

[vitaliii, 2017-03-10 09:04:38]

https://codereview.chromium.org/2737723002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
(right):

https://codereview.chromium.org/2737723002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:496:
Intent viewIntent = createViewIntentForDownloadItem(getUriForItem(file),
mimeType);
On 2017/03/10 08:46:55, shaktisahu wrote:
> On 2017/03/10 08:15:46, vitaliii wrote:
> > Why not to update last access time before "return true"?
> > 
> > It looks like returning true here means that the file has been (or will be)
> > opened outside of Chrome.
> 
> Actually, it presents a choice of apps to the user and user may or may not
> select to open the file, which we have no way to know. I am not sure whether
to
> update the last access time in that case, may be we should.

I see. However, the user has clicked on the item to open it, so I think that
this can be considered as an access.

https://codereview.chromium.org/2737723002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/content_suggestion.h (right):

https://codereview.chromium.org/2737723002/diff/100001/components/ntp_snippet...
components/ntp_snippets/content_suggestion.h:24: DownloadSuggestionExtra(const
DownloadSuggestionExtra&);
On 2017/03/10 08:46:55, shaktisahu wrote:
> On 2017/03/10 08:15:46, vitaliii wrote:
> > Why do you need a copy constructor here?
> 
> To make the compiler happy. It complained about something like copy
constructor
> needed for complex structs.

I am not sure what you mean. I just patched the CL and removed the copy
constructor and everything compiled fine (signed build for Android). 

I just cannot understand why adding another std::string would make the struct
complex (especially taking into account |std::string mime_type|).

[shaktisahu, 2017-03-10 15:23:01]

https://codereview.chromium.org/2737723002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
(right):

https://codereview.chromium.org/2737723002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:496:
Intent viewIntent = createViewIntentForDownloadItem(getUriForItem(file),
mimeType);
On 2017/03/10 09:04:38, vitaliii wrote:
> On 2017/03/10 08:46:55, shaktisahu wrote:
> > On 2017/03/10 08:15:46, vitaliii wrote:
> > > Why not to update last access time before "return true"?
> > > 
> > > It looks like returning true here means that the file has been (or will
be)
> > > opened outside of Chrome.
> > 
> > Actually, it presents a choice of apps to the user and user may or may not
> > select to open the file, which we have no way to know. I am not sure whether
> to
> > update the last access time in that case, may be we should.
> 
> I see. However, the user has clicked on the item to open it, so I think that
> this can be considered as an access.

Ok. I will add that, although technically this is not an access until the file
is read.

https://codereview.chromium.org/2737723002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/content_suggestion.h (right):

https://codereview.chromium.org/2737723002/diff/100001/components/ntp_snippet...
components/ntp_snippets/content_suggestion.h:24: DownloadSuggestionExtra(const
DownloadSuggestionExtra&);
On 2017/03/10 09:04:38, vitaliii wrote:
> On 2017/03/10 08:46:55, shaktisahu wrote:
> > On 2017/03/10 08:15:46, vitaliii wrote:
> > > Why do you need a copy constructor here?
> > 
> > To make the compiler happy. It complained about something like copy
> constructor
> > needed for complex structs.
> 
> I am not sure what you mean. I just patched the CL and removed the copy
> constructor and everything compiled fine (signed build for Android). 
> 
> I just cannot understand why adding another std::string would make the struct
> complex (especially taking into account |std::string mime_type|).

This error comes up during compiling download_suggestions_provider_unittest.cc
in linux. See the bot failure in patch 1 for linux_chromium_rel_ng.
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux...

[vitaliii, 2017-03-10 16:41:21]

https://codereview.chromium.org/2737723002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
(right):

https://codereview.chromium.org/2737723002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:496:
Intent viewIntent = createViewIntentForDownloadItem(getUriForItem(file),
mimeType);
On 2017/03/10 15:23:01, shaktisahu wrote:
> On 2017/03/10 09:04:38, vitaliii wrote:
> > On 2017/03/10 08:46:55, shaktisahu wrote:
> > > On 2017/03/10 08:15:46, vitaliii wrote:
> > > > Why not to update last access time before "return true"?
> > > > 
> > > > It looks like returning true here means that the file has been (or will
> be)
> > > > opened outside of Chrome.
> > > 
> > > Actually, it presents a choice of apps to the user and user may or may not
> > > select to open the file, which we have no way to know. I am not sure
whether
> > to
> > > update the last access time in that case, may be we should.
> > 
> > I see. However, the user has clicked on the item to open it, so I think that
> > this can be considered as an access.
> 
> Ok. I will add that, although technically this is not an access until the file
> is read.

I think that this approach is better. Otherwise, you would ignore opening of a
file when it is really opened.

https://codereview.chromium.org/2737723002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/content_suggestion.h (right):

https://codereview.chromium.org/2737723002/diff/100001/components/ntp_snippet...
components/ntp_snippets/content_suggestion.h:24: DownloadSuggestionExtra(const
DownloadSuggestionExtra&);
On 2017/03/10 15:23:01, shaktisahu wrote:
> On 2017/03/10 09:04:38, vitaliii wrote:
> > On 2017/03/10 08:46:55, shaktisahu wrote:
> > > On 2017/03/10 08:15:46, vitaliii wrote:
> > > > Why do you need a copy constructor here?
> > > 
> > > To make the compiler happy. It complained about something like copy
> > constructor
> > > needed for complex structs.
> > 
> > I am not sure what you mean. I just patched the CL and removed the copy
> > constructor and everything compiled fine (signed build for Android). 
> > 
> > I just cannot understand why adding another std::string would make the
struct
> > complex (especially taking into account |std::string mime_type|).
> 
> This error comes up during compiling download_suggestions_provider_unittest.cc
> in linux. See the bot failure in patch 1 for linux_chromium_rel_ng.
>
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux...

ACK

[shaktisahu, 2017-03-10 18:15:54]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2017-03-10 18:15:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, vitaliii@chromium.org, qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/2737723002/#ps120001
(title: "more")

[commit-bot: I haz the power, 2017-03-10 18:16:28]

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-10 18:25:38]

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

[commit-bot: I haz the power, 2017-03-10 18:25:40]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[shaktisahu, 2017-03-10 19:17:54]

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

[shaktisahu, 2017-03-10 19:17:55]

dfalcantara@ - PTAL at DuplicateDownloadInfoBar.java

[gone, 2017-03-10 22:03:53]

The infobar lgtm.

[shaktisahu, 2017-03-10 22:38:39]

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

[commit-bot: I haz the power, 2017-03-10 22:39:14]

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-10 23:02:57]

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

[commit-bot: I haz the power, 2017-03-10 23:02:58]

Dry run: This issue passed the CQ dry run.

[shaktisahu, 2017-03-10 23:08:54]

The CQ bit was checked by shaktisahu@chromium.org

[commit-bot: I haz the power, 2017-03-10 23:09:24]

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-10 23:14:13]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489187334295320,
"parent_rev": "2e87029682690de48907b97369a45718a46c0c92", "commit_rev":
"aa394d93072283f792da83ae88412562169539e3"}

[commit-bot: I haz the power, 2017-03-10 23:14:50]

Description was changed from

==========
Downloads : Last access time update for NTP and notifications

This CL tries to update the last access time for a download item correctly.
The download items have several clients and the access does not go through
a common code path. One option is to look at the file system's last access
time (atime in Unix) and update it from a non-UI thread task. However, it
was found that the last access time is not updated or at best lazily updated
by most of the POSIX systems in case of a file read. So the other approach
is to update the last access time manually whenever the file is opened
through chrome. NTP, download infobar, duplicate download infobar,
download home and download notification are the entry points where
we fire an intent to open the downloaded file. To find the download
item, we need a GUID and the correct profile which was passed correctly
to the appropriate call sites.

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

to

==========
Downloads : Last access time update for NTP and notifications

This CL tries to update the last access time for a download item correctly.
The download items have several clients and the access does not go through
a common code path. One option is to look at the file system's last access
time (atime in Unix) and update it from a non-UI thread task. However, it
was found that the last access time is not updated or at best lazily updated
by most of the POSIX systems in case of a file read. So the other approach
is to update the last access time manually whenever the file is opened
through chrome. NTP, download infobar, duplicate download infobar,
download home and download notification are the entry points where
we fire an intent to open the downloaded file. To find the download
item, we need a GUID and the correct profile which was passed correctly
to the appropriate call sites.

BUG=689981

Review-Url: https://codereview.chromium.org/2737723002
Cr-Commit-Position: refs/heads/master@{#456211}
Committed:
https://chromium.googlesource.com/chromium/src/+/aa394d93072283f792da83ae8841...
==========

[commit-bot: I haz the power, 2017-03-10 23:14:51]

Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/aa394d93072283f792da83ae8841...