[MD Bookmarks] Add search.

[MD Bookmarks] Add search.

This CL implements search by sending a query to the existing backend
search and then updating the ui to reflect the search result. The
bookmarks list will now either display the contents of a folder or the
search results.

BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2614703003
Cr-Commit-Position: refs/heads/master@{#443499}
Committed: https://chromium.googlesource.com/chromium/src/+/2c22f5c684f13a4b7b9eeb3ff7007e80e5b1e8b0

[angelayang, 2017-01-05 05:35:02]

Description was changed from

==========
[MD Bookmarks] Add search.

This CL implements search by sending a query to the existing backend
search and then updating the ui to reflect the search result. The
bookmarks list will now either display the contents of a folder or the
search results.

BUG=658980
==========

to

==========
[MD Bookmarks] Add search.

This CL implements search by sending a query to the existing backend
search and then updating the ui to reflect the search result. The
bookmarks list will now either display the contents of a folder or the
search results.

BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[angelayang, 2017-01-05 05:36:21]

Description was changed from

==========
[MD Bookmarks] Add search.

This CL implements search by sending a query to the existing backend
search and then updating the ui to reflect the search result. The
bookmarks list will now either display the contents of a folder or the
search results.

BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Bookmarks] Add search.

This CL implements search by sending a query to the existing backend
search and then updating the ui to reflect the search result. The
bookmarks list will now either display the contents of a folder or the
search results.

BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[angelayang, 2017-01-05 05:36:21]

angelayang@google.com changed reviewers:
+ calamity@chromium.org, jiaxi@google.com, tsergeant@chromium.org

[angelayang, 2017-01-05 05:36:37]

Take a look? thanks

[angelayang, 2017-01-05 05:37:59]

The CQ bit was checked by angelayang@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 05:38:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[angelayang, 2017-01-05 05:44:52]

https://codereview.chromium.org/2614703003/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/store.js (right):

https://codereview.chromium.org/2614703003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/store.js:92: // Active the sidebar.
// Set sidebar to active.

[angelayang, 2017-01-05 05:54:14]

Patchset #1 (id:1) has been deleted

[angelayang, 2017-01-05 05:54:51]

The CQ bit was checked by angelayang@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 05:55:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 06:51:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 06:51:44]

Dry run: This issue passed the CQ dry run.

[tsergeant, 2017-01-06 02:43:54]

I'm a little concerned about how this is going to integrate with the rest of the
page. Eg, if I do a search and then edit one of the results, how is the store
going to update the search results page? Same with removing an item.

Ideally we'd be able to just pretend like search results are just a different
section of the tree, but I'm not sure how that would work.

Here are a couple of high-level comments to start thinking about this issue. I'm
happy to chat about it in person later, if you like.

https://codereview.chromium.org/2614703003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/folder_node.js (right):

https://codereview.chromium.org/2614703003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.js:18: sidebarActive: {
To avoid passing this all the way down the tree, is it possible to actually have
no node selected in the tree?

You'd just need to make sure the page recovers correctly when the search is
cleared.

This is kind of similar to what the old page does, and it goes back to selecting
'Bookmarks bar' when a search is cleared.

https://codereview.chromium.org/2614703003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2614703003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:12: searchTerm: String,
Is it possible to make it so that the list doesn't need to know about these
things?

Ideally, the store should say "here are the items that need displaying", and the
<bookmark-list> should just go ahead ad display them.

[angelayang, 2017-01-12 04:58:03]

Patchset #3 (id:60001) has been deleted

[angelayang, 2017-01-12 05:04:32]

Hello, I have made major changes to the search.

I've also added search tests and fixed the store tests to work with the new
store. 

I'm yet to merge in the empty list patch and jiaxi's edit patch. I will do that
in another patch after.

https://codereview.chromium.org/2614703003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/folder_node.js (right):

https://codereview.chromium.org/2614703003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.js:18: sidebarActive: {
On 2017/01/06 02:43:54, tsergeant wrote:
> To avoid passing this all the way down the tree, is it possible to actually
have
> no node selected in the tree?
> 
> You'd just need to make sure the page recovers correctly when the search is
> cleared.
> 
> This is kind of similar to what the old page does, and it goes back to
selecting
> 'Bookmarks bar' when a search is cleared.

Done.

https://codereview.chromium.org/2614703003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2614703003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:12: searchTerm: String,
On 2017/01/06 02:43:54, tsergeant wrote:
> Is it possible to make it so that the list doesn't need to know about these
> things?
> 
> Ideally, the store should say "here are the items that need displaying", and
the
> <bookmark-list> should just go ahead ad display them.

That makes a lot of sense yep

https://codereview.chromium.org/2614703003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/app.js (right):

https://codereview.chromium.org/2614703003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/app.js:26: sidebarActive_: function() {
I will delete this in the next patch.

https://codereview.chromium.org/2614703003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/folder_node.html (right):

https://codereview.chromium.org/2614703003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.html:69:
icon="[[getFolderIcon_(isSelected)]]"></iron-icon>
nit. will move to line above

[angelayang, 2017-01-12 06:08:06]

I've merged.

take a look :)

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/store.js (right):

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:89: this.selectedId = '';
Closure doesn't like it when i set this to null, so i've set it to '' instead.

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:161: // TODO (angelayang): does
this ever happen?
Sorry i'll delete this comment. Just checking whether i can remove this piece of
logic.  i think it's no longer possible to remove the selectedNode unless you're
in search. In which case we don't need to update the selectedId.

[tsergeant, 2017-01-12 06:45:06]

Yay!

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/app.js (right):

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/app.js:16: displayedList: Array,
Closure annotation here:

/** @type{Array<BookmarkTreeNode>} */

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/list.html (right):

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.html:75: <!--TODO(angelayang): add
message for no search results found-->
You can get rid of this TODO

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:12: displayedList: Array,
Closure annotation here

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/sidebar.js (right):

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/sidebar.js:11: sidebarActive: Boolean,
Get rid of this?

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/store.js (right):

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:27: /** @type {BookmarkTreeNode}
*/
Might be good to leave a comment here explaining what this does now: It's no
longer super straightforward.

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:87: deselectFolders_: function()
{
You should probably do an unlinkPaths that's the equivalent of the linkPaths in
updateSelectedDisplay(). This stops Polymer from thinking that displayedList
connects to anything in the tree.

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:89: this.selectedId = '';
On 2017/01/12 06:08:06, angelayang wrote:
> Closure doesn't like it when i set this to null, so i've set it to '' instead.

What specifically doesn't it like? I think using null is probably better, so we
can try and fix any problems with Closure to allow it.

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:161: // TODO (angelayang): does
this ever happen?
Try:
- Create 'folder'
- Create 'subfolder'
- Select 'subfolder' in MD Bookmarks 
- Delete 'folder' from bookmarks bar

Then the selected item no longer exists and this line selects the parent of
'folder'

It's important to remember that modifications we see here could be coming from
anywhere: the bookmarks bar, the bookmark manager open in another tab, or even
synced from another device.

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:183: initNodes_:
function(bookmarkNode) {
This function got moved by Charlotte: It now exists below as
BookmarksStore.initNodes(). You should delete it again.

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:199: onSearchTermChanged_:
function(e) {
Also, Charlotte's patch added sections to store.js to try and keep everything in
order. This new function should go under "bookmarks app event listeners"

[angelayang, 2017-01-12 07:51:24]

Hi I fixed the some of the issues from before. 
Take a look when you get a moment. Thanks.

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/app.js (right):

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/app.js:16: displayedList: Array,
On 2017/01/12 06:45:05, tsergeant wrote:
> Closure annotation here:
> 
> /** @type{Array<BookmarkTreeNode>} */

Done.

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/list.html (right):

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.html:75: <!--TODO(angelayang): add
message for no search results found-->
On 2017/01/12 06:45:05, tsergeant wrote:
> You can get rid of this TODO

Done.

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/list.js (right):

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:12: displayedList: Array,
On 2017/01/12 06:45:05, tsergeant wrote:
> Closure annotation here

Done.

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/sidebar.js (right):

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/sidebar.js:11: sidebarActive: Boolean,
On 2017/01/12 06:45:06, tsergeant wrote:
> Get rid of this?

Done.

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/store.js (right):

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:27: /** @type {BookmarkTreeNode}
*/
On 2017/01/12 06:45:06, tsergeant wrote:
> Might be good to leave a comment here explaining what this does now: It's no
> longer super straightforward.

Done.

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:87: deselectFolders_: function()
{
On 2017/01/12 06:45:06, tsergeant wrote:
> You should probably do an unlinkPaths that's the equivalent of the linkPaths
in
> updateSelectedDisplay(). This stops Polymer from thinking that displayedList
> connects to anything in the tree.

Done.

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:89: this.selectedId = '';
On 2017/01/12 06:45:06, tsergeant wrote:
> On 2017/01/12 06:08:06, angelayang wrote:
> > Closure doesn't like it when i set this to null, so i've set it to ''
instead.
> 
> What specifically doesn't it like? I think using null is probably better, so
we
> can try and fix any problems with Closure to allow it.

Closure says it should be of type string. So i'll put it to null and we can fix
the closure issue after yep

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:161: // TODO (angelayang): does
this ever happen?
On 2017/01/12 06:45:06, tsergeant wrote:
> Try:
> - Create 'folder'
> - Create 'subfolder'
> - Select 'subfolder' in MD Bookmarks 
> - Delete 'folder' from bookmarks bar
> 
> Then the selected item no longer exists and this line selects the parent of
> 'folder'
> 
> It's important to remember that modifications we see here could be coming from
> anywhere: the bookmarks bar, the bookmark manager open in another tab, or even
> synced from another device.

Cool thanks! I'll definitely keep that in mind! I forgot about the bookmarks bar
etc.

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:183: initNodes_:
function(bookmarkNode) {
On 2017/01/12 06:45:06, tsergeant wrote:
> This function got moved by Charlotte: It now exists below as
> BookmarksStore.initNodes(). You should delete it again.

oh thanks i didn't spot that

https://codereview.chromium.org/2614703003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:199: onSearchTermChanged_:
function(e) {
On 2017/01/12 06:45:06, tsergeant wrote:
> Also, Charlotte's patch added sections to store.js to try and keep everything
in
> order. This new function should go under "bookmarks app event listeners"

Done.

https://codereview.chromium.org/2614703003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/store.js (right):

https://codereview.chromium.org/2614703003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/store.js:94: this.selectedId = null;
This is the closure compilation error.
##     found   : null
##     required: string
##     this.selectedId = null;
##     ^^^^^^^^^^^^^^^^^^^^^^

[tsergeant, 2017-01-12 23:44:21]

looking good!

https://codereview.chromium.org/2614703003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/list.html (left):

https://codereview.chromium.org/2614703003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/list.html:71:
<hidden$="[[isListEmpty_(selectedNode.children.length)]]">
whoops

https://codereview.chromium.org/2614703003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/store.js (right):

https://codereview.chromium.org/2614703003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/store.js:15: selectedId: {
I think you'll need to put /** @type {?string} */ here. If that doesn't work,
I'll debug with you in person.

https://codereview.chromium.org/2614703003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/store.js:18: notify: true,
While you're here:

selectedId is public, but never used by anyone outside of bookmarks-store. Maybe
we should just make it private? I can't really think of any need for it to stay
public.

https://codereview.chromium.org/2614703003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/store.js:165: if
(this.isAncestorOfSelected_(this.idToNodeMap_[id])) {
No need to add {} here

https://codereview.chromium.org/2614703003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/store.js:188:
this.set(this.idToNodeMap_[id].path + '.url', changeInfo.url);
Should the search list also be refreshed here?

[angelayang, 2017-01-13 00:25:11]

Hello, I fixed some things.
Thanks :)

https://codereview.chromium.org/2614703003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/list.html (left):

https://codereview.chromium.org/2614703003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/list.html:71:
<hidden$="[[isListEmpty_(selectedNode.children.length)]]">
On 2017/01/12 23:44:20, tsergeant wrote:
> whoops

Done.

https://codereview.chromium.org/2614703003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/store.js (right):

https://codereview.chromium.org/2614703003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/store.js:15: selectedId: {
On 2017/01/12 23:44:20, tsergeant wrote:
> I think you'll need to put /** @type {?string} */ here. If that doesn't work,
> I'll debug with you in person.

yep that works thankyou

https://codereview.chromium.org/2614703003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/store.js:18: notify: true,
On 2017/01/12 23:44:21, tsergeant wrote:
> While you're here:
> 
> selectedId is public, but never used by anyone outside of bookmarks-store.
Maybe
> we should just make it private? I can't really think of any need for it to
stay
> public.

left public for URL/router

https://codereview.chromium.org/2614703003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/store.js:94: this.selectedId = null;
On 2017/01/12 07:51:24, angelayang wrote:
> This is the closure compilation error.
> ##     found   : null
> ##     required: string
> ##     this.selectedId = null;
> ##     ^^^^^^^^^^^^^^^^^^^^^^

Done.

https://codereview.chromium.org/2614703003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/store.js:165: if
(this.isAncestorOfSelected_(this.idToNodeMap_[id])) {
On 2017/01/12 23:44:21, tsergeant wrote:
> No need to add {} here

Done.

https://codereview.chromium.org/2614703003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/store.js:188:
this.set(this.idToNodeMap_[id].path + '.url', changeInfo.url);
On 2017/01/12 23:44:20, tsergeant wrote:
> Should the search list also be refreshed here?

Yes it definitely should :)

[tsergeant, 2017-01-13 01:56:23]

lgtm

https://codereview.chromium.org/2614703003/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/md_bookmarks/store_test.js (right):

https://codereview.chromium.org/2614703003/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/store_test.js:180: chrome.bookmarks.search =
function(searchTerm, callback) {
No need to do anything here, but it'd be good to do something to make this kind
of mocking easier to do (with some kind of test util). This is not going to be
the only time we'll want to overwrite a bookmark manager function.

Just something to think about for the future.

[angelayang, 2017-01-13 02:06:03]

The CQ bit was checked by angelayang@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 02:07:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 05:04:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 05:04:56]

Dry run: This issue passed the CQ dry run.

[angelayang, 2017-01-13 05:27:39]

The CQ bit was checked by angelayang@google.com

[commit-bot: I haz the power, 2017-01-13 05:28:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 05:36:56]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484285259196540,
"parent_rev": "05529947077574fb6d7ebd4e0a9ced6159847fc9", "commit_rev":
"2c22f5c684f13a4b7b9eeb3ff7007e80e5b1e8b0"}

[commit-bot: I haz the power, 2017-01-13 05:38:27]

Description was changed from

==========
[MD Bookmarks] Add search.

This CL implements search by sending a query to the existing backend
search and then updating the ui to reflect the search result. The
bookmarks list will now either display the contents of a folder or the
search results.

BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Bookmarks] Add search.

This CL implements search by sending a query to the existing backend
search and then updating the ui to reflect the search result. The
bookmarks list will now either display the contents of a folder or the
search results.

BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2614703003
Cr-Commit-Position: refs/heads/master@{#443499}
Committed:
https://chromium.googlesource.com/chromium/src/+/2c22f5c684f13a4b7b9eeb3ff700...
==========

[commit-bot: I haz the power, 2017-01-13 05:38:28]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/2c22f5c684f13a4b7b9eeb3ff700...