offline_items_collection : Added helper class to determine progress

offline_items_collection : Added helper class to determine progress

Added a struct in OfflineItem to contain the current progress value,
max value and progress unit. The progress unit can be bytes, files or
percent. The information should be set in native and subsequently it is
passed to java where various types of messages such as "x files left"
or "x seconds left" is shown in the notifications and download home.

BUG=718673
Review-Url: https://codereview.chromium.org/2861863002
Cr-Commit-Position: refs/heads/master@{#470683}
Committed: https://chromium.googlesource.com/chromium/src/+/fa97f9c932cf4a69a4952a3f962782906390a18e

[David Trainor- moved to gerrit, 2017-05-03 22:06:31]

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

[David Trainor- moved to gerrit, 2017-05-03 22:06:32]

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
File
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/OfflineItem.java
(right):

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/OfflineItem.java:19:
* Contains relevant pieces of information to determine the download progress
percentage for
Make this comment a bit more like the one above so that we can keep all of the
actual real comments in one place

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
File
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/bridges/OfflineItemBridge.java
(right):

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/bridges/OfflineItemBridge.java:11:
import org.chromium.components.offline_items_collection.OfflineItemState;
Add import
org.chromium.components.offline_items_collection.OfflineItemProgressUnit;

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/bridges/OfflineItemBridge.java:49:
long receivedBytes, long progressValue, long progressMax, int progressUnit,
Add @OfflineItemProgressUnit before "int progressUnit".

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
File components/offline_items_collection/core/offline_item.h (right):

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
components/offline_items_collection/core/offline_item.h:51: // download progress
percentage for an offline item to display in the UI.
remove "percentage"

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
components/offline_items_collection/core/offline_item.h:62: // The maximum value
of the download progress. -1 indicates indeterminate.
Can this just be a base::Optional?  If it's not there we will treat it as
indeterminate.

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
components/offline_items_collection/core/offline_item.h:145: // Represents the
current progress of this item.
Keep some of the information here.  The field is not used if |state| is
COMPLETE.

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
File components/offline_items_collection/core/offline_item_progress_unit.h
(right):

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
components/offline_items_collection/core/offline_item_progress_unit.h:12: enum
OfflineItemProgressUnit {
Does this have to be in a separate file?  Also, we should either prefix the
values or make this an "enum class".  I don't know if "enum class" works with
the JNI generator though so try that out first IMO :).

[shaktisahu, 2017-05-05 01:28:21]

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

[commit-bot: I haz the power, 2017-05-05 01:28:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shaktisahu, 2017-05-05 02:03:53]

Description was changed from

==========
offline_items_collection : Added helper class to determine progress

BUG=
==========

to

==========
offline_items_collection : Added helper class to determine progress

BUG=718673
==========

[shaktisahu, 2017-05-05 02:08:40]

Description was changed from

==========
offline_items_collection : Added helper class to determine progress

BUG=718673
==========

to

==========
offline_items_collection : Added helper class to determine progress

Added a struct in OfflineItem to contain the current progress value,
max value and progress unit. The progress unit can be bytes, files or
percent. The information should be set in native and subsequently it is
passed to java where various types of messages such as "x files left"
or "x seconds left" is shown in the notifications and download home.

BUG=718673
==========

[shaktisahu, 2017-05-05 02:09:37]

PTAL

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
File
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/OfflineItem.java
(right):

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/OfflineItem.java:19:
* Contains relevant pieces of information to determine the download progress
percentage for
On 2017/05/03 22:06:31, David Trainor-ping if over 24h wrote:
> Make this comment a bit more like the one above so that we can keep all of the
> actual real comments in one place

Done.

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
File
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/bridges/OfflineItemBridge.java
(right):

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/bridges/OfflineItemBridge.java:11:
import org.chromium.components.offline_items_collection.OfflineItemState;
On 2017/05/03 22:06:32, David Trainor-ping if over 24h wrote:
> Add import
> org.chromium.components.offline_items_collection.OfflineItemProgressUnit;

Done.

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/bridges/OfflineItemBridge.java:49:
long receivedBytes, long progressValue, long progressMax, int progressUnit,
On 2017/05/03 22:06:32, David Trainor-ping if over 24h wrote:
> Add @OfflineItemProgressUnit before "int progressUnit".

Done.

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
File components/offline_items_collection/core/offline_item.h (right):

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
components/offline_items_collection/core/offline_item.h:51: // download progress
percentage for an offline item to display in the UI.
On 2017/05/03 22:06:32, David Trainor-ping if over 24h wrote:
> remove "percentage"

Done.

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
components/offline_items_collection/core/offline_item.h:62: // The maximum value
of the download progress. -1 indicates indeterminate.
On 2017/05/03 22:06:32, David Trainor-ping if over 24h wrote:
> Can this just be a base::Optional?  If it's not there we will treat it as
> indeterminate.

Done.

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
components/offline_items_collection/core/offline_item.h:145: // Represents the
current progress of this item.
On 2017/05/03 22:06:32, David Trainor-ping if over 24h wrote:
> Keep some of the information here.  The field is not used if |state| is
> COMPLETE.

Done.

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
File components/offline_items_collection/core/offline_item_progress_unit.h
(right):

https://codereview.chromium.org/2861863002/diff/20001/components/offline_item...
components/offline_items_collection/core/offline_item_progress_unit.h:12: enum
OfflineItemProgressUnit {
On 2017/05/03 22:06:32, David Trainor-ping if over 24h wrote:
> Does this have to be in a separate file?  Also, we should either prefix the
> values or make this an "enum class".  I don't know if "enum class" works with
> the JNI generator though so try that out first IMO :).

Done.
Enum class works fine with JNI generator. We just have to use static_cast<int>
before passing to java.

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

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

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

Dry run: 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...)

[David Trainor- moved to gerrit, 2017-05-09 05:36:02]

lgtm % nits thanks!

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

https://codereview.chromium.org/2861863002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:594:
public static String getTimeOrFilesLeftString(
Should this just create the string completely and not just for files or time? 
If it's different for notifications and downloads home, should we just have two
different methods?

Just a nit, I'm fine either way.

https://codereview.chromium.org/2861863002/diff/70001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemView.java
(right):

https://codereview.chromium.org/2861863002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemView.java:195:
mDownloadPercentageView.setText(
Should we add a TODO and file a bug to investigate showing more detailed
information here?

https://codereview.chromium.org/2861863002/diff/70001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadNotificationServiceTest.java
(right):

https://codereview.chromium.org/2861863002/diff/70001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadNotificationServiceTest.java:435:
public void testFormatRemainingTime() {
Should we add a test validating the new DownloadUtils method too?  If you added
one later ignore this :).

https://codereview.chromium.org/2861863002/diff/70001/components/offline_item...
File
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/OfflineItem.java
(right):

https://codereview.chromium.org/2861863002/diff/70001/components/offline_item...
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/OfflineItem.java:63:
result = (int) (31 * result + (max == null ? 0 : max));
max.hashCode()?

https://codereview.chromium.org/2861863002/diff/70001/components/offline_item...
File components/offline_items_collection/core/android/offline_item_bridge.cc
(right):

https://codereview.chromium.org/2861863002/diff/70001/components/offline_item...
components/offline_items_collection/core/android/offline_item_bridge.cc:41:
item.progress.max.value_or(-1), static_cast<int>(item.progress.unit),
int -> jint?

[shaktisahu, 2017-05-09 18:55:17]

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

[commit-bot: I haz the power, 2017-05-09 18:56:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shaktisahu, 2017-05-09 19:04:31]

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

[shaktisahu, 2017-05-09 19:04:32]

twellington@ - PTAL at chrome/browser/download/ui/*.java

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

https://codereview.chromium.org/2861863002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:594:
public static String getTimeOrFilesLeftString(
On 2017/05/09 05:36:01, David Trainor-ping if over 24h wrote:
> Should this just create the string completely and not just for files or time? 
> If it's different for notifications and downloads home, should we just have
two
> different methods?
> 
> Just a nit, I'm fine either way.

Yes, the strings are different for notifications and downloads home. Not sure if
making two separate methods in DownloadUtils would look better though.

https://codereview.chromium.org/2861863002/diff/70001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemView.java
(right):

https://codereview.chromium.org/2861863002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemView.java:195:
mDownloadPercentageView.setText(
On 2017/05/09 05:36:01, David Trainor-ping if over 24h wrote:
> Should we add a TODO and file a bug to investigate showing more detailed
> information here?

I didn't get it. We are just showing the percentage here.

https://codereview.chromium.org/2861863002/diff/70001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadNotificationServiceTest.java
(right):

https://codereview.chromium.org/2861863002/diff/70001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadNotificationServiceTest.java:435:
public void testFormatRemainingTime() {
On 2017/05/09 05:36:01, David Trainor-ping if over 24h wrote:
> Should we add a test validating the new DownloadUtils method too?  If you
added
> one later ignore this :).

Done.

https://codereview.chromium.org/2861863002/diff/70001/components/offline_item...
File
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/OfflineItem.java
(right):

https://codereview.chromium.org/2861863002/diff/70001/components/offline_item...
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/OfflineItem.java:63:
result = (int) (31 * result + (max == null ? 0 : max));
On 2017/05/09 05:36:01, David Trainor-ping if over 24h wrote:
> max.hashCode()?

Done.

https://codereview.chromium.org/2861863002/diff/70001/components/offline_item...
File components/offline_items_collection/core/android/offline_item_bridge.cc
(right):

https://codereview.chromium.org/2861863002/diff/70001/components/offline_item...
components/offline_items_collection/core/android/offline_item_bridge.cc:41:
item.progress.max.value_or(-1), static_cast<int>(item.progress.unit),
On 2017/05/09 05:36:01, David Trainor-ping if over 24h wrote:
> int -> jint?

Done.

[shaktisahu, 2017-05-09 23:31:19]

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

[commit-bot: I haz the power, 2017-05-09 23:33:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2017-05-10 00:18:57]

download/ui lgtm

[commit-bot: I haz the power, 2017-05-10 00:31:07]

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

[commit-bot: I haz the power, 2017-05-10 00:31:08]

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

[shaktisahu, 2017-05-10 16:58:06]

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

[commit-bot: I haz the power, 2017-05-10 16:59:46]

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-05-10 19:36:01]

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

[commit-bot: I haz the power, 2017-05-10 19:36:02]

Dry run: This issue passed the CQ dry run.

[shaktisahu, 2017-05-10 20:08:14]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2017-05-10 20:08:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2861863002/#ps110001
(title: "comments")

[commit-bot: I haz the power, 2017-05-10 20:10:12]

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-05-10 20:25:28]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1494446894099190,
"parent_rev": "182427f7777cd8173f248d89ae18f8b44a94794e", "commit_rev":
"fa97f9c932cf4a69a4952a3f962782906390a18e"}

[commit-bot: I haz the power, 2017-05-10 20:25:40]

Description was changed from

==========
offline_items_collection : Added helper class to determine progress

Added a struct in OfflineItem to contain the current progress value,
max value and progress unit. The progress unit can be bytes, files or
percent. The information should be set in native and subsequently it is
passed to java where various types of messages such as "x files left"
or "x seconds left" is shown in the notifications and download home.

BUG=718673
==========

to

==========
offline_items_collection : Added helper class to determine progress

Added a struct in OfflineItem to contain the current progress value,
max value and progress unit. The progress unit can be bytes, files or
percent. The information should be set in native and subsequently it is
passed to java where various types of messages such as "x files left"
or "x seconds left" is shown in the notifications and download home.

BUG=718673

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

[commit-bot: I haz the power, 2017-05-10 20:25:41]

Committed patchset #7 (id:110001) as
https://chromium.googlesource.com/chromium/src/+/fa97f9c932cf4a69a4952a3f9627...