886110 - Convert WifiManager to WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[David Bruant]

When someone works on that, be aware that there is an outdated comment. In essence, the success value of getNetworks isn't an object key'd on SSIDs, but rather an array.

Source: https://groups.google.com/d/msg/mozilla.dev.webapi/SmnyimTe2Dk/jYXKQOVpFFcJ

Comment 2

[Reuben Morais [:reuben]]

Attachment: WIP

Up for grabs. I wrote most of this, then went to test it and found out there are no tests. Heh.

Comment 3

[Reuben Morais [:reuben]]

Attachment: WIP

Oops, attached before qref'ing.

Note that users of the mysterious connectionInfoUpdate event will need to be fixed.

Comment 4

[Masatoshi Kimura [:emk]]

Comment on attachment 767527 [details] [diff] [review]
WIP

::: dom/webidl/WifiManager.webidl
@@ -0,0 +1,179 @@
>+[JSImplementation="@mozilla.org/wifinetwork;1"]
>+interface WifiNetwork {

>+[JSImplementation="@mozilla.org/wificonnection;1"]
>+interface WifiConnection {

>+[JSImplementation="@mozilla.org/wificonnectioninfo;1"]
>+interface WifiConnectionInfo {

>+[JSImplementation="@mozilla.org/wifimanager;1",
>+ NavigatorProperty="mozWifiManager"]
>+interface WifiManager : EventTarget {

Will these interfaces always be defined on the global object?
Where is the spec for them? I couldn't find the spec on W3C nor WHATWG.
Please do not play around with the global object by sticking Mozilla-specific extensions (I have to say so).

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Masatoshi Kimura [:emk] from comment #4)
> Will these interfaces always be defined on the global object?

No, they're defined as navigator.mozWifiManger.

> Where is the spec for them? I couldn't find the spec on W3C nor WHATWG.

This is one of the APIs that's only visible to certified apps and I don't think anybody's gotten around to starting the standardization process for them. They're not visible to the web in general so I don't think that's been a super high priority to this point.

> Please do not play around with the global object by sticking
> Mozilla-specific extensions (I have to say so).

I second this. There's a reason that many of these new APIs are getting added to the navigator object instead of the global.

Comment 6

[Henry Chang [:hchang]]

Attachment: Reference patch (for WifiP2pManager)

Hi dimi: 

I attached a patch for your reference. Actually, the current WIP patch is the reference of my patch but it has some tiny errors. My patch for wifi p2p has proven working well even though it's still under review in bug 811365 :p Hope it helps!

Comment 7

[Henry Chang [:hchang]]

(In reply to Henry Chang [:henry] from comment #6)
> Created attachment 830093 [details] [diff] [review]
> Reference patch (for WifiP2pManager)
> 
> Hi dimi: 
> 
> I attached a patch for your reference. Actually, the current WIP patch is
> the reference of my patch but it has some tiny errors. My patch for wifi p2p
> has proven working well even though it's still under review in bug 811365 :p
> Hope it helps!

Oops it should be bug 811635...

Comment 8

[Dimi Lee [ooo 16.04 - 21.04］]

(In reply to Henry Chang [:henry] from comment #7)
> (In reply to Henry Chang [:henry] from comment #6)
> > Created attachment 830093 [details] [diff] [review]
> > Reference patch (for WifiP2pManager)
> > 
> > Hi dimi: 
> > 
> > I attached a patch for your reference. Actually, the current WIP patch is
> > the reference of my patch but it has some tiny errors. My patch for wifi p2p
> > has proven working well even though it's still under review in bug 811365 :p
> > Hope it helps!
> 
> Oops it should be bug 811635...

Great! Thanks, I will check it.

Comment 9

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: WebIdl for DOMWifiManager

Comment 10

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: WebIdl for DOMWifiManager (v2)

Comment 11

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: gaia patch for webidl

Comment 12

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8338408 [details] [diff] [review]
WebIdl for DOMWifiManager (v2)

Review of attachment 8338408 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for waiting for a long time. 
Instead of introducing new interfaces, I think we can use object type in WifiManager interface.
If you would like to define the object pass between gecko and gaia explicitly, we can use dictionary type.

::: dom/webidl/WifiManager.webidl
@@ +39,5 @@
> +           attribute DOMString? eap;
> +           attribute DOMString? pin;
> +
> +  object toJSON();
> +};

Can we expose web content using JSON format object by declare object or any data type in interface WifiManager ? So that we can eliminate the new interface WifiNetwork and make life easier when add/remove the data elements.

@@ +46,5 @@
> +interface WifiConnection {
> +  readonly attribute DOMString status;
> +  readonly attribute WifiNetwork? network;
> +};
> +

Ditto, we don't need a new interface, use JSON format data should be good enough. Maybe we can use dictionary If you would like to define it explicitly.

@@ +53,5 @@
> +  readonly attribute short signalStrength;
> +  readonly attribute short relSignalStrength;
> +  readonly attribute long linkSpeed;
> +  readonly attribute DOMString? ipAddress;
> +};

Ditto, we don't need a new interface, use JSON format data should be good enough. Maybe we can use dictionary If you would like to define it explicitly.

@@ +63,5 @@
> +  short maskLength;
> +  DOMString gateway;
> +  DOMString dns1;
> +  DOMString dns2;
> +};

Ditto, we don't need a new interface, use JSON format data should be good enough. Maybe we can use dictionary If you would like to define it explicitly.

@@ +97,5 @@
> +   *            request.value is true.
> +   * onerror: We were unable to select the network. This most likely means a
> +   *          configuration error.
> +   */
> +  DOMRequest associate(WifiNetwork network);

associate(WifiNetwork network) => associate(object network)

@@ +109,5 @@
> +   *            connected to it, we have started reconnecting to the next
> +   *            network in the list.
> +   * onerror: We were unable to remove the network.
> +   */
> +  DOMRequest forget(WifiNetwork network);

forget(WifiNetwork network) => forget(object network)

@@ +161,5 @@
> +   *        set info to null to clear http proxy.
> +   * onsuccess: We have successfully configure http proxy.
> +   * onerror: We have failed to configure http proxy.
> +   */
> +  DOMRequest setHttpProxy(WifiNetwork network, any info);

setHttpProxy(WifiNetwork network, any info) => setHttpProxy(object network, optional dictionary info);

@@ +214,5 @@
> +  /**
> +   * An event listener that is called with information about the signal
> +   * strength and link speed every 5 seconds.
> +   */
> +  attribute EventHandler onconnectioninfoupdate;

Can we keep the same name in nsIWifi.idl ? s/onconnectioninfoupdate/onconnectionInfoUpdate

Comment 13

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: WebIdl for DOMWifiManager (v3)

Hi Vincent,
  This patch update the review comment except useing object instead of interface.
  Because i am working on NFC right now.
  So need help to do the remaining work, thanks

Comment 14

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: WebIdl for DOMWifiManager (v4)

Comment 15

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.0

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8362869 [details] [diff] [review]
Patch v1.0

Review of attachment 8362869 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/WifiManager.webidl
@@ +85,5 @@
> +   *        - dns2 configured seconf DNS server address
> +   * onsuccess: We have successfully configure the static ip mode.
> +   * onerror: We have failed to configure the static ip mode.
> +   */
> +  DOMRequest setStaticIpMode(any network, optional any info);

Here and below: can we add a 'network' dictionary type that defines exactly what the object should contain?

Also, for reference, I think the type here without the dictionary should be 'object' instead of 'any' since we wouldn't expect e.g. a string.

@@ +117,5 @@
> +   *
> +   *  Note that the object returned is read only. Any changes required must
> +   *  be done by calling other APIs.
> +   */
> +  readonly attribute any connection;

Let's make this a dictionary as well, containing a union for the status and a network dictionary for the network.

@@ +124,5 @@
> +   * A connectionInformation object with the same information found in an
> +   * nsIDOMMozWifiConnectionInfoEvent (but without the network).
> +   * If we are not currently connected to a network, this will be null.
> +   */
> +  readonly attribute any connectionInformation;

We should make this a dictionary as well.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8362869 [details] [diff] [review]
Patch v1.0

Review of attachment 8362869 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/wifi/DOMWifiManager.js
@@ +351,1 @@
>      return exposeReadOnly({ status: this._connectionStatus,

Also: we should cache this object and return it for every call. Additionally, you can nuke exposeReadOnly now: the bindings take care of exposing the object correctly for us.

Comment 18

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Patch v2.0

Comment 19

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Patch v2.0

Comment 20

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8366459 [details] [diff] [review]
Patch v2.0

Use interface instead of dictionary because
WebIDL.WebIDLError: error: An attribute cannot be of a dictionary type

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8366459 [details] [diff] [review]
Patch v2.0

Review of attachment 8366459 [details] [diff] [review]:
-----------------------------------------------------------------

Before I go further, some questions to answer...

::: dom/webidl/WifiManager.webidl
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +dictionary WPSInfo {		

Nit: delete the trailing whitespace.

@@ +43,5 @@
> +};
> +
> +[JSImplementation="@mozilla.org/wificonnection;1"]
> +interface WifiConnection {
> +  readonly attribute DOMString status;

Last comment about this, I swear! Can we use an enum to eliminate doubt about what the possible values are?

::: dom/wifi/DOMWifiManager.js
@@ +36,5 @@
> +  },
> +
> +  toJSON: function() {
> +    var json = {
> +      __exposedProps__: {

Why is this __exposedProps__ needed? It looks like you're sending it to chrome, and my understanding of the new bindings is that they do away with the need for __exposedProps__ at all.

In addition, can you explain why you need this function at all? What breaks if you just send these objects through the message manager?

@@ +119,3 @@
>  // For smaller, read-only APIs, we expose any property that doesn't begin with
>  // an underscore.
>  function exposeReadOnly(obj) {

I think we can delete this function now, right?

Comment 22

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Patch v3

Comment 23

[Dimi Lee [ooo 16.04 - 21.04］]

(In reply to Blake Kaplan (:mrbkap) from comment #21)
> Comment on attachment 8366459 [details] [diff] [review]
> Patch v2.0
> 
> Review of attachment 8366459 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Before I go further, some questions to answer...
> 
> ::: dom/webidl/WifiManager.webidl
> @@ +1,5 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +dictionary WPSInfo {		
> 
> Nit: delete the trailing whitespace.
> 
> @@ +43,5 @@
> > +};
> > +
> > +[JSImplementation="@mozilla.org/wificonnection;1"]
> > +interface WifiConnection {
> > +  readonly attribute DOMString status;
> 
> Last comment about this, I swear! Can we use an enum to eliminate doubt
> about what the possible values are?
> 
> ::: dom/wifi/DOMWifiManager.js
> @@ +36,5 @@
> > +  },
> > +
> > +  toJSON: function() {
> > +    var json = {
> > +      __exposedProps__: {
> 
> Why is this __exposedProps__ needed? It looks like you're sending it to
> chrome, and my understanding of the new bindings is that they do away with
> the need for __exposedProps__ at all.
> 
> In addition, can you explain why you need this function at all? What breaks
> if you just send these objects through the message manager?

Yes __exposedProps__ should be removed, updated in the latest patch.
the sendAsyncMessage of message manager expect argument with JSON format, WifiNetwork passed here
is type "object XrayWrapper [object WifiNetwork]".
If do not convert it to JSON format and pass it directly then the receiver side will receive and empty object.

> 
> @@ +119,3 @@
> >  // For smaller, read-only APIs, we expose any property that doesn't begin with
> >  // an underscore.
> >  function exposeReadOnly(obj) {
> 
> I think we can delete this function now, right?
Removed, updated in patch v3

Comment 24

[Vincent Chang[:vchang][changyihsin]]

Hi mrbkap, any luck to review the patch this week ?

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8372147 [details] [diff] [review]
Patch v3

Review of attachment 8372147 [details] [diff] [review]:
-----------------------------------------------------------------

Two last questions and I think I'm happy with this.

::: dom/webidl/MozWifiManager.webidl
@@ +51,5 @@
> +           attribute DOMString? phase2;
> +           attribute DOMString? eap;
> +           attribute DOMString? pin;
> +
> +  object toJSON();

Do you have to expose toJSON on the interface itself? That seems wrong. It also seems like the toJSON function could be free standing, obviating the need for this.

::: dom/wifi/DOMWifiManager.js
@@ +37,5 @@
> +
> +  toJSON: function() {
> +    var json = {};
> +
> +    if (this.ssid != undefined)               json.ssid = this.ssid;

Is there any reason you can't use a for-in loop?

for (let key in this) {
  json[key] = this[key];
}

Comment 26

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Patch v4

Comment 27

[Dimi Lee [ooo 16.04 - 21.04］]

(In reply to Dimi Lee[:dimi][:dlee] from comment #26)
> Created attachment 8378106 [details] [diff] [review]
> Patch v4

Modified part
1.move toJSON function out of webidl WifiNetwork interface
2.refine toJSON function

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8378106 [details] [diff] [review]
Patch v4

Review of attachment 8378106 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/wifi/DOMWifiManager.js
@@ +126,5 @@
> +    let json = {};
> +
> +    for (let key in aNetwork) {
> +      // In WifiWorker.js there are lots of check using "key in network".
> +      // So if the value of any property of WifiNetwork is undefined, do not assign it to json object.

I'd mention in this comment that we're actually structured cloning this object, not strictly JSONing it (JSON will strip function and undefined values).

Comment 29

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Patch v5

Comment 30

[Dimi Lee [ooo 16.04 - 21.04］]

Attach is try server result:
https://tbpl.mozilla.org/?tree=Try&rev=0fcb3d0c83fd

Comment 31

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Rebased patch v6

Comment 32

[Dimi Lee [ooo 16.04 - 21.04］]

Push rebased patch to try server
https://tbpl.mozilla.org/?tree=Try&rev=519ca1ce2382

Comment 33

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Rebased patch v7

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/463fa756b43b

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/463fa756b43b

Comment 36

[Boris Zbarsky [:bzbarsky]]

How come "connection" is documented to be non-null but marked nullable in the IDL?

Comment 37

[Boris Zbarsky [:bzbarsky]]

Also, why are "security" and "capabilities" of type "any"?  That seems pretty fishy to me, since as far as I can tell it'll just return unusable objects if the caller is not chrome.  Do we have tests for that?

I'm also a bit confused by the test_interfaces changes.  They say permission: "wifi-manage", but MozWifiNetwork, MozWifiConnection, MozWifiConnectionInfo seem to be exposed unconditionally, no?

And in fact, MozWifiNetwork seems to expose the constructor unconditionally to all web pages.  Is that expected?

Comment 38

[Dimi Lee [ooo 16.04 - 21.04］]

(In reply to Boris Zbarsky [:bz] from comment #36)
> How come "connection" is documented to be non-null but marked nullable in
> the IDL?

You are right, i create bug 986374 to fix this issue

Comment 39

[Nicolas B. Pierron [:nbp]]

This patch is likely the patch which broke Marionette tests used on AreWeFastYet [1], after the merge from b2g-inbound into inbound.

We should back it out of inbound & b2g-inbound, and make sure that the gaia-ui-test's TestBrowserLan is working fine *on a device*.  The problem seems to be be that when we setup the wifi settings (in gaia/tests/atoms/gaia_data_layer.js), and we do not have a proper MozWifiNetwork, which is expected as argument of of the associate function.

My test var contains:
  "wifi": {
     "ssid": "AreWeFastYet", "keyManagement": "WPA-PSK", "psk": "aprivatepassword"
  },

ERROR
======================================================================
ERROR: None
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/awsa/unagi/B2G/gecko/testing/marionette/client/marionette/marionette_test.py", line 145, in run
self.setUp()
File "/home/awsa/unagi/B2G/perso/gaia-ui-tests/gaiatest/tests/browser/benchmarks/test_bench_octane.py", line 19, in setUp
self.connect_to_local_area_network()
File "/home/awsa/unagi/B2G/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 971, in connect_to_local_area_network
self.data_layer.connect_to_wifi()
File "/home/awsa/unagi/B2G/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 272, in connect_to_wifi
script_timeout = max(self.marionette.timeout, 60000))
File "/home/awsa/unagi/B2G/gecko/testing/marionette/client/marionette/marionette.py", line 1177, in execute_async_script
filename=os.path.basename(frame[0]))
File "/home/awsa/unagi/B2G/gecko/testing/marionette/client/marionette/decorators.py", line 35, in _
return func(*args, **kwargs)
File "/home/awsa/unagi/B2G/gecko/testing/marionette/client/marionette/marionette.py", line 624, in _send_message
self._handle_error(response)
File "/home/awsa/unagi/B2G/gecko/testing/marionette/client/marionette/marionette.py", line 669, in _handle_error
raise JavascriptException(message=message, status=status, stacktrace=stacktrace)
JavascriptException: JavascriptException: TypeError: Argument 1 of MozWifiManager.associate does not implement interface MozWifiNetwork.
stacktrace:
execute_async_script @gaia_test.py, line 272
inline javascript, line 589
src: "      var req = manager.associate(aNetwork);"
TEST-UNEXPECTED-FAIL | test_bench_octane.py test_bench_octane.TestBenchOctane.test_octane |

[1] http://www.arewefastyet.com/#machine=14

Comment 40

[Boris Zbarsky [:bzbarsky]]

Dimi Lee, what about comment 37?

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Backed out for the various regressions filed as deps here.
https://hg.mozilla.org/integration/b2g-inbound/rev/c16f36021958

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/c16f36021958

Comment 43

[Dimi Lee [ooo 16.04 - 21.04］]

Following is the root cause for GAIA UI test fail :
After converting WifiManager to WebIDL, associate will require a MozWifiNetwork object as parameter, but in GAIA UI test, aNetwork parameter may be fetched from JSON file as json object. This cause manager.associate function fail.

I will fix bug 988117 first

Comment 44

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Patch v8

Comment 45

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Patch v9

Comment 46

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Patch v10

Update patch for addressing bz's comment and fix some issues
1."security" & "capabilities" are string list, change it from type any to sequence<DOMString>
2.MozWifiNetwork, MozWifiConnection, MozWifiConnectionInfo do not require wifi-manage permission
3.connection parameter should be non-null
4.Add dontConnect field for webidl

Comment 47

[Dimi Lee [ooo 16.04 - 21.04］]

Try server result for patch v10
https://tbpl.mozilla.org/?tree=Try&rev=b23037927d48

Comment 48

[Boris Zbarsky [:bzbarsky]]

>2.MozWifiNetwork, MozWifiConnection, MozWifiConnectionInfo do not require wifi-manage permission

Why not?  Shouldn't they?  It seems a bit odd to be exposing those to all web pages.

Why is "dontConnect" nullable?  What does null mean?  Needs documentation.

Also, I assume there's a reason none of the nullable attributes there are initialized in the constructor?  What _does_ initialize them?

Comment 49

[Dimi Lee [ooo 16.04 - 21.04］]

(In reply to Boris Zbarsky [:bz] from comment #48)
> >2.MozWifiNetwork, MozWifiConnection, MozWifiConnectionInfo do not require wifi-manage permission
> 
> Why not?  Shouldn't they?  It seems a bit odd to be exposing those to all
> web pages.
> 
Yes i think it is not required to expose MozWifiConnection, MozWifiConnectionInfo to all web pages, i will add [ChromOnly] to these two interfaces.
For MozWifiNetwork, it might still be used, bug 988117 is an example.

How do you think ?

> Why is "dontConnect" nullable?  What does null mean?  Needs documentation.
> 
> Also, I assume there's a reason none of the nullable attributes there are
> initialized in the constructor?  What _does_ initialize them?
Current Wifi API architecture use Network object as parameter for different APIs, for example, associate, forget, setHttpProxy ... etc

There are many parameters in Network object is set by GAIA for associate API. And the parameter is set
or not depends on associated network. So null means the parameter is not require for this API call.
I could add documentation for this in next patch.

Comment 50

[Boris Zbarsky [:bzbarsky]]

> For MozWifiNetwork, it might still be used, bug 988117 is an example.

Used by random web pages?  Or only used by some gaia bits?

> I could add documentation for this in next patch.

Please.  I'm not sure why we're not using a dictionary if what we want is just an options object, but I'm going to assume the API was generally already designed and there are good reasons it is the way it is.

Comment 51

[Dimi Lee [ooo 16.04 - 21.04］]

(In reply to Boris Zbarsky [:bz] from comment #50)
> > For MozWifiNetwork, it might still be used, bug 988117 is an example.
> 
> Used by random web pages?  Or only used by some gaia bits?
> 
  In the future, we may implement feature that web pages can fetch wifi-network data from database or files(like the usage of GAIA UI test). So web pages may need MozWifiNetwork interface.

> > I could add documentation for this in next patch.
> 
> Please.  I'm not sure why we're not using a dictionary if what we want is
> just an options object, but I'm going to assume the API was generally
> already designed and there are good reasons it is the way it is.

The reason that we did not use dictionary is because of the compile error : "An attribute cannot be of a dictionary type".

Comment 52

[Boris Zbarsky [:bzbarsky]]

> So web pages may need MozWifiNetwork interface.

_That_ is when we'll go ahead and expose it to web pages.  And standardize it in the process.

We shouldn't be exposing random non-standard, non-standards-track stuff to web pages.  Especially not when we have the capability to _not_ do that pretty easily.

> "An attribute cannot be of a dictionary type".

I was talking about using a dictionary (or in fact different dictionaries) for the parameter case.  The attribute can stay of type Network (though even here you could use a dictionary if the attribute were Pure or Constant and hence could be Cached... but it makes some sense to use an interface here but a dictionary for things that sure look like options objects to me).  Note that if you pass a Network object with some expandos set to an API that expects a dictionary with those property names it will work just fine.

Comment 53

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8396902 [details] [diff] [review]
Patch v10

Going to r- the overexposing stuff to web pages bit...

Comment 54

[Dimi Lee [ooo 16.04 - 21.04］]

(In reply to Boris Zbarsky [:bz] from comment #52)
> > So web pages may need MozWifiNetwork interface.
> 
> _That_ is when we'll go ahead and expose it to web pages.  And standardize
> it in the process.
> 
> We shouldn't be exposing random non-standard, non-standards-track stuff to
> web pages.  Especially not when we have the capability to _not_ do that
> pretty easily.
>
Ok,so i will add wifi-manage permission check for MozWifinetwork interface so GAIA UI test can still use it.
For MozWifiConnection and MozWifiConnectionInfo i will set to ChromeOnly to not expose to web pages.
Or do you have any suggestion ? Thanks!

Comment 55

[Boris Zbarsky [:bzbarsky]]

That sounds reasonable to me, yes.

Comment 56

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Patch v11

Comment 57

[Dimi Lee [ooo 16.04 - 21.04］]

Hi bz,
  If you are available, could you help review this bug ? thanks

Comment 58

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8397633 [details] [diff] [review]
Patch v11

This seems ok, but how is test_interfaces passing?  The interfaces are marked ChromeOnly, but are still listed in that file, aren't they?

r=me with the test fixed...

Comment 59

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Patch v12

Comment 60

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Patch v12

Comment 61

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8400439 [details] [diff] [review]
Patch v12

Hi Bz,
 After testing different parameters for GAIA UI test there is still one issue need to fix in this patch, so please help review again.
 There are two parts modified in this patch 
1. test_interface addressed in Comment 58
2. Changed the constructor of MozWifiNetwork so GAIA UI test can pass different parameters

Comment 62

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8400439 [details] [diff] [review]
Patch v12

Can you please explain why we're not using a dictionary anymore?  That seems unsafe from a security point of view (esp. if we then allow the caller to override any properties on "this" that it wants!).

Comment 63

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: WebIdl for DOMWifiManager v13

Sorry, my mistake. Not notice the security issue

Comment 64

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8402436 [details] [diff] [review]
WebIdl for DOMWifiManager v13

Why are all those nullable  I assume we explicitly want to treat null as null and not "null" for the strings?  But what about null vs 0 for relSignalStrength and nulll vs false for dontConnect?

Note that these are already optional no matter what, just in case you were confusing optional and nullable...

Comment 65

[Dimi Lee [ooo 16.04 - 21.04］]

(In reply to Boris Zbarsky [:bz] from comment #64)
> Comment on attachment 8402436 [details] [diff] [review]
> WebIdl for DOMWifiManager v13

Yes, should remove nullable because both GAIA and GECKO will not pass null for those parameters.

Comment 66

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: WebIdl for DOMWifiManager v14

Comment 67

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8404413 [details] [diff] [review]
WebIdl for DOMWifiManager v14

r=me

Comment 68

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://tbpl.mozilla.org/?tree=Try&rev=015d06cdb669

Comment 69

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce623d85f0c4

Comment 70

[Carsten Book [:Tomcat]]

was backedout again for the suspecion this caused the m7 orange like https://tbpl.mozilla.org/php/getParsedLog.php?id=37750735&tree=Mozilla-Inbound but we will reland again since this was not the case

Comment 71

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b2c4a85a5e1

Comment 72

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

For posterity:

Landed     https://hg.mozilla.org/mozilla-central/rev/ce623d85f0c4
Backed out https://hg.mozilla.org/mozilla-central/rev/83d0d281cfbc
Relanded   https://hg.mozilla.org/mozilla-central/rev/9b2c4a85a5e1