Combine consecutive entry-changed events into one.

Combine consecutive entry-changed events into one.

BUG=383232
TEST=ran browser_tests, tested the performance manually.

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

[fukino, 2014-08-12 07:26:42]

@hirono - could you take a look?

I wonder if it is right to combine events in the event router. (maybe it should
be implemented in another class?)

[fukino, 2014-08-12 07:29:10]

https://codereview.chromium.org/463783002/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/directory_model.js (right):

https://codereview.chromium.org/463783002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_model.js:597:
this.getFileList().push(entriesToAdd);
This line is what I want to do.
this.getFileList().push() always make the list sorted, so I'd like to reduce the
count to call this push().

[hirono, 2014-08-12 08:45:43]

https://codereview.chromium.org/463783002/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/file_manager.js (right):

https://codereview.chromium.org/463783002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/file_manager.js:408:
'entries-changed', this.onEntryChangedBound_);
nit: Could you update the handler's name as well?

[fukino, 2014-08-12 09:35:43]

Fixed handler names, and updated a unit test which I forgot to upload. Could you
take a look? Thanks!

https://codereview.chromium.org/463783002/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/file_manager.js (right):

https://codereview.chromium.org/463783002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/file_manager.js:408:
'entries-changed', this.onEntryChangedBound_);
On 2014/08/12 08:45:43, hirono wrote:
> nit: Could you update the handler's name as well?

Done. Changed following handlers.
- FileManager.prototype.onEntryChanged_()
- DirectoryModel.prototype.onEntryChanged()

[hirono, 2014-08-12 10:43:13]

https://codereview.chromium.org/463783002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/directory_model.js (right):

https://codereview.chromium.org/463783002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_model.js:575: var
onEntriesCreated = function(entries) {
Maybe we don't need to prepare the function.

https://codereview.chromium.org/463783002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_model.js:578:
parentPromises.push(new Promise(function(resolve, reject) {
The name 'fulfill' is more common for the success callback of Promise.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...

https://codereview.chromium.org/463783002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_model.js:579:
entries[i].getParent(function(parentEntry) {
Please handle error of getParent.

https://codereview.chromium.org/463783002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_model.js:597:
this.getFileList().push(entriesToAdd);
ArrayDataModel#push does not take an Array.

[fukino, 2014-08-15 09:12:31]

Thank you for comments and I'm sorry for late update.

https://codereview.chromium.org/463783002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/directory_model.js (right):

https://codereview.chromium.org/463783002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_model.js:575: var
onEntriesCreated = function(entries) {
On 2014/08/12 10:43:13, hirono wrote:
> Maybe we don't need to prepare the function.

Done.

https://codereview.chromium.org/463783002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_model.js:578:
parentPromises.push(new Promise(function(resolve, reject) {
On 2014/08/12 10:43:13, hirono wrote:
> The name 'fulfill' is more common for the success callback of Promise.
> 
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
> 

I referred to the sample code of this page, and I thought resolve/reject was
more consistent with Promise.resolve() and Promise.reject(). WDYT?

https://codereview.chromium.org/463783002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_model.js:579:
entries[i].getParent(function(parentEntry) {
On 2014/08/12 10:43:13, hirono wrote:
> Please handle error of getParent.

Done.

https://codereview.chromium.org/463783002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_model.js:597:
this.getFileList().push(entriesToAdd);
On 2014/08/12 10:43:13, hirono wrote:
> ArrayDataModel#push does not take an Array.

Done. I didn't realize this serious bug... Thank you very much!

[hirono, 2014-08-15 09:13:59]

https://codereview.chromium.org/463783002/diff/60001/chrome/test/data/file_ma...
File chrome/test/data/file_manager/unit_tests/file_operation_manager_unittest.js
(right):

https://codereview.chromium.org/463783002/diff/60001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/file_operation_manager_unittest.js:242:
return event.type === 'entries-changed' &&
Sorry I added more tests here.
Could you rebase it?

[fukino, 2014-08-15 10:22:28]

Rebased, and update
FileOperationManager.EventRouter.prototype.sendProgressEvent() to pass the test.
(Please see the comment in
FileOperationManager.EventRouter.prototype.sendProgressEvent()).

https://codereview.chromium.org/463783002/diff/60001/chrome/test/data/file_ma...
File chrome/test/data/file_manager/unit_tests/file_operation_manager_unittest.js
(right):

https://codereview.chromium.org/463783002/diff/60001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/file_operation_manager_unittest.js:242:
return event.type === 'entries-changed' &&
On 2014/08/15 09:13:59, hirono wrote:
> Sorry I added more tests here.
> Could you rebase it?

Done.

https://codereview.chromium.org/463783002/diff/80001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/file_operation_manager.js
(right):

https://codereview.chromium.org/463783002/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/file_operation_manager.js:412:
this.entryChangedEventRateLimiter_.runImmediately();
To ensure that the entries-changed event is dispatched before the operation
finished(i.e. SUCCESS or CANCELED progress event is dispatched), pending
entries-changed event is flushed here. (This matters at least for a unit test)

[hirono, 2014-08-15 10:28:42]

lgtm with a nit!

https://codereview.chromium.org/463783002/diff/80001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/directory_model.js (right):

https://codereview.chromium.org/463783002/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_model.js:579:
parentPromises.push(new Promise(function(resolve, reject) {
nit: You can do

new Promise(function(resolve, reject) {
  entries[i].getParent(resolve, reject);
});

or even

new Promise(entries[i].getParent.bind(entries[i]));

[fukino, 2014-08-15 10:46:23]

https://codereview.chromium.org/463783002/diff/80001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/directory_model.js (right):

https://codereview.chromium.org/463783002/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_model.js:579:
parentPromises.push(new Promise(function(resolve, reject) {
On 2014/08/15 10:28:42, hirono wrote:
> nit: You can do
> 
> new Promise(function(resolve, reject) {
>   entries[i].getParent(resolve, reject);
> });
> 
> or even
> 
> new Promise(entries[i].getParent.bind(entries[i]));
> 

Wow, That's simple!
To avoid binding in the loop, I adopted former one.
Thanks!

[fukino, 2014-08-15 11:53:37]

The CQ bit was checked by fukino@chromium.org

[commit-bot: I haz the power, 2014-08-15 11:55:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fukino@chromium.org/463783002/100001

[commit-bot: I haz the power, 2014-08-15 13:51:28]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-15 16:21:40]

Committed patchset #6 (100001) as 289899