Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2193333002:
Setting Router Refactor: Implement RouteObserverBehavior. (Closed)
|
Created:
4 years, 4 months ago by tommycli Modified:
4 years, 4 months ago Reviewers:
Dan Beam 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@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSetting Router Refactor: Implement RouteObserverBehavior.
This also removes most of the contents of settings-router. It still
exists for now to propagate the property update, but will be removed
once everything uses RouteObserverBehavior.
BUG=608115
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/431600500292c1394cee9c76746395bf525bd1ec
Cr-Commit-Position: refs/heads/master@{#409047}
Patch Set 1 #
Total comments: 19
Patch Set 2 : address comments #Patch Set 3 : merge origin/master #Patch Set 4 : small fix #Patch Set 5 : Fix naming issue in route.js #
Messages
Total messages: 30 (22 generated)
Description was changed from ========== Setting Router Refactor: Implement RouteObserverBehavior. This also removes most of the contents of settings-router. It still exists for now to propagate the property update, but will be removed once everything uses RouteObserverBehavior. BUG=608115 ========== to ========== Setting Router Refactor: Implement RouteObserverBehavior. This also removes most of the contents of settings-router. It still exists for now to propagate the property update, but will be removed once everything uses RouteObserverBehavior. BUG=608115 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
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...
tommycli@chromium.org changed reviewers: + dbeam@chromium.org
dbeam: PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:279: routeObservers_.delete(this); assert(routeObservers_.delete(this)); https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:281: nit: /** @abstract */ https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:282: currentRouteChanged_: function() { this should not be named with _, as that is for @private (this should arguably be @protected or just public) https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:284: 'override currentRouteChanged_.'); i don't know that this message adds much, but I guess you feel it does otherwise the code could just be: currentRouteChanged: assertNotReached, https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:294: var getRouteFor = function(path) { in my utopia: return Object.values(Routes).find(r => r.path == path) || Routes.Basic; https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:303: return Route[matchingKey]; return Route[matchingKey] || Route.BASIC; https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:322: assert(!!route); nit: if (assert(route) == currentRoute_) https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:322: assert(!!route); assert(!!thing) is the same as assert(thing) the only reason you need to do !! is when you're using assertTrue(!!truthy) in unit tests as it checks for === true or something https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:329: observer.currentRouteChanged_(); nit: var setCurrentRoute = function(route) { currentRoute_ = route; for (var observer of routeObservers_) observer.currentRouteChanged_(); }; and call from navigateTo and 'popstate'
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
dbeam: Thanks! https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:279: routeObservers_.delete(this); On 2016/07/30 01:13:02, Dan Beam wrote: > assert(routeObservers_.delete(this)); Done. https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:281: On 2016/07/30 01:13:02, Dan Beam wrote: > nit: /** @abstract */ Done. Ideally I could use @override in settings-router.js (and other classes that mixin this behavior) but it doesn't seem to work. If I try, I get: (ERROR) Error in: settings_router.js ## /mnt/src/chromium/src/chrome/browser/resources/settings/settings_page/settings_router.js:34: ERROR - property currentRouteChanged not defined on any superclass of SettingsRouterElement ## currentRouteChanged: function() { ## ^ https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:282: currentRouteChanged_: function() { On 2016/07/30 01:13:02, Dan Beam wrote: > this should not be named with _, as that is for @private (this should arguably > be @protected or just public) Done. https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:284: 'override currentRouteChanged_.'); On 2016/07/30 01:13:02, Dan Beam wrote: > i don't know that this message adds much, but I guess you feel it does > > otherwise the code could just be: > > currentRouteChanged: assertNotReached, Done. https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:294: var getRouteFor = function(path) { On 2016/07/30 01:13:02, Dan Beam wrote: > in my utopia: > > return Object.values(Routes).find(r => r.path == path) || Routes.Basic; Acknowledged. https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:303: return Route[matchingKey]; On 2016/07/30 01:13:02, Dan Beam wrote: > return Route[matchingKey] || Route.BASIC; Done. https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:322: assert(!!route); On 2016/07/30 01:13:02, Dan Beam wrote: > assert(!!thing) is the same as assert(thing) > > the only reason you need to do !! is when you're using assertTrue(!!truthy) in > unit tests as it checks for === true or something Done. https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:322: assert(!!route); On 2016/07/30 01:13:02, Dan Beam wrote: > nit: if (assert(route) == currentRoute_) Done. https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:329: observer.currentRouteChanged_(); On 2016/07/30 01:13:02, Dan Beam wrote: > nit: > > var setCurrentRoute = function(route) { > currentRoute_ = route; > for (var observer of routeObservers_) > observer.currentRouteChanged_(); > }; > > and call from navigateTo and 'popstate' Done.
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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 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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
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...
dbeam: Thanks! https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2193333002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/route.js:303: return Route[matchingKey]; On 2016/08/01 17:33:24, tommycli wrote: > On 2016/07/30 01:13:02, Dan Beam wrote: > > return Route[matchingKey] || Route.BASIC; > > Done. FYI, in a different patch we changed the behavior to return null for a missing route, and that got merged in.
The CQ bit was unchecked by tommycli@chromium.org
The CQ bit was checked by tommycli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2193333002/#ps80001 (title: "Fix naming issue in route.js")
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.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Setting Router Refactor: Implement RouteObserverBehavior. This also removes most of the contents of settings-router. It still exists for now to propagate the property update, but will be removed once everything uses RouteObserverBehavior. BUG=608115 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Setting Router Refactor: Implement RouteObserverBehavior. This also removes most of the contents of settings-router. It still exists for now to propagate the property update, but will be removed once everything uses RouteObserverBehavior. BUG=608115 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/431600500292c1394cee9c76746395bf525bd1ec Cr-Commit-Position: refs/heads/master@{#409047} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/431600500292c1394cee9c76746395bf525bd1ec Cr-Commit-Position: refs/heads/master@{#409047} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
