Support folders in bookmark search

chrome/browser/resources/bookmark_manager/js/main.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.
 
 (function() {
 /** @const */ var BookmarkList = bmm.BookmarkList;
 /** @const */ var BookmarkTree = bmm.BookmarkTree;
 /** @const */ var Command = cr.ui.Command;
 /** @const */ var CommandBinding = cr.ui.CommandBinding;
 /** @const */ var LinkKind = cr.LinkKind;
@@ skipping 806 matching lines @@
   var bookmarkNodes = getSelectedBookmarkNodes();
   // If we double clicked or pressed enter on a single folder, navigate to it.
   if (bookmarkNodes.length == 1 && bmm.isFolder(bookmarkNodes[0])) {
     navigateTo(bookmarkNodes[0].id, updateHash);
   } else {
     openBookmarks(LinkKind.FOREGROUND_TAB);
   }
 }
 
 /**
+ * Refreshes search results after delete or undo-delete.
+ * This ensures children of deleted folders do not remain in results
+ */
+function updateSearchResults() {

[tfarina, 2013/12/10 23:59:32]

you need to get arv or dbeam to review the JS part.

+  if (list.isSearch()) {
+    // Refresh search results
+    navigateTo(tree.items[0].id, updateHash);
+    $('term').focus();

[arv (Not doing code reviews), 2013/12/11 16:41:38]

This is not very nice. Why are we changing focus here?

My concern is that the focus may be somewhere else and the user deletes a
bookmark and now the focus is moved.

[d.halman, 2013/12/13 21:04:07]

This was a somewhat hacky way to force search results to update. I'll look into
modifying the data model without stealing focus or rebuilding the search.

+    setSearch($('term').value);
+  }
+}
+
+/**
  * Deletes the selected bookmarks. The bookmarks are saved in memory in case
  * the user needs to undo the deletion.
  */
 function deleteBookmarks() {
   var selectedIds = getSelectedBookmarkIds();
   lastDeletedNodes = [];
 
   function performDelete() {
     chrome.bookmarkManagerPrivate.removeTrees(selectedIds);
     $('undo-delete-command').canExecuteChange();
     performGlobalUndo = undoDelete;
   }
 
   // First, store information about the bookmarks being deleted.
   selectedIds.forEach(function(id) {
     chrome.bookmarks.getSubTree(id, function(results) {
       lastDeletedNodes.push(results);
 
       // When all nodes have been saved, perform the deletion.
       if (lastDeletedNodes.length === selectedIds.length)
         performDelete();
+        updateSearchResults();

[arv (Not doing code reviews), 2013/12/11 16:41:38]

I don't understand this. Currently, if I'm deleting an item from the search
results it is removed from the result. Why do we need to refresh the search
results? Is this when the use deletes a bookmark from the bookmarks toolbar?
(no, that also works)

[d.halman, 2013/12/13 21:04:07]

I understand your other comments referring to updating the model. I agree that
is the best approach to this problem and I will look into doing so.

To explain this particular line: yes, deleting a bookmark will update the data
model and remove it from search results. However, deleting a folder of bookmarks
from search results will remove the folder without removing any children of the
folder that may also be showing in the results. This behavior resulted in
deleted bookmarks remaining in the results list, allowing users to interact with
them and crash chromium.

I agree the root-level folder stripping is orthogonal to updating search
results, but both of the issues exhibit the same crash behavior and stem from
the last change I made to include folders in search results. I can make a
separate CL if that is the proper approach.

     });
   });
 }
 
 /**
  * Restores a tree of bookmarks under a specified folder.
  * @param {BookmarkTreeNode} node The node to restore.
  * @param {=string} parentId The ID of the folder to restore under. If not
  *     specified, the original parentId of the node will be used.
  */
 function restoreTree(node, parentId) {
   var bookmarkInfo = {
     parentId: parentId || node.parentId,
     title: node.title,
     index: node.index,
     url: node.url
   };
 
   chrome.bookmarks.create(bookmarkInfo, function(result) {
     if (!result) {
       console.error('Failed to restore bookmark.');
       return;
     }
 
     if (node.children) {
       // Restore the children using the new ID for this node.
       node.children.forEach(function(child) {
         restoreTree(child, result.id);
       });
+
+      updateSearchResults();

[arv (Not doing code reviews), 2013/12/11 16:41:38]

This one I think is needed (undo delete does not update the search results).

However, updateSearchResults does not seem like the optimal way to handle this.
It would be better if we could check if any of the new bookmarks would be shown
in the search result and just update the data model. That way focus and
selection is not reset.

[d.halman, 2013/12/13 21:04:07]

Would there be an efficient way to check restored nodes against the current
search parameters in the case where a folder is restored while a search is
active? Or would running a single search once the action has been completed be
preferable? 

[arv (Not doing code reviews), 2013/12/13 22:08:14]

Not at the moment AFIAK.
I think that would be the best way to handle this at this point.

     }
   });
 }
 
 /**
  * Restores the last set of bookmarks that was deleted.
  */
 function undoDelete() {
   lastDeletedNodes.forEach(function(arr) {
     arr.forEach(restoreTree);
@@ skipping 416 matching lines @@
 
   cr.ui.FocusOutlineManager.forDocument(document);
   initializeSplitter();
   bmm.addBookmarkModelListeners();
   dnd.init(selectItemsAfterUserAction);
   tree.reload();
 }
 
 initializeBookmarkManager();
 })();