Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2248083004:
Settings: Fix Clear Browsing Data dialog scrolling to Privacy page (Closed)
|
Created:
4 years, 4 months ago by tommycli Modified:
4 years, 4 months ago Reviewers:
michaelpg 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. |
DescriptionSettings: Fix Clear Browsing Data dialog scrolling to Privacy page
BUG=637470
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/42c9606e1e8173a645cf08450959465abf2325ac
Cr-Commit-Position: refs/heads/master@{#413074}
Patch Set 1 #Patch Set 2 : fix #
Total comments: 4
Patch Set 3 : address comments #
Total comments: 8
Patch Set 4 : address comments #Patch Set 5 : update test #Patch Set 6 : merge #
Messages
Total messages: 39 (24 generated)
Description was changed from ========== Settings: Fix Clear Browsing Data dialog scrolling to Privacy page BUG=637470 ========== to ========== Settings: Fix Clear Browsing Data dialog scrolling to Privacy page BUG=637470 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: + michaelpg@chromium.org
michaelpg: PTAL Fixing it so that it doesn't navigate to PRIVACY (and cause a scroll) needs multiple changes. Hopefully once this is a subpage, things will be better. Navigable dialog is hard.
alternative: don't open the dialog by navigating. just don't do it ever. just open the dialog by calling open() like any other dialog. then, special-case startup: if the route is /clearBrowserData, we navigate to Advanced and open the dialog. that might have to be hacky, but it can't be worse than "navigable dialogs"... of course, that probably is more complicated than I think, and not worth the time to change now. https://codereview.chromium.org/2248083004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2248083004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:87: window.history.back(); Seems like a behavior the router should be responsible for. e.g., navigate to "the previous route", whatever it was, with a fallback of Basic. Since this is a navigable route... history.back() could go back to before Settings. https://codereview.chromium.org/2248083004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2248083004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/route.js:128: // of a section that's not a subpage. Don't add any more routes like these. Update comment?
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 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...
michaelpg: Thanks! see below https://codereview.chromium.org/2248083004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2248083004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:87: window.history.back(); On 2016/08/16 23:28:02, michaelpg wrote: > Seems like a behavior the router should be responsible for. e.g., navigate to > "the previous route", whatever it was, with a fallback of Basic. > > Since this is a navigable route... history.back() could go back to before > Settings. Done. Yes you are absolutely correct here. I realized the desired behavior is the same as settings-subpage. If there is no previous route, it goes to Advanced. https://codereview.chromium.org/2248083004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2248083004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/route.js:128: // of a section that's not a subpage. Don't add any more routes like these. On 2016/08/16 23:28:02, michaelpg wrote: > Update comment? Done.
On 2016/08/16 23:50:08, tommycli wrote: > michaelpg: Thanks! see below > > https://codereview.chromium.org/2248083004/diff/20001/chrome/browser/resource... > File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): > > https://codereview.chromium.org/2248083004/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/privacy_page/privacy_page.js:87: > window.history.back(); > On 2016/08/16 23:28:02, michaelpg wrote: > > Seems like a behavior the router should be responsible for. e.g., navigate to > > "the previous route", whatever it was, with a fallback of Basic. > > > > Since this is a navigable route... history.back() could go back to before > > Settings. > > Done. > > Yes you are absolutely correct here. I realized the desired behavior is the same > as settings-subpage. > > If there is no previous route, it goes to Advanced. > > https://codereview.chromium.org/2248083004/diff/20001/chrome/browser/resource... > File chrome/browser/resources/settings/route.js (right): > > https://codereview.chromium.org/2248083004/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/route.js:128: // of a section that's not a > subpage. Don't add any more routes like these. > On 2016/08/16 23:28:02, michaelpg wrote: > > Update comment? > > Done. michaelpg: Regarding the special-casing: yeah: the best solution is just to make it a subpage. It's not important enough to deserve this much special casing in our code.
If you think I'm bikeshedding too much on these reviews, LMK. Otherwise, I'll keep at it... https://codereview.chromium.org/2248083004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2248083004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/route.js:77: return !!this.parent && !!this.section && lol, bet that was fun to debug :( https://codereview.chromium.org/2248083004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/route.js:346: * @private no private https://codereview.chromium.org/2248083004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/route.js:353: if (previousRoute && previousRoute.contains(settings.getCurrentRoute())) why does it matter if it contains the current route? e.g. if I navigate from one subpage to another at the same level, shouldn't navigateToPreviousRoute go back to the previous subpage? https://codereview.chromium.org/2248083004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/route.js:356: navigateTo(assert(settings.getCurrentRoute().parent)); so navigateToPreviousRoute throws if called on a root page? oh, I see you took this behavior from settings_subpage.js. "navigateToPreviousRoute" sounds more general, like it wouldn't make the assertions onTapBack_ did about the route.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
michaelpg: No worries on the bikeshedding. I think this CL is no longer needed, since UI has agreed to make CBD a subpage. thanks for the review anyways.
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...
michaelpg: Nevermind. it's still up in the air if we are going to subpage this. In the meantime, here are some changes that address your comments. Thanks! https://codereview.chromium.org/2248083004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2248083004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/route.js:77: return !!this.parent && !!this.section && On 2016/08/17 00:21:31, michaelpg wrote: > lol, bet that was fun to debug :( Acknowledged. https://codereview.chromium.org/2248083004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/route.js:346: * @private On 2016/08/17 00:21:31, michaelpg wrote: > no private Done. https://codereview.chromium.org/2248083004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/route.js:353: if (previousRoute && previousRoute.contains(settings.getCurrentRoute())) On 2016/08/17 00:21:31, michaelpg wrote: > why does it matter if it contains the current route? e.g. if I navigate from one > subpage to another at the same level, shouldn't navigateToPreviousRoute go back > to the previous subpage? Done. https://codereview.chromium.org/2248083004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/route.js:356: navigateTo(assert(settings.getCurrentRoute().parent)); On 2016/08/17 00:21:31, michaelpg wrote: > so navigateToPreviousRoute throws if called on a root page? > > oh, I see you took this behavior from settings_subpage.js. > "navigateToPreviousRoute" sounds more general, like it wouldn't make the > assertions onTapBack_ did about the route. 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...
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 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.
On 2016/08/17 16:48:49, tommycli wrote: > michaelpg: Nevermind. it's still up in the air if we are going to subpage this. > In the meantime, here are some changes that address your comments. Thanks! > > https://codereview.chromium.org/2248083004/diff/40001/chrome/browser/resource... > File chrome/browser/resources/settings/route.js (right): > > https://codereview.chromium.org/2248083004/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/route.js:77: return !!this.parent && > !!this.section && > On 2016/08/17 00:21:31, michaelpg wrote: > > lol, bet that was fun to debug :( > > Acknowledged. > > https://codereview.chromium.org/2248083004/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/route.js:346: * @private > On 2016/08/17 00:21:31, michaelpg wrote: > > no private > > Done. > > https://codereview.chromium.org/2248083004/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/route.js:353: if (previousRoute && > previousRoute.contains(settings.getCurrentRoute())) > On 2016/08/17 00:21:31, michaelpg wrote: > > why does it matter if it contains the current route? e.g. if I navigate from > one > > subpage to another at the same level, shouldn't navigateToPreviousRoute go > back > > to the previous subpage? > > Done. > > https://codereview.chromium.org/2248083004/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/route.js:356: > navigateTo(assert(settings.getCurrentRoute().parent)); > On 2016/08/17 00:21:31, michaelpg wrote: > > so navigateToPreviousRoute throws if called on a root page? > > > > oh, I see you took this behavior from settings_subpage.js. > > "navigateToPreviousRoute" sounds more general, like it wouldn't make the > > assertions onTapBack_ did about the route. > > Done. michaelpg: ping thanks
lgtm
On 2016/08/18 23:08:24, michaelpg wrote: > lgtm michaelpg: thank you!
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 michaelpg@chromium.org
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 #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Settings: Fix Clear Browsing Data dialog scrolling to Privacy page BUG=637470 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Settings: Fix Clear Browsing Data dialog scrolling to Privacy page BUG=637470 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/42c9606e1e8173a645cf08450959465abf2325ac Cr-Commit-Position: refs/heads/master@{#413074} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/42c9606e1e8173a645cf08450959465abf2325ac Cr-Commit-Position: refs/heads/master@{#413074} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
