Fixes to enable indication of bytes downloaded for Offline Pages in Download Home.

This fixes issues that prevent progress indication for Offline Pages in Download Home:
- The OfflinePageItemWrapper has to produce status string.
- The DownloadUIAdapter wrong hookup was causing byte counts to not update.

The David's CL https://codereview.chromium.org/2704853002/ is still necessary as it adds buttons hookup. 


BUG=681269
Review-Url: https://codereview.chromium.org/2716013003
Cr-Commit-Position: refs/heads/master@{#453431}
Committed: https://chromium.googlesource.com/chromium/src/+/07bec82ec92e1047614b17192569f69260945d42

[Dmitry Titov, 2017-02-25 03:07:02]

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

[commit-bot: I haz the power, 2017-02-25 03:07:20]

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-25 03:14:16]

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

[commit-bot: I haz the power, 2017-02-25 03:14:17]

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

[Dmitry Titov, 2017-02-25 03:15:29]

Description was changed from

==========
Fixes to enable indication of bytes downloaded for Offline Pages in Download
Home.

BUG=681269
==========

to

==========
This fixes issues that prevent progress indication for Offline Pages in Download
Home:
- The OfflinePageItemWrapper has to produce status string.
- The DownloadUIAdapter wrong hookup was causing byte counts to not update.

The David's CL https://codereview.chromium.org/2704853002/ is still necessary as
it adds buttons hookup. 


BUG=681269
==========

[Dmitry Titov, 2017-02-25 03:18:20]

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

[commit-bot: I haz the power, 2017-02-25 03:18: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-02-25 03:20:24]

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

[Dmitry Titov, 2017-02-25 03:20:24]

PTAL:
twellington@: DownloadHistoryItemWrapper.java
fgorski@: Offlline Pages

Thanks!

[commit-bot: I haz the power, 2017-02-25 04:04:50]

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

[commit-bot: I haz the power, 2017-02-25 04:04:51]

Dry run: This issue passed the CQ dry run.

[Theresa, 2017-02-27 16:01:52]

DownloadHistoryItemWrapper lgtm % nit

https://codereview.chromium.org/2716013003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryItemWrapper.java
(right):

https://codereview.chromium.org/2716013003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryItemWrapper.java:421:
static final int[] BYTES_DOWNLOADED_STRINGS = {R.string.file_size_downloaded_kb,
Can you use the string array in DownloadUtils rather than copying it here?

[Pete Williamson, 2017-02-27 16:13:56]

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

[Pete Williamson, 2017-02-27 16:13:57]

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

https://codereview.chromium.org/2716013003/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.cc:40:
DownloadUIAdapter::FromOfflinePageModel(offline_page_model_);
Should we be making a similar change to the background loader offliner?

[fgorski, 2017-02-27 16:22:56]

lgtm with comments.

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

https://codereview.chromium.org/2716013003/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.cc:40:
DownloadUIAdapter::FromOfflinePageModel(offline_page_model_);
On 2017/02/27 16:13:56, Pete Williamson wrote:
> Should we be making a similar change to the background loader offliner?

Yes, if similar implementation is in place it causes trouble.

https://codereview.chromium.org/2716013003/diff/40001/components/offline_page...
File components/offline_pages/core/downloads/download_ui_adapter.cc (right):

https://codereview.chromium.org/2716013003/diff/40001/components/offline_page...
components/offline_pages/core/downloads/download_ui_adapter.cc:20: // Value of
this constant doesn't matter, only its address is used.
Are you not worried that compiler might optimize this address from under this
code?

https://codereview.chromium.org/2716013003/diff/40001/components/offline_page...
components/offline_pages/core/downloads/download_ui_adapter.cc:28:
OfflinePageModel* model) {
dcheck(model); please

https://codereview.chromium.org/2716013003/diff/40001/components/offline_page...
components/offline_pages/core/downloads/download_ui_adapter.cc:35:
OfflinePageModel* model) {
dcheck both please.

https://codereview.chromium.org/2716013003/diff/40001/components/offline_page...
File components/offline_pages/core/downloads/download_ui_adapter.h (right):

https://codereview.chromium.org/2716013003/diff/40001/components/offline_page...
components/offline_pages/core/downloads/download_ui_adapter.h:81: //
TODO(dimich): Refactor all pieces of OfflinePages into a single service
I think we have a bug for it. Todo is not necessary here.

Since this is public, you might want to add documentation instead.

[Dmitry Titov, 2017-02-27 20:02:16]

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

[Dmitry Titov, 2017-02-27 20:02:18]

David, could you do OWNERS look at DownloadUtils.java? I made strings for
DownloadUtils.getStringForBytes() public to use for OfflienPages...

https://codereview.chromium.org/2716013003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryItemWrapper.java
(right):

https://codereview.chromium.org/2716013003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryItemWrapper.java:421:
static final int[] BYTES_DOWNLOADED_STRINGS = {R.string.file_size_downloaded_kb,
On 2017/02/27 16:01:52, Theresa wrote:
> Can you use the string array in DownloadUtils rather than copying it here?

Done. Had to make it public which brings more OWNERS in :)

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

https://codereview.chromium.org/2716013003/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.cc:40:
DownloadUIAdapter::FromOfflinePageModel(offline_page_model_);
On 2017/02/27 16:22:55, fgorski wrote:
> On 2017/02/27 16:13:56, Pete Williamson wrote:
> > Should we be making a similar change to the background loader offliner?
> 
> Yes, if similar implementation is in place it causes trouble.

If you mean adding the ability for Background Loader to report network bytes,
this is not yet implemented and Cathy is looking into this. It will be using a
different mechanism because there is no prerenderer.

https://codereview.chromium.org/2716013003/diff/40001/components/offline_page...
File components/offline_pages/core/downloads/download_ui_adapter.cc (right):

https://codereview.chromium.org/2716013003/diff/40001/components/offline_page...
components/offline_pages/core/downloads/download_ui_adapter.cc:20: // Value of
this constant doesn't matter, only its address is used.
On 2017/02/27 16:22:55, fgorski wrote:
> Are you not worried that compiler might optimize this address from under this
> code?

Shouldn't since the address of this is passed and stored in a map, later used
for look up - if it was optimized out it'd be a bug, right?

https://codereview.chromium.org/2716013003/diff/40001/components/offline_page...
components/offline_pages/core/downloads/download_ui_adapter.cc:28:
OfflinePageModel* model) {
On 2017/02/27 16:22:55, fgorski wrote:
> dcheck(model); please

Done.

https://codereview.chromium.org/2716013003/diff/40001/components/offline_page...
components/offline_pages/core/downloads/download_ui_adapter.cc:35:
OfflinePageModel* model) {
On 2017/02/27 16:22:55, fgorski wrote:
> dcheck both please.

Done.

https://codereview.chromium.org/2716013003/diff/40001/components/offline_page...
File components/offline_pages/core/downloads/download_ui_adapter.h (right):

https://codereview.chromium.org/2716013003/diff/40001/components/offline_page...
components/offline_pages/core/downloads/download_ui_adapter.h:81: //
TODO(dimich): Refactor all pieces of OfflinePages into a single service
On 2017/02/27 16:22:56, fgorski wrote:
> I think we have a bug for it. Todo is not necessary here.
> 
> Since this is public, you might want to add documentation instead.

Done. Removed TODO, added more info/context to http://crbug.com/685362

[Dmitry Titov, 2017-02-27 22:05:15]

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

[commit-bot: I haz the power, 2017-02-27 22:05:49]

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

[David Trainor- moved to gerrit, 2017-02-27 22:18:48]

lgtm!

[Dmitry Titov, 2017-02-27 22:30:35]

The CQ bit was unchecked by dimich@chromium.org

[Dmitry Titov, 2017-02-27 22:30:41]

The CQ bit was checked by dimich@chromium.org

[Dmitry Titov, 2017-02-27 22:30:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2716013003/#ps60001
(title: "nits and comments")

[commit-bot: I haz the power, 2017-02-27 22:31:29]

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-27 23:12:19]

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

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

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

[Dmitry Titov, 2017-02-28 00:09:15]

The CQ bit was checked by dimich@chromium.org

[Dmitry Titov, 2017-02-28 00:09:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, dtrainor@chromium.org, fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2716013003/#ps80001
(title: "fix build break")

[commit-bot: I haz the power, 2017-02-28 00:09:41]

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-28 01:08:59]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488240555048620,
"parent_rev": "21ca7c7f71295c31a418e6dfe5dc03148e7bb9b9", "commit_rev":
"07bec82ec92e1047614b17192569f69260945d42"}

[commit-bot: I haz the power, 2017-02-28 01:10:17]

Description was changed from

==========
This fixes issues that prevent progress indication for Offline Pages in Download
Home:
- The OfflinePageItemWrapper has to produce status string.
- The DownloadUIAdapter wrong hookup was causing byte counts to not update.

The David's CL https://codereview.chromium.org/2704853002/ is still necessary as
it adds buttons hookup. 


BUG=681269
==========

to

==========
This fixes issues that prevent progress indication for Offline Pages in Download
Home:
- The OfflinePageItemWrapper has to produce status string.
- The DownloadUIAdapter wrong hookup was causing byte counts to not update.

The David's CL https://codereview.chromium.org/2704853002/ is still necessary as
it adds buttons hookup. 


BUG=681269

Review-Url: https://codereview.chromium.org/2716013003
Cr-Commit-Position: refs/heads/master@{#453431}
Committed:
https://chromium.googlesource.com/chromium/src/+/07bec82ec92e1047614b17192569...
==========

[commit-bot: I haz the power, 2017-02-28 01:10:18]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/07bec82ec92e1047614b17192569...

[Dmitry Titov, 2017-02-28 01:21:56]

Theresa and David: I had to add a helper function to DownloadsUtil, because
FindBugs marked "public static final array" as a problem, which it is (looks
like a constant but can be modified outside).

I've added a helper function to DownloadUtils instead. Let me know if you'd like
to see something else there.