Add types to networkigPrivate IDL

Add types to networkigPrivate IDL

This CL just adds the new types to the networkingPrivate IDL.
These types will be used in the JS to allow type checking with the
closure compiler. We do not intend to use them directly in the C++ code
at this point in time; they are intended to match (a subset of) the
chrome ONC types.

Once these types are added to the IDL, we can add them to (and update)
chrome_extensions.js, pull that into the chrome repro, then use them
in the JS code.

BUG=470262
Committed: https://crrev.com/96e860c75affc683c84dfa8bfb7849b82647ed44
Cr-Commit-Position: refs/heads/master@{#322872}

[stevenjb, 2015-03-26 18:34:02]

stevenjb@chromium.org changed reviewers:
+ asargent@chromium.org, pneubeck@chromium.org

[stevenjb, 2015-03-26 18:34:03]

This is based on https://codereview.chromium.org/1038873004/ and just introduces
types to networking_private.idl.

[asargent_no_longer_on_chrome, 2015-03-26 20:05:40]

typo in CL description: "networkigPrivate" 

otherwise lgtm w/ one nit

https://codereview.chromium.org/1036033002/diff/1/extensions/common/api/netwo...
File extensions/common/api/networking_private.idl (right):

https://codereview.chromium.org/1036033002/diff/1/extensions/common/api/netwo...
extensions/common/api/networking_private.idl:126: 
nit: usually the names for members of a dictionary are capitalized fooBar (eg do
not have the first letter capitalized) - do they need to have the first letter
capitalized for compatibility with the existing API? If not, please change.

[stevenjb, 2015-03-27 17:03:22]

https://codereview.chromium.org/1036033002/diff/1/extensions/common/api/netwo...
File extensions/common/api/networking_private.idl (right):

https://codereview.chromium.org/1036033002/diff/1/extensions/common/api/netwo...
extensions/common/api/networking_private.idl:126: 
On 2015/03/26 20:05:40, Antony Sargent wrote:
> nit: usually the names for members of a dictionary are capitalized fooBar (eg
do
> not have the first letter capitalized) - do they need to have the first letter
> capitalized for compatibility with the existing API? If not, please change. 
> 

These names need to match the ONC spec. I will clarify that in the top level
comment.

[stevenjb, 2015-03-27 18:40:57]

Fixed some types.

[stevenjb, 2015-03-27 18:44:46]

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

[stevenjb, 2015-03-27 18:44:47]

+michaelpg@ - can you take a look at the test.js changes? The JS for that is
pretty crusty, so the formatting is not entirely clang compliant, but check for
any obvious problems. Thanks!

[pneubeck (no reviews), 2015-03-27 19:13:44]

lgtm with some comments

https://codereview.chromium.org/1036033002/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/networking_private/chromeos/test.js
(right):

https://codereview.chromium.org/1036033002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:188:
Type: chrome.networkingPrivate.NetworkType.WiFi,
as in some other place where you did that already, you could define
'NetworkType' as a short-cut at the top of the file.

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
File extensions/common/api/networking_private.idl (right):

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:11: //
http://www.chromium.org/chromium-os/chromiumos-design-docs/open-network-confi...
please check what the notation for urls was

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:16: // "State" properties describe
just the ONC properties returned by
an alternative naming scheme to StateProperties vs ConfigProperties
could be simply 'State' and 'Config'
like WiFiState and WiFiConfig

if it's used for both, maybe WiFiProperties

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:19: // "Config" properties describe
just the ONC properties that can be configured
we'll really have to add the dictionary inheritance to the IDL parser, otherwise
this is not maintainable/scalable:

every Config dictionary will copy all members of the according State dictionary.
Hopefully IDL allows concretizing the type of a member in the derived
dictionary, otherwise this will conflict with the parallel hierarchy of State
and Config.

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:20: // through this API. NOTE: Not
all configuration propertes are exposed at this
propertes -> properties

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:49: dictionary APNProperties {
neither Config* nor StateProperties
but this is maybe on purpose.

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:66: boolean? OutOfCredits;
can't find this in onc_signature.cc nor onc_spec

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:72: DOMString? Authentication;
mandatory

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:75: dictionary IPConfigProperties {
neither Config* nor StateProperties
but this is maybe on purpose.

(would have to be called IPConfigConfigProperties, which is very weird)

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:84: dictionary IPSecProperties {
neither Config* nor StateProperties
but this is maybe on purpose.

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:85: DOMString? AuthenticationType;
mandatory in state

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:88: dictionary
ThirdPartyVPNProperties {
neither Config* nor StateProperties
but this is maybe on purpose.

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:100: DOMString? Type;
mandatory

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:108: DOMString? HiddenSSID;
must be bool not string

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:115: DOMString? Security;
mandatory

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:116: long? SignalStrength;
wasn't this always set, i.e. 0 for not-visible
then it should be mandatory

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:124: long? SignalStrength;
same as for wifi

[stevenjb, 2015-03-27 22:11:15]

https://codereview.chromium.org/1036033002/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/networking_private/chromeos/test.js
(right):

https://codereview.chromium.org/1036033002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:188:
Type: chrome.networkingPrivate.NetworkType.WiFi,
On 2015/03/27 19:13:43, pneubeck wrote:
> as in some other place where you did that already, you could define
> 'NetworkType' as a short-cut at the top of the file.

Done.

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
File extensions/common/api/networking_private.idl (right):

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:11: //
http://www.chromium.org/chromium-os/chromiumos-design-docs/open-network-confi...
On 2015/03/27 19:13:43, pneubeck wrote:
> please check what the notation for urls was

Done.

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:16: // "State" properties describe
just the ONC properties returned by
On 2015/03/27 19:13:43, pneubeck wrote:
> an alternative naming scheme to StateProperties vs ConfigProperties
> could be simply 'State' and 'Config'
> like WiFiState and WiFiConfig
> 
> if it's used for both, maybe WiFiProperties

I think I prefer the 'Properties'. It makes it a little more explicit in the JS
what is an enum and what is a dictionary.

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:19: // "Config" properties describe
just the ONC properties that can be configured
On 2015/03/27 19:13:43, pneubeck wrote:
> we'll really have to add the dictionary inheritance to the IDL parser,
otherwise
> this is not maintainable/scalable:
> 
> every Config dictionary will copy all members of the according State
dictionary.
> Hopefully IDL allows concretizing the type of a member in the derived
> dictionary, otherwise this will conflict with the parallel hierarchy of State
> and Config.

We should discuss this at some point, but it fact the Config and State
properties don't have a lot of overlap. It's the "Complete" property sets (i.e.
everything returned by Get{Managed}Properties, currently just an object) that
are a superset of the two. (Note: In the C++ code, "State" == "Complete", where
as here it refers to the result of getState. This is unfortunate, but I'm not
sure what to do about that).

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:20: // through this API. NOTE: Not
all configuration propertes are exposed at this
On 2015/03/27 19:13:43, pneubeck wrote:
> propertes -> properties

Done.

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:49: dictionary APNProperties {
On 2015/03/27 19:13:43, pneubeck wrote:
> neither Config* nor StateProperties
> but this is maybe on purpose.

For sub-dictionaries that can be used in either I didn't specify.

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:66: boolean? OutOfCredits;
On 2015/03/27 19:13:43, pneubeck wrote:
> can't find this in onc_signature.cc nor onc_spec

Looks like we don't translate it. I think we used to use to determine whether or
not to show a "carrier info" button, but I simplified that (ridiculously
complex) logic at one point to err on the side of just showing the button.
Removed.

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:72: DOMString? Authentication;
On 2015/03/27 19:13:44, pneubeck wrote:
> mandatory

Done.

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:75: dictionary IPConfigProperties {
On 2015/03/27 19:13:43, pneubeck wrote:
> neither Config* nor StateProperties
> but this is maybe on purpose.
> 
> (would have to be called IPConfigConfigProperties, which is very weird)

Yeah, "IPConfig" is unfortunately named. I think we just live with it, even
though it is used for both State and Config.

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:84: dictionary IPSecProperties {
On 2015/03/27 19:13:44, pneubeck wrote:
> neither Config* nor StateProperties
> but this is maybe on purpose.

Also on purpose.

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:85: DOMString? AuthenticationType;
On 2015/03/27 19:13:44, pneubeck wrote:
> mandatory in state

Done.

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:88: dictionary
ThirdPartyVPNProperties {
On 2015/03/27 19:13:44, pneubeck wrote:
> neither Config* nor StateProperties
> but this is maybe on purpose.

Correct.

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:100: DOMString? Type;
On 2015/03/27 19:13:43, pneubeck wrote:
> mandatory

Done.

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:108: DOMString? HiddenSSID;
On 2015/03/27 19:13:43, pneubeck wrote:
> must be bool not string

Oops. Done.

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:115: DOMString? Security;
On 2015/03/27 19:13:43, pneubeck wrote:
> mandatory

Done.

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:116: long? SignalStrength;
On 2015/03/27 19:13:44, pneubeck wrote:
> wasn't this always set, i.e. 0 for not-visible
> then it should be mandatory

Actually, for non-visible networks we don't provide SignalStrength at all.

https://codereview.chromium.org/1036033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:124: long? SignalStrength;
On 2015/03/27 19:13:44, pneubeck wrote:
> same as for wifi

Ditto.

[stevenjb, 2015-03-27 22:11:25]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2015-03-27 22:11:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
asargent@chromium.org, pneubeck@chromium.org
Link to the patchset: https://codereview.chromium.org/1036033002/#ps60001
(title: "Rebase + Feedback")

[commit-bot: I haz the power, 2015-03-27 22:11:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1036033002/60001

[michaelpg, 2015-03-27 22:54:19]

lgtm

https://codereview.chromium.org/1036033002/diff/60001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/networking_private/chromeos/test.js
(right):

https://codereview.chromium.org/1036033002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:159:
configured : true
no space before colon

https://codereview.chromium.org/1036033002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:162:
networks) {
wonky indent

https://codereview.chromium.org/1036033002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:279: },
 {
extra space

https://codereview.chromium.org/1036033002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:340: }
], result);
extra space

https://codereview.chromium.org/1036033002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:397:
chrome.networkingPrivate.ActivationStateType.NotActivated,
use ActivationStateType from header

https://codereview.chromium.org/1036033002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:430:
Active: NetworkType.WiFi,
I don't understand why Type is an object instead of an enum, but I presume it's
correct, and having Effective be a string instead of an enum is also fine.
Yes/no?

https://codereview.chromium.org/1036033002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:445:
FrequencyList : [2400, 5000],
no space before colon, 444 and 445

[stevenjb, 2015-03-27 23:16:03]

https://codereview.chromium.org/1036033002/diff/60001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/networking_private/chromeos/test.js
(right):

https://codereview.chromium.org/1036033002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:159:
configured : true
On 2015/03/27 22:54:19, michaelpg wrote:
> no space before colon

Fixed in next CL

https://codereview.chromium.org/1036033002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:162:
networks) {
On 2015/03/27 22:54:18, michaelpg wrote:
> wonky indent

That's actually clang approved for just the line. I'll clang format the entire
function in the next CL, it looks better that way.

https://codereview.chromium.org/1036033002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:279: },
 {
On 2015/03/27 22:54:19, michaelpg wrote:
> extra space

Done.

https://codereview.chromium.org/1036033002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:340: }
], result);
On 2015/03/27 22:54:19, michaelpg wrote:
> extra space

Done.

https://codereview.chromium.org/1036033002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:397:
chrome.networkingPrivate.ActivationStateType.NotActivated,
On 2015/03/27 22:54:19, michaelpg wrote:
> use ActivationStateType from header

Done in next CL

https://codereview.chromium.org/1036033002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:430:
Active: NetworkType.WiFi,
On 2015/03/27 22:54:19, michaelpg wrote:
> I don't understand why Type is an object instead of an enum, but I presume
it's
> correct, and having Effective be a string instead of an enum is also fine.
> Yes/no?

The managed properties are funky. Every property may become an object with
'Active' and 'Effective' values (as well as
{User|Device}{Policy|Editable|Setting}). I've no clue if or how we might type
that correctly in the IDL.

https://codereview.chromium.org/1036033002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:445:
FrequencyList : [2400, 5000],
On 2015/03/27 22:54:19, michaelpg wrote:
> no space before colon, 444 and 445

Done in next CL.

[commit-bot: I haz the power, 2015-03-27 23:59:45]

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

[commit-bot: I haz the power, 2015-03-27 23:59:46]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[stevenjb, 2015-03-30 20:28:59]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2015-03-30 20:28:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
asargent@chromium.org, pneubeck@chromium.org, michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/1036033002/#ps80001
(title: "Fix apitest, test.js nits")

[commit-bot: I haz the power, 2015-03-30 20:29:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1036033002/80001

[commit-bot: I haz the power, 2015-03-30 21:24:56]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-30 21:26:09]

Patchset 5 (id:??) landed as
https://crrev.com/96e860c75affc683c84dfa8bfb7849b82647ed44
Cr-Commit-Position: refs/heads/master@{#322872}