Files.app: Add subfolders in the left nav

chrome/browser/resources/file_manager/js/sidebar.js

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/**
+ * Update sub-elements of {@code parentElement} reading {@code DirectoryEntry}

[mtomasz, 2013/03/14 10:15:39]

Update -> updates

[yoshiki, 2013/03/14 13:43:42]

Done.

+ * with calling {@code iterator}.
+ *
+ * @param {DirectoryItem|DirectoryTree} parentElement Parent element of newly
+ *     created items.
+ * @param {function(number): DirectoryEntry} iterator Function which returns
+ *     the n-th Entry in the directory.
+ * @param {DirectoryModel} directoryModel Current DirectoryModel.
+ */
+function updateSubElementsFromList(parentElement, iterator, directoryModel) {
+  var index = 0;
+  while (iterator(index)) {
+    var currentEntry = iterator(index);
+    var currentElement = parentElement.items[index];
+
+    if (index >= parentElement.items.length) {
+      var item = new DirectoryItem(currentEntry, parentElement, directoryModel);
+      parentElement.add(item);
+      index++;
+    } else if (currentEntry.fullPath === currentElement.fullPath) {
+      index++;
+    } else if (currentEntry.fullPath < currentElement.fullPath) {
+      var item = new DirectoryItem(currentEntry, parentElement, directoryModel);
+      parentElement.addAt(item, index);
+      index++;
+    } else if (currentEntry.fullPath > currentElement.fullPath) {
+      parentElement.remove(currentElement);
+    }
+  }
+
+  var removedChild;
+  while (removedChild = parentElement.items[index]) {
+    parentElement.remove(removedChild);
+  }
+
+  if (index === 0) {
+    parentElement.hasChildren = false;
+    parentElement.expanded = false;
+  } else {
+    parentElement.hasChildren = true;
+  }
+}
+
+/**
+ * A directory in the tree. One this element reprecents one directory. One this

[mtomasz, 2013/03/14 10:15:39]

reprecents -> represents
One this -> each

[yoshiki, 2013/03/14 13:43:42]

Done. Thanks!

+ * element reprecents one director.
+ *
+ * @param {DirectoryEntry} parentDirEntry DirectoryEntry of this item.
+ * @param {DirectoryItem|DirectoryTree} parentDirItem Parent of this item.
+ * @param {DirectoryModel} directoryModel Current DirectoryModel.
+ * @extends {cr.ui.TreeItem}
+ * @constructor
+ */
+function DirectoryItem(parentDirEntry, parentDirItem, directoryModel) {
+  var item = cr.doc.createElement('div');
+  DirectoryItem.decorate(item, parentDirEntry, parentDirItem, directoryModel);
+  return item;
+}
+
+/**
+ * @param {HTMLElement} el Element to be DirectoryItem.
+ * @param {DirectoryEntry} parentDirEntry DirectoryEntry of this item.
+ * @param {DirectoryItem|DirectoryTree} parentDirItem Parent of this item.
+ * @param {DirectoryModel} directoryModel Current DirectoryModel.
+ */
+DirectoryItem.decorate =
+    function(el, parentDirEntry, parentDirItem, directoryModel) {
+  el.__proto__ = DirectoryItem.prototype;
+  (/** @type {DirectoryItem} */ el).decorate(
+      parentDirEntry, parentDirItem, directoryModel);
+};
+
+DirectoryItem.prototype = {
+  __proto__: cr.ui.TreeItem.prototype,
+
+  /**
+   * The element containing the label text and the icon.
+   * @type {!HTMLElement}
+   * @override
+   **/
+  get labelElement() {
+      return this.firstElementChild.querySelector('.label');
+  }
+};
+
+/**
+ * @param {DirectoryEntry} parentDirEntry DirectoryEntry of this item.
+ * @param {DirectoryItem|DirectoryTree} parentDirItem Parent of this item.
+ * @param {DirectoryModel} directoryModel Current DirectoryModel.
+ */
+DirectoryItem.prototype.decorate =

[mtomasz, 2013/03/14 10:15:39]

This indentation looks strange. I think we never do that. How about:

(1) DirectoryItem.prototype.decorate = function(
      parentDirEntry, parentDirItem, directoryModel) {
or
(2) DirectoryItem.prototype.decorate = function(parentDirEntry,
                                                parentDirItem,
                                                directoryModel) {

?

[yoshiki, 2013/03/14 13:43:42]

I prefer to (1). But there are some 'decorate =\n    function(...) {' style
code. I think there are no much difference and any of three is acceptable for
me.

+    function(parentDirEntry, parentDirItem, directoryModel) {
+  var path = parentDirEntry.fullPath;
+  var label = PathUtil.isRootPath(path) ?
+                  PathUtil.getRootLabel(path) : parentDirEntry.name;

[mtomasz, 2013/03/14 10:15:39]

Indentation is off.
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...

Search "Ternary".

[yoshiki, 2013/03/14 13:43:42]

Thanks!

+
+  this.className = 'tree-item';
+  this.innerHTML =

[mtomasz, 2013/03/14 10:15:39]

1. nit: It's suprising that in html5 quoting is not a must. However, for
consistency, I think we should do that.

2. I think we never create elements using innerHTML, besides short messages. I
believe this structure should be created via javascript. WDYT?

[yoshiki, 2013/03/14 13:43:42]

nit: It's suprising that in html5 quoting is not a must. However, for
Done.
I think this style is easy to understand what dom are generated. Same style is
used in tree.js to generate TreeItem. And I think there are no strong reason to
use JavaScript to create the element.

+        '<div class=tree-row>' +

[mtomasz, 2013/03/14 10:15:39]

Indentation off. 6 -> 4 spaces.

[yoshiki, 2013/03/14 13:43:42]

Done.

+        '<span class=expand-icon></span>' +
+        '<span class=icon></span>' +
+        '<span class=label></span>' +
+        '<div class=root-eject></div>' +
+        '</div>' +
+        '<div class=tree-children></div>';
+  this.setAttribute('role', 'treeitem');

[mtomasz, 2013/03/14 10:15:39]

nit: Does ChromeVox work fine with the new sidebar?

[yoshiki, 2013/03/14 13:43:42]

As I tested, it partially works but it's not completed. cr.ui.Tree seems not to
support ChromeVox.

However, this attribute is added as same way as tree.js.

+
+  this.directoryModel_ = directoryModel;
+  this.parent_ = parentDirItem;
+  this.label = label;
+  this.fullPath = path;
+
+  // Sets hasChildren=true tentatively. This will be overridden after
+  // scanning sub-directories in updateSubElementsFromList.
+  this.hasChildren = true;
+
+  this.addEventListener('expand', this.onExpand_.bind(this), true);
+
+  var volumeManager = VolumeManager.getInstance();
+  var icon = this.querySelector('.icon');
+  if (PathUtil.isRootPath(path)) {
+    icon.classList.add('volume-icon');
+    var iconType = PathUtil.getRootType(path);
+    icon.setAttribute('volume-type-icon', iconType);
+
+    if (iconType === RootType.REMOVABLE)

[mtomasz, 2013/03/14 10:15:39]

== should be sufficient here.

[yoshiki, 2013/03/14 13:43:42]

Done.

+      icon.setAttribute('volume-subtype', volumeManager.getDeviceType(path));
+  }
+
+  var eject = this.querySelector('.root-eject');
+  eject.hidden = !PathUtil.isUnmountableByUser(path);
+  eject.addEventListener('click',
+      function(event) {
+        event.stopPropagation();
+        if (!PathUtil.isUnmountableByUser(path))
+          return;
+
+        volumeManager.unmount(path, function() {}, function() {});
+      }.bind(this));
+
+  if ('expanded' in parentDirItem || parentDirItem.expanded)
+    this.updateSubDirectoriesWithEntry_(parentDirEntry);
+};
+
+/**
+ * Invoked when the item is being expanded.
+ * @param {!UIEvent} e Event.
+ * @private
+ **/
+DirectoryItem.prototype.onExpand_ = function(e) {
+  this.updateSubDirectories(function() {
+    this.expanded = false;
+  }.bind(this));
+
+  e.stopPropagation();
+};
+
+/**
+ * Retrieve the latest subdirectories and update them on the tree.
+ *

[mtomasz, 2013/03/14 10:15:39]

Remove line here, since it is only 2 line jsdoc. In #152 it is 3 line, and
doesn't have this space.

[yoshiki, 2013/03/14 13:43:42]

Done.

+ * @param {function()=} opt_errorCallback Callback called on error.
+ */
+DirectoryItem.prototype.updateSubDirectories = function(opt_errorCallback) {
+  this.directoryModel_.resolveDirectory(
+      this.fullPath,
+      function(entry) {
+        this.updateSubDirectoriesWithEntry_(entry, opt_errorCallback);
+      }.bind(this),
+      opt_errorCallback);
+};
+
+/**
+ * Retrieve the latest subdirectories and update them on the tree.
+ *
+ * @param {!DirectoryEntry} dirEntry DirectoryEntry to read from.
+ * @param {function()=} opt_errorCallback Callback called on error.
+ * @private
+ */
+DirectoryItem.prototype.updateSubDirectoriesWithEntry_ =
+    function(dirEntry, opt_errorCallback) {
+  // Skips if the entry is dummy.
+  if (!('createReader' in dirEntry)) {

[mtomasz, 2013/03/14 10:15:39]

instanceof doesn't work here?

[yoshiki, 2013/03/14 13:43:42]

instanceof doesn't work here, since we can't get the constructor of
DirectoryEntry. Instead, we can use 'dirEntry.constructor.name ==
"DirectoryEntry"' to check if this is an instance of DirectoryEntry or not. 

But here the only thing we want to know is we have 'createReader' or not, so I
think this is enough for us.

[mtomasz, 2013/03/15 04:23:20]

Got it.

+    if (opt_errorCallback)
+      opt_errorCallback();
+    return;
+  }
+
+  var reader = dirEntry.createReader();
+  var entries = [];
+
+  var readEntry = function() {
+    reader.readEntries(function(results) {
+      if (results.length === 0) {

[mtomasz, 2013/03/14 10:15:39]

nit: I think == 0 is sufficient, or even if (!result.length), since it is an
array.

In general, we should discuss/check how to deal with ==/===, since everywhere in
Files.app is different.

[yoshiki, 2013/03/14 13:43:42]

Done. 
I think '===' is safer to use, but '==' is also ok if we use it carefully.

+        this.entries_ = entries.sort();
+        this.redrawSubDirectoryList_();
+        return;
+      }
+
+      for (var i = 0; i < results.length; i++) {
+        var entry = results[i];
+        if (entry.isDirectory)
+          entries.push(entry);
+      }
+      readEntry();
+    }.bind(this));
+  }.bind(this);
+  readEntry();
+};
+
+/**
+ * @private
+ */
+DirectoryItem.prototype.redrawSubDirectoryList_ = function() {
+  var entries = this.entries_;
+  updateSubElementsFromList(this,
+                            function(i) { return entries[i]; },
+                            this.directoryModel_);
+};
+
+/**
+ * Tree of directories on sidebar. This element is also the root of items, in

[mtomasz, 2013/03/14 10:15:39]

nit: on sidebar -> on the sidebar.

[yoshiki, 2013/03/14 13:43:42]

Done.

+ * other words, this is the parent of the top-level items.
+ *
+ * @constructor
+ * @extends {cr.ui.Tree}
+ */
+function DirectoryTree() {}
+
+/**
+ * Decorates element.

[mtomasz, 2013/03/14 10:15:39]

nit: Decorates an element.

[yoshiki, 2013/03/14 13:43:42]

Done.

+ * @param {HTMLElement} el Element to be DirectoryTree.
+ * @param {DirectoryModel} directoryModel Current DirectoryModel.
+ */
+DirectoryTree.decorate = function(el, directoryModel) {
+  el.__proto__ = DirectoryTree.prototype;
+  (/** @type {DirectoryTree} */ el).decorate(directoryModel);
+};
+
+DirectoryTree.prototype = {
+  __proto__: cr.ui.Tree.prototype,
+};
+
+/**
+ * Decorates element.
+ * @param {DirectoryModel} directoryModel Current DirectoryModel.
+ */
+DirectoryTree.prototype.decorate = function(directoryModel) {
+  cr.ui.Tree.prototype.decorate.call(this);
+
+  this.directoryModel_ = directoryModel;
+
+  this.rootsList_ = this.directoryModel_.getRootsList();
+  this.rootsList_.addEventListener('change',
+                                   this.onRootsListChanged_.bind(this));
+  this.rootsList_.addEventListener('permuted',
+                                   this.onRootsListChanged_.bind(this));
+  this.onRootsListChanged_();
+};
+
+/**
+ * Sets a context menu. Context menu is enabled only archive and removable

[mtomasz, 2013/03/14 10:15:39]

only -> only on

[yoshiki, 2013/03/14 13:43:42]

Done. Thanks!

+ * volumes as of now.
+ *
+ * @param {cr.ui.Menu} menu Context menu.
+ */
+DirectoryTree.prototype.setContextMenu = function(menu) {
+  this.contextMenu_ = menu;
+
+  for (var i = 0; i < this.rootsList_.length; i++) {
+    var item = this.rootsList_.item(i);
+    var type = PathUtil.getRootType(item.fullPath);
+    // Context menu is set only to archive and removable volumes.
+    if (type === RootType.ARCHIVE || type === RootType.REMOVABLE) {
+      cr.ui.contextMenuHandler.setContextMenu(this.items[i].rowElement,
+                                              this.contextMenu_);
+    }
+  }
+};
+
+/**
+ * Invoked when the root list is changed.
+ * @private
+ */
+DirectoryTree.prototype.onRootsListChanged_ = function() {
+  var rootsList = this.rootsList_;
+  updateSubElementsFromList(this,
+                            rootsList.item.bind(rootsList),
+                            this.directoryModel_);
+  this.setContextMenu(this.contextMenu_);
+};
+
+/**
+ * Returns the path of the current selected item.

[mtomasz, 2013/03/14 10:15:39]

nit: current -> currently (or remove).

[yoshiki, 2013/03/14 13:43:42]

Done.

+ * @return {string} The current path.
+ */
+DirectoryTree.prototype.getCurrentPath = function() {
+  return this.selectedItem ? this.selectedItem.fullPath : null;
+};