Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2184893002:
Settings Router Refactor: Remove route.page legacy property. (Closed)
|
Created:
4 years, 4 months ago by tommycli Modified:
4 years, 4 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@0217-settings-refactor-settings-menu Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSettings Router Refactor: Remove route.page legacy property.
This property is not used that much, and can be removed.
BUG=608115, 628502
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/aa9c34d72c27a54b09d1790fa425ea2f25cc9716
Cr-Commit-Position: refs/heads/master@{#408815}
Patch Set 1 #
Total comments: 16
Patch Set 2 : merge and address comments #Patch Set 3 : change method to parent.contains(child) #Patch Set 4 : update test #
Total comments: 14
Patch Set 5 : fix #Patch Set 6 : fix contains issue. #
Total comments: 3
Depends on Patchset: Messages
Total messages: 45 (28 generated)
Description was changed from ========== Settings Router Refactor: Remove route.page legacy property. This property is not used that much, and can be removed. BUG=608115 ========== to ========== Settings Router Refactor: Remove route.page legacy property. This property is not used that much, and can be removed. BUG=608115 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Settings Router Refactor: Remove route.page legacy property. This property is not used that much, and can be removed. BUG=608115 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Settings Router Refactor: Remove route.page legacy property. This property is not used that much, and can be removed. BUG=608115 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
tommycli@chromium.org changed reviewers: + michaelpg@chromium.org
michaelpg: PTAL, this axes the usage of .page as a route property. Also updates a few legacy usages of routes in general.
The CQ bit was checked by tommycli@chromium.org
The CQ bit was unchecked by tommycli@chromium.org
https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/device_page.js (right): https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/device_page.js:132: * TODO(michaelpg): create generic fn for this and isCurrentRouteOnSyncPage_. see next comment: delete isCurrentRouteOnSyncPage_ https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/device_page.js:137: return this.currentRoute == settings.Route.POINTERS; meh, optionally: just inline this -- it's literally the same as the function name, and device_page_tests.js doesn't need to use it since it knows the route too https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:83: inSubtreeOf: function(route) { bikeshedding: kind of a confusing name and concept. *generally* subtree means "strict subtree". combining isDescendantOf and == is kinda awkward, IMO: this function is used for two entirely different mutually exclusive reasons in the app (checking the root) and the tests (checking the parent chain). see comments below about .root. https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_main/settings_main.js:115: if (newRoute.inSubtreeOf(settings.Route.ABOUT)) { so, what we care about is what is the *root* of newRoute. I think 'page' was useful for providing that info, even if the name sucks. I move to create a read-only 'root' property of settings.Route which provides the root of the route (one of ABOUT, BASIC or ADVANCED). here, we would just check (newRoute.root == settings.Route.ABOUT), instead of asking if we're equal to or a descendant of ABOUT. https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_page/main_page_behavior.js:410: if (newRoute && newRoute.section && this.getSection_(newRoute.section)) is this just asking if the section is visible? can we just check .hidden or offsetHeight or something?
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
michaelpg: thanks for the review. I added some comments below. https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/device_page.js (right): https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/device_page.js:132: * TODO(michaelpg): create generic fn for this and isCurrentRouteOnSyncPage_. On 2016/07/28 23:22:31, michaelpg wrote: > see next comment: delete isCurrentRouteOnSyncPage_ Done. https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/device_page.js:137: return this.currentRoute == settings.Route.POINTERS; On 2016/07/28 23:22:31, michaelpg wrote: > meh, optionally: just inline this -- it's literally the same as the function > name, and device_page_tests.js doesn't need to use it since it knows the route > too Done. https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:83: inSubtreeOf: function(route) { On 2016/07/28 23:22:32, michaelpg wrote: > bikeshedding: kind of a confusing name and concept. *generally* subtree means > "strict subtree". combining isDescendantOf and == is kinda awkward, IMO: this > function is used for two entirely different mutually exclusive reasons in the > app (checking the root) and the tests (checking the parent chain). see comments > below about .root. It's a confusing name. I changed it to isEqualOrInSubtreeOf. I found it a useful concept since we need to check exactly this in settings_main and settings_menu. Though for section expansion, we actually need to check inStrictSubtreeOf, so that's kind of unfortunate... I didn't want to do .root, because we also need to do this operation for .section, and I wanted to consolidate those two concepts with the tree structure. Still open to more suggestions - I realize this is kind of confusing. https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_main/settings_main.js:115: if (newRoute.inSubtreeOf(settings.Route.ABOUT)) { On 2016/07/28 23:22:32, michaelpg wrote: > so, what we care about is what is the *root* of newRoute. I think 'page' was > useful for providing that info, even if the name sucks. > > I move to create a read-only 'root' property of settings.Route which provides > the root of the route (one of ABOUT, BASIC or ADVANCED). here, we would just > check (newRoute.root == settings.Route.ABOUT), instead of asking if we're equal > to or a descendant of ABOUT. So... I would create a 'root', except that we also need to know if we are within a section (to get rid of the section property). I think the isEqualOrInSubtreeOf concept allows us to cover both cases without adding more properties. https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_page/main_page_behavior.js:410: if (newRoute && newRoute.section && this.getSection_(newRoute.section)) On 2016/07/28 23:22:32, michaelpg wrote: > is this just asking if the section is visible? can we just check .hidden or > offsetHeight or something? I added a comment. This actually scrolls to the desired section if this main page contains the route's section. For instance, BASIC page should ignore it if we're navigating to the PRIVACY section, since the ADVANCED main page will handle the scrolling.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:83: inSubtreeOf: function(route) { On 2016/07/29 00:03:35, tommycli wrote: > On 2016/07/28 23:22:32, michaelpg wrote: > > bikeshedding: kind of a confusing name and concept. *generally* subtree means > > "strict subtree". combining isDescendantOf and == is kinda awkward, IMO: this > > function is used for two entirely different mutually exclusive reasons in the > > app (checking the root) and the tests (checking the parent chain). see > comments > > below about .root. > > It's a confusing name. I changed it to isEqualOrInSubtreeOf. > > I found it a useful concept since we need to check exactly this in settings_main > and settings_menu. Though for section expansion, we actually need to check > inStrictSubtreeOf, so that's kind of unfortunate... Like, that's my point: every use in this CL could be replaced by a function "isRoot()", so why muddle things. > > I didn't want to do .root, because we also need to do this operation for > .section, and I wanted to consolidate those two concepts with the tree > structure. > > Still open to more suggestions - I realize this is kind of confusing. how about isEqualOrDescendantOf? maybe it's just me, but I have to translate "subtree" in my head to "descendant of" because to me, the subtree of this route means this route and *its* descendants. https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_main/settings_main.js:115: if (newRoute.inSubtreeOf(settings.Route.ABOUT)) { On 2016/07/29 00:03:35, tommycli wrote: > On 2016/07/28 23:22:32, michaelpg wrote: > > so, what we care about is what is the *root* of newRoute. I think 'page' was > > useful for providing that info, even if the name sucks. > > > > I move to create a read-only 'root' property of settings.Route which provides > > the root of the route (one of ABOUT, BASIC or ADVANCED). here, we would just > > check (newRoute.root == settings.Route.ABOUT), instead of asking if we're > equal > > to or a descendant of ABOUT. > > So... I would create a 'root', except that we also need to know if we are within > a section (to get rid of the section property). in a later CL, you mean? Why do we want to get rid of .section? It's a concept of its own, that represents something specific in the UI. Will we have to mandate that any route that's a child of a root route has to correspond to a section? If so, why pretend section isn't structural? If not, how do we identify whether something is a section or a sub-page? I guess I don't know why we have to get rid of the concepts we've been using the whole time. We can achieve the objectives in https://docs.google.com/document/d/1nI22YBBNeYnBCB07wR7QL_k6NmGmfPI_G7A8l5-YTUM/ without removing convenient properties. > > I think the isEqualOrInSubtreeOf concept allows us to cover both cases without > adding more properties. https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_page/main_page_behavior.js (right): https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_page/main_page_behavior.js:410: if (newRoute && newRoute.section && this.getSection_(newRoute.section)) On 2016/07/29 00:03:35, tommycli wrote: > On 2016/07/28 23:22:32, michaelpg wrote: > > is this just asking if the section is visible? can we just check .hidden or > > offsetHeight or something? > > I added a comment. This actually scrolls to the desired section if this main > page contains the route's section. > > For instance, BASIC page should ignore it if we're navigating to the PRIVACY > section, since the ADVANCED main page will handle the scrolling. oh, that makes sense. Ugh, this element was never meant to be repeated in the DOM. Some of this stuff should probably move at some point.
https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_main/settings_main.js:115: if (newRoute.inSubtreeOf(settings.Route.ABOUT)) { On 2016/07/29 00:21:12, michaelpg wrote: > On 2016/07/29 00:03:35, tommycli wrote: > > On 2016/07/28 23:22:32, michaelpg wrote: > > > so, what we care about is what is the *root* of newRoute. I think 'page' was > > > useful for providing that info, even if the name sucks. > > > > > > I move to create a read-only 'root' property of settings.Route which > provides > > > the root of the route (one of ABOUT, BASIC or ADVANCED). here, we would just > > > check (newRoute.root == settings.Route.ABOUT), instead of asking if we're > > equal > > > to or a descendant of ABOUT. > > > > So... I would create a 'root', except that we also need to know if we are > within > > a section (to get rid of the section property). > > in a later CL, you mean? Why do we want to get rid of .section? It's a concept > of its own, that represents something specific in the UI. > > Will we have to mandate that any route that's a child of a root route has to > correspond to a section? If so, why pretend section isn't structural? If not, > how do we identify whether something is a section or a sub-page? just realized I'm conflating .section and .subpage, though presumably we're removing both properties. I see that checking route.section is the same as checking route.isEqualOrDescendantOf(sectionRoute). and we can still check if something is a subpage via route.subpage.length. Do the guys in your office think we already reached consensus on removing these properties? If so then let's do it. Regarding the function itself, I concede route.isEqualOrDescendantOf(ROOT_PAGE) is equivalent to isRootedAt(ROOT_PAGE) when ROOT_PAGE is a root page, so I guess we don't need a separate function/property for it... it's not the simplest API but it keeps it small. > > I guess I don't know why we have to get rid of the concepts we've been using the > whole time. We can achieve the objectives in > https://docs.google.com/document/d/1nI22YBBNeYnBCB07wR7QL_k6NmGmfPI_G7A8l5-YTUM/ > without removing convenient properties. > > > > > I think the isEqualOrInSubtreeOf concept allows us to cover both cases without > > adding more properties. >
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tommycli@chromium.org 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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
michaelpg: thanks! i have one response below. https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_main/settings_main.js:115: if (newRoute.inSubtreeOf(settings.Route.ABOUT)) { On 2016/07/29 00:29:14, michaelpg wrote: > On 2016/07/29 00:21:12, michaelpg wrote: > > On 2016/07/29 00:03:35, tommycli wrote: > > > On 2016/07/28 23:22:32, michaelpg wrote: > > > > so, what we care about is what is the *root* of newRoute. I think 'page' > was > > > > useful for providing that info, even if the name sucks. > > > > > > > > I move to create a read-only 'root' property of settings.Route which > > provides > > > > the root of the route (one of ABOUT, BASIC or ADVANCED). here, we would > just > > > > check (newRoute.root == settings.Route.ABOUT), instead of asking if we're > > > equal > > > > to or a descendant of ABOUT. > > > > > > So... I would create a 'root', except that we also need to know if we are > > within > > > a section (to get rid of the section property). > > > > in a later CL, you mean? Why do we want to get rid of .section? It's a concept > > of its own, that represents something specific in the UI. > > > > Will we have to mandate that any route that's a child of a root route has to > > correspond to a section? If so, why pretend section isn't structural? If not, > > how do we identify whether something is a section or a sub-page? > > just realized I'm conflating .section and .subpage, though presumably we're > removing both properties. I see that checking route.section is the same as > checking route.isEqualOrDescendantOf(sectionRoute). and we can still check if > something is a subpage via route.subpage.length. > > Do the guys in your office think we already reached consensus on removing these > properties? If so then let's do it. > > Regarding the function itself, I concede route.isEqualOrDescendantOf(ROOT_PAGE) > is equivalent to isRootedAt(ROOT_PAGE) when ROOT_PAGE is a root page, so I guess > we don't need a separate function/property for it... it's not the simplest API > but it keeps it small. > > > > > I guess I don't know why we have to get rid of the concepts we've been using > the > > whole time. We can achieve the objectives in > > > https://docs.google.com/document/d/1nI22YBBNeYnBCB07wR7QL_k6NmGmfPI_G7A8l5-YTUM/ > > without removing convenient properties. > > > > > > > > I think the isEqualOrInSubtreeOf concept allows us to cover both cases > without > > > adding more properties. > > > Hey, I thought about the variety of things you said and talked to Dan about this. Dan suggested changing this to parent.contains(child). This follows the same semantics as Node.contains, which is also inclusive. https://developer.mozilla.org/en-US/docs/Web/API/Node/contains
The CQ bit was checked by tommycli@chromium.org 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.
I like the contains change, thanks! https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2184893002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_main/settings_main.js:115: if (newRoute.inSubtreeOf(settings.Route.ABOUT)) { On 2016/07/29 18:23:43, tommycli wrote: > On 2016/07/29 00:29:14, michaelpg wrote: > > On 2016/07/29 00:21:12, michaelpg wrote: > > > On 2016/07/29 00:03:35, tommycli wrote: > > > > On 2016/07/28 23:22:32, michaelpg wrote: > > > > > so, what we care about is what is the *root* of newRoute. I think 'page' > > was > > > > > useful for providing that info, even if the name sucks. > > > > > > > > > > I move to create a read-only 'root' property of settings.Route which > > > provides > > > > > the root of the route (one of ABOUT, BASIC or ADVANCED). here, we would > > just > > > > > check (newRoute.root == settings.Route.ABOUT), instead of asking if > we're > > > > equal > > > > > to or a descendant of ABOUT. > > > > > > > > So... I would create a 'root', except that we also need to know if we are > > > within > > > > a section (to get rid of the section property). > > > > > > in a later CL, you mean? Why do we want to get rid of .section? It's a > concept > > > of its own, that represents something specific in the UI. > > > > > > Will we have to mandate that any route that's a child of a root route has to > > > correspond to a section? If so, why pretend section isn't structural? If > not, > > > how do we identify whether something is a section or a sub-page? > > > > just realized I'm conflating .section and .subpage, though presumably we're > > removing both properties. I see that checking route.section is the same as > > checking route.isEqualOrDescendantOf(sectionRoute). and we can still check if > > something is a subpage via route.subpage.length. > > > > Do the guys in your office think we already reached consensus on removing > these > > properties? If so then let's do it. > > > > Regarding the function itself, I concede > route.isEqualOrDescendantOf(ROOT_PAGE) > > is equivalent to isRootedAt(ROOT_PAGE) when ROOT_PAGE is a root page, so I > guess > > we don't need a separate function/property for it... it's not the simplest API > > but it keeps it small. > > > > > > > > I guess I don't know why we have to get rid of the concepts we've been using > > the > > > whole time. We can achieve the objectives in > > > > > > https://docs.google.com/document/d/1nI22YBBNeYnBCB07wR7QL_k6NmGmfPI_G7A8l5-YTUM/ > > > without removing convenient properties. > > > > > > > > > > > I think the isEqualOrInSubtreeOf concept allows us to cover both cases > > without > > > > adding more properties. > > > > > > > Hey, I thought about the variety of things you said and talked to Dan about > this. > > Dan suggested changing this to parent.contains(child). This follows the same > semantics as Node.contains, which is also inclusive. > > https://developer.mozilla.org/en-US/docs/Web/API/Node/contains "The contains(other) method must return true if other is an inclusive descendant of the context object" Well then... can't argue with crazy ;-) https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/route.js:84: if (this == route) optional alternative: for (; route != null; route = route.parent) { if (this == route) return true; } return false; or with no loop body *evil grin* for (; route && route != this; route = route.parent); return !!route; https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/route.js:274: * @return {settings.Route} {?s.R} https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_menu/settings_menu.html (right): https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.html:95: <div data-path="/internet" on-tap="openPage_"> dang, if only there were some element with a built-in notion of a REFerence to another Hyperlink we could Anchor here.... https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_menu/settings_menu.js (right): https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.js:61: this.$.advancedMenu.selected = this.currentRoute.path; if this works, can we just use (one-way) data binding in the HTML for these? https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.js:88: var route = settings.getRouteForPath(event.currentTarget.dataset.path); unfortunate that the menu has to do this -- settings.navigateTo(path) would be nice to have... https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_page/settings_router.js (right): https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_page/settings_router.js:52: assert(historicRoute); hmmm is it right to assert rather than return silently? What is the programmer error the assert catches? Suppose I make a change that disables a route (or, heck, update Chrome and restore my pages on restart) and use the back button. That's a valid action for the user, that should do something smart, right?
michaelpg: Thanks! https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/route.js:84: if (this == route) On 2016/07/29 20:28:53, michaelpg wrote: > optional alternative: > > for (; route != null; route = route.parent) { > if (this == route) > return true; > } > return false; > > or with no loop body *evil grin* > > for (; route && route != this; route = route.parent); > return !!route; Done. Though I had to make it a slight variant since .parent can be null but the route parameter variable can't be. https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/route.js:274: * @return {settings.Route} On 2016/07/29 20:28:53, michaelpg wrote: > {?s.R} Done. https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_menu/settings_menu.html (right): https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.html:95: <div data-path="/internet" on-tap="openPage_"> On 2016/07/29 20:28:53, michaelpg wrote: > dang, if only there were some element with a built-in notion of a REFerence to > another Hyperlink we could Anchor here.... I think I understand what you're saying. It is a little unfortunate that it looks and acts like an <a> but isn't. At one point I tried doing something like data-path="[[routes.INTERNET]]", but that does not work. I'm not a huge fan of this referencing a path instead of the object, but I couldn't think of another option that wasn't very verbose. https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_menu/settings_menu.js (right): https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.js:61: this.$.advancedMenu.selected = this.currentRoute.path; On 2016/07/29 20:28:53, michaelpg wrote: > if this works, can we just use (one-way) data binding in the HTML for these? Done. Nice https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.js:88: var route = settings.getRouteForPath(event.currentTarget.dataset.path); On 2016/07/29 20:28:53, michaelpg wrote: > unfortunate that the menu has to do this -- settings.navigateTo(path) would be > nice to have... Yeah I know. Ideally I'd prefer to do settings.navigateTo(event.currentTarget.dataset.route), but I wasn't able to bind an object to the div element. https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_page/settings_router.js (right): https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_page/settings_router.js:52: assert(historicRoute); On 2016/07/29 20:28:53, michaelpg wrote: > hmmm is it right to assert rather than return silently? What is the programmer > error the assert catches? > > Suppose I make a change that disables a route (or, heck, update Chrome and > restore my pages on restart) and use the back button. That's a valid action for > the user, that should do something smart, right? Done.
The CQ bit was checked by tommycli@chromium.org 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...
lgtm https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_menu/settings_menu.html (right): https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.html:95: <div data-path="/internet" on-tap="openPage_"> On 2016/07/29 21:04:39, tommycli wrote: > On 2016/07/29 20:28:53, michaelpg wrote: > > dang, if only there were some element with a built-in notion of a REFerence to > > another Hyperlink we could Anchor here.... > > I think I understand what you're saying. It is a little unfortunate that it > looks and acts like an <a> but isn't. > > At one point I tried doing something like data-path="[[routes.INTERNET]]", but > that does not work. Yeah, we could try [[routes.INTERNET.path]] if we declare |routes| as a property. > > I'm not a huge fan of this referencing a path instead of the object, but I > couldn't think of another option that wasn't very verbose. Is this the only place where we'll have to reference particular routes by path? If so, it's not a big deal. https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_menu/settings_menu.js (right): https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.js:61: this.$.advancedMenu.selected = this.currentRoute.path; On 2016/07/29 21:04:40, tommycli wrote: > On 2016/07/29 20:28:53, michaelpg wrote: > > if this works, can we just use (one-way) data binding in the HTML for these? > > Done. Nice Awesome, we got rid of an entire currentRouteChanged!
On 2016/07/29 21:32:56, michaelpg wrote: > lgtm > > https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... > File chrome/browser/resources/settings/settings_menu/settings_menu.html (right): > > https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... > chrome/browser/resources/settings/settings_menu/settings_menu.html:95: <div > data-path="/internet" on-tap="openPage_"> > On 2016/07/29 21:04:39, tommycli wrote: > > On 2016/07/29 20:28:53, michaelpg wrote: > > > dang, if only there were some element with a built-in notion of a REFerence > to > > > another Hyperlink we could Anchor here.... > > > > I think I understand what you're saying. It is a little unfortunate that it > > looks and acts like an <a> but isn't. > > > > At one point I tried doing something like data-path="[[routes.INTERNET]]", but > > that does not work. > > Yeah, we could try [[routes.INTERNET.path]] if we declare |routes| as a > property. > > > > I'm not a huge fan of this referencing a path instead of the object, but I > > couldn't think of another option that wasn't very verbose. > > Is this the only place where we'll have to reference particular routes by path? > If so, it's not a big deal. > > https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... > File chrome/browser/resources/settings/settings_menu/settings_menu.js (right): > > https://codereview.chromium.org/2184893002/diff/60001/chrome/browser/resource... > chrome/browser/resources/settings/settings_menu/settings_menu.js:61: > this.$.advancedMenu.selected = this.currentRoute.path; > On 2016/07/29 21:04:40, tommycli wrote: > > On 2016/07/29 20:28:53, michaelpg wrote: > > > if this works, can we just use (one-way) data binding in the HTML for these? > > > > Done. Nice > > Awesome, we got rid of an entire currentRouteChanged! michaelpg: thanks for the review!
The CQ bit was unchecked by tommycli@chromium.org
The CQ bit was checked by tommycli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Settings Router Refactor: Remove route.page legacy property. This property is not used that much, and can be removed. BUG=608115 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Settings Router Refactor: Remove route.page legacy property. This property is not used that much, and can be removed. BUG=608115, 628502 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tommycli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/2184893002/#ps100001 (title: "fix contains issue.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Settings Router Refactor: Remove route.page legacy property. This property is not used that much, and can be removed. BUG=608115, 628502 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Settings Router Refactor: Remove route.page legacy property. This property is not used that much, and can be removed. BUG=608115, 628502 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Settings Router Refactor: Remove route.page legacy property. This property is not used that much, and can be removed. BUG=608115, 628502 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Settings Router Refactor: Remove route.page legacy property. This property is not used that much, and can be removed. BUG=608115, 628502 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/aa9c34d72c27a54b09d1790fa425ea2f25cc9716 Cr-Commit-Position: refs/heads/master@{#408815} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/aa9c34d72c27a54b09d1790fa425ea2f25cc9716 Cr-Commit-Position: refs/heads/master@{#408815}
Message was sent while issue was closed.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2184893002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_menu/settings_menu.html (left): https://codereview.chromium.org/2184893002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_menu/settings_menu.html:41: div[data-section] { this used to make the advanced toggle cursor: pointer; https://codereview.chromium.org/2184893002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_menu/settings_menu.html:137: <div class="menu-trigger" data-section="" on-tap="openPage_"> but you removed data-section="" here can you fix?
Message was sent while issue was closed.
https://codereview.chromium.org/2184893002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_menu/settings_menu.html (left): https://codereview.chromium.org/2184893002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/settings_menu/settings_menu.html:137: <div class="menu-trigger" data-section="" on-tap="openPage_"> On 2016/08/01 19:37:02, Dan Beam wrote: > but you removed data-section="" here > > can you fix? Ah, I see. I will fix. |
