Fix the Download Notifications for Offline Pages to indicate bytes loaded.

Fix the Download Notifications for Offline Pages to indicate bytes loaded.

This wires the UpdateProgress call from PrerenderingOffliner via DownloadUIAdapter to the DownloadNotifyingObserver. As a result, the Android notifications about ongoing Offline Page downloads start to be updated with a real byte count.

BUG=698020
Review-Url: https://codereview.chromium.org/2736843002
Cr-Commit-Position: refs/heads/master@{#455486}
Committed: https://chromium.googlesource.com/chromium/src/+/0e2f6cb4e64a3d6c251e9aa782927cffb829a5d3

[Dmitry Titov, 2017-03-07 00:02:01]

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

[commit-bot: I haz the power, 2017-03-07 00:03:21]

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

[Dmitry Titov, 2017-03-07 00:04:25]

dimich@chromium.org changed reviewers:
+ fgorski@chromium.org, qinmin@chromium.org

[Dmitry Titov, 2017-03-07 00:04:26]

PTAL:

fgorski: Offline pages, overall
qinmin: DownloadNotificationService.java

[Dmitry Titov, 2017-03-07 00:06:25]

Description was changed from

==========
Fix the Download Notifications for Offline Pages to indicate bytes loaded.

BUG=698020
==========

to

==========
Fix the Download Notifications for Offline Pages to indicate bytes loaded.

This wires the UpdateProgress call from PrerenderingOffliner via
DownloadUIAdapter to the DownloadNotifyingObserver. As a result, the Android
notifications about ongoing Offline Page downloads start to be updated with a
real byte count.

BUG=698020
==========

[commit-bot: I haz the power, 2017-03-07 01:59:01]

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

[commit-bot: I haz the power, 2017-03-07 01:59:02]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[qinmin, 2017-03-07 07:06:32]

lgtm % comment

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

https://codereview.chromium.org/2736843002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:502:
|| isDownloadPending || isOfflinePage;
rather than using isOfflinePage, you should still use percentage ==
DownloadItem.INVALID_DOWNLOAD_PERCENTAGE. Having a invalid percentage and a
non-zero received bytes is normal for download. Since it is common for http
response to not have a content-length field.

[Dmitry Titov, 2017-03-07 21:48:47]

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

[commit-bot: I haz the power, 2017-03-07 21:50:27]

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

[Dmitry Titov, 2017-03-07 21:55:30]

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

[commit-bot: I haz the power, 2017-03-07 21:56:30]

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

[Dmitry Titov, 2017-03-07 21:58:07]

Changed a bit the routing of the network progress. The previous variant only
worked when the Download Home was open (and the DownloadUIAdapter had its cache
of items loaded in RAM) and didn't work when it was closed :)

Now the OnNetworkProgress is a callback on RequestCoordinator::Observer and all
observerrs have access to it, so there is no need to tunnel it through
DownloadUIAdapter. This also simplifies the overall routing of this
notification.

PTAL!

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

https://codereview.chromium.org/2736843002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:502:
|| isDownloadPending || isOfflinePage;
On 2017/03/07 07:06:32, qinmin wrote:
> rather than using isOfflinePage, you should still use percentage ==
> DownloadItem.INVALID_DOWNLOAD_PERCENTAGE. Having a invalid percentage and a
> non-zero received bytes is normal for download. Since it is common for http
> response to not have a content-length field.

Ok, moved back to using DownloadItem.INVALID_DOWNLOAD_PERCENTAGE, which I've
made public to indicate 'indeterminate'.

[commit-bot: I haz the power, 2017-03-07 22:07:12]

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

[commit-bot: I haz the power, 2017-03-07 22:07:13]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[fgorski, 2017-03-07 22:26:43]

lgtm mod compilation issues and a few comments.

https://codereview.chromium.org/2736843002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadItem.java
(right):

https://codereview.chromium.org/2736843002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadItem.java:19:
public static final int INVALID_DOWNLOAD_PERCENTAGE = -1;
Is there any other condition causing invalid?

why not call it indeterminate_download_percentage if that is not the case?

...

https://codereview.chromium.org/2736843002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageNotificationBridge.java
(right):

https://codereview.chromium.org/2736843002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageNotificationBridge.java:78:
// To signal 'indeterminate' progress that doesn't have percentage value.
... this comment would probably not be necessary then

https://codereview.chromium.org/2736843002/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_offliner.cc (right):

https://codereview.chromium.org/2736843002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.cc:201:
pending_request_.reset(nullptr);
note: it seems like we could reset both callbacks as well, when we reset
pending_request_ (as in: let's not keep stuff around we could accidentally
call).

https://codereview.chromium.org/2736843002/diff/40001/components/offline_page...
File components/offline_pages/core/background/request_notifier.h (right):

https://codereview.chromium.org/2736843002/diff/40001/components/offline_page...
components/offline_pages/core/background/request_notifier.h:41: 
Nit: // Notifies observers of progress on |request|, which received
|received_bytes| at that point.

[Dmitry Titov, 2017-03-07 22:56:14]

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

[commit-bot: I haz the power, 2017-03-07 22:57:09]

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

[Dmitry Titov, 2017-03-07 23:16:30]

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

[commit-bot: I haz the power, 2017-03-07 23:17: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-03-07 23:42:51]

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

[commit-bot: I haz the power, 2017-03-07 23:42:52]

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...)

[Dmitry Titov, 2017-03-08 00:00:44]

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

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

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

[Dmitry Titov, 2017-03-08 00:47:38]

https://codereview.chromium.org/2736843002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadItem.java
(right):

https://codereview.chromium.org/2736843002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadItem.java:19:
public static final int INVALID_DOWNLOAD_PERCENTAGE = -1;
On 2017/03/07 22:26:43, fgorski wrote:
> Is there any other condition causing invalid?
> 
> why not call it indeterminate_download_percentage if that is not the case?
> 
> ...

Renamed, hope Min concurs.

https://codereview.chromium.org/2736843002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageNotificationBridge.java
(right):

https://codereview.chromium.org/2736843002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageNotificationBridge.java:78:
// To signal 'indeterminate' progress that doesn't have percentage value.
On 2017/03/07 22:26:43, fgorski wrote:
> ... this comment would probably not be necessary then

Done.

https://codereview.chromium.org/2736843002/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_offliner.cc (right):

https://codereview.chromium.org/2736843002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.cc:201:
pending_request_.reset(nullptr);
On 2017/03/07 22:26:43, fgorski wrote:
> note: it seems like we could reset both callbacks as well, when we reset
> pending_request_ (as in: let's not keep stuff around we could accidentally
> call).

Couple of reasons I think it probably should stay as it is:

- there is no good way to reset it other than to assign another copy of an empty
callback, like = base::Bind([] {}); which looks weird...

- there are multiple places here that only reset the request, and most of those
should not reset callbacks,at least at first look. It would be indeed good to
reset all 'pending request state' at right moments but a bigger/more careful
change is required for that.

https://codereview.chromium.org/2736843002/diff/40001/components/offline_page...
File components/offline_pages/core/background/request_notifier.h (right):

https://codereview.chromium.org/2736843002/diff/40001/components/offline_page...
components/offline_pages/core/background/request_notifier.h:41: 
On 2017/03/07 22:26:43, fgorski wrote:
> Nit: // Notifies observers of progress on |request|, which received
> |received_bytes| at that point.

Done.

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

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

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

Dry run: This issue passed the CQ dry run.

[Dmitry Titov, 2017-03-08 17:37:25]

The CQ bit was checked by dimich@chromium.org

[Dmitry Titov, 2017-03-08 17:37:25]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org,
fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2736843002/#ps100001
(title: "more fixes to more tests.")

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

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 17:44:04]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488994645104820,
"parent_rev": "1a3ea6d2b334319300f45f1bc74b9db0de0e7d67", "commit_rev":
"0e2f6cb4e64a3d6c251e9aa782927cffb829a5d3"}

[commit-bot: I haz the power, 2017-03-08 17:44:47]

Description was changed from

==========
Fix the Download Notifications for Offline Pages to indicate bytes loaded.

This wires the UpdateProgress call from PrerenderingOffliner via
DownloadUIAdapter to the DownloadNotifyingObserver. As a result, the Android
notifications about ongoing Offline Page downloads start to be updated with a
real byte count.

BUG=698020
==========

to

==========
Fix the Download Notifications for Offline Pages to indicate bytes loaded.

This wires the UpdateProgress call from PrerenderingOffliner via
DownloadUIAdapter to the DownloadNotifyingObserver. As a result, the Android
notifications about ongoing Offline Page downloads start to be updated with a
real byte count.

BUG=698020

Review-Url: https://codereview.chromium.org/2736843002
Cr-Commit-Position: refs/heads/master@{#455486}
Committed:
https://chromium.googlesource.com/chromium/src/+/0e2f6cb4e64a3d6c251e9aa78292...
==========

[commit-bot: I haz the power, 2017-03-08 17:44:48]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/0e2f6cb4e64a3d6c251e9aa78292...

[fgorski, 2017-03-08 17:46:24]

still lgtm and glad it landed

https://codereview.chromium.org/2736843002/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_offliner.cc (right):

https://codereview.chromium.org/2736843002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.cc:201:
pending_request_.reset(nullptr);
On 2017/03/08 00:47:38, Dmitry Titov wrote:
> On 2017/03/07 22:26:43, fgorski wrote:
> > note: it seems like we could reset both callbacks as well, when we reset
> > pending_request_ (as in: let's not keep stuff around we could accidentally
> > call).
> 
> Couple of reasons I think it probably should stay as it is:
> 
> - there is no good way to reset it other than to assign another copy of an
empty
> callback, like = base::Bind([] {}); which looks weird...
> 
> - there are multiple places here that only reset the request, and most of
those
> should not reset callbacks,at least at first look. It would be indeed good to
> reset all 'pending request state' at right moments but a bigger/more careful
> change is required for that.

Makes sense. Thanks for looking into it.