Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2106103006:
MD Settings: cr/cros - Guest mode page visibility (Closed)
|
Created:
4 years, 5 months ago by Moe Modified:
4 years, 4 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-elements_chromium.org, dbeam+watch-closure_chromium.org, dbeam+watch-settings_chromium.org, jlklein+watch-closure_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, michaelpg+watch-elements_chromium.org, oshima+watch_chromium.org, stevenjb+watch-md-settings_chromium.org, vitalyp+closure_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: cr/cros - Guest mode page visibility
BUG=604517
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/369fb42299c568256ce9c8821967b9c413ea4151
Cr-Commit-Position: refs/heads/master@{#408112}
Patch Set 1 #Patch Set 2 : Itr#2 #
Total comments: 25
Patch Set 3 : Addressed comments #
Total comments: 3
Patch Set 4 : Addressed comments #
Total comments: 4
Patch Set 5 : Left out settings_router and fixed broken tests #
Total comments: 14
Patch Set 6 : Addressed comments + took out cr_element parts #
Total comments: 16
Patch Set 7 : Addressed comments + bug fix #
Total comments: 10
Patch Set 8 : Addressed comments #
Total comments: 13
Patch Set 9 : rebase #Patch Set 10 : Addressed comments #
Total comments: 5
Patch Set 11 : Addressed comments + rebase + fixed broken tests #Patch Set 12 : rebase after overscrollHeight_() fix #Messages
Total messages: 152 (97 generated)
Description was changed from ========== MD Settings: cr/cros - Guest mode page visibility BUG=604517 ========== to ========== MD Settings: cr/cros - Guest mode page visibility BUG=604517 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
mahmadi@chromium.org changed reviewers: + michaelpg@chromium.org
Hi Michael, Please take a look.
michaelpg@chromium.org changed reviewers: + dbeam@chromium.org
I think we should put some more thought into how to do this -- how about a short
design doc?
pageVisibility is intended for testing, where we don't need super-granular
visibility settings. This also adds a lot of behaviors and extra bindings we
normally won't need -- since guest mode is rare, we should optimize for the
common case and try to keep things simple.
Could we maybe do something similar to the old Options page?
<basic-page $i18n{isGuestMode}>
...
<foo-page guest-hidden>
where $i18n{isGuestMode} is set in C++ to "guest-mode" or "", and then we can
hide everything with a CSS selector like
:host([guest-mode]) [guest-hidden] {
display: none !important;
}
Also, this doesn't address routing: routes that are invalid in guest mode should
probably redirect to the root page. We should consider adding the
guest-visibility flag to settings-router.
On 2016/07/04 17:47:16, michaelpg wrote:
> I think we should put some more thought into how to do this -- how about a
short
> design doc?
>
> pageVisibility is intended for testing, where we don't need super-granular
> visibility settings. This also adds a lot of behaviors and extra bindings we
> normally won't need -- since guest mode is rare, we should optimize for the
> common case and try to keep things simple.
>
> Could we maybe do something similar to the old Options page?
>
> <basic-page $i18n{isGuestMode}>
> ...
> <foo-page guest-hidden>
>
> where $i18n{isGuestMode} is set in C++ to "guest-mode" or "", and then we can
> hide everything with a CSS selector like
> :host([guest-mode]) [guest-hidden] {
> display: none !important;
> }
why would we render the page just to hide it?
>
>
> Also, this doesn't address routing: routes that are invalid in guest mode
should
> probably redirect to the root page. We should consider adding the
> guest-visibility flag to settings-router.
The CQ bit was checked by mahmadi@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 tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by mahmadi@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 tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
Message was sent while issue was closed.
Hi Michael, please take another look at this. There are some flaky tests that i need to address. I'm also still using SettingsPageVisibility in a couple of places where dom-if template is used. otherwise the binding has to be explicitly true/false (e.g., in cr_toolbar)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: cr/cros - Guest mode page visibility BUG=604517 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: cr/cros - Guest mode page visibility BUG=604517 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Patchset #2 (id:40001) has been deleted
On 2016/07/11 22:16:15, Moe wrote: > Hi Michael, please take another look at this. There are some flaky tests that i > need to address. > > I'm also still using SettingsPageVisibility in a couple of places where dom-if > template is used. otherwise the binding has to be explicitly true/false (e.g., > in cr_toolbar) Update: SettingsPageVisibility behavior was messing with the test (like you predicted), so I removed it and now declare a pageVisiblity property in settings-main instead.
The CQ bit was checked by mahmadi@chromium.org to run a CQ dry run
The CQ bit was unchecked by mahmadi@chromium.org
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 mahmadi@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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by mahmadi@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 to run a CQ dry run
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
Thanks. A bit more work needed IMO but I'm happy with the approach. https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/appearance_page/appearance_page.html (right): https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.html:33: on-tap="openWallpaperManager_" actionable Not relevant here, but: Are there any rows in Settings that are *visible* in guest mode but not actionable? (The "actionable" attribute was added recently to change the cursor for items that do something on click, so we would want to suppress it on buttons whose actions are removed in guest mode.) https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.html:95: first$="[[!pageVisibility.appearance.bookmarksBar]]"> hmm, why? the other "first" divs aren't related to bookmarksBar. https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.html:46: <template is="dom-if" if="[[!pageVisibility.hideAdvancedSettings]]"> Please address this in currentRouteChanged_ instead of creating a new dom-if. The logic there needs some more work to integrate this. https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:127: this.showAdvancedToggle_ = !this.showAboutPage_ && !isSubpage; && !this.pageVisibility.hideAdvancedSettings https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:133: newRoute.page == 'advanced'; It's probably worth asserting !pageVisibility.hideAdvancedSettings if newRoute.page == 'advanced' here. https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:159: this.ensureInDefaultSearchPage_(); assert !hideSearch if that's a thing. https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_page/settings_router.js (right): https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_page/settings_router.js:203: url: '/siteSettings', I'd really like to avoid duplicating all of these routes. Let's find another way of including them conditionally depending on platform and guest mode. https://codereview.chromium.org/2106103006/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js (right): https://codereview.chromium.org/2106103006/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:38: hideSearch: { unused?
https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/appearance_page/appearance_page.html (right): https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.html:33: on-tap="openWallpaperManager_" actionable On 2016/07/13 00:01:35, michaelpg (slow) wrote: > Not relevant here, but: Are there any rows in Settings that are *visible* in > guest mode but not actionable? (The "actionable" attribute was added recently to > change the cursor for items that do something on click, so we would want to > suppress it on buttons whose actions are removed in guest mode.) Good point. The only case where we're removing an action in guest mode is "setting the UI language" in cros and windows. I updated the CL to remove the 'actionable' class from the list of classes too. https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/appearance_page/appearance_page.html:95: first$="[[!pageVisibility.appearance.bookmarksBar]]"> On 2016/07/13 00:01:35, michaelpg (slow) wrote: > hmm, why? the other "first" divs aren't related to bookmarksBar. correct, but I thought since homeButton, bookmarksBar, setTheme, and setWallpaper are all hidden in guest mode, a computed binding may not be necessary to check if all are set to false. https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.html:46: <template is="dom-if" if="[[!pageVisibility.hideAdvancedSettings]]"> On 2016/07/13 00:01:35, michaelpg (slow) wrote: > Please address this in currentRouteChanged_ instead of creating a new dom-if. > The logic there needs some more work to integrate this. I did initially handle this in the observer (it needs to observe currentRoute as well as pageVisibility). It adds more obscurity to the already difficult-to-follow logic there. This is a lot cleaner IMO. Unless you really want me to do it that way :) https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:127: this.showAdvancedToggle_ = !this.showAboutPage_ && !isSubpage; On 2016/07/13 00:01:35, michaelpg (slow) wrote: > && !this.pageVisibility.hideAdvancedSettings the logic should probably be more like (which IMO is too much): currentRouteOrPageVisbilityChanged_: function(newRoute, newPageVisibility) { var isSubpage = !!newRoute.subpage.length; this.showAboutPage_ = newRoute.page == 'about'; var isAboutPageOrSubpage = this.showAboutPage_ || isSubpage; this.showAdvancedToggle_ = !isAboutPageOrSubpage && this.showPage(newPageVisibility.advancedSettings); this.showBasicPage_ = !isAboutPageOrSubpage || newRoute.page == 'basic'; this.showAdvancedPage_ = ((this.isAdvancedMenuOpen_ && !isAboutPageOrSubpage) || newRoute.page == 'advanced') && this.showPage(newPageVisibility.advancedSettings); this.style.height = isSubpage ? '100%' : ''; } https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:133: newRoute.page == 'advanced'; On 2016/07/13 00:01:36, michaelpg (slow) wrote: > It's probably worth asserting > !pageVisibility.hideAdvancedSettings > if > newRoute.page == 'advanced' > here. do you think this is this still needed if we go with the dom-if? https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:159: this.ensureInDefaultSearchPage_(); On 2016/07/13 00:01:36, michaelpg (slow) wrote: > assert !hideSearch > if that's a thing. Done. https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_page/settings_router.js (right): https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_page/settings_router.js:203: url: '/siteSettings', On 2016/07/13 00:01:36, michaelpg (slow) wrote: > I'd really like to avoid duplicating all of these routes. Let's find another way > of including them conditionally depending on platform and guest mode. I grouped most things into arrays which are later concatenated depending on the platform and guest mode. No more duplication. https://codereview.chromium.org/2106103006/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js (right): https://codereview.chromium.org/2106103006/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:38: hideSearch: { On 2016/07/13 00:01:36, michaelpg (slow) wrote: > unused? No, it's used in cr_toolbar.html to hide the search field.
/cc tommycli for settings_router.js changes: https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.html:46: <template is="dom-if" if="[[!pageVisibility.hideAdvancedSettings]]"> On 2016/07/13 20:17:17, Moe wrote: > On 2016/07/13 00:01:35, michaelpg (slow) wrote: > > Please address this in currentRouteChanged_ instead of creating a new dom-if. > > The logic there needs some more work to integrate this. > > I did initially handle this in the observer (it needs to observe currentRoute as > well as pageVisibility). It adds more obscurity to the already > difficult-to-follow logic there. This is a lot cleaner IMO. Unless you really > want me to do it that way :) Meh. It's inherently illogical that (!hideAdvancedSettings) doesn't imply (showAdvancedPage_). But I'll be changing these to computed properties sooner or later anyway, so that should simplify things. https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:127: this.showAdvancedToggle_ = !this.showAboutPage_ && !isSubpage; On 2016/07/13 20:17:17, Moe wrote: > On 2016/07/13 00:01:35, michaelpg (slow) wrote: > > && !this.pageVisibility.hideAdvancedSettings > > the logic should probably be more like (which IMO is too much): > > currentRouteOrPageVisbilityChanged_: function(newRoute, newPageVisibility) { > var isSubpage = !!newRoute.subpage.length; > > this.showAboutPage_ = newRoute.page == 'about'; > > var isAboutPageOrSubpage = this.showAboutPage_ || isSubpage; > > this.showAdvancedToggle_ = > !isAboutPageOrSubpage && > this.showPage(newPageVisibility.advancedSettings); > > this.showBasicPage_ = !isAboutPageOrSubpage || newRoute.page == 'basic'; > > this.showAdvancedPage_ = > ((this.isAdvancedMenuOpen_ && !isAboutPageOrSubpage) || > newRoute.page == 'advanced') && > this.showPage(newPageVisibility.advancedSettings); > > this.style.height = isSubpage ? '100%' : ''; > } Acknowledged. https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:133: newRoute.page == 'advanced'; On 2016/07/13 20:17:17, Moe wrote: > On 2016/07/13 00:01:36, michaelpg (slow) wrote: > > It's probably worth asserting > > !pageVisibility.hideAdvancedSettings > > if > > newRoute.page == 'advanced' > > here. > > do you think this is this still needed if we go with the dom-if? Yeah. My reasoning is that asking for an Advanced route should be considered an exception if Advanced settings are force-hidden. In guest mode, if we initially land on an advanced page (e.g. chrome://md-settings/advanced), settings-router should fix that immediately. After that, the UI shouldn't be allowed to set an Advanced route. (I have no idea what state the page would get into if you tried.) https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_page/settings_router.js (right): https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_page/settings_router.js:203: url: '/siteSettings', On 2016/07/13 20:17:17, Moe wrote: > On 2016/07/13 00:01:36, michaelpg (slow) wrote: > > I'd really like to avoid duplicating all of these routes. Let's find another > way > > of including them conditionally depending on platform and guest mode. > > I grouped most things into arrays which are later concatenated depending on the > platform and guest mode. No more duplication. Cool, thank you. +tommycli@ for FYI. https://codereview.chromium.org/2106103006/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js (right): https://codereview.chromium.org/2106103006/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:38: hideSearch: { On 2016/07/13 20:17:17, Moe wrote: > On 2016/07/13 00:01:36, michaelpg (slow) wrote: > > unused? > > No, it's used in cr_toolbar.html to hide the search field. but where is it set? https://codereview.chromium.org/2106103006/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/settings_page/settings_router.js (right): https://codereview.chromium.org/2106103006/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_page/settings_router.js:109: var searchRoutes_ = [ nit: local variables aren't "private", shouldn't have trailing underscores
Description was changed from ========== MD Settings: cr/cros - Guest mode page visibility BUG=604517 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: cr/cros - Guest mode page visibility BUG=604517 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by mahmadi@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
Patchset #4 (id:100001) has been deleted
https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.html:46: <template is="dom-if" if="[[!pageVisibility.hideAdvancedSettings]]"> On 2016/07/14 00:31:28, michaelpg (slow) wrote: > On 2016/07/13 20:17:17, Moe wrote: > > On 2016/07/13 00:01:35, michaelpg (slow) wrote: > > > Please address this in currentRouteChanged_ instead of creating a new > dom-if. > > > The logic there needs some more work to integrate this. > > > > I did initially handle this in the observer (it needs to observe currentRoute > as > > well as pageVisibility). It adds more obscurity to the already > > difficult-to-follow logic there. This is a lot cleaner IMO. Unless you really > > want me to do it that way :) > > Meh. It's inherently illogical that (!hideAdvancedSettings) doesn't imply > (showAdvancedPage_). But I'll be changing these to computed properties sooner or > later anyway, so that should simplify things. +1 for computed properties. https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:133: newRoute.page == 'advanced'; On 2016/07/14 00:31:28, michaelpg (slow) wrote: > On 2016/07/13 20:17:17, Moe wrote: > > On 2016/07/13 00:01:36, michaelpg (slow) wrote: > > > It's probably worth asserting > > > !pageVisibility.hideAdvancedSettings > > > if > > > newRoute.page == 'advanced' > > > here. > > > > do you think this is this still needed if we go with the dom-if? > > Yeah. My reasoning is that asking for an Advanced route should be considered an > exception if Advanced settings are force-hidden. > > In guest mode, if we initially land on an advanced page (e.g. > chrome://md-settings/advanced), settings-router should fix that immediately. > After that, the UI shouldn't be allowed to set an Advanced route. (I have no > idea what state the page would get into if you tried.) We should be good here. chrome://md-settings/advanced is an invalid route in cr guest mode. So we shouldn't get here with newRoute.page == 'advanced'. I'll add an assert here and one to settings_menu.js where we have a similar situation. https://codereview.chromium.org/2106103006/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js (right): https://codereview.chromium.org/2106103006/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:38: hideSearch: { On 2016/07/14 00:31:28, michaelpg (slow) wrote: > On 2016/07/13 20:17:17, Moe wrote: > > On 2016/07/13 00:01:36, michaelpg (slow) wrote: > > > unused? > > > > No, it's used in cr_toolbar.html to hide the search field. > > but where is it set? in settings_ui.html https://codereview.chromium.org/2106103006/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/settings_page/settings_router.js (right): https://codereview.chromium.org/2106103006/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_page/settings_router.js:109: var searchRoutes_ = [ On 2016/07/14 00:31:28, michaelpg (slow) wrote: > nit: local variables aren't "private", shouldn't have trailing underscores Done.
https://codereview.chromium.org/2106103006/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/settings_page/settings_router.js (right): https://codereview.chromium.org/2106103006/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_page/settings_router.js:109: var searchRoutes_ = [ On 2016/07/14 00:31:28, michaelpg (slow) wrote: > nit: local variables aren't "private", shouldn't have trailing underscores fyi: i've gotten conflicting points of view on this topic since becoming a readability reviewer. i'm considering removing the presubmit for this, but until then there will be lint bother.
The CQ bit was checked by mahmadi@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
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_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
tommycli@chromium.org changed reviewers: + tommycli@chromium.org
Hi Moe, Here's my feedback. Tommy https://codereview.chromium.org/2106103006/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/settings_router.js (right): https://codereview.chromium.org/2106103006/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_router.js:98: canonicalRoutes_: [], I hate to do this to you Moe, but I'm not a big fan of concatting the canonicalRoutes_ list based on if the user is a guest user. I would have suggested: 1. Adding either a guestAccess or noGuestAccess to properties that are either permitted or forbidden (whichever is the less common case). 2. Using if statement or assert to control access to those routes. Why does the list of routes have to modified for guest users anyways? Is it to prevent guest users from directly navigating to those locations via URL? If that's the case then yeah... I'd suggest filtering out items in the last rather than conditionally concatting. https://codereview.chromium.org/2106103006/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2106103006/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.js:48: this.pageVisibility_ = { I noticed that the guest visibility info is in both the settings-router and here. This will be kind of annoying to keep in-sync, especially as new routes are added. In my ideal world, the visibility test would look something like: settings.Route.PEOPLE.guestCanAccess. That above example would depend on an in-progress router refactor, but you could add some globals in the setting-router code as a temporary measure. It would just be good to confine the guess-can-access information to one place. I'd open to suggestions on how best to accomplish that...
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
hey, we're re-doing a lot of routing stuff right now (tommycli@ generally). can we just handle the other stuff (i.e. pageVisibility) for now while the routes undergo significant change? also, this CL is a lot of things changing at once (which isn't helpful to bundle together, IMO).
Description was changed from ========== MD Settings: cr/cros - Guest mode page visibility BUG=604517 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: cr/cros - Guest mode page visibility BUG=604517 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation ==========
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
tommycli@ thanks for the feedback! dbeam@ I agree that it's best to leave the routing part out until tommy's done working on it. https://codereview.chromium.org/2106103006/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_page/settings_router.js (right): https://codereview.chromium.org/2106103006/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_page/settings_router.js:98: canonicalRoutes_: [], On 2016/07/14 22:55:39, tommycli wrote: > I hate to do this to you Moe, but I'm not a big fan of concatting the > canonicalRoutes_ list based on if the user is a guest user. > > I would have suggested: > 1. Adding either a guestAccess or noGuestAccess to properties that are either > permitted or forbidden (whichever is the less common case). > > 2. Using if statement or assert to control access to those routes. > > Why does the list of routes have to modified for guest users anyways? Is it to > prevent guest users from directly navigating to those locations via URL? If > that's the case then yeah... I'd suggest filtering out items in the last rather > than conditionally concatting. That's correct. Basically, in guest mode, some routes aren't valid routes and should be treated like any other invalid route. One way is not to have them in the list in the first place and the other is to add a guestMode state to the routes. We later agreed on conditionally adding canonical routes based on platform and guest mode here: http://shortn/_1lQBsSDclJ https://codereview.chromium.org/2106103006/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2106103006/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.js:48: this.pageVisibility_ = { On 2016/07/14 22:55:40, tommycli wrote: > I noticed that the guest visibility info is in both the settings-router and > here. This will be kind of annoying to keep in-sync, especially as new routes > are added. > > In my ideal world, the visibility test would look something like: > > settings.Route.PEOPLE.guestCanAccess. > > That above example would depend on an in-progress router refactor, but you could > add some globals in the setting-router code as a temporary measure. > > It would just be good to confine the guess-can-access information to one place. > I'd open to suggestions on how best to accomplish that... I don't expect guest-mode page visibility to change often. Only new routes have to keep in-sync with it as they are added. Also not every section/dialog may have a route, but it may have to be hidden/disabled in guest mode. And in some instances it is a certain functionality that is disabled in guest mode, not the route. But whether this can be a global in settings-router, probably.
The CQ bit was checked by mahmadi@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
No objections from me if the router stuff is left out. I'm removing myself from the list of reviewers since there's no routing stuff now.
tommycli@chromium.org changed reviewers: - tommycli@chromium.org
any further changes needed here Michael?
michaelpg@chromium.org changed reviewers: + dpapad@chromium.org
michaelpg@chromium.org changed required reviewers: + michaelpg@chromium.org
dpapad@ do you see any issues with how this hides search? https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:131: if (newRoute.page == 'advanced') {} around multi-line if bodies https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:132: assert(! /** @type {{hideAdvancedSettings: boolean}} */ this cast isn't right: the thing in parens is a boolean. https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:163: assert(! /** @type {{hideToolbarSearchField: boolean}} */ ditto
On 2016/07/20 at 19:40:46, michaelpg wrote: > dpapad@ do you see any issues with how this hides search? Do we not want to have search available in Guest mode? Regarding the actual way search textbox is hidden please add @tsergeant for reviewing cr-toolbar related changes. Also ideally we should add tests for cr-toolbar. > > https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... > File chrome/browser/resources/settings/settings_main/settings_main.js (right): > > https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... > chrome/browser/resources/settings/settings_main/settings_main.js:131: if (newRoute.page == 'advanced') > {} around multi-line if bodies > > https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... > chrome/browser/resources/settings/settings_main/settings_main.js:132: assert(! /** @type {{hideAdvancedSettings: boolean}} */ > this cast isn't right: the thing in parens is a boolean. > > https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... > chrome/browser/resources/settings/settings_main/settings_main.js:163: assert(! /** @type {{hideToolbarSearchField: boolean}} */ > ditto
On 2016/07/20 20:50:33, dpapad wrote: > On 2016/07/20 at 19:40:46, michaelpg wrote: > > dpapad@ do you see any issues with how this hides search? > Do we not want to have search available in Guest mode? Some history: in old Options, search is unavailable in Guest mode on Desktop because there are so few settings in Guest mode. I think that's still true. (Also for technical reasons that don't matter here.) https://bugs.chromium.org/p/chromium/issues/detail?id=408030#c9 On Chrome OS, there are many more settings available to Guest users, so Search still makes sense. Ultimately, it's not a *problem* if Search is enabled in Guest mode, it's just probably not ideal. > > Regarding the actual way search textbox is hidden please add @tsergeant for > reviewing cr-toolbar related > changes. Also ideally we should add tests for cr-toolbar. > > > > > > https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... > > File chrome/browser/resources/settings/settings_main/settings_main.js (right): > > > > > https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... > > chrome/browser/resources/settings/settings_main/settings_main.js:131: if > (newRoute.page == 'advanced') > > {} around multi-line if bodies > > > > > https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... > > chrome/browser/resources/settings/settings_main/settings_main.js:132: assert(! > /** @type {{hideAdvancedSettings: boolean}} */ > > this cast isn't right: the thing in parens is a boolean. > > > > > https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... > > chrome/browser/resources/settings/settings_main/settings_main.js:163: assert(! > /** @type {{hideToolbarSearchField: boolean}} */ > > ditto Moe, I know you want to get this landed. I think it would be easier to move the stuff related to search visibility to a later CL, and land the rest (for which you only need me as a reviewer).
https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/appearance_page/appearance_page.html (right): https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/appearance_page/appearance_page.html:94: first$="[[!pageVisibility.appearance.bookmarksBar]]"> This is easy enough to do in |class|, thanks to string interpolation: class$="settings-box [[getFirst_(pageVisibility.appearance.bookmarksBar)]]" then add the method getFirst_(bookmarksBarVisible) that returns 'first' or the empty string. https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_menu/settings_menu.js (right): https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_menu/settings_menu.js:28: * @type {Object} type unnecessary -- closure's Polymer pass figures out the type from the property itself (line 31) we only have it on line 18 because that's more specific than just Object https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_menu/settings_menu.js:32: value: function() { return {}; }, This shouldn't need a default value here-- set it once (like in settings-main) and let data binding do the rest. Otherwise we may get some wasteful change notifications on initialization. https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_shared_css.html:266: .settings-box[first], Let's not mix classes and attributes, this makes sense as a class. See appearance_page.html comment.
The CQ bit was checked by mahmadi@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
Sounds good. I took out the search related code. PTAL. https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/appearance_page/appearance_page.html (right): https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/appearance_page/appearance_page.html:94: first$="[[!pageVisibility.appearance.bookmarksBar]]"> On 2016/07/21 04:23:03, michaelpg wrote: > This is easy enough to do in |class|, thanks to string interpolation: > > class$="settings-box [[getFirst_(pageVisibility.appearance.bookmarksBar)]]" > > then add the method getFirst_(bookmarksBarVisible) that returns 'first' or the > empty string. Done. https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:131: if (newRoute.page == 'advanced') On 2016/07/20 19:40:46, michaelpg wrote: > {} around multi-line if bodies Done. https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:132: assert(! /** @type {{hideAdvancedSettings: boolean}} */ On 2016/07/20 19:40:46, michaelpg wrote: > this cast isn't right: the thing in parens is a boolean. Done. https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:163: assert(! /** @type {{hideToolbarSearchField: boolean}} */ On 2016/07/20 19:40:46, michaelpg wrote: > ditto took search related parts out. https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_menu/settings_menu.js (right): https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_menu/settings_menu.js:28: * @type {Object} On 2016/07/21 04:23:03, michaelpg wrote: > type unnecessary -- closure's Polymer pass figures out the type from the > property itself (line 31) > > we only have it on line 18 because that's more specific than just Object Done. Thanks for the explanation! https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_menu/settings_menu.js:32: value: function() { return {}; }, On 2016/07/21 04:23:03, michaelpg wrote: > This shouldn't need a default value here-- set it once (like in settings-main) > and let data binding do the rest. Otherwise we may get some wasteful change > notifications on initialization. Done. https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2106103006/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_shared_css.html:266: .settings-box[first], On 2016/07/21 04:23:03, michaelpg wrote: > Let's not mix classes and attributes, this makes sense as a class. See > appearance_page.html comment. Done.
https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_menu/settings_menu.js (right): https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_menu/settings_menu.js:29: pageVisibility: { This needs an @type annotation (here and elsewhere). https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.html:120: hide-search="[[pageVisibility_.hideToolbarSearchField]]" Did you move the cr-toolbar changes to a different CL, or did you chose not to hide the search textbox? Is this still relevant? https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.js:76: this.pageVisibility_ = { Where is this private member var declared? I don't see it in the "properties" section above. Also please create a typedef for this so that it is properly type checked.
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...) closure_compilation on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
Just a couple small things, that plus dpapad's suggested @typedef and hopefully this will be good to go! https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/2106103006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.html:46: <template is="dom-if" if="[[!pageVisibility.hideAdvancedSettings]]"> On 2016/07/14 17:19:55, Moe wrote: > On 2016/07/14 00:31:28, michaelpg (slow) wrote: > > On 2016/07/13 20:17:17, Moe wrote: > > > On 2016/07/13 00:01:35, michaelpg (slow) wrote: > > > > Please address this in currentRouteChanged_ instead of creating a new > > dom-if. > > > > The logic there needs some more work to integrate this. > > > > > > I did initially handle this in the observer (it needs to observe > currentRoute > > as > > > well as pageVisibility). It adds more obscurity to the already > > > difficult-to-follow logic there. This is a lot cleaner IMO. Unless you > really > > > want me to do it that way :) > > > > Meh. It's inherently illogical that (!hideAdvancedSettings) doesn't imply > > (showAdvancedPage_). But I'll be changing these to computed properties sooner > or > > later anyway, so that should simplify things. > > +1 for computed properties. FYI, unfortunately computed properties ended up being painful too, because we now want to adjust these manually when searching the page. I have a CL that simplifies currentRouteChanged_ a bit, maybe I'll go back and do this later, the dom-if's are okay for now though. https://codereview.chromium.org/2160713002/ https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/advanced_page/advanced_page.html (right): https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/advanced_page/advanced_page.html:45: page-visibility="[[pageVisibility]]"> nit: 4-space indent (the above line was wrong) https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/appearance_page/appearance_page.html (right): https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/appearance_page/appearance_page.html:93: !pageVisibility.appearance.bookmarksBar)]]"> This fails silently, because ! can't appear before a function parameter in a binding annotation. The function has to do the negating :-( https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/privacy_page/privacy_page.html:47: hidden="[[!pageVisibility.privacy.networkPrediction]]"> just checking this isn't a typo: this should be tied to network prediction as well? https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:167: assert(! /** @type {{hideToolbarSearchField: boolean}} */ remove for now? https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_shared_css.html:273: .settings-box[first], remove
The CQ bit was checked by mahmadi@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
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...)
Patchset #7 (id:180001) has been deleted
The CQ bit was checked by mahmadi@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
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...)
Michael, Demetrios, hopefully we're getting there :) do you know why closure compiler doesn't like the way I'm defining the type? https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/advanced_page/advanced_page.html (right): https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/advanced_page/advanced_page.html:45: page-visibility="[[pageVisibility]]"> On 2016/07/21 21:58:44, michaelpg wrote: > nit: 4-space indent (the above line was wrong) Done. https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/appearance_page/appearance_page.html (right): https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/appearance_page/appearance_page.html:93: !pageVisibility.appearance.bookmarksBar)]]"> On 2016/07/21 21:58:44, michaelpg wrote: > This fails silently, because ! can't appear before a function parameter in a > binding annotation. The function has to do the negating :-( Done. https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/privacy_page/privacy_page.html:47: hidden="[[!pageVisibility.privacy.networkPrediction]]"> On 2016/07/21 21:58:44, michaelpg wrote: > just checking this isn't a typo: this should be tied to network prediction as > well? this was meant to disable both prediction options. but thinking about it again, it's probably better to change this to searchPrediction. https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:167: assert(! /** @type {{hideToolbarSearchField: boolean}} */ On 2016/07/21 21:58:44, michaelpg wrote: > remove for now? done. https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_menu/settings_menu.js (right): https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_menu/settings_menu.js:29: pageVisibility: { On 2016/07/21 18:33:33, dpapad wrote: > This needs an @type annotation (here and elsewhere). according to michaelpg@ type is unnecessary IF we decide not to typedef pageVisibility. Closure's Polymer pass figures out the type is Object. https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_shared_css.html:273: .settings-box[first], On 2016/07/21 21:58:45, michaelpg wrote: > remove done. https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.html:120: hide-search="[[pageVisibility_.hideToolbarSearchField]]" On 2016/07/21 18:33:33, dpapad wrote: > Did you move the cr-toolbar changes to a different CL, or did you chose not to > hide the search textbox? Is this still relevant? done. https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2106103006/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.js:76: this.pageVisibility_ = { On 2016/07/21 18:33:33, dpapad wrote: > Where is this private member var declared? I don't see it in the "properties" > section above. Also please create a typedef for this so that it is properly type > checked. Done. https://codereview.chromium.org/2106103006/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2106103006/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:73: value: function() { return {}; }, michaelpg@, I had to bring this back b/c dom-if doesn't like being bound to undefined properties. Its effect should be very local since advanced_page and basic_page both already have SettingsPageVisibility behavior in which pageVisibility has a default {} value.
On 2016/07/22 at 16:59:56, mahmadi wrote:
> Michael, Demetrios, hopefully we're getting there :)
>
> do you know why closure compiler doesn't like the way I'm defining the type?
This error is very odd. I tried your CL locally and was able to bypass the error
by adding a dummy function
after the typedef declaration, for example "function foo() {}". After that error
is fixed there are other (legit)
compilation errors happening.
FYI, I am expecting the "page visibility" mechanism introduced in this CL to have to be modified very soon, for the purposes of searching. Specifically, when marking an element as hidden using the "hidden" attribute, it is not a clear indicator to the searching algorithm whether this element is hidden but should be searched, or hidden and not applicable to current user so it should not be searched. https://codereview.chromium.org/2106103006/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/advanced_page/advanced_page.html (right): https://codereview.chromium.org/2106103006/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/advanced_page/advanced_page.html:45: page-visibility="[[pageVisibility.privacy]]"> Here you are passing |pageVisibility.privacy| to the privacy page, which is supposed to be of type {networkPrediction: boolean, searchPrediction: boolean}, but in privacy_page.html you are accessing |pageVisibility.privacy.searchPrediction| which seems incorrect. Shouldn't it simply be |pageVisibility.searchPrediction| ? https://codereview.chromium.org/2106103006/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui_types.js (right): https://codereview.chromium.org/2106103006/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui_types.js:12: * advancedSettings: (boolean|undefined), Why are all top-level boolean on this object, undefined or a boolean? Can't we always give them an appropriate default value, such that nothing is ever undefined? https://codereview.chromium.org/2106103006/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui_types.js:30: * privacy: (undefined|{ If you decide to pass subsets of the GuestModePageVisibility object to specific pages (instead of always passing the entire object), then it is better to extract some smaller types here, for example /** @typedef {{networkPrediction: boolean, searchPrediction: boolean}} */ var PrivacyPageVisibility; Otherwise how are you going to type annotate |pageVisibility| property of the privacy page?
The CQ bit was checked by mahmadi@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
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-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
Thanks for the headsup. I think this mechanism of hiding things can be slightly tweaked to accommodate search too. in desktop, only one section is visible in guest mode. so it makes sense to just disable search in guest mode in desktop. in cros, almost all the sections are visible in guest mode. just some controls are hidden. We can choose not to render the hidden controls using dom-ifs. That should solve the problem with search. https://codereview.chromium.org/2106103006/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/advanced_page/advanced_page.html (right): https://codereview.chromium.org/2106103006/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/advanced_page/advanced_page.html:45: page-visibility="[[pageVisibility.privacy]]"> On 2016/07/22 18:33:04, dpapad wrote: > Here you are passing |pageVisibility.privacy| to the privacy page, which is > supposed to be of type > {networkPrediction: boolean, searchPrediction: boolean}, but in > privacy_page.html you are accessing |pageVisibility.privacy.searchPrediction| > which seems incorrect. Shouldn't it simply be |pageVisibility.searchPrediction| > ? |pageVisibility.privacy.searchPrediction| is incorrect. thanks for catching it. https://codereview.chromium.org/2106103006/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui_types.js (right): https://codereview.chromium.org/2106103006/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui_types.js:12: * advancedSettings: (boolean|undefined), On 2016/07/22 18:33:04, dpapad wrote: > Why are all top-level boolean on this object, undefined or a boolean? Can't we > always give them an appropriate default value, such that nothing is ever > undefined? I've seen this elsewhere too. Look at sync_browser_proxy.js for instance. I think it's a lot less maintenance to have 'undefined' in the type rather than maintaining default values for pageVisbility everywhere it is initialized. wdyt? https://codereview.chromium.org/2106103006/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui_types.js:30: * privacy: (undefined|{ On 2016/07/22 18:33:04, dpapad wrote: > If you decide to pass subsets of the GuestModePageVisibility object to specific > pages (instead of always passing the entire object), then it is better to > extract some smaller types here, for example > > /** @typedef {{networkPrediction: boolean, searchPrediction: boolean}} */ > var PrivacyPageVisibility; > > Otherwise how are you going to type annotate |pageVisibility| property of the > privacy page? That's a good idea! but I'm not sure if it's needed now. Currently individual sections only use pageVisibility in the html to hide/disable controls. Can we postpone adding the subtypes when they're needed?
https://codereview.chromium.org/2106103006/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui_types.js (right): https://codereview.chromium.org/2106103006/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui_types.js:12: * advancedSettings: (boolean|undefined), On 2016/07/22 at 20:53:50, Moe wrote: > On 2016/07/22 18:33:04, dpapad wrote: > > Why are all top-level boolean on this object, undefined or a boolean? Can't we > > always give them an appropriate default value, such that nothing is ever > > undefined? > > I've seen this elsewhere too. Look at sync_browser_proxy.js for instance. I think it's a lot less maintenance to have 'undefined' in the type rather than maintaining default values for pageVisbility everywhere it is initialized. wdyt? My thinking was that there is only one place where a GuestModePageVisibility would be initialized and that is in settings_ui.js. Everywhere else it would be undefined until the data binding from parent to child takes effect. I also find it a bit counter-intuitive that a visibility value of undefined means "show this element". https://codereview.chromium.org/2106103006/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui_types.js:30: * privacy: (undefined|{ On 2016/07/22 at 20:53:49, Moe wrote: > On 2016/07/22 18:33:04, dpapad wrote: > > If you decide to pass subsets of the GuestModePageVisibility object to specific > > pages (instead of always passing the entire object), then it is better to > > extract some smaller types here, for example > > > > /** @typedef {{networkPrediction: boolean, searchPrediction: boolean}} */ > > var PrivacyPageVisibility; > > > > Otherwise how are you going to type annotate |pageVisibility| property of the > > privacy page? > > That's a good idea! but I'm not sure if it's needed now. Currently individual sections only use pageVisibility in the html to hide/disable controls. Can we postpone adding the subtypes when they're needed? I am seeing several |pageVisibility| variables in different HTML files, and they all have different types (for example date_time_page.html, privacy_page.html). As a future reader, I will have to trace all the way back to what subset of the initial GuestModePageVisibility object was assigned to a page's |pageVisibility| variable to understand its type. I am suggesting either of the following: 1) Pass the entire GuestModePageVisibility object everywhere and declare |pageVisibility| with an @type {!GuestModePageVisibility} annotation, even if only a specific path is used. Or 2) Declare smaller types in this file, keep passing subsets of the GuestModePageVisibility to sub-pages, and annotate them with the appropriate smaller type. https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/date_time_page/date_time_page.html (right): https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/date_time_page/date_time_page.html:10: <div disabled$="[[!pageVisibility.timeZoneSelector]]"> +michaelpg Are we OK with having HTML refer to Polymer properties that are not defined in the |properties| section of the equivalent JS file (referring to |pageVisibility| variable)? https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages_page.js (right): https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages_page.js:71: if ((!cr.isChromeOS && !cr.isWindows) || loadTimeData.getBoolean('isGuest')) Why is this button enabled/visible in cases were it results in a no-op? https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.html:47: if="[[showAdvancedSettings(pageVisibility.advancedSettings)]]"> IIUC, showAdvancedSettings() is only used to convert a (boolean|undefined) to a boolean. If pageVisibility.advancedSettings was always defined a proper default value, there would be no need for an extra method, you could use the visibility value directly. if="[[pageVisibility.advancedSettings]]"> https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:188: */ Nit: Mark as @private and rename to showAdvancedSettings_. https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_menu/settings_menu.html (right): https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/settings_menu/settings_menu.html:100: hidden="[[!pageVisibility.people]]"> IIUC <settings-ui>'s pageVisibility_ is undefined when we are not in Guest mode. What happens to all similar bindings in those cases? https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.js:87: if (loadTimeData.getBoolean('isGuest')) { Can you not move this logic at line 69 as follows? pageVisibility_: { type: Object, value: function() { if (loadTimeData.getBoolean('isGuest')) { return ....; } return ....; }, },
Patchset #9 (id:240001) has been deleted
The CQ bit was checked by mahmadi@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
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-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #9 (id:260001) has been deleted
The CQ bit was checked by mahmadi@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
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 mahmadi@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
https://codereview.chromium.org/2106103006/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui_types.js (right): https://codereview.chromium.org/2106103006/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui_types.js:30: * privacy: (undefined|{ On 2016/07/22 22:40:48, dpapad wrote: > On 2016/07/22 at 20:53:49, Moe wrote: > > On 2016/07/22 18:33:04, dpapad wrote: > > > If you decide to pass subsets of the GuestModePageVisibility object to > specific > > > pages (instead of always passing the entire object), then it is better to > > > extract some smaller types here, for example > > > > > > /** @typedef {{networkPrediction: boolean, searchPrediction: boolean}} */ > > > var PrivacyPageVisibility; > > > > > > Otherwise how are you going to type annotate |pageVisibility| property of > the > > > privacy page? > > > > That's a good idea! but I'm not sure if it's needed now. Currently individual > sections only use pageVisibility in the html to hide/disable controls. Can we > postpone adding the subtypes when they're needed? > > I am seeing several |pageVisibility| variables in different HTML files, and they > all have different types (for example date_time_page.html, privacy_page.html). > As a future reader, I will have to trace all the way back to what subset of the > initial GuestModePageVisibility object was assigned to a page's |pageVisibility| > variable to understand its type. I am suggesting either of the following: > > 1) Pass the entire GuestModePageVisibility object everywhere and declare > |pageVisibility| with an @type {!GuestModePageVisibility} annotation, even if > only a specific path is used. Or > 2) Declare smaller types in this file, keep passing subsets of the > GuestModePageVisibility to sub-pages, and annotate them with the appropriate > smaller type. I'd go with option 2. The first option won't work because basic-page and advanced-page have the pageVisibility property from the SettingsPageVisibility behavior which is initialized with an empty object. If we bind pageVisibility to pageVisibility of their children, [[pageVisibility.foo.bar]] will be evaluated to false and not won't be ignored (which is the behavior we want) https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/date_time_page/date_time_page.html (right): https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/date_time_page/date_time_page.html:10: <div disabled$="[[!pageVisibility.timeZoneSelector]]"> On 2016/07/22 22:40:48, dpapad wrote: > +michaelpg > > Are we OK with having HTML refer to Polymer properties that are not defined in > the |properties| section of the equivalent JS file (referring to > |pageVisibility| variable)? I think after introducing the sub-types it won't hurt to declare the properties in the JS. https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages_page.js (right): https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages_page.js:71: if ((!cr.isChromeOS && !cr.isWindows) || loadTimeData.getBoolean('isGuest')) On 2016/07/22 22:40:48, dpapad wrote: > Why is this button enabled/visible in cases were it results in a no-op? The language row always remains visible. line 285 prevents it from looking actionable in guest mode and the tap event is ignored here. https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.html:47: if="[[showAdvancedSettings(pageVisibility.advancedSettings)]]"> On 2016/07/22 22:40:48, dpapad wrote: > IIUC, showAdvancedSettings() is only used to convert a (boolean|undefined) to a > boolean. If pageVisibility.advancedSettings was always defined a proper default > value, there would be no need for an extra method, you could use the visibility > value directly. > > if="[[pageVisibility.advancedSettings]]"> michaelpg@ suggested that we leave pageVisibility undefined in non-guest-mode so that bindings to hidden/disabled attrs simply get ignored. that doesn't quite work for dom-if. hence this helper method. https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:188: */ On 2016/07/22 22:40:48, dpapad wrote: > Nit: Mark as @private and rename to showAdvancedSettings_. Done. https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_menu/settings_menu.html (right): https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/settings_menu/settings_menu.html:100: hidden="[[!pageVisibility.people]]"> On 2016/07/22 22:40:48, dpapad wrote: > IIUC <settings-ui>'s pageVisibility_ is undefined when we are not in Guest mode. > What happens to all similar bindings in those cases? They are ignored which is what we want. https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.js:87: if (loadTimeData.getBoolean('isGuest')) { On 2016/07/22 22:40:48, dpapad wrote: > Can you not move this logic at line 69 as follows? > > pageVisibility_: { > type: Object, > value: function() { > if (loadTimeData.getBoolean('isGuest')) { > return ....; > } > return ....; > }, > }, Unfortunately not. @suppress won't work for declared properties. we would also have to have a return value so we can't leave pageVisibility undefined.
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...)
LGTM with nits. Had a chat today with other settings devs about page visibility, since it affects both routing and searching (see "MD Settings Eng" group hangout for summary). Since multiple pieces are still in flux, it is OK to land this CL as-is and modify the code iteratively until the needs of both search and routing can be served. https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_menu/settings_menu.html (right): https://codereview.chromium.org/2106103006/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/settings_menu/settings_menu.html:100: hidden="[[!pageVisibility.people]]"> On 2016/07/24 at 23:54:27, Moe wrote: > On 2016/07/22 22:40:48, dpapad wrote: > > IIUC <settings-ui>'s pageVisibility_ is undefined when we are not in Guest mode. > > What happens to all similar bindings in those cases? > > They are ignored which is what we want. Can we at least add a comment somewhere (probably in settings_ui.js), stating that fact explicitly? https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/appearance_page/appearance_page.js:115: pageVisibility: { Nit: Isn't this equivalent to the following (here and elsewhere)? pageVisibility: Object
lgtm https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/appearance_page/appearance_page.js:115: pageVisibility: { On 2016/07/25 19:37:10, dpapad wrote: > Nit: Isn't this equivalent to the following (here and elsewhere)? > > pageVisibility: Object yes https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui_types.js (right): https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui_types.js:21: * reset:(boolean|undefined), I thought closure objects couldn't end in commas like this -- if it compiles I guess I'm wrong!
The CQ bit was checked by mahmadi@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
Patchset #11 (id:320001) has been deleted
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-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
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-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by mahmadi@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
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_...)
Patchset #11 (id:340001) has been deleted
The CQ bit was checked by mahmadi@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
I noticed some tests were failing with the last patch. So I made a couple of
changes to settings_main.js
this.$$('settings-advanced-page') was null in overscrollHeight_. So i added a
call to Polymer.dom.flush() to make sure DOM is updated after dom-if changes.
not quite sure why it wasn't needed before my patch.
Also added a check to see this.$$('#toggleContainer') isn't null. which happens
in guest mode.
Please take a look to see if these changes are fine.
https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):
https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc...
chrome/browser/resources/settings/appearance_page/appearance_page.js:115:
pageVisibility: {
On 2016/07/26 04:02:55, michaelpg wrote:
> On 2016/07/25 19:37:10, dpapad wrote:
> > Nit: Isn't this equivalent to the following (here and elsewhere)?
> >
> > pageVisibility: Object
>
> yes
Done.
https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_ui/settings_ui_types.js (right):
https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_ui/settings_ui_types.js:21: *
reset:(boolean|undefined),
On 2016/07/26 04:02:55, michaelpg wrote:
> I thought closure objects couldn't end in commas like this -- if it compiles I
> guess I'm wrong!
It seems to compile just fine.
On 2016/07/27 at 01:11:05, mahmadi wrote:
> I noticed some tests were failing with the last patch. So I made a couple of
changes to settings_main.js
>
> this.$$('settings-advanced-page') was null in overscrollHeight_. So i added a
call to Polymer.dom.flush() to make sure DOM is updated after dom-if changes.
not quite sure why it wasn't needed before my patch.
> Also added a check to see this.$$('#toggleContainer') isn't null. which
happens in guest mode.
Are you referring to this line
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...
If so, I faced the same problem today, so did Tommy. The resolution for now is
to change it to use setTimeout() instead of this.async() which fixes the issue,
and @dschuyler might investigate using a dom-change event in the future.
>
> Please take a look to see if these changes are fine.
>
>
https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc...
> File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):
>
>
https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc...
> chrome/browser/resources/settings/appearance_page/appearance_page.js:115:
pageVisibility: {
> On 2016/07/26 04:02:55, michaelpg wrote:
> > On 2016/07/25 19:37:10, dpapad wrote:
> > > Nit: Isn't this equivalent to the following (here and elsewhere)?
> > >
> > > pageVisibility: Object
> >
> > yes
>
> Done.
>
>
https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc...
> File chrome/browser/resources/settings/settings_ui/settings_ui_types.js
(right):
>
>
https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc...
> chrome/browser/resources/settings/settings_ui/settings_ui_types.js:21: *
reset:(boolean|undefined),
> On 2016/07/26 04:02:55, michaelpg wrote:
> > I thought closure objects couldn't end in commas like this -- if it compiles
I
> > guess I'm wrong!
>
> It seems to compile just fine.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/27 01:16:00, dpapad wrote:
> On 2016/07/27 at 01:11:05, mahmadi wrote:
> > I noticed some tests were failing with the last patch. So I made a couple of
> changes to settings_main.js
> >
> > this.$$('settings-advanced-page') was null in overscrollHeight_. So i added
a
> call to Polymer.dom.flush() to make sure DOM is updated after dom-if changes.
> not quite sure why it wasn't needed before my patch.
> > Also added a check to see this.$$('#toggleContainer') isn't null. which
> happens in guest mode.
>
> Are you referring to this line
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...
> If so, I faced the same problem today, so did Tommy. The resolution for now is
> to change it to use setTimeout() instead of this.async() which fixes the
issue,
> and @dschuyler might investigate using a dom-change event in the future.
>
>
> >
> > Please take a look to see if these changes are fine.
> >
> >
>
https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc...
> > File chrome/browser/resources/settings/appearance_page/appearance_page.js
> (right):
> >
> >
>
https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc...
> > chrome/browser/resources/settings/appearance_page/appearance_page.js:115:
> pageVisibility: {
> > On 2016/07/26 04:02:55, michaelpg wrote:
> > > On 2016/07/25 19:37:10, dpapad wrote:
> > > > Nit: Isn't this equivalent to the following (here and elsewhere)?
> > > >
> > > > pageVisibility: Object
> > >
> > > yes
> >
> > Done.
> >
> >
>
https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc...
> > File chrome/browser/resources/settings/settings_ui/settings_ui_types.js
> (right):
> >
> >
>
https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc...
> > chrome/browser/resources/settings/settings_ui/settings_ui_types.js:21: *
> reset:(boolean|undefined),
> > On 2016/07/26 04:02:55, michaelpg wrote:
> > > I thought closure objects couldn't end in commas like this -- if it
compiles
> I
> > > guess I'm wrong!
> >
> > It seems to compile just fine.
I changed it to setTimeout in PS23 here:
https://codereview.chromium.org/2156413002/#ps440001, but it just failed in a
different way.
Perhaps either Moe's way (which I like better) or the this.async(foo, 1) hack is
what's needed.
In either case: I support adding a hack so we can land these mega CLs. It's 10
steps forward one step back.
On 2016/07/27 03:09:31, tommycli wrote:
> On 2016/07/27 01:16:00, dpapad wrote:
> > On 2016/07/27 at 01:11:05, mahmadi wrote:
> > > I noticed some tests were failing with the last patch. So I made a couple
of
> > changes to settings_main.js
> > >
> > > this.$$('settings-advanced-page') was null in overscrollHeight_. So i
added
> a
> > call to Polymer.dom.flush() to make sure DOM is updated after dom-if
changes.
> > not quite sure why it wasn't needed before my patch.
> > > Also added a check to see this.$$('#toggleContainer') isn't null. which
> > happens in guest mode.
> >
> > Are you referring to this line
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...
> > If so, I faced the same problem today, so did Tommy. The resolution for now
is
> > to change it to use setTimeout() instead of this.async() which fixes the
> issue,
> > and @dschuyler might investigate using a dom-change event in the future.
> >
> >
> > >
> > > Please take a look to see if these changes are fine.
> > >
> > >
> >
>
https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc...
> > > File chrome/browser/resources/settings/appearance_page/appearance_page.js
> > (right):
> > >
> > >
> >
>
https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc...
> > > chrome/browser/resources/settings/appearance_page/appearance_page.js:115:
> > pageVisibility: {
> > > On 2016/07/26 04:02:55, michaelpg wrote:
> > > > On 2016/07/25 19:37:10, dpapad wrote:
> > > > > Nit: Isn't this equivalent to the following (here and elsewhere)?
> > > > >
> > > > > pageVisibility: Object
> > > >
> > > > yes
> > >
> > > Done.
> > >
> > >
> >
>
https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc...
> > > File chrome/browser/resources/settings/settings_ui/settings_ui_types.js
> > (right):
> > >
> > >
> >
>
https://codereview.chromium.org/2106103006/diff/300001/chrome/browser/resourc...
> > > chrome/browser/resources/settings/settings_ui/settings_ui_types.js:21: *
> > reset:(boolean|undefined),
> > > On 2016/07/26 04:02:55, michaelpg wrote:
> > > > I thought closure objects couldn't end in commas like this -- if it
> compiles
> > I
> > > > guess I'm wrong!
> > >
> > > It seems to compile just fine.
>
> I changed it to setTimeout in PS23 here:
> https://codereview.chromium.org/2156413002/#ps440001, but it just failed in a
> different way.
>
> Perhaps either Moe's way (which I like better) or the this.async(foo, 1) hack
is
> what's needed.
>
> In either case: I support adding a hack so we can land these mega CLs. It's 10
> steps forward one step back.
setTimeout is less hacky than Polymer.dom.flush() which I would really like to
avoid. Tommy, I commented on your patch -- I saw an obvious error which is
hopefully the new failure you're referring to. If so (if adding .bind(this)
fixes the setTimeout) let's go with that.
The CQ bit was checked by mahmadi@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
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 mahmadi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelpg@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2106103006/#ps380001 (title: "rebase after overscrollHeight_() fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
Thank you!
Message was sent while issue was closed.
Description was changed from ========== MD Settings: cr/cros - Guest mode page visibility BUG=604517 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: cr/cros - Guest mode page visibility BUG=604517 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #12 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: cr/cros - Guest mode page visibility BUG=604517 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: cr/cros - Guest mode page visibility BUG=604517 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/369fb42299c568256ce9c8821967b9c413ea4151 Cr-Commit-Position: refs/heads/master@{#408112} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/369fb42299c568256ce9c8821967b9c413ea4151 Cr-Commit-Position: refs/heads/master@{#408112} |
