MD Settings: Fix the Network Prediction toggle box.

MD Settings: Fix the Network Prediction toggle box.

In order to fix this toggle, I had to update the 
SettingsBooleanControlBehavior to support non 0/1 values for numeric 
prefs.

BUG=664048
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2681373002
Cr-Commit-Position: refs/heads/master@{#450063}
Committed: https://chromium.googlesource.com/chromium/src/+/898ecc48fb6440230a819bcbd113f90c0d0df11e

[tommycli, 2017-02-09 17:50:38]

Description was changed from

==========
MD Settings: Fix the Network Prediction toggle box.

BUG=664048
==========

to

==========
MD Settings: Fix the Network Prediction toggle box.

BUG=664048
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tommycli, 2017-02-09 17:55:29]

Description was changed from

==========
MD Settings: Fix the Network Prediction toggle box.

BUG=664048
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Fix the Network Prediction toggle box.

In order to fix this toggle, I had to update the 
SettingsBooleanControlBehavior to support non 0/1 values
for numeric prefs.

BUG=664048
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tommycli, 2017-02-09 17:55:29]

tommycli@chromium.org changed reviewers:
+ stevenjb@chromium.org

[tommycli, 2017-02-09 17:56:06]

Description was changed from

==========
MD Settings: Fix the Network Prediction toggle box.

In order to fix this toggle, I had to update the 
SettingsBooleanControlBehavior to support non 0/1 values
for numeric prefs.

BUG=664048
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Fix the Network Prediction toggle box.

In order to fix this toggle, I had to update the 
SettingsBooleanControlBehavior to support non 0/1 values for numeric 
prefs.

BUG=664048
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tommycli, 2017-02-09 17:56:22]

stevenjb: PTAL since you wrote this behavior. Thanks, - Tommy

[Dan Beam, 2017-02-09 18:39:10]

general nit because this behavior is so high occurrence

https://codereview.chromium.org/2681373002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/controls/settings_boolean_control_behavior.js
(right):

https://codereview.chromium.org/2681373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_boolean_control_behavior.js:77:
}
can we only add numericUncheckedValue until we need a numericCheckedValue?

also, can we make these vanilla prototype members instead of in properties
(which sets up getters/setters and potentially observers or something)

[stevenjb, 2017-02-09 18:58:38]

On 2017/02/09 18:39:10, Dan Beam wrote:
> general nit because this behavior is so high occurrence
> 
>
https://codereview.chromium.org/2681373002/diff/20001/chrome/browser/resource...
> File
>
chrome/browser/resources/settings/controls/settings_boolean_control_behavior.js
> (right):
> 
>
https://codereview.chromium.org/2681373002/diff/20001/chrome/browser/resource...
>
chrome/browser/resources/settings/controls/settings_boolean_control_behavior.js:77:
> }
> can we only add numericUncheckedValue until we need a numericCheckedValue?
> 
> also, can we make these vanilla prototype members instead of in properties
> (which sets up getters/setters and potentially observers or something)

Even with the changes Dan suggests, this seems like a fair bit of overhead for a
one off. Maybe there are one or two other similar cases? Even so, I would
consider making a separate element and moving the shared logic to a behavior.

I think that our next performance win may come from optimizing our frequently
used elements; this seems like a step in the opposite direction.

That said, it may make sense for you and Dan to talk this through in person, I
am fine with whatever you decide.

[tommycli, 2017-02-09 19:19:22]

https://codereview.chromium.org/2681373002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/controls/settings_boolean_control_behavior.js
(right):

https://codereview.chromium.org/2681373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_boolean_control_behavior.js:77:
}
On 2017/02/09 18:39:10, Dan Beam wrote:
> can we only add numericUncheckedValue until we need a numericCheckedValue?
> 
> also, can we make these vanilla prototype members instead of in properties
> (which sets up getters/setters and potentially observers or something)

Done.

But I can't make this a plain prototype member because then this value is
initialized at 0, and then becomes 2 at some point later. This causes havoc with
the pref (sends spurious pref updates).

When it's a property it's initialized at 2. I'm not sure why.

[tommycli, 2017-02-09 19:20:56]

On 2017/02/09 18:58:38, stevenjb wrote:
> On 2017/02/09 18:39:10, Dan Beam wrote:
> > general nit because this behavior is so high occurrence
> > 
> >
>
https://codereview.chromium.org/2681373002/diff/20001/chrome/browser/resource...
> > File
> >
>
chrome/browser/resources/settings/controls/settings_boolean_control_behavior.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/2681373002/diff/20001/chrome/browser/resource...
> >
>
chrome/browser/resources/settings/controls/settings_boolean_control_behavior.js:77:
> > }
> > can we only add numericUncheckedValue until we need a numericCheckedValue?
> > 
> > also, can we make these vanilla prototype members instead of in properties
> > (which sets up getters/setters and potentially observers or something)
> 
> Even with the changes Dan suggests, this seems like a fair bit of overhead for
a
> one off. Maybe there are one or two other similar cases? Even so, I would
> consider making a separate element and moving the shared logic to a behavior.
> 
> I think that our next performance win may come from optimizing our frequently
> used elements; this seems like a step in the opposite direction.
> 
> That said, it may make sense for you and Dan to talk this through in person, I
> am fine with whatever you decide.

stevenjb: I hear you. It's a bit sad that web components did not succeed at
making abstractions near-zero cost and we have to think about this stuff...

[tommycli, 2017-02-10 20:06:15]

tommycli@chromium.org changed reviewers:
+ dbeam@chromium.org

[tommycli, 2017-02-10 20:06:16]

dbeam/stevenjb: May I have a second look at this?

I realize it adds another property to all of these controls, but since this is a
pretty simple solution, my recommendation is to let it pass, and if it
regresses, it should show up on the automated perf dashboard. (So far i haven't
seen any detectable regressions in load time from our other changes)

[stevenjb, 2017-02-10 21:03:46]

I would still prefer a custom control here (can an element replace, i.e.
override, a method defined in a behavior?) but I will let Dan decode if I am
being overly concerned about performance impact. I just feel like we have a lot
of boolean controls...

[Dan Beam, 2017-02-10 22:50:28]

On 2017/02/10 21:03:46, stevenjb wrote:
> I would still prefer a custom control here (can an element replace, i.e.
> override, a method defined in a behavior?) but I will let Dan decode if I am
> being overly concerned about performance impact. I just feel like we have a
lot
> of boolean controls...

i told tommycli@ to prove it doesn't have a performance impact (i.e. run the
page with and without this change and measure load time and memory).

he's working on it.

[stevenjb, 2017-02-10 22:51:53]

Do we have a way to test with that level of accuracy? Unfortunately when I
run the load time browser tests I get at least a 10% difference from run to
run, making it challenging to detect the effect small changes :( I'd love
to have a more robust mechanism...


On Fri, Feb 10, 2017 at 2:50 PM, <dbeam@chromium.org> wrote:

> On 2017/02/10 21:03:46, stevenjb wrote:
> > I would still prefer a custom control here (can an element replace, i.e.
> > override, a method defined in a behavior?) but I will let Dan decode if
> I am
> > being overly concerned about performance impact. I just feel like we
> have a
> lot
> > of boolean controls...
>
> i told tommycli@ to prove it doesn't have a performance impact (i.e. run
> the
> page with and without this change and measure load time and memory).
>
> he's working on it.
>
> https://codereview.chromium.org/2681373002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-02-10 23:34:02]

Hey guys,

The perf impact was pretty much completely in the noise:

FMP w/patch Memory w/ patch FMP baseline Memory baseline
1 3295.1 91492 3726.5 94328
2 3673.5 92400 3502.4 88848
3 3593.3 94552 3736.3 97860
4 3793.7 88824 3721.7 91428
5 3698.2 94448 3720.8 89980
median 3673.5 92400 3721.7 91428
I do have this automated tracker to detect large regressions:
https://docs.google.com/a/google.com/spreadsheets/d/1-NWS8M6BW6u6VwEYdFypD4SK...

I don't think this CL will cause a large regression...

Tommy

On Fri, Feb 10, 2017 at 2:51 PM, Steven Bennetts <stevenjb@chromium.org>
wrote:

> Do we have a way to test with that level of accuracy? Unfortunately when I
> run the load time browser tests I get at least a 10% difference from run to
> run, making it challenging to detect the effect small changes :( I'd love
> to have a more robust mechanism...
>
>
> On Fri, Feb 10, 2017 at 2:50 PM, <dbeam@chromium.org> wrote:
>
>> On 2017/02/10 21:03:46, stevenjb wrote:
>> > I would still prefer a custom control here (can an element replace, i.e.
>> > override, a method defined in a behavior?) but I will let Dan decode if
>> I am
>> > being overly concerned about performance impact. I just feel like we
>> have a
>> lot
>> > of boolean controls...
>>
>> i told tommycli@ to prove it doesn't have a performance impact (i.e. run
>> the
>> page with and without this change and measure load time and memory).
>>
>> he's working on it.
>>
>> https://codereview.chromium.org/2681373002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stevenjb, 2017-02-11 00:16:44]

I didn't expect this CL to have a measurable impact either, but I do have a
mental TOD item to start experiment with more drastic reductions for
commonly used elements to see if hand optimization is worthwhile. I guess
adding this isn't going to make any optimization measurably (rimshot) more
difficult, so this seems fine. Thanks for doing the analysis!



On Fri, Feb 10, 2017 at 3:33 PM, Tommy Li <tommycli@google.com> wrote:

> Hey guys,
>
> The perf impact was pretty much completely in the noise:
>
> FMP w/patch Memory w/ patch FMP baseline Memory baseline
> 1 3295.1 91492 3726.5 94328
> 2 3673.5 92400 3502.4 88848
> 3 3593.3 94552 3736.3 97860
> 4 3793.7 88824 3721.7 91428
> 5 3698.2 94448 3720.8 89980
> median 3673.5 92400 3721.7 91428
> I do have this automated tracker to detect large regressions:
> https://docs.google.com/a/google.com/spreadsheets/d/1-
> NWS8M6BW6u6VwEYdFypD4SKDVtfW8XN3mtjMLXKQAQ/edit?usp=sharing
>
> I don't think this CL will cause a large regression...
>
> Tommy
>
> On Fri, Feb 10, 2017 at 2:51 PM, Steven Bennetts <stevenjb@chromium.org>
> wrote:
>
>> Do we have a way to test with that level of accuracy? Unfortunately when
>> I run the load time browser tests I get at least a 10% difference from run
>> to run, making it challenging to detect the effect small changes :( I'd
>> love to have a more robust mechanism...
>>
>>
>> On Fri, Feb 10, 2017 at 2:50 PM, <dbeam@chromium.org> wrote:
>>
>>> On 2017/02/10 21:03:46, stevenjb wrote:
>>> > I would still prefer a custom control here (can an element replace,
>>> i.e.
>>> > override, a method defined in a behavior?) but I will let Dan decode
>>> if I am
>>> > being overly concerned about performance impact. I just feel like we
>>> have a
>>> lot
>>> > of boolean controls...
>>>
>>> i told tommycli@ to prove it doesn't have a performance impact (i.e.
>>> run the
>>> page with and without this change and measure load time and memory).
>>>
>>> he's working on it.
>>>
>>> https://codereview.chromium.org/2681373002/
>>>
>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tommycli, 2017-02-11 00:17:59]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-11 00:18:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommycli, 2017-02-11 00:35:42]

On 2017/02/11 00:16:44, stevenjb wrote:
> I didn't expect this CL to have a measurable impact either, but I do have a
> mental TOD item to start experiment with more drastic reductions for
> commonly used elements to see if hand optimization is worthwhile. I guess
> adding this isn't going to make any optimization measurably (rimshot) more
> difficult, so this seems fine. Thanks for doing the analysis!
> 
> 
> 
> On Fri, Feb 10, 2017 at 3:33 PM, Tommy Li <mailto:tommycli@google.com> wrote:
> 
> > Hey guys,
> >
> > The perf impact was pretty much completely in the noise:
> >
> > FMP w/patch Memory w/ patch FMP baseline Memory baseline
> > 1 3295.1 91492 3726.5 94328
> > 2 3673.5 92400 3502.4 88848
> > 3 3593.3 94552 3736.3 97860
> > 4 3793.7 88824 3721.7 91428
> > 5 3698.2 94448 3720.8 89980
> > median 3673.5 92400 3721.7 91428
> > I do have this automated tracker to detect large regressions:
> > https://docs.google.com/a/google.com/spreadsheets/d/1-
> > NWS8M6BW6u6VwEYdFypD4SKDVtfW8XN3mtjMLXKQAQ/edit?usp=sharing
> >
> > I don't think this CL will cause a large regression...
> >
> > Tommy
> >
> > On Fri, Feb 10, 2017 at 2:51 PM, Steven Bennetts
<mailto:stevenjb@chromium.org>
> > wrote:
> >
> >> Do we have a way to test with that level of accuracy? Unfortunately when
> >> I run the load time browser tests I get at least a 10% difference from run
> >> to run, making it challenging to detect the effect small changes :( I'd
> >> love to have a more robust mechanism...
> >>
> >>
> >> On Fri, Feb 10, 2017 at 2:50 PM, <mailto:dbeam@chromium.org> wrote:
> >>
> >>> On 2017/02/10 21:03:46, stevenjb wrote:
> >>> > I would still prefer a custom control here (can an element replace,
> >>> i.e.
> >>> > override, a method defined in a behavior?) but I will let Dan decode
> >>> if I am
> >>> > being overly concerned about performance impact. I just feel like we
> >>> have a
> >>> lot
> >>> > of boolean controls...
> >>>
> >>> i told tommycli@ to prove it doesn't have a performance impact (i.e.
> >>> run the
> >>> page with and without this change and measure load time and memory).
> >>>
> >>> he's working on it.
> >>>
> >>> https://codereview.chromium.org/2681373002/
> >>>
> >>
> >>
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Okay great! May I have a stamp on this CL? thanks

[Dan Beam, 2017-02-11 00:42:46]

lgtm

[tommycli, 2017-02-11 00:44:45]

The CQ bit was unchecked by tommycli@chromium.org

[tommycli, 2017-02-11 00:44:45]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2017-02-11 00:45:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-11 01:31:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-11 01:31:28]

Failed to apply patch for
chrome/browser/resources/settings/privacy_page/privacy_page.js:
While running git apply --index -p1;
  error: patch failed:
chrome/browser/resources/settings/privacy_page/privacy_page.js:185
  error: chrome/browser/resources/settings/privacy_page/privacy_page.js: patch
does not apply

Patch:       chrome/browser/resources/settings/privacy_page/privacy_page.js
Index: chrome/browser/resources/settings/privacy_page/privacy_page.js
diff --git a/chrome/browser/resources/settings/privacy_page/privacy_page.js
b/chrome/browser/resources/settings/privacy_page/privacy_page.js
index
8e0c4b029c5ad5253e57388a4c64b732355b14aa..e1c12a79f868ebed8e9cdfb66f9458f9c5c9304e
100644
--- a/chrome/browser/resources/settings/privacy_page/privacy_page.js
+++ b/chrome/browser/resources/settings/privacy_page/privacy_page.js
@@ -7,6 +7,19 @@
  * 'settings-privacy-page' is the settings page containing privacy and
  * security settings.
  */
+(function() {
+
+/**
+ * Must be kept in sync with the C++ enum of the same name.
+ * @enum {number}
+ */
+var NetworkPredictionOptions = {
+  ALWAYS: 0,
+  WIFI_ONLY: 1,
+  NEVER: 2,
+  DEFAULT: 1
+};
+
 Polymer({
   is: 'settings-privacy-page',
 
@@ -42,6 +55,17 @@ Polymer({
 
     /** @private */
     showClearBrowsingDataDialog_: Boolean,
+
+    /**
+     * Used for HTML bindings. This is defined as a property rather than within
+     * the ready callback, because the value needs to be available before
+     * local DOM initialization - otherwise, the toggle has unexpected
behavior.
+     * @private
+     */
+    networkPredictionEnum_: {
+      type: Object,
+      value: NetworkPredictionOptions,
+    },
   },
 
   ready: function() {
@@ -185,3 +209,4 @@ Polymer({
     return value ? trueLabel : falseLabel;
   },
 });
+})();

[tommycli, 2017-02-13 18:59:32]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2017-02-13 18:59:33]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2681373002/#ps60001
(title: "fix the merge")

[commit-bot: I haz the power, 2017-02-13 18:59:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 20:29:59]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487012372656310,
"parent_rev": "2441837767de19419b168ee54b8234598db57f69", "commit_rev":
"898ecc48fb6440230a819bcbd113f90c0d0df11e"}

[commit-bot: I haz the power, 2017-02-13 20:31:02]

Description was changed from

==========
MD Settings: Fix the Network Prediction toggle box.

In order to fix this toggle, I had to update the 
SettingsBooleanControlBehavior to support non 0/1 values for numeric 
prefs.

BUG=664048
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Fix the Network Prediction toggle box.

In order to fix this toggle, I had to update the 
SettingsBooleanControlBehavior to support non 0/1 values for numeric 
prefs.

BUG=664048
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2681373002
Cr-Commit-Position: refs/heads/master@{#450063}
Committed:
https://chromium.googlesource.com/chromium/src/+/898ecc48fb6440230a819bcbd113...
==========

[commit-bot: I haz the power, 2017-02-13 20:31:03]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/898ecc48fb6440230a819bcbd113...

[Dan Beam, 2017-02-14 05:16:49]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2691223002/ by dbeam@chromium.org.

The reason for reverting is: Causing stack overflow errors on
chrome://md-settings.