ONC: Translate EAP from Shill to ONC

ONC: Translate EAP from Shill to ONC

This CL adds support for additional WiFi.EAP fields and translates Shill properties to WiFi.EAP.Inner and WiFi.EAP.Outer.

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

Review-Url: https://codereview.chromium.org/2853563002
Cr-Commit-Position: refs/heads/master@{#468645}
Committed: https://chromium.googlesource.com/chromium/src/+/d66bcd45485740e2bf64e73cd6586b5d596d9f0d

[stevenjb, 2017-04-28 21:58:46]

Description was changed from

==========
ONC: Translate EAP from Shill to ONC (WIP)

BUG=380937
==========

to

==========
ONC: Translate EAP from Shill to ONC (WIP)

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

[stevenjb, 2017-04-28 22:46:07]

Description was changed from

==========
ONC: Translate EAP from Shill to ONC (WIP)

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

to

==========
ONC: Translate EAP from Shill to ONC

This CL adds support for additional WiFi.EAP fields and translates Shill
properties to WiFi.EAP.Inner and WiFi.EAP.Outer.

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

[stevenjb, 2017-04-28 22:46:19]

stevenjb@chromium.org changed reviewers:
+ emaxx@chromium.org, tbarzic@chromium.org

[tbarzic, 2017-05-01 18:05:56]

https://codereview.chromium.org/2853563002/diff/60001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2853563002/diff/60001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:1501: <message name="IDS_ONC_WIFI_EAP_INNER"
desc="ONC Property label for WiFi.EAP.Iner (Phase 2 authentication)">
WiFi.EAP.Inner

https://codereview.chromium.org/2853563002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_property_list.js
(right):

https://codereview.chromium.org/2853563002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_property_list.js:112:
var value = this.getPropertyValue_(key, this.prefix, this.propertyDict);
the rest of the private methods pass in prefix, propertyDict and editFieldTypes
as function parameters. Should that be the case here, too?

(I'd at least update computeFiter_ invocations in network_property_list.html to
match the method signature here).

https://codereview.chromium.org/2853563002/diff/60001/chromeos/network/onc/on...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/2853563002/diff/60001/chromeos/network/onc/on...
chromeos/network/onc/onc_translator_shill_to_onc.cc:372: std::string eap;
Could this be done as part of TranslateAndAddNestedObject(::onc::wifi::kEAP); so
the same does not have to be repeated for all occurrences of EAP property?

e.g. introduce
TranslateEAP() {
  CopyPropertiesAccordingToSignature();
  TranslateWithTableAndSet(shill::kEapMethodProperty, kEAPOuterTable,
::onc::eap::kOuter);
  TranslateWithTableAndSet(shill::kEapPhase2AuthProperty, kEAP_TTLS_InnerTable,
::onc::eap::kInner);
}

and add 
if (onc_signature_ == &kEAPSignature) {
  TranslateEAP();
}

to ShillToONCTranslator::CreateTranslatedONCObject

[stevenjb, 2017-05-01 20:12:04]

stevenjb@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[stevenjb, 2017-05-01 20:12:05]

PTAL

+rdevlin.cronin@ for idl changes

https://codereview.chromium.org/2853563002/diff/60001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2853563002/diff/60001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:1501: <message name="IDS_ONC_WIFI_EAP_INNER"
desc="ONC Property label for WiFi.EAP.Iner (Phase 2 authentication)">
On 2017/05/01 18:05:56, tbarzic wrote:
> WiFi.EAP.Inner

Done.

https://codereview.chromium.org/2853563002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_property_list.js
(right):

https://codereview.chromium.org/2853563002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_property_list.js:112:
var value = this.getPropertyValue_(key, this.prefix, this.propertyDict);
On 2017/05/01 18:05:56, tbarzic wrote:
> the rest of the private methods pass in prefix, propertyDict and
editFieldTypes
> as function parameters. Should that be the case here, too?
> 
> (I'd at least update computeFiter_ invocations in network_property_list.html
to
> match the method signature here).

I'm not sure what the relevance of "the rest of the private methods" is, but you
are correct that computeFilter should be passed prefix from the HTML, so I fixed
that.

https://codereview.chromium.org/2853563002/diff/60001/chromeos/network/onc/on...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/2853563002/diff/60001/chromeos/network/onc/on...
chromeos/network/onc/onc_translator_shill_to_onc.cc:372: std::string eap;
On 2017/05/01 18:05:56, tbarzic wrote:
> Could this be done as part of TranslateAndAddNestedObject(::onc::wifi::kEAP);
so
> the same does not have to be repeated for all occurrences of EAP property?
> 
> e.g. introduce
> TranslateEAP() {
>   CopyPropertiesAccordingToSignature();
>   TranslateWithTableAndSet(shill::kEapMethodProperty, kEAPOuterTable,
> ::onc::eap::kOuter);
>   TranslateWithTableAndSet(shill::kEapPhase2AuthProperty,
kEAP_TTLS_InnerTable,
> ::onc::eap::kInner);
> }
> 
> and add 
> if (onc_signature_ == &kEAPSignature) {
>   TranslateEAP();
> }
> 
> to ShillToONCTranslator::CreateTranslatedONCObject

Yes, thanks, I've been out of this code for too long, that's a much better
approach. Done.

[Devlin, 2017-05-01 20:57:20]

idls lgtm

[tbarzic, 2017-05-01 21:04:48]

lgtm

https://codereview.chromium.org/2853563002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_property_list.js
(right):

https://codereview.chromium.org/2853563002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_property_list.js:112:
var value = this.getPropertyValue_(key, this.prefix, this.propertyDict);
On 2017/05/01 20:12:04, stevenjb wrote:
> On 2017/05/01 18:05:56, tbarzic wrote:
> > the rest of the private methods pass in prefix, propertyDict and
> editFieldTypes
> > as function parameters. Should that be the case here, too?
> > 
> > (I'd at least update computeFiter_ invocations in network_property_list.html
> to
> > match the method signature here).
> 
> I'm not sure what the relevance of "the rest of the private methods" is, but
you
> are correct that computeFilter should be passed prefix from the HTML, so I
fixed
> that.

I mean, the relevance is consistency, e.g. isEditable_ takes arguments passed
from HTML, and uses those to get result, while computeFilter_ uses the network
list element properties directly (though, arguably, this is existing problem,
not one added with this cl).

If I'm reading the code correctly, this implementation ignores arguments passed
from HTML template.
So, I'd either change this to
computeFilter_ = function(prefix, propertyDict, editFieldTypes) {
  return function(key) {
     // same logic, but using [prefix, propertyDict, editFieldTypes] instead of
[this.prefix, this.propertyDict, this.editFieldTypes]
  }.bind(this);
}
or change invocation from network_property_list.html to
  filter="[[computeFilter_()]]"


(personally, I prefer the latter, but the former is more consistent with the
rest of this file).

[stevenjb, 2017-05-01 21:54:26]

https://codereview.chromium.org/2853563002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_property_list.js
(right):

https://codereview.chromium.org/2853563002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_property_list.js:112:
var value = this.getPropertyValue_(key, this.prefix, this.propertyDict);
On 2017/05/01 21:04:48, tbarzic wrote:
> On 2017/05/01 20:12:04, stevenjb wrote:
> > On 2017/05/01 18:05:56, tbarzic wrote:
> > > the rest of the private methods pass in prefix, propertyDict and
> > editFieldTypes
> > > as function parameters. Should that be the case here, too?
> > > 
> > > (I'd at least update computeFiter_ invocations in
network_property_list.html
> > to
> > > match the method signature here).
> > 
> > I'm not sure what the relevance of "the rest of the private methods" is, but
> you
> > are correct that computeFilter should be passed prefix from the HTML, so I
> fixed
> > that.
> 
> I mean, the relevance is consistency, e.g. isEditable_ takes arguments passed
> from HTML, and uses those to get result, while computeFilter_ uses the network
> list element properties directly (though, arguably, this is existing problem,
> not one added with this cl).
> 
> If I'm reading the code correctly, this implementation ignores arguments
passed
> from HTML template.
> So, I'd either change this to
> computeFilter_ = function(prefix, propertyDict, editFieldTypes) {
>   return function(key) {
>      // same logic, but using [prefix, propertyDict, editFieldTypes] instead
of
> [this.prefix, this.propertyDict, this.editFieldTypes]
>   }.bind(this);
> }
> or change invocation from network_property_list.html to
>   filter="[[computeFilter_()]]"
> 
> 
> (personally, I prefer the latter, but the former is more consistent with the
> rest of this file).

Oh, yeah, that's complicated.

*Generally* we prefer to use members explicitly in functions, i.e. this.prefix,
and provide them as parameters in the HTML for generating dependencies.

NOTE: Providing the parameters in the HTML is essential to create dependencies
(i.e. if |prefix| changes, be sure to call computeFilter_), even if we ignore
them in the method.

However, when -inside- a template, using members doesn't work reliably (the
member may still have the old value), so it is safer to pass members explicitly.

Since computeFilter_ is the only function in this pseudo-class that uses members
instead of parameters, I can see how it would be more clear if we also used
explicit parameters here, so I will do that.

(Sadly we do not have any type checking between the HTML and the JS, and there
are subtle problems with both approaches.)

[stevenjb, 2017-05-01 21:54:35]

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

[commit-bot: I haz the power, 2017-05-01 21:55:41]

Dry run: 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-05-01 23:35:02]

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

[commit-bot: I haz the power, 2017-05-01 23:35:03]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2017-05-01 23:45:08]

emaxx@ - PTAL if you have a chance, thanks!

[emaxx, 2017-05-02 14:24:35]

LGTM with a nit

https://codereview.chromium.org/2853563002/diff/100001/components/onc/docs/on...
File components/onc/docs/onc_spec.md (left):

https://codereview.chromium.org/2853563002/diff/100001/components/onc/docs/on...
components/onc/docs/onc_spec.md:1065: *EAP-MSCHAPv2*,
The "EAP-MSCHAPv2" constant is mentioned in histograms.xml. I guess that file
should be updated too?

[stevenjb, 2017-05-02 15:37:33]

https://codereview.chromium.org/2853563002/diff/100001/components/onc/docs/on...
File components/onc/docs/onc_spec.md (left):

https://codereview.chromium.org/2853563002/diff/100001/components/onc/docs/on...
components/onc/docs/onc_spec.md:1065: *EAP-MSCHAPv2*,
On 2017/05/02 14:24:35, emaxx wrote:
> The "EAP-MSCHAPv2" constant is mentioned in histograms.xml. I guess that file
> should be updated too?

Looking through the code, Shill supports this (kEapPhase2AuthTTLSEAPMSCHAPV2)
but Chrome (and therefore ONC) does not. The histogram is populated by Shill, so
we don't actually want to change it.

[stevenjb, 2017-05-02 15:37:41]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-05-02 15:37:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, tbarzic@chromium.org
Link to the patchset: https://codereview.chromium.org/2853563002/#ps100001
(title: "Use params")

[commit-bot: I haz the power, 2017-05-02 15:38:19]

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-05-02 15:43:09]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493739461314610,
"parent_rev": "0db7318a5466e8e235ce60f01a12f8d8779c14b0", "commit_rev":
"d66bcd45485740e2bf64e73cd6586b5d596d9f0d"}

[commit-bot: I haz the power, 2017-05-02 15:43:23]

Description was changed from

==========
ONC: Translate EAP from Shill to ONC

This CL adds support for additional WiFi.EAP fields and translates Shill
properties to WiFi.EAP.Inner and WiFi.EAP.Outer.

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

to

==========
ONC: Translate EAP from Shill to ONC

This CL adds support for additional WiFi.EAP fields and translates Shill
properties to WiFi.EAP.Inner and WiFi.EAP.Outer.

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

Review-Url: https://codereview.chromium.org/2853563002
Cr-Commit-Position: refs/heads/master@{#468645}
Committed:
https://chromium.googlesource.com/chromium/src/+/d66bcd45485740e2bf64e73cd658...
==========

[commit-bot: I haz the power, 2017-05-02 15:43:27]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/d66bcd45485740e2bf64e73cd658...