Move the initialization code of volume item to VolumeItem constructor.

chrome/browser/resources/file_manager/js/volume_list.js

Index: chrome/browser/resources/file_manager/js/volume_list.js
diff --git a/chrome/browser/resources/file_manager/js/volume_list.js b/chrome/browser/resources/file_manager/js/volume_list.js
index e27301c40cd03e2753c286c7b83605a754dd77d1..1736d2be6ba41eb9c6a218b4c077793e8be3d521 100644
--- a/chrome/browser/resources/file_manager/js/volume_list.js
+++ b/chrome/browser/resources/file_manager/js/volume_list.js
@@ -137,10 +137,85 @@ var VolumeItem = cr.ui.define('li');
 
 VolumeItem.prototype = {
   __proto__: HTMLLIElement.prototype,
+};
+
+/**
+ * Decorate the item.
+ */
+VolumeItem.prototype.decorate = function() {
+  // decorate() may be called twice: from the constructor and from
+  // List.createItem().
+  if (this.decorated_)

[mtomasz, 2013/08/06 01:33:06]

It looks tricky but it seems that we have to do it this way...

[yoshiki, 2013/08/06 04:42:20]

This is because of a design mismatch between cr.ui.List and cr.ui.define(). I
changed slightly to check this.className instead for readability.

+    return;
+  this.decorated_ = true;
+
+  this.className = 'root-item';
+  this.setAttribute('role', 'option');
+
+  this.iconDiv_ = cr.doc.createElement('div');
+  this.iconDiv_.className = 'volume-icon';
+  this.appendChild(this.iconDiv_);
+
+  this.label_ = cr.doc.createElement('div');
+  this.label_.className = 'root-label';
+  this.appendChild(this.label_);
+
+  cr.defineProperty(this, 'lead', cr.PropertyKind.BOOL_ATTR);
+  cr.defineProperty(this, 'selected', cr.PropertyKind.BOOL_ATTR);
+};
+
+/**
+ * Associate a path with this item.
+ * @param {string} path Path of this item.
+ */
+VolumeItem.prototype.setPath = function(path) {
+  this.path_ = path;
+
+  var rootType = PathUtil.getRootType(path);
+
+  this.iconDiv_.setAttribute('volume-type-icon', rootType);
+  if (rootType === RootType.REMOVABLE) {
+    this.iconDiv_.setAttribute('volume-subtype',
+        VolumeManager.getInstance().getDeviceType(path));
+  }
+
+  this.label_.textContent = PathUtil.getFolderLabel(path);
+
+  if (rootType === RootType.ARCHIVE || rootType === RootType.REMOVABLE) {
+    if (!this.eject_) {
+      this.eject_ = cr.doc.createElement('div');
+      // Block other mouse handlers.
+      this.eject_.addEventListener(
+          'mouseup', function(e) { e.stopPropagation() });
+      this.eject_.addEventListener(
+          'mousedown', function(e) { e.stopPropagation() });
+
+      this.eject_.className = 'root-eject';
+      this.eject_.addEventListener('click', function(event) {
+        event.stopPropagation();
+        cr.dispatchSimpleEvent(this, 'eject');
+      }.bind(this));
+
+      this.appendChild(this.eject_);
+    }
+  } else {

[mtomasz, 2013/08/06 01:33:06]

nit: Can setPath be called more than once?

[yoshiki, 2013/08/06 04:42:20]

Shouldn't be. Removed the code around and added the warning in case of
double-calling.

+    if (this.eject_) {
+      this.removeChild(this.eject_);
+      this.eject_ = null;
+    }
+  }
+};
 
-  decorate: function() {
-    // Nothing to do.
-  },
+/**
+ * Associate a context menu with this item.

[mtomasz, 2013/08/06 01:33:06]

I think this is confusing. This method sometimes will not set the context menu
after calling it. I think we should move the 'if' back to the calling place and
keep this method simple - just setting the context menu passed as an argument.
WDYT?

[yoshiki, 2013/08/06 04:42:20]

This method is called from 2 places, so moving 'if' back to caller is increasing
the code amount. Instead, to prevent the confusion, I changed the name to
'maybeSetContextMenu()'.


Note that I'm planning to check in the patch which makes the menu hidden when
there are no visible menu items in the menu. Which the patch, we can controls
the visibility of the menu from file_manager_commands.js, and we can remove the
'if'-code here.
https://codereview.chromium.org/22154002/

[mtomasz, 2013/08/06 04:52:23]

SGTM. Hiding menu with no items is a great idea. As for now, please just add a
short comment to the method description when the menu is added, when not.

[yoshiki, 2013/08/06 04:58:44]

Done.

+ * @param {cr.ui.Menu} menu Menu this item.
+ */
+VolumeItem.prototype.setContextMenu = function(menu) {
+  var isRoot = PathUtil.isRootPath(this.path_);
+  var rootType = PathUtil.getRootType(this.path_);
+  if (!isRoot ||
+      rootType != RootType.DRIVE && rootType != RootType.DOWNLOADS)

[mtomasz, 2013/08/06 04:52:23]

nit: Add () around for readability?

[yoshiki, 2013/08/06 04:58:44]

Done.

+    cr.ui.contextMenuHandler.setContextMenu(this, menu);
 };
 
 /**
@@ -216,61 +291,29 @@ VolumeList.prototype.decorate = function(directoryModel, pinnedFolderModel) {
  * @private
  */
 VolumeList.prototype.renderRoot_ = function(path) {
-  var li = new VolumeItem;
-  // TODO(yoshiki): Move the following initialization code to the constructor
-  // of VolumeItem.
-  li.className = 'root-item';
-  li.setAttribute('role', 'option');
-  var dm = this.directoryModel_;
+  var item = new VolumeItem();
+  item.setPath(path);
+
   var handleClick = function() {
-    if (li.selected && path !== dm.getCurrentDirPath()) {
+    if (item.selected && path !== this.directoryModel_.getCurrentDirPath()) {
       this.directoryModel_.changeDirectory(path);
     }
   }.bind(this);
-  li.addEventListener('click', handleClick);
-  li.addEventListener(cr.ui.TouchHandler.EventType.TOUCH_START, handleClick);
-
-  var isRoot = PathUtil.isRootPath(path);
-  var rootType = PathUtil.getRootType(path);
-
-  var iconDiv = cr.doc.createElement('div');
-  iconDiv.className = 'volume-icon';
-  iconDiv.setAttribute('volume-type-icon', rootType);
-  if (rootType === RootType.REMOVABLE) {
-    iconDiv.setAttribute('volume-subtype',
-        this.volumeManager_.getDeviceType(path));
-  }
-  li.appendChild(iconDiv);
-
-  var div = cr.doc.createElement('div');
-  div.className = 'root-label';
-
-  div.textContent = PathUtil.getFolderLabel(path);
-  li.appendChild(div);
-
-  if (rootType === RootType.ARCHIVE || rootType === RootType.REMOVABLE) {
-    var eject = cr.doc.createElement('div');
-    eject.className = 'root-eject';
-    eject.addEventListener('click', function(event) {
-      event.stopPropagation();
-      var unmountCommand = cr.doc.querySelector('command#unmount');
-      // Let's make sure 'canExecute' state of the command is properly set for
-      // the root before executing it.
-      unmountCommand.canExecuteChange(li);
-      unmountCommand.execute(li);
-    }.bind(this));
-    // Block other mouse handlers.
-    eject.addEventListener('mouseup', function(e) { e.stopPropagation() });
-    eject.addEventListener('mousedown', function(e) { e.stopPropagation() });
-    li.appendChild(eject);
-  }
+  item.addEventListener('click', handleClick);
+
+  var handleEject = function() {
+    var unmountCommand = cr.doc.querySelector('command#unmount');
+    // Let's make sure 'canExecute' state of the command is properly set for
+    // the root before executing it.
+    unmountCommand.canExecuteChange(item);
+    unmountCommand.execute(item);
+  };
+  item.addEventListener('eject', handleEject);
 
-  if (this.contextMenu_ && (!isRoot ||
-      rootType != RootType.DRIVE && rootType != RootType.DOWNLOADS))
-    cr.ui.contextMenuHandler.setContextMenu(li, this.contextMenu_);
+  item.addEventListener(cr.ui.TouchHandler.EventType.TOUCH_START, handleClick);

[mtomasz, 2013/08/06 01:33:06]

Is this necessary? Isn't touching emitting click events?

[yoshiki, 2013/08/06 04:42:20]

I'm not sure but it has been here for a long time. Since this is a re-factoring
patch, I keep it in this patch. And I'll check if it is necessary, and will
remove it if unnecessary in a separate patch.

[mtomasz, 2013/08/06 04:52:23]

nit: Please add a TODO, so we won't forget.

[yoshiki, 2013/08/06 04:58:44]

Done.

 
-  cr.defineProperty(li, 'lead', cr.PropertyKind.BOOL_ATTR);
-  cr.defineProperty(li, 'selected', cr.PropertyKind.BOOL_ATTR);
+  if (this.contextMenu_)
+    item.setContextMenu(this.contextMenu_);
 
   // If the current directory is already set.
   if (this.currentVolume_ == path) {
@@ -278,8 +321,7 @@ VolumeList.prototype.renderRoot_ = function(path) {
       this.selectedItem = path;
     }.bind(this), 0);
   }
-
-  return li;
+  return item;
 };
 
 /**
@@ -292,15 +334,7 @@ VolumeList.prototype.setContextMenu = function(menu) {
   this.contextMenu_ = menu;
 
   for (var i = 0; i < this.dataModel.length; i++) {
-    var path = this.dataModel.item(i);
-    var itemType = this.dataModel.getItemType(i);
-    var type = PathUtil.getRootType(path);
-    // Context menu is set only to archive and removable volumes.
-    if (itemType == VolumeListModel.ItemType.PINNED ||
-        type == RootType.ARCHIVE || type == RootType.REMOVABLE) {
-      cr.ui.contextMenuHandler.setContextMenu(this.getListItemByIndex(i),
-                                              this.contextMenu_);
-    }
+    this.getListItemByIndex(i).setContextMenu(this.contextMenu_);

[mtomasz, 2013/08/06 01:33:06]

nit: I think the if logic here was a cleaner design.

   }
 };