Closed Bug 886110 Opened 7 years ago Closed 6 years ago

Convert WifiManager to WebIDL

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: bruant.d, Assigned: dimi)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 23 obsolete files)

40.97 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
No description provided.
Component: DOM → DOM: Device Interfaces
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
Attached patch WIP (obsolete) — Splinter Review
Up for grabs. I wrote most of this, then went to test it and found out there are no tests. Heh.
Attached patch WIP (obsolete) — Splinter Review
Oops, attached before qref'ing.

Note that users of the mysterious connectionInfoUpdate event will need to be fixed.
Attachment #767526 - Attachment is obsolete: true
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).
(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.
Blocks: SH-APIs
Assignee: nobody → dlee
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!
(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...
(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.
Attached patch WebIdl for DOMWifiManager (obsolete) — Splinter Review
Attachment #8337510 - Flags: review?(vchang)
Attachment #8337510 - Attachment is obsolete: true
Attachment #8337510 - Flags: review?(vchang)
Attached patch WebIdl for DOMWifiManager (v2) (obsolete) — Splinter Review
Attachment #8338408 - Flags: review?(vchang)
Attached patch gaia patch for webidl (obsolete) — Splinter Review
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
Attachment #8338408 - Flags: review?(vchang)
Attached patch WebIdl for DOMWifiManager (v3) (obsolete) — Splinter Review
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
Attachment #8338408 - Attachment is obsolete: true
Attachment #8355477 - Attachment is obsolete: true
Attachment #8360835 - Attachment is obsolete: true
Attached patch WebIdl for DOMWifiManager (v4) (obsolete) — Splinter Review
Attached patch Patch v1.0 (obsolete) — Splinter Review
Attachment #8360840 - Attachment is obsolete: true
Attachment #8362869 - Flags: review?(mrbkap)
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.
Attachment #8362869 - Flags: review?(mrbkap)
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.
Depends on: 963382
Depends on: 963388
Attached patch Patch v2.0 (obsolete) — Splinter Review
Attachment #8362869 - Attachment is obsolete: true
Attached patch Patch v2.0 (obsolete) — Splinter Review
Attachment #8366457 - Attachment is obsolete: true
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
Attachment #8366459 - Flags: review?(mrbkap)
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?
Attachment #8366459 - Flags: review?(mrbkap)
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #8366459 - Attachment is obsolete: true
(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
Attachment #8372147 - Flags: review?(mrbkap)
Hi mrbkap, any luck to review the patch this week ?
Flags: needinfo?(mrbkap)
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];
}
Attachment #8372147 - Flags: review?(mrbkap)
Flags: needinfo?(mrbkap)
Attached patch Patch v4 (obsolete) — Splinter Review
Attachment #8372147 - Attachment is obsolete: true
Attachment #8378106 - Flags: review?(mrbkap)
(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 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).
Attachment #8378106 - Flags: review?(mrbkap) → review+
Attached patch Patch v5 (obsolete) — Splinter Review
Attachment #8378106 - Attachment is obsolete: true
Blocks: 981845
Attached patch Rebased patch v6 (obsolete) — Splinter Review
Push rebased patch to try server
https://tbpl.mozilla.org/?tree=Try&rev=519ca1ce2382
Attached patch Rebased patch v7 (obsolete) — Splinter Review
Attachment #8378745 - Attachment is obsolete: true
Attachment #8392765 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #830093 - Attachment is obsolete: true
Attachment #767527 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/463fa756b43b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
How come "connection" is documented to be non-null but marked nullable in the IDL?
Flags: needinfo?(dlee)
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?
Flags: needinfo?(mrbkap)
(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
Flags: needinfo?(dlee)
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
Blocks: 986446
Dimi Lee, what about comment 37?
Flags: needinfo?(dlee)
Depends on: 986374
No longer blocks: 986446
Depends on: 986446
Depends on: 986557
Backed out for the various regressions filed as deps here.
https://hg.mozilla.org/integration/b2g-inbound/rev/c16f36021958
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla31 → ---
Depends on: 986520
Depends on: 988117
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
Flags: needinfo?(dlee)
Attached patch Patch v8 (obsolete) — Splinter Review
Attached patch Patch v9 (obsolete) — Splinter Review
Attachment #8393320 - Attachment is obsolete: true
Attachment #8396873 - Attachment is obsolete: true
Attached patch Patch v10 (obsolete) — Splinter Review
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
Attachment #8396881 - Attachment is obsolete: true
Attachment #8396902 - Flags: review?(bzbarsky)
Try server result for patch v10
https://tbpl.mozilla.org/?tree=Try&rev=b23037927d48
>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?
Flags: needinfo?(dlee)
(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.
Flags: needinfo?(dlee) → needinfo?(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.
Flags: needinfo?(bzbarsky)
(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".
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Comment on attachment 8396902 [details] [diff] [review]
Patch v10

Going to r- the overexposing stuff to web pages bit...
Attachment #8396902 - Flags: review?(bzbarsky) → review-
(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!
Flags: needinfo?(bzbarsky)
That sounds reasonable to me, yes.
Flags: needinfo?(bzbarsky)
Attached patch Patch v11 (obsolete) — Splinter Review
Attachment #8396902 - Attachment is obsolete: true
Attachment #8397633 - Flags: review?(bzbarsky)
Hi bz,
  If you are available, could you help review this bug ? thanks
Flags: needinfo?(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...
Attachment #8397633 - Flags: review?(bzbarsky) → review+
Flags: needinfo?(bzbarsky)
Flags: needinfo?(mrbkap)
Attached patch Patch v12 (obsolete) — Splinter Review
Attachment #8397633 - Attachment is obsolete: true
Attachment #8400435 - Attachment is obsolete: true
Attached patch Patch v12 (obsolete) — Splinter Review
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
Attachment #8400439 - Flags: review?(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!).
Attachment #8400439 - Flags: review?(bzbarsky) → review-
Attached patch WebIdl for DOMWifiManager v13 (obsolete) — Splinter Review
Sorry, my mistake. Not notice the security issue
Attachment #8400439 - Attachment is obsolete: true
Attachment #8402436 - Flags: review?(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...
Flags: needinfo?(dlee)
(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.
Flags: needinfo?(dlee)
Attachment #8402436 - Attachment is obsolete: true
Attachment #8402436 - Flags: review?(bzbarsky)
Attachment #8404413 - Flags: review?(bzbarsky)
Comment on attachment 8404413 [details] [diff] [review]
WebIdl for DOMWifiManager v14

r=me
Attachment #8404413 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/9b2c4a85a5e1
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Depends on: 1023142
Blocks: 937559
You need to log in before you can comment on or make changes to this bug.