Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
509643003:
Use GetManagedProperties in InternetOptionsHandler (Closed)
|
Created:
6 years, 3 months ago by stevenjb Modified:
6 years, 3 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, stevenjb+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@issue_279351_internet_options_8a Project:
chromium Visibility:
Public. |
DescriptionUse GetManagedProperties in InternetOptionsHandler
This CL does the following:
* Eliminates auto connect glue code
* Elim SetManaged* helpers
* Converts StaticIPConfig, SavedIPConfig, and WebProxyAutoDiscoveryUrl
properties from Shill to ONC
* Uses ManagedNetworkConfigurationHandler::GetManagedProperties in
internet_options_handler.cc to get (almost) all properites. The
remaining properties will require additional translation and/or design
BUG=279351
Committed: https://crrev.com/9f86a5419435e3a4e89fadbc6f0caa68035ea30b
Cr-Commit-Position: refs/heads/master@{#293816}
Patch Set 1 #
Total comments: 25
Patch Set 2 : Feedback #
Total comments: 8
Patch Set 3 : Rebase with IPConfig translation in tree #Patch Set 4 : Merge + logic fixes #Patch Set 5 : Rebase, Clarify PrefixLengthToNetmask #
Messages
Total messages: 13 (1 generated)
stevenjb@chromium.org changed reviewers: + armansito@chromium.org, michaelpg@chromium.org, pneubeck@chromium.org
OK, this is the big switch to using GetManagedNetworkProperties. There is still a lot of work to do, but this is a huge step towards making the settings all ONC based instead of Shill based.
I would add unit tests for the new fields that are being translated.
If my suggestions to the ONC part are confusing, I can also extract that part into a separate CL that I could complete. As you like. You might consider separating the change 'GetProperties -> GetManagedProperties' into its own CL to ease a potential search for bugs. https://codereview.chromium.org/509643003/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/509643003/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/chromeos/internet_detail.js:1165: if (staticIpconfig && Object.keys(staticIpconfig).length > 0) maybe add a comment that explains what the intention of the second check is. https://codereview.chromium.org/509643003/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/chromeos/internet_detail.js:1167: $('ip-automatic-configuration-checkbox').checked = ipAutoConfig; before this was assigned a boolean. Not sure what happens when you assign a string as you do now. maybe this should be ...checked = ipAutoConfig == 'automatic'; ? https://codereview.chromium.org/509643003/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/chromeos/internet_detail.js:1226: if (ipAutoConfig == 'user' && 'StaticIPConfig' in data) { optional nit: second check is redundant. ipAutoConfig is only set to 'user' if staticIpConfig is available. https://codereview.chromium.org/509643003/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/chromeos/internet_detail.js:1244: if (staticNameServers == inetNameServers) note that this comparison fails if the order of the nameservers differs, which might not be the intended behavior. https://codereview.chromium.org/509643003/diff/1/chromeos/network/onc/onc_tra... File chromeos/network/onc/onc_translation_tables.cc (right): https://codereview.chromium.org/509643003/diff/1/chromeos/network/onc/onc_tra... chromeos/network/onc/onc_translation_tables.cc:191: { ::onc::ipconfig::kWebProxyAutoDiscoveryUrl, add this only to the saved_ipconfig_fields array as mentioned in my comment below https://codereview.chromium.org/509643003/diff/1/chromeos/network/onc/onc_tra... File chromeos/network/onc/onc_translator_shill_to_onc.cc (right): https://codereview.chromium.org/509643003/diff/1/chromeos/network/onc/onc_tra... chromeos/network/onc/onc_translator_shill_to_onc.cc:119: // |dictionary|. nit: mention that it copies to the key |onc_property_name| (called onc_field_name in all other comments) https://codereview.chromium.org/509643003/diff/1/chromeos/network/onc/onc_tra... chromeos/network/onc/onc_translator_shill_to_onc.cc:428: scoped_ptr<base::DictionaryValue> static_ip(new base::DictionaryValue); I think if you can save most of this code by supporting both translation directions by adding the right signature and entries to the translation tables: Add two new signatures that point to the base IPConfig signature (these signatures inherit all members from kIPConfigSignature): const OncValueSignature kStaticIPConfigSignature = { base::Value::TYPE_DICTIONARY, NULL, NULL, &kIPConfigSignature }; const OncValueSignature kSavedIPConfigSignature = { base::Value::TYPE_DICTIONARY, NULL, NULL, &kIPConfigSignature }; ... network_configuration_fields[] = { ... { ::onc::network_config::kSavedIPConfig, &kSavedIPConfigSignature}, { ::onc::network_config::kStaticIPConfig, &kStaticIPConfigSignature}, ... }; And for each signature add a translation table: const FieldTranslationEntry static_ipconfig_fields[] = { { ::onc::ipconfig::kIPAddress, shill::kStaticIPAddressProperty}, { ::onc::ipconfig::kGateway, shill::kStaticIPGatewayProperty}, { ::onc::ipconfig::kRoutingPrefix, shill::kStaticIPPrefixlenProperty}, { ::onc::ipconfig::kNameServers, shill::kStaticIPNameServersProperty} }; const FieldTranslationEntry saved_ipconfig_fields[] = { ... }; const OncValueTranslationEntry onc_value_translation_table[] = { ... { &kSavedIPConfigSignature, saved_ipconfig_fields }, { &kStaticIPConfigSignature, static_ipconfig_fields }, ... }; To trigger the translation of these nested objects, add here the calls TranslateAndAddNestedObject(::onc::network_config::kSavedIPConfig) TranslateAndAddNestedObject(::onc::network_config::kStaticIPConfig) The new function CopyPropertyToDictionary will not be necessary anymore and you get the other direction of translation at the same time. https://codereview.chromium.org/509643003/diff/1/components/onc/docs/onc_spec... File components/onc/docs/onc_spec.html (right): https://codereview.chromium.org/509643003/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:266: (optional) this could/should be the original (optional if <span class="field">Remove</span> is ... otherwise ignored) https://codereview.chromium.org/509643003/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:269: IPConfig property that overrides IPConfig parameters received over DHCP. nit: maybe add 'individual': ... overrides individual parameters ... ? to make clear that it is not a all or nothing override. https://codereview.chromium.org/509643003/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:275: (optional, read-only) is it limited to connected networks like 'IPConfigs' above? https://codereview.chromium.org/509643003/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:278: IPConfig property containing the configuration that was recieved from the typo: recieved -> received https://codereview.chromium.org/509643003/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:568: (optional, read-only) 'read-only' -> optional if part of <...>SavedIPConfig<...> not need to mention read-only as this should be inherited from SavedIPConfig.
https://codereview.chromium.org/509643003/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/509643003/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/chromeos/internet_detail.js:1165: if (staticIpconfig && Object.keys(staticIpconfig).length > 0) On 2014/09/03 15:14:28, pneubeck wrote: > maybe add a comment that explains what the intention of the second check is. Done. https://codereview.chromium.org/509643003/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/chromeos/internet_detail.js:1167: $('ip-automatic-configuration-checkbox').checked = ipAutoConfig; On 2014/09/03 15:14:28, pneubeck wrote: > before this was assigned a boolean. Not sure what happens when you assign a > string as you do now. > > maybe this should be > ...checked = ipAutoConfig == 'automatic'; > ? Good catch. Done. https://codereview.chromium.org/509643003/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/chromeos/internet_detail.js:1226: if (ipAutoConfig == 'user' && 'StaticIPConfig' in data) { On 2014/09/03 15:14:28, pneubeck wrote: > optional nit: > second check is redundant. ipAutoConfig is only set to 'user' if staticIpConfig > is available. Done. https://codereview.chromium.org/509643003/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/chromeos/internet_detail.js:1244: if (staticNameServers == inetNameServers) On 2014/09/03 15:14:28, pneubeck wrote: > note that this comparison fails if the order of the nameservers differs, which > might not be the intended behavior. Hmm. This should be the same as the existing C++ logic. I don't know enough about how these are used to know whether or not we should file that as a separate issue. My (naturally lazy) inclination is to wait until/unless it come sup - we have no shortage of tasks... https://codereview.chromium.org/509643003/diff/1/chromeos/network/onc/onc_tra... File chromeos/network/onc/onc_translation_tables.cc (right): https://codereview.chromium.org/509643003/diff/1/chromeos/network/onc/onc_tra... chromeos/network/onc/onc_translation_tables.cc:191: { ::onc::ipconfig::kWebProxyAutoDiscoveryUrl, On 2014/09/03 15:14:28, pneubeck wrote: > add this only to the saved_ipconfig_fields array as mentioned in my comment > below Acknowledged. https://codereview.chromium.org/509643003/diff/1/chromeos/network/onc/onc_tra... File chromeos/network/onc/onc_translator_shill_to_onc.cc (right): https://codereview.chromium.org/509643003/diff/1/chromeos/network/onc/onc_tra... chromeos/network/onc/onc_translator_shill_to_onc.cc:119: // |dictionary|. On 2014/09/03 15:14:29, pneubeck wrote: > nit: mention that it copies to the key |onc_property_name| (called > onc_field_name in all other comments) Done, and renamed -> onc_field_name for consistency. https://codereview.chromium.org/509643003/diff/1/chromeos/network/onc/onc_tra... chromeos/network/onc/onc_translator_shill_to_onc.cc:428: scoped_ptr<base::DictionaryValue> static_ip(new base::DictionaryValue); On 2014/09/03 15:14:28, pneubeck wrote: > I think if you can save most of this code by supporting both translation > directions by adding the right signature and entries to the translation tables: > > Add two new signatures that point to the base IPConfig signature (these > signatures inherit all members from kIPConfigSignature): > > const OncValueSignature kStaticIPConfigSignature = { > base::Value::TYPE_DICTIONARY, NULL, NULL, &kIPConfigSignature }; > > const OncValueSignature kSavedIPConfigSignature = { > base::Value::TYPE_DICTIONARY, NULL, NULL, &kIPConfigSignature }; > > ... network_configuration_fields[] = { > ... > { ::onc::network_config::kSavedIPConfig, &kSavedIPConfigSignature}, > { ::onc::network_config::kStaticIPConfig, &kStaticIPConfigSignature}, > ... > }; > > And for each signature add a translation table: > > const FieldTranslationEntry static_ipconfig_fields[] = { > { ::onc::ipconfig::kIPAddress, shill::kStaticIPAddressProperty}, > { ::onc::ipconfig::kGateway, shill::kStaticIPGatewayProperty}, > { ::onc::ipconfig::kRoutingPrefix, shill::kStaticIPPrefixlenProperty}, > { ::onc::ipconfig::kNameServers, shill::kStaticIPNameServersProperty} > }; > > const FieldTranslationEntry saved_ipconfig_fields[] = { > ... > }; > > const OncValueTranslationEntry onc_value_translation_table[] = { > ... > { &kSavedIPConfigSignature, saved_ipconfig_fields }, > { &kStaticIPConfigSignature, static_ipconfig_fields }, > ... > }; > > > To trigger the translation of these nested objects, add here the calls > TranslateAndAddNestedObject(::onc::network_config::kSavedIPConfig) > TranslateAndAddNestedObject(::onc::network_config::kStaticIPConfig) > > The new function CopyPropertyToDictionary will not be necessary anymore and you > get the other direction of translation at the same time. I'll... try doing this in a separate CL. I think I agree that sounds better, but it all still kind of makes my head hurt. https://codereview.chromium.org/509643003/diff/1/components/onc/docs/onc_spec... File components/onc/docs/onc_spec.html (right): https://codereview.chromium.org/509643003/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:266: (optional) On 2014/09/03 15:14:29, pneubeck wrote: > this could/should be the original > (optional if <span class="field">Remove</span> is ... otherwise ignored) Done. https://codereview.chromium.org/509643003/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:269: IPConfig property that overrides IPConfig parameters received over DHCP. On 2014/09/03 15:14:29, pneubeck wrote: > nit: maybe add 'individual': > ... overrides individual parameters ... > ? > > to make clear that it is not a all or nothing override. Done. https://codereview.chromium.org/509643003/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:275: (optional, read-only) On 2014/09/03 15:14:29, pneubeck wrote: > is it limited to connected networks like 'IPConfigs' above? Presumably. Updated comment. https://codereview.chromium.org/509643003/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:568: (optional, read-only) On 2014/09/03 15:14:29, pneubeck wrote: > 'read-only' -> optional if part of <...>SavedIPConfig<...> > > not need to mention read-only as this should be inherited from SavedIPConfig. Done.
https://codereview.chromium.org/509643003/diff/20001/chromeos/network/onc/onc... File chromeos/network/onc/onc_translator_shill_to_onc.cc (left): https://codereview.chromium.org/509643003/diff/20001/chromeos/network/onc/onc... chromeos/network/onc/onc_translator_shill_to_onc.cc:116: void CopyProperty(const OncFieldSignature* field_signature); Can you include the new fields below in the unit tests?
lgtm with nits https://codereview.chromium.org/509643003/diff/1/chromeos/network/onc/onc_tra... File chromeos/network/onc/onc_translator_shill_to_onc.cc (right): https://codereview.chromium.org/509643003/diff/1/chromeos/network/onc/onc_tra... chromeos/network/onc/onc_translator_shill_to_onc.cc:428: scoped_ptr<base::DictionaryValue> static_ip(new base::DictionaryValue); On 2014/09/03 21:28:06, stevenjb wrote: > On 2014/09/03 15:14:28, pneubeck wrote: > > I think if you can save most of this code by supporting both translation > > directions by adding the right signature and entries to the translation > tables: > > > > Add two new signatures that point to the base IPConfig signature (these > > signatures inherit all members from kIPConfigSignature): > > > > const OncValueSignature kStaticIPConfigSignature = { > > base::Value::TYPE_DICTIONARY, NULL, NULL, &kIPConfigSignature }; > > > > const OncValueSignature kSavedIPConfigSignature = { > > base::Value::TYPE_DICTIONARY, NULL, NULL, &kIPConfigSignature }; > > > > ... network_configuration_fields[] = { > > ... > > { ::onc::network_config::kSavedIPConfig, &kSavedIPConfigSignature}, > > { ::onc::network_config::kStaticIPConfig, &kStaticIPConfigSignature}, > > ... > > }; > > > > And for each signature add a translation table: > > > > const FieldTranslationEntry static_ipconfig_fields[] = { > > { ::onc::ipconfig::kIPAddress, shill::kStaticIPAddressProperty}, > > { ::onc::ipconfig::kGateway, shill::kStaticIPGatewayProperty}, > > { ::onc::ipconfig::kRoutingPrefix, shill::kStaticIPPrefixlenProperty}, > > { ::onc::ipconfig::kNameServers, shill::kStaticIPNameServersProperty} > > }; > > > > const FieldTranslationEntry saved_ipconfig_fields[] = { > > ... > > }; > > > > const OncValueTranslationEntry onc_value_translation_table[] = { > > ... > > { &kSavedIPConfigSignature, saved_ipconfig_fields }, > > { &kStaticIPConfigSignature, static_ipconfig_fields }, > > ... > > }; > > > > > > To trigger the translation of these nested objects, add here the calls > > TranslateAndAddNestedObject(::onc::network_config::kSavedIPConfig) > > TranslateAndAddNestedObject(::onc::network_config::kStaticIPConfig) > > > > The new function CopyPropertyToDictionary will not be necessary anymore and > you > > get the other direction of translation at the same time. > > I'll... try doing this in a separate CL. I think I agree that sounds better, but > it all still kind of makes my head hurt. Thanks https://codereview.chromium.org/509643003/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/509643003/diff/20001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:93: return netmask; nit: just return '' and move the variable declaration below https://codereview.chromium.org/509643003/diff/20001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:104: var value = nit: this term is already a bit long to grasp. maybe initialize with 0 and use an 'if' for the case 'remainder != 0'. 2 << (remainder -1) --> (1 << remainder) or Math.pow(2, remainder) maybe add a comment that you set here the 'remainder' most-significant bits to 1 and leave the less significant bits at 0.
https://codereview.chromium.org/509643003/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/509643003/diff/20001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:104: var value = On 2014/09/04 14:54:13, pneubeck wrote: > nit: this term is already a bit long to grasp. maybe initialize with 0 and use > an 'if' for the case 'remainder != 0'. > > 2 << (remainder -1) --> (1 << remainder) or Math.pow(2, remainder) > > maybe add a comment that you set here the 'remainder' most-significant bits to 1 > and leave the less significant bits at 0. While you're at it, can you also add a short comment explaining what this line's doing?
https://codereview.chromium.org/509643003/diff/1/components/onc/docs/onc_spec... File components/onc/docs/onc_spec.html (right): https://codereview.chromium.org/509643003/diff/1/components/onc/docs/onc_spec... components/onc/docs/onc_spec.html:278: IPConfig property containing the configuration that was recieved from the On 2014/09/03 15:14:29, pneubeck wrote: > typo: recieved -> received Done. https://codereview.chromium.org/509643003/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/internet_detail.js (right): https://codereview.chromium.org/509643003/diff/20001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:93: return netmask; On 2014/09/04 14:54:13, pneubeck wrote: > nit: just return '' and move the variable declaration below Done. https://codereview.chromium.org/509643003/diff/20001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:104: var value = On 2014/09/04 15:41:38, armansito wrote: > On 2014/09/04 14:54:13, pneubeck wrote: > > nit: this term is already a bit long to grasp. maybe initialize with 0 and use > > an 'if' for the case 'remainder != 0'. > > > > 2 << (remainder -1) --> (1 << remainder) or Math.pow(2, remainder) > > > > maybe add a comment that you set here the 'remainder' most-significant bits to > 1 > > and leave the less significant bits at 0. > > While you're at it, can you also add a short comment explaining what this line's > doing? I'm not really sure how to explain this better than the logic itself (I just copied the original logic from the C++ code which I did not write). https://codereview.chromium.org/509643003/diff/20001/chrome/browser/resources... chrome/browser/resources/options/chromeos/internet_detail.js:104: var value = On 2014/09/04 14:54:13, pneubeck wrote: > nit: this term is already a bit long to grasp. maybe initialize with 0 and use > an 'if' for the case 'remainder != 0'. > > 2 << (remainder -1) --> (1 << remainder) or Math.pow(2, remainder) > > maybe add a comment that you set here the 'remainder' most-significant bits to 1 > and leave the less significant bits at 0. I added an 'if' instead of a ?, but I don't want to change the logic from the C++ any, especially since we don't have a simple way of unit testing this that I am aware of (we probably need to address that soon). Also added 'toString()' to make this a little more clear. https://codereview.chromium.org/509643003/diff/20001/chromeos/network/onc/onc... File chromeos/network/onc/onc_translator_shill_to_onc.cc (left): https://codereview.chromium.org/509643003/diff/20001/chromeos/network/onc/onc... chromeos/network/onc/onc_translator_shill_to_onc.cc:116: void CopyProperty(const OncFieldSignature* field_signature); On 2014/09/03 21:45:02, armansito wrote: > Can you include the new fields below in the unit tests? Addressed in a separate CL.
The CQ bit was checked by stevenjb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevenjb@chromium.org/509643003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as f23bb6b9f055fcb5ff12c91343e8d068a8adb09a
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/9f86a5419435e3a4e89fadbc6f0caa68035ea30b Cr-Commit-Position: refs/heads/master@{#293816} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
