Support folders in bookmark search

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 d5e157fb5fb0758117cefaa7b93ad98fcba3c23e..8ff09857472e9e470fa84e5c88a4ec9eb82e1d2a 100644
--- a/chrome/browser/resources/bookmark_manager/js/main.js
+++ b/chrome/browser/resources/bookmark_manager/js/main.js
@@ -824,6 +824,19 @@ function openItem() {
 }
 
 /**
+ * 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.
  */
@@ -845,6 +858,7 @@ function deleteBookmarks() {
       // 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.

     });
   });
 }
@@ -874,6 +888,8 @@ function restoreTree(node, parentId) {
       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.

     }
   });
 }