[MD Bookmarks] Persist collapsed folders between page loads.

[MD Bookmarks] Persist collapsed folders between page loads.

This CL persists the closed folder state to local storage.

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

Review-Url: https://codereview.chromium.org/2776993002
Cr-Commit-Position: refs/heads/master@{#461638}
Committed: https://chromium.googlesource.com/chromium/src/+/e9e0139202b4bf30fd7c6bb3baedce981a72e265

[calamity, 2017-03-27 03:22:38]

Description was changed from

==========
[MD Bookmarks] Persist collapsed folders between page loads.

This CL persists the closed folder state to local storage.

BUG=692868
==========

to

==========
[MD Bookmarks] Persist collapsed folders between page loads.

This CL persists the closed folder state to local storage.

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

[calamity, 2017-03-29 02:24:15]

calamity@chromium.org changed reviewers:
+ tsergeant@chromium.org

[calamity, 2017-03-29 02:24:51]

The diff here is mostly from adding constants.html.

[calamity, 2017-03-29 02:49:00]

Patchset #1 (id:1) has been deleted

[calamity, 2017-03-29 02:49:09]

Patchset #1 (id:20001) has been deleted

[tsergeant, 2017-03-29 04:43:24]

Generally lg, but one question and a nit:

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

https://codereview.chromium.org/2776993002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/constants.js:19: var
LOCAL_STORAGE_CLOSED_FOLDERS_KEY = 'bookmarkPageClosedState';
Ultra nit: bookmarkManagerClosedState, maybe? 'Bookmark manager' is kind of the
full name of the page.

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

https://codereview.chromium.org/2776993002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.js:82:
window.localStorage[LOCAL_STORAGE_CLOSED_FOLDERS_KEY] =
I'm a bit torn about where this should live. On one hand, maybe it should live
in app.js, alongside the code which loads the state from localstorage. On the
other hand, it does kind of make sense to have it here where it changes.

Did you think about putting it in app? Any particular reason why you went for
here?

[calamity, 2017-03-30 03:16:04]

Added a test.

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

https://codereview.chromium.org/2776993002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/constants.js:19: var
LOCAL_STORAGE_CLOSED_FOLDERS_KEY = 'bookmarkPageClosedState';
On 2017/03/29 04:43:24, tsergeant wrote:
> Ultra nit: bookmarkManagerClosedState, maybe? 'Bookmark manager' is kind of
the
> full name of the page.

Done.

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

https://codereview.chromium.org/2776993002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.js:82:
window.localStorage[LOCAL_STORAGE_CLOSED_FOLDERS_KEY] =
On 2017/03/29 04:43:24, tsergeant wrote:
> I'm a bit torn about where this should live. On one hand, maybe it should live
> in app.js, alongside the code which loads the state from localstorage. On the
> other hand, it does kind of make sense to have it here where it changes.
> 
> Did you think about putting it in app? Any particular reason why you went for
> here?

Yeah, basically because this is where it changes. I think you're right actually.
This won't pick up more subtle things like openAncestorFolders etc.

[calamity, 2017-03-30 03:16:12]

Patchset #2 (id:60001) has been deleted

[tsergeant, 2017-03-30 04:19:57]

Cool, looks better this way (and thanks for adding a test)

https://codereview.chromium.org/2776993002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/app_test.js (right):

https://codereview.chromium.org/2776993002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/app_test.js:5: suite('app', function() {
Nit: Name the suite with the full element name: <bookmarks-app>

https://codereview.chromium.org/2776993002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/app_test.js:35: store = new
bookmarks.TestStore({});
I'm not keen on duplicating all the initialization stuff again here.

How about you pull this out into a reset() function, then call reset() from
setup() and from here. This version of the initialization code does some extra
stuff (acceptInit), but it doesn't really matter if that runs for the first part
of the test as well.

[calamity, 2017-04-03 03:48:42]

https://codereview.chromium.org/2776993002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/app_test.js (right):

https://codereview.chromium.org/2776993002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/app_test.js:5: suite('app', function() {
On 2017/03/30 04:19:57, tsergeant wrote:
> Nit: Name the suite with the full element name: <bookmarks-app>

Done.

https://codereview.chromium.org/2776993002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/app_test.js:35: store = new
bookmarks.TestStore({});
On 2017/03/30 04:19:57, tsergeant wrote:
> I'm not keen on duplicating all the initialization stuff again here.
> 
> How about you pull this out into a reset() function, then call reset() from
> setup() and from here. This version of the initialization code does some extra
> stuff (acceptInit), but it doesn't really matter if that runs for the first
part
> of the test as well.

Done. I also changed acceptInit to acceptInitOnce so that we won't run into
bookmarks.getTree coming back at an inopportune time and ruining our TestStore.

[tsergeant, 2017-04-03 04:01:17]

lgtm

[calamity, 2017-04-04 02:58:43]

The CQ bit was checked by calamity@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 02:59:12]

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-04-04 03:11:27]

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

[commit-bot: I haz the power, 2017-04-04 03:11:28]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[calamity, 2017-04-04 03:57:01]

The CQ bit was checked by calamity@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 03:57:27]

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-04-04 05:00:33]

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

[commit-bot: I haz the power, 2017-04-04 05:00:34]

Dry run: This issue passed the CQ dry run.

[calamity, 2017-04-04 05:35:25]

The CQ bit was checked by calamity@chromium.org

[calamity, 2017-04-04 05:35:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org
Link to the patchset: https://codereview.chromium.org/2776993002/#ps120001
(title: "fix closure")

[commit-bot: I haz the power, 2017-04-04 05:35:42]

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-04-04 05:39:56]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491284125651670,
"parent_rev": "d9271fbbafb0fcc5e104d7e397cf3dbb9f9aeda1", "commit_rev":
"e9e0139202b4bf30fd7c6bb3baedce981a72e265"}

[commit-bot: I haz the power, 2017-04-04 05:40:44]

Description was changed from

==========
[MD Bookmarks] Persist collapsed folders between page loads.

This CL persists the closed folder state to local storage.

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

to

==========
[MD Bookmarks] Persist collapsed folders between page loads.

This CL persists the closed folder state to local storage.

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

Review-Url: https://codereview.chromium.org/2776993002
Cr-Commit-Position: refs/heads/master@{#461638}
Committed:
https://chromium.googlesource.com/chromium/src/+/e9e0139202b4bf30fd7c6bb3baed...
==========

[commit-bot: I haz the power, 2017-04-04 05:40:45]

Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/e9e0139202b4bf30fd7c6bb3baed...