Use bookmark manager to add/edit bookmark pages with webui_dialogs=1.

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

 // Copyright (c) 2011 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.
 
 const BookmarkList = bmm.BookmarkList;
 const BookmarkTree = bmm.BookmarkTree;
 const ListItem = cr.ui.ListItem;
 const TreeItem = cr.ui.TreeItem;
 const LinkKind = cr.LinkKind;
 const Command = cr.ui.Command;
@@ skipping 136 matching lines @@
 function updateParentId(id) {
   list.parentId = id;
   if (id in bmm.treeLookup)
     tree.selectedItem = bmm.treeLookup[id];
 }
 
 // We listen to hashchange so that we can update the currently shown folder when
 // the user goes back and forward in the history.
 window.onhashchange = function(e) {
   var id = window.location.hash.slice(1);
+  if (!id) {
+    // If we do not have a hash select first item in the tree.
+    id = tree.items[0].bookmarkId;
+  }
 
   var valid = false;
+  if (/^e=/.test(id)) {
+    var editId = id.slice(2);

[Rick Byers, 2011/08/04 18:26:07]

This 'slice' line is duplicated in 3 places now.  I also find it confusing that
id is sometimes the hash string and sometimes a bookmark ID.  I'd suggest
changing this (above) to something that immediately tests the hash string for
the presence of '=' an sets 'id' to the RHS, etc.

[flackr, 2011/08/12 18:11:24]

Done. In the case of q= the id is not changed.

[Rick Byers, 2011/08/15 14:59:41]

Ah, I missed this subtlety.  Thanks for adding comments - certainly better now.

+    // If hash contains e= edit the item specified.
+    chrome.bookmarks.get(editId, function(bookmarkNodes) {
+      // Verify the node to edit is a valid node.
+      if (!bookmarkNodes || bookmarkNodes.length != 1)
+        return;
+      var bookmarkNode = bookmarkNodes[0];
+      // After the list reloads edit the desired bookmark.
+      var f = function(e) {

[Rick Byers, 2011/08/04 18:26:07]

This add/remove pattern is duplicated often enough that I'd suggest adding a
little helper function somewhere, eg:
function addOneShotEventListener(node, name, handler) {
  var f = function(e) {
    handler(e);
    node.removeEventListener(name, f);
  }
  node.addEventListener(name, f);
}

[Rick Byers, 2011/08/04 18:26:07]

Should probably use a variable name more descriptive than 'f'.  How about
editBookmark or something?

[flackr, 2011/08/12 18:11:24]

Done.

[flackr, 2011/08/12 18:11:24]

Done.

+        var index;
+        if ((index = list.dataModel.findIndexById(bookmarkNode.id)) != -1) {
+          var sm = list.selectionModel;
+          sm.anchorIndex = sm.leadIndex = sm.selectedIndex = index;
+          scrollIntoViewAndMakeEditable(index);
+        }
+        list.removeEventListener('load', f);
+      }
+      list.addEventListener('load', f);
 
-  // In case we got a search hash update the text input and the bmm.treeLookup
-  // to use the new id.
-  if (/^q=/.test(id)) {
+      // Navigate to the parent folder of the node to edit and call function.

[Rick Byers, 2011/08/04 18:26:07]

I find this a little confusing (especially given the "after the list reload"
comment above the declaration of f) - the control flow (immediate verses load
event callback) wasn't clear to me initially.  If you have the above helper,
then this could be simplified as:
 if (list.parentId == bookmarkNode.parentId)
    editBookmark();
 else {
    // Navigate to the parent folder, and once it's loaded edit the bookmark
    addOneShotEventListener(list, 'load', editBookmark);
    updateParentId(bookmarkNode.parentId);
  }

[flackr, 2011/08/12 18:11:24]

Done.

+      if (list.parentId == bookmarkNode.parentId)
+        f();
+      else
+        updateParentId(bookmarkNode.parentId);
+    });
+    // We need more information before we know which folder to navigate to.

[Rick Byers, 2011/08/04 18:26:07]

What is this comment referring to?  The delayed call to 'f' above?  Seems like
it's in the wrong place.  I'd just say something like 'we handle the two cases
of navigating to the bookmark to be edited above, don't run the standard
navigation code below'.

[flackr, 2011/08/12 18:11:24]

Done.

+    return;
+  } else if (/^a=/.test(id)) {
+    id = id.slice(2);
+    // After the list reloads add the desired bookmark.
+    var f = function(e) {
+      addPage();
+      list.removeEventListener('load', f);
+    }

[arv (Not doing code reviews), 2011/08/11 22:32:22]

missing semicolon

[flackr, 2011/08/12 18:11:24]

Done.

+    list.addEventListener('load', f);
+  } else if (/^q=/.test(id)) {
+    // In case we got a search hash update the text input and the
+    // bmm.treeLookup to use the new id.
     setSearch(id.slice(2));
     valid = true;
   } else if (id == 'recent') {
     valid = true;
   }
 

[Rick Byers, 2011/08/04 18:26:07]

Since this is now non-trivial, add a comment like:
// Navigate to bookmark 'id'.

[flackr, 2011/08/12 18:11:24]

Done.

   if (valid) {
     updateParentId(id);
   } else {
     // We need to verify that this is a correct ID.
     chrome.bookmarks.get(id, function(items) {
       if (items && items.length == 1)
         updateParentId(id);
     });
   }
 };
@@ skipping 161 matching lines @@
     if (parentNode.parentId != ROOT_ID) {
       return getFolder(parentNode.parentId) + '/' + s;
     }
     return s;
   }
 }
 
 tree.addEventListener('load', function(e) {
   // Add hard coded tree items
   tree.add(recentTreeItem);
-
-  // Now we can select a tree item.
-  var hash = window.location.hash.slice(1);
-  if (!hash) {
-    // If we do not have a hash select first item in the tree.
-    hash = tree.items[0].bookmarkId;
-  }
-
-  if (/^q=/.test(hash)) {
-    var searchTerm = hash.slice(2);
-    $('term').value = searchTerm;
-    setSearch(searchTerm);
-  } else {
-    navigateTo(hash);
-  }
+  // Call onhashchange to navigate to hash location.
+  window.onhashchange();

[Rick Byers, 2011/08/04 18:26:07]

Technically I'd consider it invalid to call this event handler without any
arguments.  Instead I'd suggest writing a common (argument-less) helper function
("processHash" or something indicating it's for both load and change) that you
call here directly, and which onhashchange just delegates to.

[arv (Not doing code reviews), 2011/08/11 22:32:22]

Yeah, don't do this. Just do what Rick suggested.

[flackr, 2011/08/12 18:11:24]

Done.

 });
 
 tree.reload();
 addBookmarkModelListeners();
 
 var dnd = {
   dragData: null,
 
   getBookmarkElement: function(el) {
     while (el && !el.bookmarkNode) {
@@ skipping 1238 matching lines @@
   document.addEventListener('copy', handle('copy-command'));
   document.addEventListener('cut', handle('cut-command'));
 
   var pasteHandler = handle('paste-command');
   document.addEventListener('paste', function(e) {
     // Paste is a bit special since we need to do an async call to see if we can
     // paste because the paste command might not be up to date.
     updatePasteCommand(pasteHandler);
   });
 })();