Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2637023002:
[MD Bookmarks] Add routing. (Closed)
|
Created:
3 years, 11 months ago by angelayang Modified:
3 years, 11 months ago CC:
chromium-reviews, michaelpg+watch-md-ui_chromium.org, vitalyp+closure_chromium.org, jlklein+watch-closure_chromium.org, arv+watch_chromium.org, dbeam+watch-closure_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD Bookmarks] Add routing.
Implement URLs that go directly to folders and searches.
The URL now reflects the selectedId and the searchTerm in the
parameter list.
BUG=658980
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2637023002
Cr-Commit-Position: refs/heads/master@{#445318}
Committed: https://chromium.googlesource.com/chromium/src/+/47247ddf01cec0329fa6ee03e2bf47fa813c540f
Patch Set 1 : First patch fix #
Total comments: 32
Patch Set 2 : Set id as a URL parameter and route non-folder ids to the default folder. #
Total comments: 11
Patch Set 3 : Id is now in the queryParam not path. Store has correct field for back/forwards navigation. #
Total comments: 4
Patch Set 4 : Reuse the existing searchTerm and selectedId handlers for query change events. #
Total comments: 4
Patch Set 5 : Fix tests for routing on store setup. #
Total comments: 2
Patch Set 6 : Move store setup into replaceStore function. #
Total comments: 2
Patch Set 7 : Fix naming. #Dependent Patchsets: Messages
Total messages: 35 (17 generated)
Description was changed from ========== [MD Bookmarks] Add routing. Implement URLs that go directly to folders and a search. The URL now reflects the selectedId in the path and the searchTerm in the parameter list. BUG=658980 ========== to ========== [MD Bookmarks] Add routing. Implement URLs that go directly to folders and a search. The URL now reflects the selectedId in the path and the searchTerm in the parameter list. BUG=658980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [MD Bookmarks] Add routing. Implement URLs that go directly to folders and a search. The URL now reflects the selectedId in the path and the searchTerm in the parameter list. BUG=658980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD Bookmarks] Add routing. Implement URLs that go directly to folders and searches. The URL now reflects the selectedId in the path and the searchTerm in the parameter list. BUG=658980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
angelayang@google.com changed reviewers: + jiaxi@google.com
Hello, take a look at my first patch for routing. thanks
lgtm https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/router.js (right): https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/router.js:18: }, queryParams: Object, https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:81: }, I have a feeling that this shouldn't be in the private section... double check with Tim and Chris.
angelayang@google.com changed reviewers: + calamity@google.com, tsergeant@chromium.org
Hello, take a look at routing https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/router.js (right): https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/router.js:18: }, On 2017/01/17 05:19:02, jiaxi wrote: > queryParams: Object, Done. https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:81: }, On 2017/01/17 05:19:02, jiaxi wrote: > I have a feeling that this shouldn't be in the private section... double check > with Tim and Chris. Yeah true, i didn't annotate as private either- i'll fix in my next patch.
angelayang@google.com changed reviewers: + calamity@chromium.org - calamity@google.com
Here's my first pass. https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/app.html (right): https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/app.html:43: selected-id="{{selectedId}}"> Remove? https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/router.html (right): https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/router.html:7: <style></style> Ain't got no style, don't need no <style></style> https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/router.js (right): https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/router.js:5: Polymer({ This element deserves a comment, particularly because it has a one-way bound interface, so mention that clients initialize themselves by reading the fields after attach. https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/router.js:10: path: { I think you can make a few of these fields private. https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/router.js:15: /** Parameter is routed to the searchTerm */ Use // for non-jsdoc comments. https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:81: }, On 2017/01/17 05:36:03, angelayang wrote: > On 2017/01/17 05:19:02, jiaxi wrote: > > I have a feeling that this shouldn't be in the private section... double check > > with Tim and Chris. > > Yeah true, i didn't annotate as private either- i'll fix in my next patch. Yeah, move it up above intialize-store and make a section for public: https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:94: // Check for a selectedId from the router. // Initialize the store's fields from the router. ? https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:98: this.searchTerm = this.router.searchTerm; Hrmmmmm. How hard is it to make it so that selectedId is preserved in the search URL? Right now, if you go to Folder X and then search, the URL loses the information so refreshing doesn't leave you in the exact same state. https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/toolbar.js (right): https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/toolbar.js:72: if (this.searchField.getValue() != this.searchTerm) What happens if you just remove this check...? https://codereview.chromium.org/2637023002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2637023002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/store_test.js:32: /** Clean up anything left in URL */ // https://codereview.chromium.org/2637023002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/store_test.js:232: // router tests: clang-format will want this indented. Restyle accordingly. I'm a fan of separators in this test. Can you go up and see what else you can lump into sections? =D https://codereview.chromium.org/2637023002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/test_util.js (right): https://codereview.chromium.org/2637023002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/test_util.js:75: * @param {Array} searchResult results https://codereview.chromium.org/2637023002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/test_util.js:77: function replaceChromeSearch (searchResult) { overrideBookmarksSearch, perhaps? https://codereview.chromium.org/2637023002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/test_util.js:81: } I like this, but maybe just put it in store_test.js somewhere for now, until more than 1 test needs it.
Hi i've fixed the patch according to the suggested. Having the selectedId persist through search will be done in follow up CL. Thankyou https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/app.html (right): https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/app.html:43: selected-id="{{selectedId}}"> On 2017/01/18 03:37:21, calamity wrote: > Remove? Done. https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/router.html (right): https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/router.html:7: <style></style> On 2017/01/18 03:37:21, calamity wrote: > Ain't got no style, don't need no <style></style> Done. https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/router.js (right): https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/router.js:5: Polymer({ On 2017/01/18 03:37:21, calamity wrote: > This element deserves a comment, particularly because it has a one-way bound > interface, so mention that clients initialize themselves by reading the fields > after attach. Done. https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/router.js:10: path: { On 2017/01/18 03:37:21, calamity wrote: > I think you can make a few of these fields private. Done. https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/router.js:15: /** Parameter is routed to the searchTerm */ On 2017/01/18 03:37:21, calamity wrote: > Use // for non-jsdoc comments. Done. https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:81: }, On 2017/01/18 03:37:21, calamity wrote: > On 2017/01/17 05:36:03, angelayang wrote: > > On 2017/01/17 05:19:02, jiaxi wrote: > > > I have a feeling that this shouldn't be in the private section... double > check > > > with Tim and Chris. > > > > Yeah true, i didn't annotate as private either- i'll fix in my next patch. > > Yeah, move it up above intialize-store and make a section for public: Done. https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:94: // Check for a selectedId from the router. On 2017/01/18 03:37:21, calamity wrote: > // Initialize the store's fields from the router. > > ? Done. https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:98: this.searchTerm = this.router.searchTerm; On 2017/01/18 03:37:21, calamity wrote: > Hrmmmmm. How hard is it to make it so that selectedId is preserved in the search > URL? > > Right now, if you go to Folder X and then search, the URL loses the information > so refreshing doesn't leave you in the exact same state. Discussed: Will do this in follow up patch. https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/toolbar.js (right): https://codereview.chromium.org/2637023002/diff/20001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/toolbar.js:72: if (this.searchField.getValue() != this.searchTerm) On 2017/01/18 03:37:21, calamity wrote: > What happens if you just remove this check...? Oh yes it would be fine, i didn't realize that thanks https://codereview.chromium.org/2637023002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2637023002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/store_test.js:32: /** Clean up anything left in URL */ On 2017/01/18 03:37:22, calamity wrote: > // Done. https://codereview.chromium.org/2637023002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/store_test.js:232: // router tests: On 2017/01/18 03:37:22, calamity wrote: > clang-format will want this indented. Restyle accordingly. > > I'm a fan of separators in this test. Can you go up and see what else you can > lump into sections? =D Done. https://codereview.chromium.org/2637023002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/test_util.js (right): https://codereview.chromium.org/2637023002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/test_util.js:75: * @param {Array} searchResult On 2017/01/18 03:37:22, calamity wrote: > results Done. https://codereview.chromium.org/2637023002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/test_util.js:77: function replaceChromeSearch (searchResult) { On 2017/01/18 03:37:22, calamity wrote: > overrideBookmarksSearch, perhaps? Done. https://codereview.chromium.org/2637023002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/test_util.js:81: } On 2017/01/18 03:37:22, calamity wrote: > I like this, but maybe just put it in store_test.js somewhere for now, until > more than 1 test needs it. Done.
Patchset #2 (id:40001) has been deleted
Cool! https://codereview.chromium.org/2637023002/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/router.js (right): https://codereview.chromium.org/2637023002/diff/60001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/router.js:37: this.selectedId = this.queryParams_.id; The back/forward buttons in the URL bar won't actually change the page state correctly. This is because clicking those buttons will call onQueryChanged_, but nothing actually gets those changes up into the store (instead, you entirely rely on setupStore_, which is only called during page initialization). https://codereview.chromium.org/2637023002/diff/60001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/router.js:49: }) ; here https://codereview.chromium.org/2637023002/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2637023002/diff/60001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:79: get router() { Does this actually need to be public? https://codereview.chromium.org/2637023002/diff/60001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:80: return /** @type {BookmarksRouterElement} */ (this.$$('bookmarks-router')); Give the router an ID in the <template> so that you can do `this.$.router` (which is a nice constant time lookup) https://codereview.chromium.org/2637023002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2637023002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/store_test.js:278: store.setupStore_(TEST_TREE); As above, it is possible to navigate the page without calling setupStore_(). You should make sure you have tests which cover both cases.
Fixed up problems with navigating back and forth and updated tests. Thanks https://codereview.chromium.org/2637023002/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/router.js (right): https://codereview.chromium.org/2637023002/diff/60001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/router.js:37: this.selectedId = this.queryParams_.id; On 2017/01/18 23:03:58, tsergeant wrote: > The back/forward buttons in the URL bar won't actually change the page state > correctly. This is because clicking those buttons will call onQueryChanged_, but > nothing actually gets those changes up into the store (instead, you entirely > rely on setupStore_, which is only called during page initialization). Oh I understand, i didn't consider this. I think in this case I will fire an event to the store https://codereview.chromium.org/2637023002/diff/60001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/router.js:49: }) On 2017/01/18 23:03:58, tsergeant wrote: > ; here Done. https://codereview.chromium.org/2637023002/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2637023002/diff/60001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:79: get router() { On 2017/01/18 23:03:58, tsergeant wrote: > Does this actually need to be public? removed https://codereview.chromium.org/2637023002/diff/60001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/store.js:80: return /** @type {BookmarksRouterElement} */ (this.$$('bookmarks-router')); On 2017/01/18 23:03:58, tsergeant wrote: > Give the router an ID in the <template> so that you can do `this.$.router` > (which is a nice constant time lookup) oh yep that sounds nicer https://codereview.chromium.org/2637023002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2637023002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/store_test.js:278: store.setupStore_(TEST_TREE); On 2017/01/18 23:03:58, tsergeant wrote: > As above, it is possible to navigate the page without calling setupStore_(). You > should make sure you have tests which cover both cases. Done. https://codereview.chromium.org/2637023002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/store_test.js:280: console.log('default ' + store.rootNode.children[0].id); oops
You should update the CL description to reflect that id comes through a queryParam https://codereview.chromium.org/2637023002/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/router.js (right): https://codereview.chromium.org/2637023002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/router.js:38: this.fire('url-changed'); Rather than adding a new handler, is it possible to reuse the search-term-changed and selected-folder-changed events from other parts of the page? This would be nice, since it means that the APIs are consistent and you only need to test one thing. https://codereview.chromium.org/2637023002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2637023002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/store_test.js:156: '6': 'rootNode.children.#2', Just leave these here now that you've written them, but for future reference, when you expand the tree above it's probably not worth appending to every test like this. The tests here should already cover all of the interesting cases, so adding extra stuff to them is just extra work (including maintenance burden) without any benefit.
Patchset #4 (id:100001) has been deleted
Hello, I improved on the last patch as per Tim's comments. I have to preserve the folder open states between page loads. But i think that'll be another patch. Take a look when you get a chance? Thankyou https://codereview.chromium.org/2637023002/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_bookmarks/router.js (right): https://codereview.chromium.org/2637023002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_bookmarks/router.js:38: this.fire('url-changed'); On 2017/01/19 06:45:56, tsergeant wrote: > Rather than adding a new handler, is it possible to reuse the > search-term-changed and selected-folder-changed events from other parts of the > page? This would be nice, since it means that the APIs are consistent and you > only need to test one thing. for sure https://codereview.chromium.org/2637023002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2637023002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/md_bookmarks/store_test.js:156: '6': 'rootNode.children.#2', On 2017/01/19 06:45:56, tsergeant wrote: > Just leave these here now that you've written them, > > but for future reference, when you expand the tree above it's probably not worth > appending to every test like this. The tests here should already cover all of > the interesting cases, so adding extra stuff to them is just extra work > (including maintenance burden) without any benefit. Okay sure no worries, i'll keep that in mind
https://codereview.chromium.org/2637023002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2637023002/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:90: if (this.$.router.searchTerm) { Remove {} here https://codereview.chromium.org/2637023002/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2637023002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:261: navigateTo('/?q=' + searchTerm); It's minor, but I think these tests are still not quite right. Calling navigateTo will cause the router to update and fire a search-term-changed event. After that, we call setupStore which can choose to update the page state based on the router state. This means that when we assert below it's not necessarily clear whether it worked because of navigateTo() or because of setupStore(). eg, A bug in setupStore() could be disguised because of navigateTo(). My preference would be to have the tests just call navigateTo, since that is the default way the router works. Then, you can have a separate suite of tests that check the initialization is correct (using setupStore), since that's the weird edge case. To do that, you'll need to create separate tests which change the url and then create a new store.
Hopefully the new store tests make more sense. Thankyou https://codereview.chromium.org/2637023002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/store.js (right): https://codereview.chromium.org/2637023002/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/store.js:90: if (this.$.router.searchTerm) { On 2017/01/20 02:13:09, tsergeant wrote: > Remove {} here Done. https://codereview.chromium.org/2637023002/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2637023002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:261: navigateTo('/?q=' + searchTerm); On 2017/01/20 02:13:09, tsergeant wrote: > It's minor, but I think these tests are still not quite right. > > Calling navigateTo will cause the router to update and fire a > search-term-changed event. After that, we call setupStore which can choose to > update the page state based on the router state. > > This means that when we assert below it's not necessarily clear whether it > worked because of navigateTo() or because of setupStore(). eg, A bug in > setupStore() could be disguised because of navigateTo(). > > My preference would be to have the tests just call navigateTo, since that is the > default way the router works. Then, you can have a separate suite of tests that > check the initialization is correct (using setupStore), since that's the weird > edge case. > > To do that, you'll need to create separate tests which change the url and then > create a new store. Yep i'll fix that up
lgtm as mentioned above, > You should update the CL description to reflect that id comes through a queryParam https://codereview.chromium.org/2637023002/diff/140001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2637023002/diff/140001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:269: store = document.createElement('bookmarks-store'); If you want, you can pull these three lines (createElement, replaceBody, setupStore) out into a separate replaceStore function, since they get used in 3 places
Description was changed from ========== [MD Bookmarks] Add routing. Implement URLs that go directly to folders and searches. The URL now reflects the selectedId in the path and the searchTerm in the parameter list. BUG=658980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD Bookmarks] Add routing. Implement URLs that go directly to folders and searches. The URL now reflects the selectedId and the searchTerm in the parameter list. BUG=658980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2017/01/23 00:02:52, tsergeant wrote: > lgtm > > as mentioned above, > > > You should update the CL description to reflect that id comes through a > queryParam > > https://codereview.chromium.org/2637023002/diff/140001/chrome/test/data/webui... > File chrome/test/data/webui/md_bookmarks/store_test.js (right): > > https://codereview.chromium.org/2637023002/diff/140001/chrome/test/data/webui... > chrome/test/data/webui/md_bookmarks/store_test.js:269: store = > document.createElement('bookmarks-store'); > If you want, you can pull these three lines (createElement, replaceBody, > setupStore) out into a separate replaceStore function, since they get used in 3 > places oops sorry i changed the patch description last time. Fixed now, all done :)
Ready to land this :) https://codereview.chromium.org/2637023002/diff/140001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2637023002/diff/140001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:269: store = document.createElement('bookmarks-store'); On 2017/01/23 00:02:52, tsergeant wrote: > If you want, you can pull these three lines (createElement, replaceBody, > setupStore) out into a separate replaceStore function, since they get used in 3 > places Done.
https://codereview.chromium.org/2637023002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/md_bookmarks/router.js (right): https://codereview.chromium.org/2637023002/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_bookmarks/router.js:26: observer: 'onSelectedChanged_', nit: onSelectedIdChanged_ https://codereview.chromium.org/2637023002/diff/160001/chrome/test/data/webui... File chrome/test/data/webui/md_bookmarks/store_test.js (right): https://codereview.chromium.org/2637023002/diff/160001/chrome/test/data/webui... chrome/test/data/webui/md_bookmarks/store_test.js:296: assertEquals(store.rootNode.children[0].id, store.selectedId); Add a test for routing to the id of a URL bookmark.
lgtm
The CQ bit was checked by angelayang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by angelayang@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jiaxi@google.com, tsergeant@chromium.org, calamity@chromium.org Link to the patchset: https://codereview.chromium.org/2637023002/#ps180001 (title: "Fix naming.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1485143322210690,
"parent_rev": "269b6bc66eb96f69d61d814ea7ec1ffef695eee3", "commit_rev":
"47247ddf01cec0329fa6ee03e2bf47fa813c540f"}
Message was sent while issue was closed.
Description was changed from ========== [MD Bookmarks] Add routing. Implement URLs that go directly to folders and searches. The URL now reflects the selectedId and the searchTerm in the parameter list. BUG=658980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD Bookmarks] Add routing. Implement URLs that go directly to folders and searches. The URL now reflects the selectedId and the searchTerm in the parameter list. BUG=658980 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2637023002 Cr-Commit-Position: refs/heads/master@{#445318} Committed: https://chromium.googlesource.com/chromium/src/+/47247ddf01cec0329fa6ee03e2bf... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as https://chromium.googlesource.com/chromium/src/+/47247ddf01cec0329fa6ee03e2bf... |
