Merge navigation list and directory tree.

Merge navigation list and directory tree.

This CL merges navigation list (the very left pane) and directory tree (middle pane).
In the conbined tree, the first-level children are VolumeItem or ShortcutItem, which correspond to NavigationModelItem.
Only Drive volume has children and the Drive volume. They are DirectoryItem and the behavior is similar to previous directory tree.

BUG=390848, 397222
TEST=run browser_tests

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

[fukino, 2014-08-06 07:47:12]

yoshiki san, could you take a look?

[yoshiki, 2014-08-06 13:37:58]

lgtm with nits.

https://codereview.chromium.org/441233002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/directory_tree.js (right):

https://codereview.chromium.org/441233002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/directory_tree.js:89: '<div
class="tree-row">' +
nit: 4 space indent;

https://codereview.chromium.org/441233002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/directory_tree.js:590: * @return
{boolean} True if this volume can be removed by user operation.
nit: private

https://codereview.chromium.org/441233002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/directory_tree.js:607: if
(volumeInfo.volumeType == 'provided') {
nit: ===

https://codereview.chromium.org/441233002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/directory_tree.js:1028:
itemPromises.push(new Promise(function(resolve, reject) {
Please get "function(...) {}.bind()" out of loop. It creates new object each
calls and heavy.

https://codereview.chromium.org/441233002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/directory_tree.js:1096: * Invoked
when the volume list or shortcut list is changed.
nit: @private

https://codereview.chromium.org/441233002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/directory_tree.js:1117: 
nit: jsdoc

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

https://codereview.chromium.org/441233002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:1079: this.dialogType
!= DialogType.SELECT_SAVEAS_FILE;
nit: !==

[fukino, 2014-08-06 20:39:26]

Thank you very much for quick review!

https://codereview.chromium.org/441233002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/directory_tree.js (right):

https://codereview.chromium.org/441233002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/directory_tree.js:89: '<div
class="tree-row">' +
On 2014/08/06 13:37:58, yoshiki wrote:
> nit: 4 space indent;

Done.

https://codereview.chromium.org/441233002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/directory_tree.js:590: * @return
{boolean} True if this volume can be removed by user operation.
On 2014/08/06 13:37:57, yoshiki wrote:
> nit: private

Done.

https://codereview.chromium.org/441233002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/directory_tree.js:607: if
(volumeInfo.volumeType == 'provided') {
On 2014/08/06 13:37:58, yoshiki wrote:
> nit: ===

Done.

https://codereview.chromium.org/441233002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/directory_tree.js:1028:
itemPromises.push(new Promise(function(resolve, reject) {
On 2014/08/06 13:37:58, yoshiki wrote:
> Please get "function(...) {}.bind()" out of loop. It creates new object each
> calls and heavy.

With needs to capturing the modelItem, I coundn't find a good way to move the
function outside... I'll ask you later about it, thanks!

https://codereview.chromium.org/441233002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/directory_tree.js:1096: * Invoked
when the volume list or shortcut list is changed.
On 2014/08/06 13:37:58, yoshiki wrote:
> nit: @private

Done.

https://codereview.chromium.org/441233002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/directory_tree.js:1117: 
On 2014/08/06 13:37:58, yoshiki wrote:
> nit: jsdoc

Done.

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

https://codereview.chromium.org/441233002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:1079: this.dialogType
!= DialogType.SELECT_SAVEAS_FILE;
On 2014/08/06 13:37:58, yoshiki wrote:
> nit: !==

Done.

[fukino, 2014-08-06 20:42:55]

The CQ bit was checked by fukino@chromium.org

[commit-bot: I haz the power, 2014-08-06 20:44:07]

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

[commit-bot: I haz the power, 2014-08-07 02:29:33]

Change committed as 287933

[yoshiki, 2014-08-07 04:16:02]

Sorry for after-commit comments.

https://codereview.chromium.org/441233002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/directory_tree.js (right):

https://codereview.chromium.org/441233002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/directory_tree.js:1028:
itemPromises.push(new Promise(function(resolve, reject) {
On 2014/08/06 20:39:25, fukino wrote:
> On 2014/08/06 13:37:58, yoshiki wrote:
> > Please get "function(...) {}.bind()" out of loop. It creates new object each
> > calls and heavy.
> 
> With needs to capturing the modelItem, I coundn't find a good way to move the
> function outside... I'll ask you later about it, thanks! 

Suggested code is like this.

Before loop:
var func = function(i) {
 ...
}.bind(this);

In the loop:
new Promise(function(resolve, reject) {
  func(i);
});


The key fact is that .bind() is much slower than capture-by-closure, so we
should avoid to use it in loop. (cf. http://jsperf.com/f-p-bind-vs-closure)

But in this case, the loop number may be <100 times so we may not need to care
about that. I think we can keep this code as it is.

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

https://codereview.chromium.org/441233002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/file_manager_commands.js:55: //
element are sub items in DirectoryTree.
I think the original comment (element is a sub item in DirectoryTree.) is
correct.

[fukino, 2014-08-07 09:43:37]

https://codereview.chromium.org/441233002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/directory_tree.js (right):

https://codereview.chromium.org/441233002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/directory_tree.js:1028:
itemPromises.push(new Promise(function(resolve, reject) {
On 2014/08/07 04:16:02, yoshiki wrote:
> On 2014/08/06 20:39:25, fukino wrote:
> > On 2014/08/06 13:37:58, yoshiki wrote:
> > > Please get "function(...) {}.bind()" out of loop. It creates new object
each
> > > calls and heavy.
> > 
> > With needs to capturing the modelItem, I coundn't find a good way to move
the
> > function outside... I'll ask you later about it, thanks! 
> 
> Suggested code is like this.
> 
> Before loop:
> var func = function(i) {
>  ...
> }.bind(this);
> 
> In the loop:
> new Promise(function(resolve, reject) {
>   func(i);
> });
> 
> 
> The key fact is that .bind() is much slower than capture-by-closure, so we
> should avoid to use it in loop. (cf. http://jsperf.com/f-p-bind-vs-closure)
> 
> But in this case, the loop number may be <100 times so we may not need to care
> about that. I think we can keep this code as it is.

Thank you for kind and clear explanation!
I didn't know the performance about Function.bind(), and now I've learned!
I'll pay attention more on the .bind() and avoid use it in loops.

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

https://codereview.chromium.org/441233002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/file_manager_commands.js:55: //
element are sub items in DirectoryTree.
On 2014/08/07 04:16:02, yoshiki wrote:
> I think the original comment (element is a sub item in DirectoryTree.) is
> correct.

Yes! I was wrong... I'll correct this comment in next CL.