Fix minor WebUI dialog issues, mostly with bookmark management.

Make the bookmark manager consistently reset the hash when starting to edit (so that when editing is complete, a new edit operation can still be invoked on the same element).

Disable the console diagnostics on navigateTo (to be consistent with the other disabled diagnostics in this file).

Make WebUI bookmark editing work on Mac (Mac was invoking the native UI directly instead of going through the BookmarkEditor class which decides which UI to show).

Make 'Add page' pre-populate the bookmark entry with the title/url of the current page (as for the non-webui behavior).  Note that based on UX feedback, we probably don't want to use the bookmark manager in this case at all, so we'll probably add a new dialog for this.  But I already had this ready and it's a step closer (removes a= from bookmark manager, etc.) so I'm still committing it for now. 

Make the certificate viewer non-modal to match the native version.

BUG=99205
TEST=Manual


Review URL: http://codereview.chromium.org/8314017

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@107039 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 70 insertions, 61 deletions

diff --git a/chrome/browser/resources/bookmark_manager/js/main.js b/chrome/browser/resources/bookmark_manager/js/main.js
index ff5f848..54f03da 100644
--- a/chrome/browser/resources/bookmark_manager/js/main.js
+++ b/chrome/browser/resources/bookmark_manager/js/main.js
@@ -130,26 +130,29 @@ function addOneShotEventListener(node, name, handler) {
 }
 
 /**
+ * Updates the location hash to reflect the current state of the application.
+ */
+function updateHash() {
+  window.location.hash = tree.selectedItem.bookmarkId;
+}
+
+/**
  * Navigates to a bookmark ID.
  * @param {string} id The ID to navigate to.
  * @param {boolean=} opt_updateHashNow Whether to immediately update the
  *     location.hash. If false then it is updated in a timeout.
  */
 function navigateTo(id, opt_updateHashNow) {
-  console.info('navigateTo', 'from', window.location.hash, 'to', id);
+  // console.info('navigateTo', 'from', window.location.hash, 'to', id);
   // Update the location hash using a timer to prevent reentrancy. This is how
   // often we add history entries and the time here is a bit arbitrary but was
   // picked as the smallest time a human perceives as instant.
 
-  function f() {
-    window.location.hash = tree.selectedItem.bookmarkId;
-  }
-
   clearTimeout(navigateTo.timer_);
   if (opt_updateHashNow)
-    f();
+    updateHash();
   else
-    navigateTo.timer_ = setTimeout(f, 250);
+    navigateTo.timer_ = setTimeout(updateHash, 250);
 
   updateParentId(id);
 }
@@ -174,41 +177,42 @@ function processHash() {
   }
 
   var valid = false;
-  if (/^[ae]=/.test(id)) {
-    var command = id[0];
+  if (/^e=/.test(id)) {
     id = id.slice(2);
-    if (command == 'e') {
-      // If hash contains e= edit the item specified.
-      chrome.bookmarks.get(id, 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 editBookmark = function(e) {
-          var index = list.dataModel.findIndexById(bookmarkNode.id);
-          if (index != -1) {
-            var sm = list.selectionModel;
-            sm.anchorIndex = sm.leadIndex = sm.selectedIndex = index;
-            scrollIntoViewAndMakeEditable(index);
-          }
-        }
 
-        if (list.parentId == bookmarkNode.parentId)
-          editBookmark();
-        else {
-          // Navigate to the parent folder, once it's loaded edit the bookmark.
-          addOneShotEventListener(list, 'load', editBookmark);
-          updateParentId(bookmarkNode.parentId);
+    // If hash contains e= edit the item specified.
+    chrome.bookmarks.get(id, 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 editBookmark = function(e) {
+        var index = list.dataModel.findIndexById(bookmarkNode.id);
+        if (index != -1) {
+          var sm = list.selectionModel;
+          sm.anchorIndex = sm.leadIndex = sm.selectedIndex = index;
+          scrollIntoViewAndMakeEditable(index);
         }
-      });
-      // We handle the two cases of navigating to the bookmark to be edited
-      // above, don't run the standard navigation code below.
-      return;
-    } else if (command == 'a') {
-      // Once the parent folder is loaded add a page bookmark.
-      addOneShotEventListener(list, 'load', addPage);
-    }
+      };
+
+      if (list.parentId == bookmarkNode.parentId) {
+        // Clear the e= from the hash so that future attemps to edit the same
+        // entry will show up as a hash change.
+        updateHash();
+        editBookmark();
+      } else {
+        // Navigate to the parent folder (which will update the hash), once
+        // it's loaded edit the bookmark.
+        addOneShotEventListener(list, 'load', editBookmark);
+        updateParentId(bookmarkNode.parentId);
+      }
+    });
+
+    // We handle the two cases of navigating to the bookmark to be edited
+    // above, don't run the standard navigation code below.
+    return;
   } 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.
@@ -233,6 +237,7 @@ function processHash() {
 // 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) {
+  // console.info('onhashchange', e.oldURL, e.newURL);
   processHash();
 };
 
diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index 00d726e..6179bae 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -2131,10 +2131,6 @@ void Browser::OpenBookmarkManagerEditNode(int64 node_id) {
   OpenBookmarkManagerWithHash("e=", node_id);
 }
 
-void Browser::OpenBookmarkManagerAddNodeIn(int64 node_id) {
-  OpenBookmarkManagerWithHash("a=", node_id);
-}
-
 void Browser::ShowAppMenu() {
   // We record the user metric for this event in WrenchMenu::RunMenu.
   window_->ShowAppMenu();
diff --git a/chrome/browser/ui/cocoa/browser_window_controller.mm b/chrome/browser/ui/cocoa/browser_window_controller.mm
index 45a5e51..304bbd3 100644
--- a/chrome/browser/ui/cocoa/browser_window_controller.mm
+++ b/chrome/browser/ui/cocoa/browser_window_controller.mm
@@ -1628,17 +1628,10 @@ enum {
   DCHECK(responds);
   if (responds) {
     const BookmarkNode* node = [sender node];
-    if (node) {
-      // A BookmarkEditorController is a sheet that owns itself, and
-      // deallocates itself when closed.
-      [[[BookmarkEditorController alloc]
-         initWithParentWindow:[self window]
-                      profile:browser_->profile()
-                       parent:node->parent()
-                         node:node
-                configuration:BookmarkEditor::SHOW_TREE]
-        runAsModalSheet];
-    }
+    if (node)
+      BookmarkEditor::Show([self window], browser_->profile(),
+          BookmarkEditor::EditDetails::EditNode(node),
+          BookmarkEditor::SHOW_TREE);
   }
 }
 
diff --git a/chrome/browser/ui/webui/bookmarks_ui.cc b/chrome/browser/ui/webui/bookmarks_ui.cc
index c420022..7673997 100644
--- a/chrome/browser/ui/webui/bookmarks_ui.cc
+++ b/chrome/browser/ui/webui/bookmarks_ui.cc
@@ -11,6 +11,7 @@
 #include "base/stringprintf.h"
 #include "chrome/browser/bookmarks/bookmark_editor.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
+#include "chrome/browser/bookmarks/bookmark_utils.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
@@ -77,19 +78,33 @@ RefCountedMemory* BookmarksUI::GetFaviconResourceBytes() {
 // static
 void BookmarkEditor::ShowWebUI(Profile* profile,
                                const EditDetails& details) {
-  GURL url(chrome::kChromeUIBookmarksURL);
+  int64 editId = 0;
   if (details.type == EditDetails::EXISTING_NODE) {
     DCHECK(details.existing_node);
-    url = url.Resolve(StringPrintf("/#e=%s",
-        base::Int64ToString(details.existing_node->id()).c_str()));
+    editId = details.existing_node->id();
   } else if (details.type == EditDetails::NEW_URL) {
     DCHECK(details.parent_node);
-    url = url.Resolve(StringPrintf("/#a=%s",
-        base::Int64ToString(details.parent_node->id()).c_str()));
+    // Add a new bookmark with the title/URL of the current tab.
+    GURL bmUrl;
+    string16 bmTitle;
+    bookmark_utils::GetURLAndTitleToBookmarkFromCurrentTab(profile, &bmUrl,
+        &bmTitle);
+    BookmarkModel* bm = profile->GetBookmarkModel();
+    const BookmarkNode* newNode = bm->AddURL(details.parent_node,
+        details.parent_node->child_count(), bmTitle, bmUrl);
+
+    // Just edit this bookmark like for editing an existing node.
+    // TODO(rbyers): This is to be replaced with a WebUI dialog to prevent
+    // the context switch to a different tab.
+    editId = newNode->id();
   } else {
     NOTREACHED() << "Unhandled bookmark edit details type";
   }
-  // Get parent browser object.
+
+  GURL url = GURL(chrome::kChromeUIBookmarksURL).Resolve(StringPrintf("/#e=%s",
+      base::Int64ToString(editId).c_str()));
+
+  // Invoke the WebUI bookmark editor to edit the specified bookmark.
   Browser* browser = BrowserList::GetLastActiveWithProfile(profile);
   DCHECK(browser);
   browser::NavigateParams params(
diff --git a/chrome/browser/ui/webui/certificate_viewer.cc b/chrome/browser/ui/webui/certificate_viewer.cc
index 22cb52e..be92012 100644
--- a/chrome/browser/ui/webui/certificate_viewer.cc
+++ b/chrome/browser/ui/webui/certificate_viewer.cc
@@ -69,7 +69,7 @@ CertificateViewerDialog::~CertificateViewerDialog() {
 }
 
 bool CertificateViewerDialog::IsDialogModal() const {
-  return true;
+  return false;
 }
 
 string16 CertificateViewerDialog::GetDialogTitle() const {