Move the initialization code of volume item to VolumeItem constructor.

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

 // Copyright (c) 2012 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.
 
 'use strict';
 
 /**
  * A volume list model. This model combines the 2 lists.
  * @param {cr.ui.ArrayDataModel} volumesList The first list of the model.
  * @param {cr.ui.ArrayDataModel} pinnedList The second list of the model.
@@ skipping 119 matching lines @@
 
 /**
  * A volume item.
  * @constructor
  * @extends {HTMLLIElement}
  */
 var VolumeItem = cr.ui.define('li');
 
 VolumeItem.prototype = {
   __proto__: HTMLLIElement.prototype,
-
-  decorate: function() {
-    // Nothing to do.
-  },
 };
 
 /**
+ * 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;
+    }
+  }
+};
+
+/**
+ * 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);
+};
+
+/**
  * A volume list.
  * @constructor
  * @extends {cr.ui.List}
  */
 function VolumeList() {
 }
 
 /**
  * VolumeList inherits cr.ui.List.
  */
@@ skipping 52 matching lines @@
 };
 
 /**
  * Creates an element of a volume. This method is called from cr.ui.List
  * internally.
  * @param {string} path Path of the directory to be rendered.
  * @return {VolumeItem} Rendered element.
  * @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);
+  item.addEventListener('click', handleClick);
 
-  var isRoot = PathUtil.isRootPath(path);
-  var rootType = PathUtil.getRootType(path);
+  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);
 
-  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);
+  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.

 
-  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);
-  }
-
-  if (this.contextMenu_ && (!isRoot ||
-      rootType != RootType.DRIVE && rootType != RootType.DOWNLOADS))
-    cr.ui.contextMenuHandler.setContextMenu(li, this.contextMenu_);
-
-  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) {
     setTimeout(function() {
       this.selectedItem = path;
     }.bind(this), 0);
   }
-
-  return li;
+  return item;
 };
 
 /**
  * Sets a context menu. Context menu is enabled only on archive and removable
  * volumes as of now.
  *
  * @param {cr.ui.Menu} menu Context menu.
  */
 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.

   }
 };
 
 /**
  * Selects the n-th volume from the list.
  * @param {number} index Volume index.
  * @return {boolean} True for success, otherwise false.
  */
 VolumeList.prototype.selectByIndex = function(index) {
   if (index < 0 || index > this.dataModel.length - 1)
@@ skipping 73 matching lines @@
     var itemPath = this.dataModel.item(i);
     if (PathUtil.getRootPath(itemPath) == newRootPath) {
       // Not to invoke the handler of this instance, sets the guard.
       this.dontHandleSelectionEvent_ = true;
       this.selectionModel.selectedIndex = i;
       this.dontHandleSelectionEvent_ = false;
       return;
     }
   }
 };