MD Settings: Internet: Add/fix remaining policy items

chrome/browser/resources/settings/internet_page/network_proxy.js

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /**
  * @fileoverview Polymer element for displaying and editing network proxy
  * values.
  */
 Polymer({
   is: 'network-proxy',
@@ skipping 328 matching lines @@
   getShowNetworkPolicyIndicator_: function() {
     let property = this.getProxySettingsTypeProperty_();
     return !!property && !this.isExtensionControlled(property) &&
         this.isNetworkPolicyEnforced(property);
   },
 
   /**
    * @return {boolean}
    * @private
    */
-  getShowPrefPolicyIndicator_: function() {
+  getShowExtensionPolicyIndicator_: function() {

[Dan Beam, 2016/11/29 05:25:34]

why does this use 2 verbs? "get" AND "show"?  why not
shouldShowExtensionIndicator()?  or just isExtensionControlled?

also, why both "Policy" and "Extension"?  that's really confusing...

[stevenjb, 2016/11/30 22:58:19]

-> shouldShowExtensionIndicator

     let property = this.getProxySettingsTypeProperty_();
     return !!property && this.isExtensionControlled(property);
   },
 
   /**
    * @param {!CrOnc.ManagedProperty} property
    * @return {boolean}
    * @private
    */
   getShowAllowShared_: function(property) {
@@ skipping 22 matching lines @@
 
   /**
    * Used to check the editable state for proxy related UI that may or may
    * not be directly controlled by a policy. We use the enforced state of the
    * 'ProxySettings.Type' property for these controls.
    * @return {boolean} Whether the proxy control is editable.
    * @private
    */
   isProxyEditable_: function() {
     let property = this.getProxySettingsTypeProperty_();
-    return !!property && this.isEditable_(property);
+    return !property || this.isEditable_(property);

[Dan Beam, 2016/11/29 05:25:34]

can you test this logic?

[stevenjb, 2016/11/30 22:58:19]

We *should* test all of 'internet'. We currently test *none* of it. The testing
is... daunting. I have been suggesting that adding tests would be a great way
for someone else to become familiar with this rather large amount of code for a
while now...

[Dan Beam, 2016/12/02 00:23:50]

now's a great time to start.

[Dan Beam, 2016/12/02 00:59:49]

let me expound on this: the more code you write, the more daunting this will be.
 i don't think "there's no tests and we wrote a lot of code that's not tested,
and will likely continue to add more untested code" is a good excuse.

anyways, as this will ultimately be your burden to bear, i'll leave you to
decide.

[stevenjb, 2016/12/02 17:36:58]

I am not unaware of any of this. The question has always been, "is it worth
taking a week or two to write some tests for this section that might actually
catch regressions?" So far, my response to myself (and I though we had also
discussed this) has been, "this would be a great way to get someone else up to
speed on this code so that I am not the sole maintainer" (since writing useful
tests will require understanding the model API that the UI is built on). Also,
"we really need to ship this sooner than late,r and tests are more valuable (in
my experience) for long term maintenance than short term stability (which is not
to say that tests don't catch short term bugs, but I find that to be more true
with complex model code, which in this case is pretyt well tested, than with UI
controller code).

Anyway, your suggestion is duly noted.

   },
 
   /**
    * @param {string} property The property to test
    * @param {string} value The value to test against
    * @return {boolean} True if property == value
    * @private
    */
   matches_: function(property, value) {
     return property == value;
@@ skipping 23 matching lines @@
    * Handles the shared proxy confirmation dialog 'Cancel' button or a cancel
    * event.
    * @private
    */
   onAllowSharedDialogCancel_: function() {
     /** @type {!SettingsCheckboxElement} */ (this.$.allowShared)
         .resetToPrefValue();
     this.$.confirmAllowSharedDialog.close();
   },
 });