Bookmark manager: Pass in explicit targets to the delete and rename commands.

chrome/browser/resources/bookmark_manager/js/main.js

Index: chrome/browser/resources/bookmark_manager/js/main.js
diff --git a/chrome/browser/resources/bookmark_manager/js/main.js b/chrome/browser/resources/bookmark_manager/js/main.js
index 5bd11d1878021c2203668ef36e614a818529cd76..dc36104d6e80ffb15b8f3b6e74f7c8fd8d7fc3ce 100644
--- a/chrome/browser/resources/bookmark_manager/js/main.js
+++ b/chrome/browser/resources/bookmark_manager/js/main.js
@@ -1018,10 +1018,12 @@ function computeParentFolderForNewItem() {
 
 /**
  * Callback for rename folder and edit command. This starts editing for
- * selected item.
+ * the passed in target, or the selected item.
+ * @param {EventTarget=} opt_target The target to start editing. If absent or
+ *     null, the selected item will be edited instead.
  */
-function editSelectedItem() {
-  if (document.activeElement == bmm.tree) {
+function editItem(opt_target) {
+  if ((opt_target || document.activeElement) == bmm.tree) {
     bmm.tree.selectedItem.editing = true;
   } else {
     var li = bmm.list.getListItem(bmm.list.selectedItem);
@@ -1306,7 +1308,7 @@ function handleCommand(e) {
       target = bmm.tree;
     case 'delete-command':
       recordUserAction('Delete');
-      deleteBookmarks(target);
+      deleteBookmarks(target || assertInstanceof(e.target, HTMLElement));
       break;
 
     case 'copy-from-folders-menu-command':
@@ -1346,16 +1348,15 @@ function handleCommand(e) {
       break;
 
     case 'rename-folder-command':
-      editSelectedItem();
+      editItem(assertInstanceof(e.target, HTMLElement));
       break;
 
     case 'rename-folder-from-folders-menu-command':
-      bmm.tree.selectedItem.editing = true;
-      break;
-
+      target = bmm.tree;
+      // Fall through.
     case 'edit-command':
       recordUserAction('Edit');
-      editSelectedItem();
+      editItem(target);

[Dan Beam, 2015/04/17 19:26:33]

what happens if target is undefined?  seems entirely possible here...

[Bernhard Bauer, 2015/04/17 21:46:51]

Yup, that's intended here. editItem() will fall back to using the currently
selected item (via document.activeElement), which matches the behavior of the
old editEditSelectedItem().

[Dan Beam, 2015/04/17 23:35:16]

ok, thanks for explaining

       break;

[Dan Beam, 2015/04/17 19:26:33]

nit: maybe:

  case 'edit-command':
    recordUserAction('Edit');
    // Fall through.

  case 'rename-folder-from-folders-menu-command':
    target = bmm.tree;
    // Fall through.

  case 'rename-folder-command':
    editItem(target || assertInstanceof(e.target, HTMLElement));
    break;

[Bernhard Bauer, 2015/04/17 21:46:51]

That won't work, because for edit we want to pass undefined (see my comment
above). What I do need to change though is to not record the user action for
rename-folder-from-folders-menu, at which point I might just directly call
editItem() and break in each of the cases. WDYT?

[Dan Beam, 2015/04/17 23:35:16]

yeah, that's fine

[Bernhard Bauer, 2015/04/20 07:40:17]

Ok, done.

 
     case 'new-folder-from-folders-menu-command':