Files.app: Make ProgressCenterItemGroup class to manage the states of items.

Files.app: Make ProgressCenterItemGroup class to manage the states of items.

Previously the logic of progress center is implemented in ProgressCenter.  But
the logic does not follow the item's animation states so another logic is needed
in the ProgressCenterItemPanel and it is getting more complex.

This CL adds ProgressCenterItemGroup to manage the states of items. This class
responsible for creating the summarized item for the close view of progress
center and controling life time error items.

This CL also adds the test of the group. 
It does not start to use the group actually.

BUG=315438
TEST=file_browser_jstest

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243873

[hirono, 2014-01-08 08:11:23]

PTAL the CL?

In the future change, we need to introduce progress items that has low priority
for summarizing, which is used for the drive sync items.

So I'm planning to add the two ProgressCenterItemGroup (for normal and low
priority) to the ProgressCenterPanel class.

Thanks!

[mtomasz, 2014-01-08 08:33:57]

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
File
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js
(right):

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js:43:
this.animated_ = {};
nit: Why not using an Array as a set instead?

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js:65:
this.totalProgressMax_ = 0.0;
nit: 0.0 -> 0?

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js:82:
* @type {ProgressItemState}
nit: New line before #82, since there is already a line in #77?

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js:147:
ProgressCenterItemGroup.shouldAnimated_ = function(
nit: shouldAnimate?

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js:164:
* @type {ProgressCenterItemGroup.State}
nit: @type -> @return? WDYT?

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js:300:
* @param {number} numOtherErros Number of errors contained by other groups.
typo: Errors

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js:304:
function(numOtherErros) {
Do we need to pass this variable? I think group shouldn't know about other
groups. Is it needed?

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js:416:
for (var id in this.items_) {
nit: id -> index / i? Do we use id an iterating variable in Files app?

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js:418:
return;
This is difficult to me to understand. Why do you return when there is an error
which is non-error?

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js:419:
else
This else is not needed?

https://codereview.chromium.org/127763002/diff/1/chrome/test/data/file_manage...
File
chrome/test/data/file_manager/unit_tests/progress_center_item_group_unittest.js
(right):

https://codereview.chromium.org/127763002/diff/1/chrome/test/data/file_manage...
chrome/test/data/file_manager/unit_tests/progress_center_item_group_unittest.js:4:
'use strict';
Awesome!

https://codereview.chromium.org/127763002/diff/1/chrome/test/data/file_manage...
chrome/test/data/file_manager/unit_tests/progress_center_item_group_unittest.js:117:
assertEquals(true, group.isAnimated(item.id));
assertTrue

https://codereview.chromium.org/127763002/diff/1/chrome/test/data/file_manage...
chrome/test/data/file_manager/unit_tests/progress_center_item_group_unittest.js:146:
assertEquals(false, group.isAnimated(item.id));
assertFalse here and in other places for bools?

[hirono, 2014-01-09 04:43:42]

Thanks!

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
File
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js
(right):

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js:43:
this.animated_ = {};
On 2014/01/08 08:33:57, mtomasz wrote:
> nit: Why not using an Array as a set instead?

Because using a hash as a set makes adding or removing operations easy and
faster.

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js:65:
this.totalProgressMax_ = 0.0;
On 2014/01/08 08:33:57, mtomasz wrote:
> nit: 0.0 -> 0?

Done.

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js:82:
* @type {ProgressItemState}
On 2014/01/08 08:33:57, mtomasz wrote:
> nit: New line before #82, since there is already a line in #77?

Done.

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js:147:
ProgressCenterItemGroup.shouldAnimated_ = function(
On 2014/01/08 08:33:57, mtomasz wrote:
> nit: shouldAnimate?

Done.

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js:164:
* @type {ProgressCenterItemGroup.State}
On 2014/01/08 08:33:57, mtomasz wrote:
> nit: @type -> @return? WDYT?

I agree with you!

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js:300:
* @param {number} numOtherErros Number of errors contained by other groups.
On 2014/01/08 08:33:57, mtomasz wrote:
> typo: Errors

Done.

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js:304:
function(numOtherErros) {
On 2014/01/08 08:33:57, mtomasz wrote:
> Do we need to pass this variable? I think group shouldn't know about other
> groups. Is it needed?

It depends on the UI design.
Given that there are two groups normal and low priority's.
When normal group has items, the low priority items are not shown in the
summarized view.
For example, [Normal] CopyA, CopyB and [Low] DriveSync are summarized as "2
copying...".
But I think error items are exceptions of the rule.
[Normal] CopyA, CopyB and [Low] DriveSyncError should be summarized as "2
copying... 1 Error.".

This is why the normal priority group needs to know the number of error in the
low priority group.

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js:416:
for (var id in this.items_) {
On 2014/01/08 08:33:57, mtomasz wrote:
> nit: id -> index / i? Do we use id an iterating variable in Files app?

Because items_ is not an array and the items_'s key is ID of the item, I think
it is OK to use 'id' here.

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js:418:
return;
On 2014/01/08 08:33:57, mtomasz wrote:
> This is difficult to me to understand. Why do you return when there is an
error
> which is non-error?

If there is non-error item (progressing, or completed but still animated), we
should stay the active state.
I added a comment.

https://codereview.chromium.org/127763002/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/progress_center_item_group.js:419:
else
On 2014/01/08 08:33:57, mtomasz wrote:
> This else is not needed?

Done.

https://codereview.chromium.org/127763002/diff/1/chrome/test/data/file_manage...
File
chrome/test/data/file_manager/unit_tests/progress_center_item_group_unittest.js
(right):

https://codereview.chromium.org/127763002/diff/1/chrome/test/data/file_manage...
chrome/test/data/file_manager/unit_tests/progress_center_item_group_unittest.js:4:
'use strict';
On 2014/01/08 08:33:57, mtomasz wrote:
> Awesome!

A unit test is very helpful for the complex state machine :)

https://codereview.chromium.org/127763002/diff/1/chrome/test/data/file_manage...
chrome/test/data/file_manager/unit_tests/progress_center_item_group_unittest.js:117:
assertEquals(true, group.isAnimated(item.id));
On 2014/01/08 08:33:57, mtomasz wrote:
> assertTrue

Done.

https://codereview.chromium.org/127763002/diff/1/chrome/test/data/file_manage...
chrome/test/data/file_manager/unit_tests/progress_center_item_group_unittest.js:146:
assertEquals(false, group.isAnimated(item.id));
On 2014/01/08 08:33:57, mtomasz wrote:
> assertFalse here and in other places for bools?

Done.

[mtomasz, 2014-01-09 09:15:59]

lgtm!

[hirono, 2014-01-09 09:57:15]

On 2014/01/09 09:15:59, mtomasz wrote:
> lgtm!

Thanks!

[commit-bot: I haz the power, 2014-01-09 09:57:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hirono@chromium.org/127763002/190001

[commit-bot: I haz the power, 2014-01-09 12:45:11]

Change committed as 243873