Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2684853003:
MD Settings: change how tap is handled on custom toggle rows (Closed)
|
Created:
3 years, 10 months ago by Dan Beam Modified:
3 years, 10 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: change how tap is handled on custom toggle rows
R=scottchen@chromium.org
BUG=680406, 689763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2684853003
Cr-Commit-Position: refs/heads/master@{#450293}
Committed: https://chromium.googlesource.com/chromium/src/+/3245561158dde6808bd24b3322304369af52abac
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #
Total comments: 13
Patch Set 4 : fake prefs #Patch Set 5 : if the compiler ain't happy, ain't nobody happy #
Total comments: 16
Patch Set 6 : asdf #Patch Set 7 : asdf #Patch Set 8 : rework events #Patch Set 9 : fix tests #
Total comments: 2
Patch Set 10 : todo #Messages
Total messages: 59 (28 generated)
Description was changed from ========== MD Settings: change how tap is handled on custom toggle rows R=scottchen@chromium.org BUG=680406 ========== to ========== MD Settings: change how tap is handled on custom toggle rows R=scottchen@chromium.org BUG=680406 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2017/02/08 22:12:08, Dan Beam wrote: > Description was changed from > > ========== > MD Settings: change how tap is handled on custom toggle rows > > mailto:R=scottchen@chromium.org > BUG=680406 > ========== > > to > > ========== > MD Settings: change how tap is handled on custom toggle rows > > mailto:R=scottchen@chromium.org > BUG=680406 > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation > ========== LGTM
dbeam@chromium.org changed reviewers: + dpapad@chromium.org
+dpapad@ for thoughts maybe we should attempt to some fake pref stuff instead of composing? both routes are less that optimal, IMO https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:84: <div class="settings-box"> actionable or on-tap on the settings-box itself is a litttttle different than on nearby <settings-toggle-box> technically the closest would be if we made a wrapper <div> inside of <div class="settings-box"> which would act similarly to <settings-toggle-button> as a wrapper around <paper-toggle-button>, but I don't really see much benefit to that https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:110: <div class="settings-box"> in this case we really don't want on-tap on the settings-box or on a container because it might affect the results as one is tapping the restart button (which is a can of worms) https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:117: // Both event types must match for stopPropagation() to work. this is mildly unfortunate https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:128: // Prevent the on-tap of the parent from double-changing the value. also mildly unfortunate https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:133: enabled = !enabled; not amazing https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:177: } the fact that all of this is copy pasta'd also isn't super
less than optimal*
dpapad@: you'll also need to stamp for committer bit for scottchen@
https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:84: <div class="settings-box"> On 2017/02/08 at 23:12:04, Dan Beam wrote: > actionable or on-tap on the settings-box itself is a litttttle different than on nearby <settings-toggle-box> > > technically the closest would be if we made a wrapper <div> inside of <div class="settings-box"> which would act similarly to <settings-toggle-button> as a wrapper around <paper-toggle-button>, but I don't really see much benefit to that Wrapping the <div> and the <paper-toggle-button> with another <div> would probably break the effects of the "start" CSS class, no? https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:110: <div class="settings-box"> On 2017/02/08 at 23:12:04, Dan Beam wrote: > in this case we really don't want on-tap on the settings-box or on a container because it might affect the results as one is tapping the restart button (which is a can of worms) I don't have a 100% understanding of our requirements. Here is the problem I see with not having the tap handler on the settings-box: The click target is narrower than the settings-box row, see http://imgur.com/a/eqOSI. Q1: Is this Alan's original intention? Q2: Regardless of yes/no, should we be consistent across all our settings-box instances? For example in settings-box instances that open a subpage the click target covers the entire row, including the padding on the left, see http://imgur.com/a/fO4bT. If we want the entire row always be clickable, I don't see another option othen than having the click/tap handler should be on settings-box, and any buttons inside the row (like the 'restart' button), having to call stopPropagation(). https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:117: // Both event types must match for stopPropagation() to work. On 2017/02/08 at 23:12:04, Dan Beam wrote: > this is mildly unfortunate This entire handler looks a bit unfortunate. I would like to understand more what exactly are we solving with this, and look for alternatives, before adopting this as an OK pattern.
have either of y'all looked at how settings-toggle-button is implemented? it's basically a more complex version of this. https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/contro... https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/contro... https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/contro...
https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:110: <div class="settings-box"> On 2017/02/08 23:46:05, dpapad wrote: > On 2017/02/08 at 23:12:04, Dan Beam wrote: > > in this case we really don't want on-tap on the settings-box or on a container > because it might affect the results as one is tapping the restart button (which > is a can of worms) > > I don't have a 100% understanding of our requirements. Here is the problem I see > with not having the tap handler on the settings-box: The click target is > narrower than the settings-box row, see http://imgur.com/a/eqOSI. that's the same for all existing <settings-toggle-box>, afaik > > Q1: Is this Alan's original intention? unclear > Q2: Regardless of yes/no, should we be consistent across all our settings-box > instances? yes, which is why I'm NOT using actionable on-top on the settings-box. > > For example in settings-box instances that open a subpage the click target > covers the entire row, including the padding on the left, see > http://imgur.com/a/fO4bT. yes, understood, those are different that a majority of <settings-toggle-button> > > If we want the entire row always be clickable, I don't see another option othen > than having the click/tap handler should be on settings-box, and any buttons > inside the row (like the 'restart' button), having to call stopPropagation(). I think we want consistency. https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:117: // Both event types must match for stopPropagation() to work. On 2017/02/08 23:46:05, dpapad wrote: > On 2017/02/08 at 23:12:04, Dan Beam wrote: > > this is mildly unfortunate > > This entire handler looks a bit unfortunate. I would like to understand more > what exactly are we solving with this, and look for alternatives, before > adopting this as an OK pattern. <settings-toggle-button> assumes a settings pref object to be bound to pref="". metrics reporting, SBER, and other things don't expose prefs directly because the C++ disallows it (or at least discourages it). generally, this is because there are accessors to ensure you're Doing It Right from the C++. metrics reporting has: ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled() IsMetricsReportingPolicyManaged() which we use here https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/metrics... I consulted gayane@ and other from the metrics team about how we should inquire / observe these prefs, and the answer was "you should probably use the accessors and potentially observe the prefs for change notifications only", so that's what we're doing. I can potentially make a fake pref object with the type of chrome.settingsPrivate.PrefObject, ex: https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/search... and use Polymer's mutation API (i.e. .set()), but that's also a little weird.
On 2017/02/09 at 00:18:32, dbeam wrote: > have either of y'all looked at how settings-toggle-button is implemented? it's basically a more complex version of this. > > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/contro... > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/contro... > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/contro... Thanks for the context. That establishes that we are indeed already using the pattern of handling tap events even when disabled, and returning early in JS. I don't think it answers questions Q1,Q2 from my previous comment. After inspecting the links above, I have a question about settings-toggle-button (we can move the discussion elsewhere if here is not appropriate). Can the JS-based implementation of a "disabled" state, be swapped for an equivalent CSS/HTML based? Specifically, could we add a "disabled" attribute when the settings-toggle-button is disabled and respect it as follows? :host([disabled]) .outer-row { pointer-events: none; } or :host([disabled]) .outer-row > div { pointer-events: none; } I am trying to understand what are the corner cases that are not addressed by a CSS/HTML based "disabled" implementation.
https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:117: // Both event types must match for stopPropagation() to work. On 2017/02/09 00:27:40, Dan Beam wrote: > On 2017/02/08 23:46:05, dpapad wrote: > > On 2017/02/08 at 23:12:04, Dan Beam wrote: > > > this is mildly unfortunate > > > > This entire handler looks a bit unfortunate. I would like to understand more > > what exactly are we solving with this, and look for alternatives, before > > adopting this as an OK pattern. > > <settings-toggle-button> assumes a settings pref object to be bound to pref="". > metrics reporting, SBER, and other things don't expose prefs directly because > the C++ disallows it (or at least discourages it). > > generally, this is because there are accessors to ensure you're Doing It Right > from the C++. > > metrics reporting has: > > ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled() > IsMetricsReportingPolicyManaged() > > which we use here > > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/metrics... > > I consulted gayane@ and other from the metrics team about how we should inquire > / observe these prefs, and the answer was "you should probably use the accessors > and potentially observe the prefs for change notifications only", so that's what > we're doing. > > I can potentially make a fake pref object with the type of > chrome.settingsPrivate.PrefObject, ex: > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/search... > > and use Polymer's mutation API (i.e. .set()), but that's also a little weird. let me try this ^ and see how it looks
https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:133: enabled = !enabled; On 2017/02/08 23:12:04, Dan Beam wrote: > not amazing Instead of checking for parent double-changing and then possibly flipping the state again here.. can we instead just say: "browserProxy.setMetricsReportingEnabled(!safeBrowsingExtendedReportingEnabled_)"? Basically, ignore what the toggle control says before/after - just flip the original value. (Similar to what I did for this: https://codereview.chromium.org/2683583008/patch/20001/30002) so the pseudo-code of this handler would just be: onMetricsReportingTap_: function(){ if(should not be clickable) return else somethingsValue = !somethingsValue; // basically you know if you get to this point of the code, you're just trying to flip the value, regardless of where the event comes from }
On 2017/02/09 00:56:43, scottchen wrote: > https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... > File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): > > https://codereview.chromium.org/2684853003/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/privacy_page/privacy_page.js:133: enabled = > !enabled; > On 2017/02/08 23:12:04, Dan Beam wrote: > > not amazing > > Instead of checking for parent double-changing and then possibly flipping the > state again here.. > > can we instead just say: > "browserProxy.setMetricsReportingEnabled(!safeBrowsingExtendedReportingEnabled_)"? > > Basically, ignore what the toggle control says before/after - just flip the > original value. > (Similar to what I did for this: > https://codereview.chromium.org/2683583008/patch/20001/30002) > > so the pseudo-code of this handler would just be: > > onMetricsReportingTap_: function(){ > if(should not be clickable) > return > else > somethingsValue = !somethingsValue; // basically you know if you get to this > point of the code, you're just trying to flip the value, regardless of where the > event comes from > } To add to my last comment - in Reactjs, it basically enforces the pattern I proposed above. i.e.: The "html value" doesn't apply to the element at all, the js controller's value it binds to is the only source of truth. This makes it easy to just have handler alter one thing and not have to fight with the double-change scenario or event bubbling; the intended-to-be-clickable dom-branch only has one event handler, and clicking on ANYTHING in that dom-branch triggers the handler only once.
The CQ bit was checked by dbeam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by dbeam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
check out this alternative approach with <settings-toggle-button> and fake pref objects https://codereview.chromium.org/2684853003/diff/10003/chrome/test/data/webui/... File chrome/test/data/webui/settings/metrics_reporting_tests.js (right): https://codereview.chromium.org/2684853003/diff/10003/chrome/test/data/webui/... chrome/test/data/webui/settings/metrics_reporting_tests.js:27: var indicatorVisible = !!control.$$('cr-policy-pref-indicator'); dpapad@: i'll do whatever you want here, this is one of those existing bad examples i told you about
So, I think this looks a bit saner than the previous version (the HTML is simpler, the JS still has a fair amount of fake pref setup, but still more readable than before). Before we commit to this approach (fake prefs + settings-toggle-button), can you comment on Scott's alternative proposal here https://codereview.chromium.org/2683583008/? If it achieves the same behavior (not 100% sure), it seems much simpler. https://codereview.chromium.org/2684853003/diff/10003/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2684853003/diff/10003/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:74: this.listen(this, 'iron-change', 'onIronChange_'); I recall an issue we discovered in the past, were relying on the iron-change event coming from the internals was racy (it fired before settings-toggle-button itself has updated its state). See context at https://bugs.chromium.org/p/chromium/issues/detail?id=665979. Not sure if this is affecting us here, but worth checking. https://codereview.chromium.org/2684853003/diff/10003/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:130: onIronChange_: function(e) { Can we not add dedicated listeners to each button? https://codereview.chromium.org/2684853003/diff/10003/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:132: var browserProxy = settings.PrivacyPageBrowserProxyImpl.getInstance(); Nit: PrivacyPageBrowserProxyImpl.getInstance() is used quite a few times in this file, I think is worth caching it in a member var. https://codereview.chromium.org/2684853003/diff/10003/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:188: this.unlisten(this, 'iron-change', 'onIronChange_'); This probably needs a comment explaining why are we unlistening and re-listening. https://codereview.chromium.org/2684853003/diff/10003/chrome/test/data/webui/... File chrome/test/data/webui/settings/metrics_reporting_tests.js (right): https://codereview.chromium.org/2684853003/diff/10003/chrome/test/data/webui/... chrome/test/data/webui/settings/metrics_reporting_tests.js:27: var indicatorVisible = !!control.$$('cr-policy-pref-indicator'); On 2017/02/09 at 16:07:42, Dan Beam wrote: > dpapad@: i'll do whatever you want here, this is one of those existing bad examples i told you about Ok, perhaps add the following? if (testBrowserProxy.metricsReporting.managed) assertTrue(control.$$('cr-policy-pref-indicator').isActive()); https://codereview.chromium.org/2684853003/diff/10003/chrome/test/data/webui/... chrome/test/data/webui/settings/metrics_reporting_tests.js:43: return testBrowserProxy.whenCalled('setMetricsReportingEnabled', toggled); whenCalled() only accepts one parameter. I am not sure why the previous code was passing |toggled| here, but it seems wrong. Perhaps what it tries to do is the following return testBrowserProxy.whenCalled('setMetricsReportingEnabled').then(function(arg) { assertEquals(control.checked, arg); });
On 2017/02/09 19:51:45, dpapad wrote: > So, I think this looks a bit saner than the previous version (the HTML is > simpler, the JS still has a fair amount of fake pref setup, but still more > readable than before). > > Before we commit to this approach (fake prefs + settings-toggle-button), can you > comment on Scott's alternative proposal here > https://codereview.chromium.org/2683583008/ If it achieves the same behavior > (not 100% sure), it seems much simpler. i'll comment on it when i try it there's a bunch of subtle stuff here that i hit last night. my manual testing: open 2 settings pages, test: -> clicking the toggle -> clicking the label -> dragging the toggle make sure that everything works usefully/in unison across both windows. https://codereview.chromium.org/2684853003/diff/10003/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2684853003/diff/10003/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:74: this.listen(this, 'iron-change', 'onIronChange_'); On 2017/02/09 19:51:45, dpapad wrote: > I recall an issue we discovered in the past, were relying on the iron-change > event coming from the internals was racy (it fired before settings-toggle-button > itself has updated its state). See context at > https://bugs.chromium.org/p/chromium/issues/detail?id=665979. Not sure if this > is affecting us here, but worth checking. > that bug talks about settings-*checkbox*, not settings-*toggle-button* dragging the toggle only dispatches an iron-change (but not a 'change') https://codereview.chromium.org/2684853003/diff/10003/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:130: onIronChange_: function(e) { On 2017/02/09 19:51:45, dpapad wrote: > Can we not add dedicated listeners to each button? you can but it doesn't buy much because you still have to .path.indexOf() in both i originally did this but this version's code is simpler, imo do you want me to show you what 2 separate listeners looks like? https://codereview.chromium.org/2684853003/diff/10003/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:132: var browserProxy = settings.PrivacyPageBrowserProxyImpl.getInstance(); On 2017/02/09 19:51:45, dpapad wrote: > Nit: PrivacyPageBrowserProxyImpl.getInstance() is used quite a few times in this > file, I think is worth caching it in a member var. Done. https://codereview.chromium.org/2684853003/diff/10003/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:188: this.unlisten(this, 'iron-change', 'onIronChange_'); On 2017/02/09 19:51:45, dpapad wrote: > This probably needs a comment explaining why are we unlistening and > re-listening. Done. https://codereview.chromium.org/2684853003/diff/10003/chrome/test/data/webui/... File chrome/test/data/webui/settings/metrics_reporting_tests.js (right): https://codereview.chromium.org/2684853003/diff/10003/chrome/test/data/webui/... chrome/test/data/webui/settings/metrics_reporting_tests.js:27: var indicatorVisible = !!control.$$('cr-policy-pref-indicator'); On 2017/02/09 19:51:45, dpapad wrote: > On 2017/02/09 at 16:07:42, Dan Beam wrote: > > dpapad@: i'll do whatever you want here, this is one of those existing bad > examples i told you about > > Ok, perhaps add the following? > if (testBrowserProxy.metricsReporting.managed) > assertTrue(control.$$('cr-policy-pref-indicator').isActive()); that still pokes into <settings-toggle-button>'s DOM i just used a different public interface https://codereview.chromium.org/2684853003/diff/10003/chrome/test/data/webui/... chrome/test/data/webui/settings/metrics_reporting_tests.js:43: return testBrowserProxy.whenCalled('setMetricsReportingEnabled', toggled); On 2017/02/09 19:51:45, dpapad wrote: > whenCalled() only accepts one parameter. I am not sure why the previous code was > passing |toggled| here, but it seems wrong. Perhaps what it tries to do is the > following > > return > testBrowserProxy.whenCalled('setMetricsReportingEnabled').then(function(arg) { > assertEquals(control.checked, arg); > }); Done. (thanks!)
The CQ bit was checked by dbeam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2684853003/diff/10003/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2684853003/diff/10003/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:74: this.listen(this, 'iron-change', 'onIronChange_'); On 2017/02/09 at 20:48:45, Dan Beam wrote: > On 2017/02/09 19:51:45, dpapad wrote: > > I recall an issue we discovered in the past, were relying on the iron-change > > event coming from the internals was racy (it fired before settings-toggle-button > > itself has updated its state). See context at > > https://bugs.chromium.org/p/chromium/issues/detail?id=665979. Not sure if this > > is affecting us here, but worth checking. > > > > that bug talks about settings-*checkbox*, not settings-*toggle-button* > > dragging the toggle only dispatches an iron-change (but not a 'change') The related code linked in the bug is inside https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/contro..., which is used by both elements. Also the issue is indeed happening with 'iron-change' not 'change', which both elements seem to fire, so I am still not so sure that settings-toggle-button is fine using iron-change. Per our previous discussion (see https://bugs.chromium.org/p/chromium/issues/detail?id=665979#c2), we were considering calling stopPropagation() on the iron-change events to prevent anyone from using those events (and potentially add our own event that is fired at the right moment). https://codereview.chromium.org/2684853003/diff/10003/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:130: onIronChange_: function(e) { On 2017/02/09 at 20:48:45, Dan Beam wrote: > On 2017/02/09 19:51:45, dpapad wrote: > > Can we not add dedicated listeners to each button? > > you can but it doesn't buy much because you still have to .path.indexOf() in both > > i originally did this but this version's code is simpler, imo > > do you want me to show you what 2 separate listeners looks like? I am curious. Polymer.dom(e).localTarget gives you the local element, so why is a call path.indexOf() needed. As another data point, I don't see any other call to that method in our code, so would like to understand what makes this case unique.
https://codereview.chromium.org/2684853003/diff/10003/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2684853003/diff/10003/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:130: onIronChange_: function(e) { On 2017/02/09 23:53:02, dpapad wrote: > On 2017/02/09 at 20:48:45, Dan Beam wrote: > > On 2017/02/09 19:51:45, dpapad wrote: > > > Can we not add dedicated listeners to each button? > > > > you can but it doesn't buy much because you still have to .path.indexOf() in > both > > > > i originally did this but this version's code is simpler, imo > > > > do you want me to show you what 2 separate listeners looks like? > > I am curious. Polymer.dom(e).localTarget gives you the local element, so why is > a call path.indexOf() needed. As another data point, I don't see any other call > to that method in our code, so would like to understand what makes this case > unique. because the localTarget is actually the paper-toggle-button inside of this.$.safeBrowsingExtendedReportingControl as well as this.$.safeBrowsingExtendedReportingControl.contains(Polymer.dom(e).localTarget) does not work (because .contains() doesn't work across shadow) i know because i tried those 2 first if you'd like i can stop the iron-change inside of SettingsBooleanPrefBehaviorWhatever and do this.fire('boolean-pref-change-or-whatever', this.checked) but it didn't really seem much better (although it wouldn't break if we changed the implementation)
On 2017/02/09 23:57:53, Dan Beam wrote: > https://codereview.chromium.org/2684853003/diff/10003/chrome/browser/resource... > File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): > > https://codereview.chromium.org/2684853003/diff/10003/chrome/browser/resource... > chrome/browser/resources/settings/privacy_page/privacy_page.js:130: > onIronChange_: function(e) { > On 2017/02/09 23:53:02, dpapad wrote: > > On 2017/02/09 at 20:48:45, Dan Beam wrote: > > > On 2017/02/09 19:51:45, dpapad wrote: > > > > Can we not add dedicated listeners to each button? > > > > > > you can but it doesn't buy much because you still have to .path.indexOf() in > > both > > > > > > i originally did this but this version's code is simpler, imo > > > > > > do you want me to show you what 2 separate listeners looks like? > > > > I am curious. Polymer.dom(e).localTarget gives you the local element, so why > is > > a call path.indexOf() needed. As another data point, I don't see any other > call > > to that method in our code, so would like to understand what makes this case > > unique. > > because the localTarget is actually the paper-toggle-button inside of > this.$.safeBrowsingExtendedReportingControl > > as well as > this.$.safeBrowsingExtendedReportingControl.contains(Polymer.dom(e).localTarget) > does not work (because .contains() doesn't work across shadow) > > i know because i tried those 2 first > > if you'd like i can stop the iron-change inside of > SettingsBooleanPrefBehaviorWhatever and do > this.fire('boolean-pref-change-or-whatever', this.checked) but it didn't really > seem much better (although it wouldn't break if we changed the implementation) After a few iterations, I was able to get an alternative solution to a working state: https://codereview.chromium.org/2688073002/ Tested on two simultaneous window with clicking the label, clicking the toggle, or dragging the toggle. Also used breakpoint to make sure the handler is only firing once (in one of my iteration it was actually firing twice but appeared as working since js run cycle was faster than C++ callback, but it's fixed now). please check it out and leave your thoughts on the comparison (here or in the other CL, either way).
The CQ bit was checked by dbeam@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...
so i think this combines a bunch of scottchen@'s work and discoveries as well as mine i think this is a good intermediate step on the way to virtualized prefs I just looked into a potential place for this and I think calling: https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_p... extensions::SettingsPrivateEventRouter::OnPreferenceChanged(const std::string& pref_name); is a good start, but I think it'll be a bit more work after that. what do you guys think?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/11 02:59:16, Dan Beam wrote: > so i think this combines a bunch of scottchen@'s work and discoveries as well as > mine > > i think this is a good intermediate step on the way to virtualized prefs > > I just looked into a potential place for this and I think calling: > > https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_p... > > extensions::SettingsPrivateEventRouter::OnPreferenceChanged(const std::string& > pref_name); > > is a good start, but I think it'll be a bit more work after that. > > what do you guys think? Based on our offline discussion, I think doing virtualized prefs is a good way to go for things that are prefs but behind a middleware, or things that are supposed to behave like prefs. Not super familiar with the actual implementation of how pref event flow works... looks like that function will eventually dispatch a event to webui? I'll let dpapad comment on the details.
On 2017/02/11 at 02:59:16, dbeam wrote: > so i think this combines a bunch of scottchen@'s work and discoveries as well as mine > > i think this is a good intermediate step on the way to virtualized prefs > > I just looked into a potential place for this and I think calling: > > https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_p... > > extensions::SettingsPrivateEventRouter::OnPreferenceChanged(const std::string& pref_name); > > is a good start, but I think it'll be a bit more work after that. > > what do you guys think? I agree that this is a good start. Regarding the follow up steps, perhaps settings_private_event_router.h is the right C++ place to implement the virtual prefs, I am not very familiar with that code either. Essentially right now we are implementing fake prefs on-the-fly within the UI elements that need them. Just for completeness I'll mention that we also discussed (last week) an alternative of implementing virtual prefs in JS, inside prefs.js instead of C++. In that case prefs.js would hide the complexity of updating fake prefs (metrics reporting, safe browsing reporting), and they would be exposed as normal prefs to UI elements, such as privacy_page.js. The spectrum of where virtual prefs can be implemented is as follows: UI element -> prefs.js -> somewhere in C++ Currently we are at "UI element". Moving them to either prefs.js or in C++ will be an improvement. I am thinking that prefs.js might be an easier first step for our virtual prefs abstraction, but I am OK if we want to go to C++ in a single step.
https://codereview.chromium.org/2684853003/diff/150001/chrome/browser/resourc... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2684853003/diff/150001/chrome/browser/resourc... chrome/browser/resources/settings/privacy_page/privacy_page.js:62: this.browserProxy_ = settings.PrivacyPageBrowserProxyImpl.getInstance(); Can we also declare this before initializing it? browserProxy_: null, ready: function() { this.browserProxy_ ... ... }, ...
On 2017/02/13 20:17:09, dpapad wrote: > On 2017/02/11 at 02:59:16, dbeam wrote: > > so i think this combines a bunch of scottchen@'s work and discoveries as well > as mine > > > > i think this is a good intermediate step on the way to virtualized prefs > > > > I just looked into a potential place for this and I think calling: > > > > > https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_p... > > > > extensions::SettingsPrivateEventRouter::OnPreferenceChanged(const std::string& > pref_name); > > > > is a good start, but I think it'll be a bit more work after that. > > > > what do you guys think? > > I agree that this is a good start. Regarding the follow up steps, perhaps > settings_private_event_router.h is the > right C++ place to implement the virtual prefs, I am not very familiar with that > code either. > > Essentially right now we are implementing fake prefs on-the-fly within the UI > elements that need them. Just for > completeness I'll mention that we also discussed (last week) an alternative of > implementing virtual prefs in JS, > inside prefs.js instead of C++. In that case prefs.js would hide the complexity > of updating fake prefs (metrics > reporting, safe browsing reporting), and they would be exposed as normal prefs > to UI elements, such as privacy_page.js. > > The spectrum of where virtual prefs can be implemented is as follows: > UI element -> prefs.js -> somewhere in C++ > > Currently we are at "UI element". Moving them to either prefs.js or in C++ will > be an improvement. I am thinking that > prefs.js might be an easier first step for our virtual prefs abstraction, but I > am OK if we want to go to C++ in a single step. i don't want to do that before launching
https://codereview.chromium.org/2684853003/diff/150001/chrome/browser/resourc... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2684853003/diff/150001/chrome/browser/resourc... chrome/browser/resources/settings/privacy_page/privacy_page.js:62: this.browserProxy_ = settings.PrivacyPageBrowserProxyImpl.getInstance(); On 2017/02/13 20:17:50, dpapad wrote: > Can we also declare this before initializing it? > > browserProxy_: null, > > ready: function() { > this.browserProxy_ ... > ... > }, > ... what benefit does that have?
On 2017/02/13 at 20:18:50, dbeam wrote: > https://codereview.chromium.org/2684853003/diff/150001/chrome/browser/resourc... > File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): > > https://codereview.chromium.org/2684853003/diff/150001/chrome/browser/resourc... > chrome/browser/resources/settings/privacy_page/privacy_page.js:62: this.browserProxy_ = settings.PrivacyPageBrowserProxyImpl.getInstance(); > On 2017/02/13 20:17:50, dpapad wrote: > > Can we also declare this before initializing it? > > > > browserProxy_: null, > > > > ready: function() { > > this.browserProxy_ ... > > ... > > }, > > ... > > what benefit does that have? First consistency with https://cs.chromium.org/search/?q=%22browserProxy_:%22+file:%5Esrc/chrome/bro.... But also, it is odd to me that compilation passes without it. Why am I asking for this? Well ready() is not a constructor, so in general any member var that is initialized somewhere should be declared in the constructor, otherwise the object is augmented after construction which is not a good practice IMO (and probably V8 performance with regards to "hidden classes"). We don't have a clear "constructor" in Polymer elements, and Polymer pass is rewriting the code in its own way (which I am not super familiar with). In short P1 Custom elements definition are so idiomatic which almost make me give up about consistency with traditional JS classes. I am fine either way.
On 2017/02/13 at 20:32:46, dpapad wrote: > On 2017/02/13 at 20:18:50, dbeam wrote: > > https://codereview.chromium.org/2684853003/diff/150001/chrome/browser/resourc... > > File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): > > > > https://codereview.chromium.org/2684853003/diff/150001/chrome/browser/resourc... > > chrome/browser/resources/settings/privacy_page/privacy_page.js:62: this.browserProxy_ = settings.PrivacyPageBrowserProxyImpl.getInstance(); > > On 2017/02/13 20:17:50, dpapad wrote: > > > Can we also declare this before initializing it? > > > > > > browserProxy_: null, > > > > > > ready: function() { > > > this.browserProxy_ ... > > > ... > > > }, > > > ... > > > > what benefit does that have? > > First consistency with https://cs.chromium.org/search/?q=%22browserProxy_:%22+file:%5Esrc/chrome/bro.... > > But also, it is odd to me that compilation passes without it. Why am I asking for this? > Well ready() is not a constructor, so in general any member var that is initialized somewhere should be declared in > the constructor, otherwise the object is augmented after construction which is not a good practice IMO (and probably > V8 performance with regards to "hidden classes"). We don't have a clear "constructor" in Polymer elements, and Polymer > pass is rewriting the code in its own way (which I am not super familiar with). In short P1 Custom elements definition > are so idiomatic which almost make me give up about consistency with traditional JS classes. I am fine either way. LGTM
The CQ bit was checked by dbeam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottchen@chromium.org Link to the patchset: https://codereview.chromium.org/2684853003/#ps150001 (title: "fix tests")
Description was changed from ========== MD Settings: change how tap is handled on custom toggle rows R=scottchen@chromium.org BUG=680406 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: change how tap is handled on custom toggle rows R=scottchen@chromium.org BUG=680406,689763 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dbeam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottchen@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2684853003/#ps170001 (title: "todo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by dbeam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 170001, "attempt_start_ts": 1487058574601940,
"parent_rev": "1175597e824387e993a7dadd093ca58cdaae9246", "commit_rev":
"3245561158dde6808bd24b3322304369af52abac"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: change how tap is handled on custom toggle rows R=scottchen@chromium.org BUG=680406,689763 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: change how tap is handled on custom toggle rows R=scottchen@chromium.org BUG=680406,689763 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2684853003 Cr-Commit-Position: refs/heads/master@{#450293} Committed: https://chromium.googlesource.com/chromium/src/+/3245561158dde6808bd24b332230... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:170001) as https://chromium.googlesource.com/chromium/src/+/3245561158dde6808bd24b332230... |
