Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1376383003:
Add cr-policy-network-indicator (Closed)
|
Created:
5 years, 2 months ago by stevenjb Modified:
5 years, 2 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, michaelpg+watch-md-settings_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, vitalyp+closure_chromium.org, dbeam+watch-settings_chromium.org, dbeam+watch-closure_chromium.org, stevenjb+watch-md-settings_chromium.org, jlklein+watch-closure_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@issue_521791_network_indicators_b Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd cr-policy-network-indicator
This CL:
* Adds CrPolicyNetworkBehavior for networking specific behavior
* Adds CrPolicyNetworkIndicator for network based indicators
NOTE: There are no visible changes resulting from this CL
BUG=521791
Committed: https://crrev.com/0022774fb65e2ca266e2dc6f81e4c448edf53d85
Cr-Commit-Position: refs/heads/master@{#355375}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Simplify #
Total comments: 11
Patch Set 3 : Feedback #
Total comments: 13
Patch Set 4 : Rebase #Patch Set 5 : Fixes and feedback #
Total comments: 14
Patch Set 6 : Rebase + feedback #Patch Set 7 : Rebase #
Total comments: 1
Patch Set 8 : Rebase + fix WS #
Dependent Patchsets: Messages
Total messages: 20 (3 generated)
stevenjb@chromium.org changed reviewers: + dbeam@chromium.org, michaelpg@chromium.org
Just the cr-policy-network-indicator changes from https://codereview.chromium.org/1369403006/
https://codereview.chromium.org/1376383003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/internet_page/internet_detail_page.css (right): https://codereview.chromium.org/1376383003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/internet_page/internet_detail_page.css:46: margin: 0 0 10px 10px; when's this page being RTLized? https://codereview.chromium.org/1376383003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/internet_page/internet_detail_page.html (right): https://codereview.chromium.org/1376383003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/internet_page/internet_detail_page.html:54: <div class="control layout horizontal center" this is two CLs in one. separate plz
https://codereview.chromium.org/1376383003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/internet_page/internet_detail_page.html (right): https://codereview.chromium.org/1376383003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/internet_page/internet_detail_page.html:54: <div class="control layout horizontal center" On 2015/10/07 18:21:07, michaelpg (OOO til 19th) wrote: > this is two CLs in one. separate plz sure.
PTAL
https://codereview.chromium.org/1376383003/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js (right): https://codereview.chromium.org/1376383003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:24: if (property.hasOwnProperty('UserPolicy') || can property.UserPolicy ever be falsey? i.e. why not just if (property.UserPolicy)? hasOwnProperty() is to avoid looking up the __proto__ chain. Seems unlikely that there'd accidentally be a base attribute named UserPolicy https://codereview.chromium.org/1376383003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:37: return false; nit: \n between conditionals
PTAL https://codereview.chromium.org/1376383003/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js (right): https://codereview.chromium.org/1376383003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:24: if (property.hasOwnProperty('UserPolicy') || On 2015/10/07 21:58:36, Dan Beam wrote: > can property.UserPolicy ever be falsey? i.e. why not just if > (property.UserPolicy)? > > hasOwnProperty() is to avoid looking up the __proto__ chain. Seems unlikely > that there'd accidentally be a base attribute named UserPolicy Fiar. Done. https://codereview.chromium.org/1376383003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:37: return false; On 2015/10/07 21:58:36, Dan Beam wrote: > nit: \n between conditionals Done.
https://codereview.chromium.org/1376383003/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_indicator.js (right): https://codereview.chromium.org/1376383003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_indicator.js:50: var indicatorType = this.indicatorType = CrPolicyIndicatorType.NONE; why is " = this.indicatorType" here? https://codereview.chromium.org/1376383003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_indicator.js:50: var indicatorType = this.indicatorType = CrPolicyIndicatorType.NONE; does this variable actually save you any lines compared with just setting this.indicatorType directly in any of the conditionals? https://codereview.chromium.org/1376383003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_indicator.js:65: } } else { this.indicatorType = CrPolicyIndicatorType.NONE; } to avoid changing the template multiple times https://codereview.chromium.org/1376383003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_indicator.js:85: return this.i18n('controlledSettingRecommendedDiffers'); rebase? https://codereview.chromium.org/1376383003/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/network/cr_onc_types.js (right): https://codereview.chromium.org/1376383003/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/network/cr_onc_types.js:46: CrOnc.NetworkProperty; a @typedef of @typedefs? we must go deeper https://codereview.chromium.org/1376383003/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js (right): https://codereview.chromium.org/1376383003/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:29: return false; return !!(property.UserPolicy || property.DevicePolicy); IMO https://codereview.chromium.org/1376383003/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:41: if (property.hasOwnProperty('UserEditable')) in the future, it may be more readable to do: if (typeof property.UserEditable != 'undefined') return !property.UserEditable; if (typeof property.DeviceEditable != 'undefined') return !property.DeviceEditable; https://codereview.chromium.org/1376383003/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:49: return true; can all this be: return property.UserEditable === false || property.DeviceEditable === false; ? https://codereview.chromium.org/1376383003/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_indicator.html (right): https://codereview.chromium.org/1376383003/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_indicator.html:14: icon="[[getIcon(indicatorType)]]" do you need to rebase?
https://codereview.chromium.org/1376383003/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_indicator.js (right): https://codereview.chromium.org/1376383003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_indicator.js:50: var indicatorType = this.indicatorType = CrPolicyIndicatorType.NONE; On 2015/10/08 00:23:19, Dan Beam wrote: > why is " = this.indicatorType" here? merge bug https://codereview.chromium.org/1376383003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_indicator.js:65: } On 2015/10/08 00:23:19, Dan Beam wrote: > } else { > this.indicatorType = CrPolicyIndicatorType.NONE; > } > > to avoid changing the template multiple times Yep. Done. https://codereview.chromium.org/1376383003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_indicator.js:85: return this.i18n('controlledSettingRecommendedDiffers'); On 2015/10/08 00:23:19, Dan Beam wrote: > rebase? Done. https://codereview.chromium.org/1376383003/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/network/cr_onc_types.js (right): https://codereview.chromium.org/1376383003/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/network/cr_onc_types.js:46: CrOnc.NetworkProperty; On 2015/10/08 00:23:19, Dan Beam wrote: > a @typedef of @typedefs? we must go deeper Uhm... yes it is. I don't understand this comment... dan, this is cryptic even for you :) If your question is "why do we need this?" the short answer is "we don't, yet", so I'll remove it from this CL and restore it when we need it with context. https://codereview.chromium.org/1376383003/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js (right): https://codereview.chromium.org/1376383003/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:29: return false; On 2015/10/08 00:23:19, Dan Beam wrote: > return !!(property.UserPolicy || property.DevicePolicy); > > IMO I wrote it this way so that I could comment the code. I find it kind of subtle and confusing and I wrote it. https://codereview.chromium.org/1376383003/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:41: if (property.hasOwnProperty('UserEditable')) On 2015/10/08 00:23:19, Dan Beam wrote: > in the future, it may be more readable to do: > > if (typeof property.UserEditable != 'undefined') > return !property.UserEditable; > > if (typeof property.DeviceEditable != 'undefined') > return !property.DeviceEditable; Any reason not to do it in the present? I will assume not and make the change. https://codereview.chromium.org/1376383003/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:49: return true; On 2015/10/08 00:23:19, Dan Beam wrote: > can all this be: > > return property.UserEditable === false || property.DeviceEditable === false; > > ? Not quite. if property.UserEditable === true we want to return false regardless of the value of property.DeviceEditable. (User policy, if set, overrides device policy). If we wanted to combine this into a single statement and somehow squash the comments together, it would look like: return property.UserEditable === false || (property.UserEditable !== true && property.DeviceEditable === false) Personally I would find that more head-scratchy to read. https://codereview.chromium.org/1376383003/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_indicator.html (right): https://codereview.chromium.org/1376383003/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_indicator.html:14: icon="[[getIcon(indicatorType)]]" On 2015/10/08 00:23:19, Dan Beam wrote: > do you need to rebase? I've been in rebase hell the past few days. If this is my only merge bug I'll be pleased. Thanks for catching it. Fixed. https://codereview.chromium.org/1376383003/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_indicator.html:15: title$="[[getTooltip_(indicatorType, property, recommended)]]"> Fixed.
lgtm https://codereview.chromium.org/1376383003/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js (right): https://codereview.chromium.org/1376383003/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:29: return false; On 2015/10/08 20:13:00, stevenjb wrote: > On 2015/10/08 00:23:19, Dan Beam wrote: > > return !!(property.UserPolicy || property.DevicePolicy); > > > > IMO > > I wrote it this way so that I could comment the code. I find it kind of subtle > and confusing and I wrote it. Acknowledged. https://codereview.chromium.org/1376383003/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:49: return true; On 2015/10/08 20:13:00, stevenjb wrote: > On 2015/10/08 00:23:19, Dan Beam wrote: > > can all this be: > > > > return property.UserEditable === false || property.DeviceEditable === false; > > > > ? > > Not quite. if property.UserEditable === true we want to return false regardless > of the value of property.DeviceEditable. (User policy, if set, overrides device > policy). > > If we wanted to combine this into a single statement and somehow squash the > comments together, it would look like: > > return property.UserEditable === false || (property.UserEditable !== true && > property.DeviceEditable === false) > > Personally I would find that more head-scratchy to read. > Acknowledged.
Do we show the same icon for "this matches the recommended value" and "this does NOT match the recommended value"? is that the right UI? https://codereview.chromium.org/1376383003/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js (right): https://codereview.chromium.org/1376383003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:13: * @return {boolean} True if the network propety is controlled by a policy property https://codereview.chromium.org/1376383003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:34: * @return {boolean} True if the network propety is controlled by a policy. property https://codereview.chromium.org/1376383003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:34: * @return {boolean} True if the network propety is controlled by a policy. this is the same comment isNetworkPolicyControlled has. something something enforced something something recommended? https://codereview.chromium.org/1376383003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:36: isNetworkPolicyEnforced: function(property) { unused? https://codereview.chromium.org/1376383003/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_indicator.js (right): https://codereview.chromium.org/1376383003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_indicator.js:50: if (property.hasOwnProperty('UserPolicy') && According to the .idl, UserEditable === true implies property.hasOwnProperty('UserPolicy'). Same for Device. https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... Just checking UserEditable or DeviceEditable would make this much clearer to me. We already know there's some policy, so there should only be two questions -- does it come from user or device, and is it recommended or mandatory? Checking 6 different conditions makes that less clear. https://codereview.chromium.org/1376383003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_indicator.js:85: return this.i18n_('controlledSettingRecommendedDiffers'); ugh, we show the same icon for "this matches the recommended value" and "this does NOT match the recommended value"?
https://codereview.chromium.org/1376383003/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js (right): https://codereview.chromium.org/1376383003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:36: isNetworkPolicyEnforced: function(property) { On 2015/10/10 05:49:56, michaelpg (OOO til 19th) wrote: > unused? disregard. https://codereview.chromium.org/1390073006/
https://codereview.chromium.org/1376383003/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js (right): https://codereview.chromium.org/1376383003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:13: * @return {boolean} True if the network propety is controlled by a policy On 2015/10/10 05:49:56, michaelpg (OOO til 19th) wrote: > property Done. https://codereview.chromium.org/1376383003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:34: * @return {boolean} True if the network propety is controlled by a policy. On 2015/10/10 05:49:56, michaelpg (OOO til 19th) wrote: > this is the same comment isNetworkPolicyControlled has. something something > enforced something something recommended? Done. https://codereview.chromium.org/1376383003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:34: * @return {boolean} True if the network propety is controlled by a policy. On 2015/10/10 05:49:56, michaelpg (OOO til 19th) wrote: > property Done. https://codereview.chromium.org/1376383003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:36: isNetworkPolicyEnforced: function(property) { On 2015/10/10 05:49:56, michaelpg (OOO til 19th) wrote: > unused? Acknowledged. https://codereview.chromium.org/1376383003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_behavior.js:36: isNetworkPolicyEnforced: function(property) { On 2015/10/10 05:53:38, michaelpg (OOO til 19th) wrote: > On 2015/10/10 05:49:56, michaelpg (OOO til 19th) wrote: > > unused? > > disregard. https://codereview.chromium.org/1390073006/ Acknowledged. https://codereview.chromium.org/1376383003/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_indicator.js (right): https://codereview.chromium.org/1376383003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_indicator.js:50: if (property.hasOwnProperty('UserPolicy') && On 2015/10/10 05:49:56, michaelpg (OOO til 19th) wrote: > According to the .idl, UserEditable === true implies > property.hasOwnProperty('UserPolicy'). Same for Device. > > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... > > Just checking UserEditable or DeviceEditable would make this much clearer to me. > We already know there's some policy, so there should only be two questions -- > does it come from user or device, and is it recommended or mandatory? Checking 6 > different conditions makes that less clear. I acknowledge that this code is confusing. However optional booleans are kind of subtle, and it would be easy for an implementation to unwittingly set UserEditable to true in the absence of a user policy, in which case we would want to ignore it, so I would prefer to leave this as-is. I will reverse the order and add a comment to help clarify. https://codereview.chromium.org/1376383003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/v1_0/policy/cr_policy_network_indicator.js:85: return this.i18n_('controlledSettingRecommendedDiffers'); On 2015/10/10 05:49:56, michaelpg (OOO til 19th) wrote: > ugh, we show the same icon for "this matches the recommended value" and "this > does NOT match the recommended value"? Currently. Improving that is on the TODO list.
michaelpg@ - PTAL (Apologies for the lack of deltas from the previous CL, this got rebased after removing the v1_0 subdirectory)
lgtm https://codereview.chromium.org/1376383003/diff/120001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/policy/cr_policy_network_indicator.js (right): https://codereview.chromium.org/1376383003/diff/120001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/policy/cr_policy_network_indicator.js:64: } else if (effective == 'DevicePolicy') { extra space before ==
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1376383003/#ps140001 (title: "Rebase + fix WS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1376383003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1376383003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/0022774fb65e2ca266e2dc6f81e4c448edf53d85 Cr-Commit-Position: refs/heads/master@{#355375} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
