[MD Bookmarks] Modify search to retain the previously selected folder.

[MD Bookmarks] Modify search to retain the previously selected folder.

This CL modifies the bookmark manager so that closing a search returns
users to the previously selected folder instead of the default folder.
The previous folder id also persists in the url during search.

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

[angelayang, 2017-01-23 05:52:10]

Description was changed from

==========
[MD Bookmarks] Modify search to retain the previously selected folder.

This CL modifies the bookmark manager so that closing a search returns
users to the previously selected folder instead of the default folder.
The previous folder id also persists in the url during search.

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

to

==========
[MD Bookmarks] Modify search to retain the previously selected folder.

This CL modifies the bookmark manager so that closing a search returns
users to the previously selected folder instead of the default folder.
The previous folder id also persists in the url during search.

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

[angelayang, 2017-01-23 06:07:03]

angelayang@google.com changed reviewers:
+ jiaxi@google.com

[angelayang, 2017-01-23 06:07:03]

The selectedId is now in the parameter list during search. This also means that
on closing search we have to manually tell the displayList to update to the
folder since selectedId remains the same.

[angelayang, 2017-01-23 06:41:44]

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

[angelayang, 2017-01-23 06:41:44]

Hi, adding tsergeant and calamity to this cl- just so you guys can take a look
while im gone and highlight any concerns you have with the implementation.

[tsergeant, 2017-01-25 00:23:18]

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

https://codereview.chromium.org/2645273002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/store.js:15: /** @type {?string} */
There's now no way for this to be null, right? You should remove this
annotation, and then go through and remove any null-checks which are no longer
needed.

https://codereview.chromium.org/2645273002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/store.js:120:
this.fire('selected-folder-changed', selectedFolder);
I mentioned this before, but I think now is the right time to do this: We should
stop firing this event from inside store. It makes the code harder to read, and
prevents closure from doing a little bit of type checking.

Instead:

1. Move the code from onSelectedFolderChanged_ into a new (public, maybe?)
method which takes an integer id and selects that folder
2. Call that method from onSelectedFolderChanged
3. Call that method from everywhere that currently fires
'selected-folder-changed'
4. Maybe? Move the code from updateSelectedDisplay_() into that new method. I'm
not sure that there are any places where we don't want those two pieces of code
to be called at the same time, so it probably makes sense to put them together.

https://codereview.chromium.org/2645273002/diff/1/chrome/test/data/webui/md_b...
File chrome/test/data/webui/md_bookmarks/store_test.js (right):

https://codereview.chromium.org/2645273002/diff/1/chrome/test/data/webui/md_b...
chrome/test/data/webui/md_bookmarks/store_test.js:280: // Searching sets the
previous folder and searchTerm as the URL parameters.
You should also test the opposite direction: If I navigate to /?id=1&q=blah, it
needs to show a search

[angelayang, 2017-01-31 00:44:35]

Patchset #2 (id:20001) has been deleted

[angelayang, 2017-01-31 01:55:56]

Patchset #2 (id:40001) has been deleted

[angelayang, 2017-01-31 02:25:04]

Hello, made a few changes to the store. Mostly i wanted the store to use the
same piece of logic to deal with routing on setup and routing on navigating
back/forth. I'm not sure if this has made it more confusing, please let me know
what you think?

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

https://codereview.chromium.org/2645273002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/store.js:15: /** @type {?string} */
On 2017/01/25 00:23:18, tsergeant wrote:
> There's now no way for this to be null, right? You should remove this
> annotation, and then go through and remove any null-checks which are no longer
> needed.

Done.

https://codereview.chromium.org/2645273002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/store.js:120:
this.fire('selected-folder-changed', selectedFolder);
On 2017/01/25 00:23:18, tsergeant wrote:
> I mentioned this before, but I think now is the right time to do this: We
should
> stop firing this event from inside store. It makes the code harder to read,
and
> prevents closure from doing a little bit of type checking.
> 
> Instead:
> 
> 1. Move the code from onSelectedFolderChanged_ into a new (public, maybe?)
> method which takes an integer id and selects that folder
> 2. Call that method from onSelectedFolderChanged
> 3. Call that method from everywhere that currently fires
> 'selected-folder-changed'
> 4. Maybe? Move the code from updateSelectedDisplay_() into that new method.
I'm
> not sure that there are any places where we don't want those two pieces of
code
> to be called at the same time, so it probably makes sense to put them
together.

Yep all that works

https://codereview.chromium.org/2645273002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/store.js:244: if (this.searchTerm)
This piece of logic reoccurs a few times so i'm going to move it into a separate
function updateSelectedId

https://codereview.chromium.org/2645273002/diff/1/chrome/test/data/webui/md_b...
File chrome/test/data/webui/md_bookmarks/store_test.js (right):

https://codereview.chromium.org/2645273002/diff/1/chrome/test/data/webui/md_b...
chrome/test/data/webui/md_bookmarks/store_test.js:280: // Searching sets the
previous folder and searchTerm as the URL parameters.
On 2017/01/25 00:23:18, tsergeant wrote:
> You should also test the opposite direction: If I navigate to /?id=1&q=blah,
it
> needs to show a search

Okay i placed those in the 'search updates from route', would i also need them
for 'update from route on setup'?

[tsergeant, 2017-01-31 03:21:01]

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

https://codereview.chromium.org/2645273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:149: updateSelectedId_:
function(id) {
The API here is really unclear. If I want to select a folder, which function do
I need to call? My expectation from reading the code is that I would call the
public `selectFolder` function, but that doesn't deselect any existing selected
folder.

Is there any reason why we can't have one `selectFolder` function that does all
the work of both these functions? That would simplify things a lot.

[angelayang, 2017-01-31 06:29:26]

I hope that's easier to follow now, thanks~

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

https://codereview.chromium.org/2645273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store.js:149: updateSelectedId_:
function(id) {
On 2017/01/31 03:21:01, tsergeant wrote:
> The API here is really unclear. If I want to select a folder, which function
do
> I need to call? My expectation from reading the code is that I would call the
> public `selectFolder` function, but that doesn't deselect any existing
selected
> folder.
> 
> Is there any reason why we can't have one `selectFolder` function that does
all
> the work of both these functions? That would simplify things a lot.

yeah that works much better and clearer thanks

[angelayang, 2017-02-08 02:23:39]

Rebased to include the item select patch. 
In addition, the selected-folder-event now only clears the search term if given
a clearSearch parameter. Take a look thanks.

[tsergeant, 2017-02-08 03:11:10]

Cool!

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

https://codereview.chromium.org/2645273002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/store.js:43: anchorIndex_: {
Any particular reason why this needed to change? Nothing else in the CL touches
anchorIndex

https://codereview.chromium.org/2645273002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/store.js:167: selectFolder_: function(id)
{
I think we should make this a public method.

This method is an important part of the public API for the element, and it's
already publicly accessible through the selected-folder-changed event.

Then, in the tests, anywhere where you don't specifically want to test that the
event is working correctly, you can just call the `store.selectFolder()` method
instead of firing the event, which is much easier to read.

[jiaxi, 2017-02-08 04:16:14]

There's a new commit for double click. Since we both touched
onSelectedFolderChanged_, it might have massive merge conflicts.....

[angelayang, 2017-02-08 04:35:21]

On 2017/02/08 04:16:14, jiaxi wrote:
> There's a new commit for double click. Since we both touched
> onSelectedFolderChanged_, it might have massive merge conflicts.....

Yep no worries. I'll pull the changes.

[angelayang, 2017-02-09 04:17:36]

Hello, i pulled Charlotte's changed into this refactor.

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

https://codereview.chromium.org/2645273002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/store.js:43: anchorIndex_: {
On 2017/02/08 03:11:10, tsergeant wrote:
> Any particular reason why this needed to change? Nothing else in the CL
touches
> anchorIndex

Previously anchorIndex is set to null on initialization by updateSelectedDisplay
i believe which is now gone in this refactor. There is a test to check if it's
null on initialization. (It's now undefined on initialization) Perhaps I could
change test to match new behavior? what do you think?