Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2206613003:
Settings Router Refactor: Support dynamic parameters (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, Dan Beam Base URL:
https://chromium.googlesource.com/chromium/src.git@223-settings-router-handle-random-urls-better Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSettings Router Refactor: Support dynamic parameters
BUG=632382
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/ad273fdd66e17f67ae992933dd9438a58554f92e
Cr-Commit-Position: refs/heads/master@{#409601}
Patch Set 1 #Patch Set 2 : fix nav logic a bit #Patch Set 3 : fix #
Total comments: 4
Patch Set 4 : Change to using URLSearchParams type #
Total comments: 2
Patch Set 5 : compare dynamic params also #
Total comments: 5
Patch Set 6 : return a copy #Patch Set 7 : Don't use Internet as test route. That's CrOS only. #
Depends on Patchset: Messages
Total messages: 38 (21 generated)
Description was changed from ========== Settings Router Refactor: Support dynamic parameters BUG=632382 ========== to ========== Settings Router Refactor: Support dynamic parameters BUG=632382 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...
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: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
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: + stevenjb@chromium.org
stevenjb (and anyone else with drivebys): How does the shape of this solution look to you? Thanks, Tommy
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2206613003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2206613003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/route.js:317: window.location.search.substring(1).split('&').forEach(function(pair) { OH NO YOU DI'NT https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams https://cs.chromium.org/chromium/src/ui/webui/resources/js/util.js?q=util.js&...
https://codereview.chromium.org/2206613003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2206613003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/route.js:310: * @private {!Object} Can/should we type this?
dbeam/stevenjb: thanks guys, do you like this version better? https://codereview.chromium.org/2206613003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2206613003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/route.js:310: * @private {!Object} On 2016/08/02 22:17:17, stevenjb wrote: > Can/should we type this? Done. I changed it to use the URLSearchParams type throughout. https://codereview.chromium.org/2206613003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/route.js:317: window.location.search.substring(1).split('&').forEach(function(pair) { On 2016/08/02 21:44:50, Dan Beam wrote: > OH NO YOU DI'NT > > https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams > https://cs.chromium.org/chromium/src/ui/webui/resources/js/util.js?q=util.js&... Thanks! I ended up using the URLSearchParams type throughout.
https://codereview.chromium.org/2206613003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2206613003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/route.js:342: if (assert(route) == currentRoute_ && !opt_dynamicParameters) why are we not comparing the dynamic parameters as well?
dbeam: thanks! https://codereview.chromium.org/2206613003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2206613003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/route.js:342: if (assert(route) == currentRoute_ && !opt_dynamicParameters) On 2016/08/03 01:30:58, Dan Beam wrote: > why are we not comparing the dynamic parameters as well? Done.
https://codereview.chromium.org/2206613003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2206613003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/route.js:344: params.toString() == currentQueryParameters_.toString()) { this breaks when append() is used in a different order
https://codereview.chromium.org/2206613003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2206613003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/route.js:333: var getQueryParameters = function() { return currentQueryParameters_; }; can we return a copy? https://codereview.chromium.org/2206613003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/route.js:344: params.toString() == currentQueryParameters_.toString()) { On 2016/08/03 17:04:28, Dan Beam wrote: > this breaks when append() is used in a different order eh, it's fine
dbeam: Thanks! https://codereview.chromium.org/2206613003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2206613003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/route.js:333: var getQueryParameters = function() { return currentQueryParameters_; }; On 2016/08/03 17:18:00, Dan Beam wrote: > can we return a copy? Done. https://codereview.chromium.org/2206613003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/route.js:344: params.toString() == currentQueryParameters_.toString()) { On 2016/08/03 17:18:00, Dan Beam wrote: > On 2016/08/03 17:04:28, Dan Beam wrote: > > this breaks when append() is used in a different order > > eh, it's fine Acknowledged.
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, i hope global navigation in tests isn't flaky
lgtm
thanks guys! sending it in. If this design needs tweaking as you guys actually use it, please let me know.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by tommycli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2206613003/#ps120001 (title: "Don't use Internet as test route. That's CrOS only.")
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 #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Settings Router Refactor: Support dynamic parameters BUG=632382 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Settings Router Refactor: Support dynamic parameters BUG=632382 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/ad273fdd66e17f67ae992933dd9438a58554f92e Cr-Commit-Position: refs/heads/master@{#409601} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ad273fdd66e17f67ae992933dd9438a58554f92e Cr-Commit-Position: refs/heads/master@{#409601} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
