Stop updating FileSelection when not changed.

Stop updating FileSelection when not changed.

When copying files, entries-changed event is fired each time file is
copied.
When you keep selecting the source files, it leads to O(N^2) times
metadata update of selected files.
For some slow storage devices such as Media Transfer Protocol connected
ones, this severely slows down the entire file copy.
Ideally, this could also be fixed by letting CHANGE_THROTTLED listener
causing metadata update to properly use metadata cache.

BUG=712121
TEST=manually tested.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

[tetsui2, 2017-04-21 03:42:31]

Description was changed from

==========
Stop updating FileSelection when not changed.

When copying files, entries-changed event is fired each time file is
copied.
When you keep selecting the source files, it leads to O(N^2) times
metadata update of selected files.
For some slow storage devices such as Media Transfer Protocol connected
ones, this severely slows down the entire file copy.
Ideally, this could also be fixed by letting CHANGE_THROTTLED listener
causing metadata update to properly use metadata cache.

BUG=712121
TEST=manually tested.
==========

to

==========
Stop updating FileSelection when not changed.

When copying files, entries-changed event is fired each time file is
copied.
When you keep selecting the source files, it leads to O(N^2) times
metadata update of selected files.
For some slow storage devices such as Media Transfer Protocol connected
ones, this severely slows down the entire file copy.
Ideally, this could also be fixed by letting CHANGE_THROTTLED listener
causing metadata update to properly use metadata cache.

BUG=712121
TEST=manually tested.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tetsui2, 2017-04-21 03:46:57]

tetsui@google.com changed reviewers:
+ fukino@chromium.org, hirono@chromium.org

[tetsui2, 2017-04-21 03:46:58]

Please take a look.

[hirono, 2017-04-21 04:30:40]

https://codereview.chromium.org/2828373003/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/js/file_selection.js (right):

https://codereview.chromium.org/2828373003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/file_selection.js:186: if (indexes[i]
!== this.selection.indexes[i] ||
Do we need to compare indexes here?
Assume that file list is changed [A, B] -> [C, A, B].
The selection indexes are also changed from [0, 1] -> [1, 2] while selection
still keep containing A and B.

[tetsui2, 2017-04-21 04:34:50]

Thank you for reviewing.

https://codereview.chromium.org/2828373003/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/js/file_selection.js (right):

https://codereview.chromium.org/2828373003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/file_selection.js:186: if (indexes[i]
!== this.selection.indexes[i] ||
On 2017/04/21 04:30:39, hirono wrote:
> Do we need to compare indexes here?
> Assume that file list is changed [A, B] -> [C, A, B].
> The selection indexes are also changed from [0, 1] -> [1, 2] while selection
> still keep containing A and B.

Done.

[hirono, 2017-04-21 04:55:03]

https://codereview.chromium.org/2828373003/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/file_selection.js (right):

https://codereview.chromium.org/2828373003/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/file_selection.js:185: for (var i =
0; i < indexes.length && !isSelectionChanged; i++) {
Sorry one more thing. The selection change like [0, 1] -> [1, 0] can happen? If
so, can we skip dispatching events for this case as well?

[tetsui2, 2017-04-21 05:15:35]

https://codereview.chromium.org/2828373003/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/file_selection.js (right):

https://codereview.chromium.org/2828373003/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/file_selection.js:185: for (var i =
0; i < indexes.length && !isSelectionChanged; i++) {
On 2017/04/21 04:55:03, hirono wrote:
> Sorry one more thing. The selection change like [0, 1] -> [1, 0] can happen?
If
> so, can we skip dispatching events for this case as well?

It's okay to overlook some non-changes unless it's fired repeatedly.
I don't think it would happen. I also tried to select files and then change the
ordering but the event didn't fire.

[hirono, 2017-04-21 05:20:14]

lgtm. Thanks!

[hirono, 2017-04-21 05:22:41]

Regarding this comment from fukino-san, please also get lgtm from fukino.
https://bugs.chromium.org/p/chromium/issues/detail?id=712121#c4

[fukino, 2017-04-21 05:44:32]

Since this check has a small overhead, I'd like to keep it uncommitted until the
root cause is identified. (not lgtm)

https://codereview.chromium.org/2828373003/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/file_selection.js (right):

https://codereview.chromium.org/2828373003/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/file_selection.js:186: if
(entries[i].toURL() !== this.selection.entries[i].toURL()) {
nit: Let's extract a utility function to compare two entry arrays (like
util.isSameEntries) in ui/file_manager/common/js/util.js, and use
util.isSameEntry inside it.