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

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 cf3c7fca2b0edb94105530bb9c73f982da5c516c..ed1903d50f48c88eeee697fa0faa34e0d0b18683 100644
--- a/chrome/browser/resources/bookmark_manager/js/main.js
+++ b/chrome/browser/resources/bookmark_manager/js/main.js
@@ -154,12 +154,51 @@ function updateParentId(id) {
 // 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') {
@@ -347,21 +386,8 @@ function getFolder(parentId) {
 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();