John Shih Assignee
	Description • 10 years ago

We are going to support this API in Firefox OS so that gaia could get the information of current connection and also observe the connection change event.

the API information can be found in https://developer.mozilla.org/en-US/docs/WebAPI/Network_Information

	John Shih Assignee
	Comment 1 • 10 years ago

Refers to [1], basically the API provides two information (includes network bandwidth and a boolean value 'metered', which is used to indicate current connection status is metered or not) and one event (listen to connection change).

What I'm curious about is how should we define 'metered' in FFOS? It would be simple and also meet our requirements (i.e, distinguish between 'wifi' and 'non-wifi' connections) if we can just set 'metered'=true when connection is 'wifi', otherwise set 'metered'=false. However the meaning of 'metered' seems like not that simple...:(

I'm not sure who can answer this..

[1] https://dvcs.w3.org/hg/dap/raw-file/tip/network-api/Overview.html#widl-NetworkInformation-connection

	Fernando Jiménez Moreno [:ferjm]
	Comment 2 • 10 years ago

Hi John,

we are still trying to decide how this API should look like or if it is even an useful addition to the web. We've recently wrote a new proposal [1] based on [2] that removes the 'bandwidth' and 'metered' properties from this API. We are waiting for feedback from other browser vendors before moving forward with next steps. So I am not sure that exposing this API as specified in [3] in FirefoxOS is the best thing to do right now.

Jonas, Marcos?

[1] https://github.com/ferjm/w3c-netinfo-v3-proposal
[2] https://github.com/w3c-webmob/netinfo
[3] https://dvcs.w3.org/hg/dap/raw-file/tip/network-api/Overview.html#widl-NetworkInformation-connection

	John Shih Assignee
	Comment 3 • 10 years ago

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #2)
> Hi John,
> 
> we are still trying to decide how this API should look like or if it is even
> an useful addition to the web. We've recently wrote a new proposal [1] based
> on [2] that removes the 'bandwidth' and 'metered' properties from this API.
> We are waiting for feedback from other browser vendors before moving forward
> with next steps. So I am not sure that exposing this API as specified in [3]
> in FirefoxOS is the best thing to do right now.
> 
> Jonas, Marcos?
> 
> [1] https://github.com/ferjm/w3c-netinfo-v3-proposal
> [2] https://github.com/w3c-webmob/netinfo
> [3]
> https://dvcs.w3.org/hg/dap/raw-file/tip/network-api/Overview.html#widl-
> NetworkInformation-connection

Really appreciate for your response!
It's really great to know the current progress of this API. I can see the new proposal could solve lots of my concerns :)
Definitely, I'll keep tracking the discussion and wait for the final decision before implementation.

Thanks!

	Marcos Caceres [:marcosc]
	Comment 4 • 10 years ago

Still waiting for the Blink guys to respond. In the mean time, does anyone know much about bridged network connections (as we will need to deal with those on other platforms)? 

If so, could really use your input here:
https://github.com/ferjm/w3c-netinfo-v3-proposal/issues/4

	John Shih Assignee
	Comment 5 • 10 years ago

Hi Marcos, Fernando
Sine there already have 'online' and 'offline' events [1], which could provide similar functionalities as 'onchange' event in new proposal does. Do you think we still need to create a new event or just integrate what we need into original one?

[1] https://developer.mozilla.org/en/docs/Online_and_offline_events

	Marcos Caceres [:marcosc]
	Comment 6 • 10 years ago

So, although you are correct in that there is overlap, the online/offline events signal something a bit different than the NetInfo's network type change event. The use case for NetInfo's type is mostly for detecting when the user switches from "wifi" to "cellular" to "none" (i.e., airplane or wifi disabled) - during the transition, the network type may also transition to "none" and the navigator.onLine attribute may change (and associated online/offline event may also fire).

	Fernando Jiménez Moreno [:ferjm]
	Comment 7 • 10 years ago

Also note that being connected to a network doesn't mean that you are online.

Check http://www.whatwg.org/specs/web-apps/current-work/multipage/offline.html#browser-state 

"This attribute is inherently unreliable. A computer can be connected to a network without having Internet access."

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 8 • 10 years ago

(In reply to Marcos Caceres [:marcosc] from comment #4)
> Still waiting for the Blink guys to respond. In the mean time, does anyone
> know much about bridged network connections (as we will need to deal with
> those on other platforms)? 

There's no way to detect that you're on a bridged connection. That is a bummer, but not much we can do. I think this is an implementation issue though, if the wifi protocols improve such that we detect that we're on a bridged wifi connection we can return whatever properties the upstream connection has.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 9 • 10 years ago

My recollection of our previous discussion was that we should for now expose the connection type, i.e. "wifi"/"wired"/"mobile".

We'll also look at adding heuristics about bandwidth and see if we can get any reliability for that. But for now we should just do internal experiments and telemetry experiments.

FWIW, I think the "connection type" should be set to "" or some such when navigator.onLine is false.

	John Shih Assignee
	Comment 11 • 10 years ago

Status update:

The implementation is going to follow the new API proposal [1].

[1] http://w3c.github.io/netinfo/

	John Shih Assignee
	Comment 12 • 10 years ago

It seems we finally meet a agreement on the new API proposal (though not finalized, but very close), I'd like to work on the IDL part first.

	Marcos Caceres [:marcosc]
	Comment 13 • 10 years ago

Comment on attachment 8385965 [details] [diff] [review]
Bug 960426 - Part 1: WebIDL changes for NetInfo API v3. r=sicking, marcosc

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

Typo in ConnectionType: celluar > cellular

Please remove "NoInterfaceObject".

	John Shih Assignee
	Comment 14 • 10 years ago

Fix Nits.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 15 • 10 years ago

But keep in mind that this can't land until we've changed the Android implementation which currently uses the old webidl.

	John Shih Assignee
	Comment 16 • 10 years ago

(In reply to Jonas Sicking (:sicking) from comment #15)
> But keep in mind that this can't land until we've changed the Android
> implementation which currently uses the old webidl.

Sure. I'm working on it as well.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 17 • 10 years ago

Awesome! An alternative to that would be to use something like [Pref="dom...."] to enable some properties on Android and other properties in FirefoxOS.

But it's definitely much better if we can implement the new API on both FirefoxOS and Android. That will help drive adoption which is a win for everyone.

	John Shih Assignee
	Comment 18 • 10 years ago

(WIP) Do the corresponding modification for interface change. Next step will be supporting this API in FFOS.

	John Shih Assignee
	Comment 19 • 10 years ago

Corresponding modification in fennec. I've verified it works on my testing Android device.

	Doug Turner (:dougt)
	Comment 20 • 10 years ago

Comment on attachment 8387484 [details] [diff] [review]
Bug 960426 - Part 3: Related change in Fennec. r=dougt

brad, can you review or find a reviewer?

	Brad Lassey [:blassey] (use needinfo?)
	Comment 21 • 10 years ago

Comment on attachment 8387484 [details] [diff] [review]
Bug 960426 - Part 3: Related change in Fennec. r=dougt

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

r+ with nits addressed. Happy to re-review an updated patch though.

::: mobile/android/base/GeckoNetworkManager.java
@@ +41,5 @@
> +    static private final int CONNECTION_TYPE_BLUETOOTH = 1;
> +    static private final int CONNECTION_TYPE_ETHERNET  = 2;
> +    static private final int CONNECTION_TYPE_WIFI      = 3;
> +    static private final int CONNECTION_TYPE_OTHER     = 4;
> +    static private final int CONNECTION_TYPE_NONE      = 5;

use an enum

private enum ConnectionType {
  CELLULAR(0),
  ... ;
  final int value;
  ConnectionType(int v) {value = v;};
}

@@ +132,5 @@
>              return;
>          }
>  
>          GeckoAppShell.sendEventToGecko(GeckoEvent.createNetworkEvent(
> +                                       mConnectionType,

with the enum, this will be mConnectionType.value

::: widget/android/nsAppShell.cpp
@@ +57,5 @@
>  #define EVLOG(args...) do { } while (0)
>  #endif
>  
>  using namespace mozilla;
> +using namespace mozilla::dom;

why is this needed?

	(no longer active)
	Comment 22 • 10 years ago

Comment on attachment 8386006 [details] [diff] [review]
Bug 960426 - Part 1: WebIDL changes for NetInfo API v3. r=sicking, marcosc

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

::: dom/webidl/MozConnection.webidl
@@ +13,4 @@
>  };
>  
> +[Pref="dom.network.enabled"]
> +interface MozConnection : EventTarget {

Can you also rename the interface to Connection please?  And drop the moz prefix from the navigator object as well? <http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Navigator.webidl#241>

	(no longer active)
	Comment 23 • 10 years ago

Also, do we feel ready to expose this to the Web in Firefox OS and Android yet?  If not, we should probably make this available only to certified apps for now.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 24 • 10 years ago

Lets make this enabled behind a pref. And turn that pref on for now. We can always disable the pref before this reaches release channel on whatever platforms that we deem as having too broad of an audience.

	(no longer active)
	Comment 25 • 10 years ago

(In reply to comment #24)
> Lets make this enabled behind a pref. And turn that pref on for now. We can
> always disable the pref before this reaches release channel on whatever
> platforms that we deem as having too broad of an audience.

Sounds good.

	Masatoshi Kimura [:emk]
	Comment 26 • 10 years ago

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #22)
> Can you also rename the interface to Connection please?

The interface name is "NetworkInformation" per the new spec:
http://w3c.github.io/netinfo/#the-networkinformation-interface

	John Shih Assignee
	Comment 27 • 10 years ago

Patch update based on Comment 22.

	John Shih Assignee
	Comment 28 • 10 years ago

Separated the modification for IDL change from implementation (support API in FFOS). Gecko support will be in another part.

	John Shih Assignee
	Comment 29 • 10 years ago

Comment on attachment 8391094 [details] [diff] [review]
Bug 960426 - Part 1: WebIDL changes for NetInfo API v3. r=sicking, marcosc

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

According to Comment 24, remove review requests till adding the additional check with pref.

	Brad Lassey [:blassey] (use needinfo?)
	Comment 30 • 10 years ago

Comment on attachment 8391097 [details] [diff] [review]
Bug 960426 - Part 2: Modification for IDL change. r=blassey

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

Looks good. Though I prefer more specific return types in idl's. Any reason to use nsISupports here?

::: dom/network/interfaces/nsIMozNavigatorNetwork.idl
@@ +11,1 @@
>    readonly attribute nsISupports mozConnection;

why isn't this be nsINetworkProperties or nsIDOMMozConnection?

	John Shih Assignee
	Comment 31 • 10 years ago

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #30)
> Comment on attachment 8391097 [details] [diff] [review]
> Bug 960426 - Part 2: Modification for IDL change. r=blassey
> 
> Review of attachment 8391097 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Though I prefer more specific return types in idl's. Any reason
> to use nsISupports here?
> 
> ::: dom/network/interfaces/nsIMozNavigatorNetwork.idl
> @@ +11,1 @@
> >    readonly attribute nsISupports mozConnection;
> 
> why isn't this be nsINetworkProperties or nsIDOMMozConnection?

AFAIK, it was modified to nsISupports from nsIDOMMozConnection (which is no longer existed) in the work of moving to webidl [1]. So I just keep it.

[1] https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=952084&attachment=8360054

	Brad Lassey [:blassey] (use needinfo?)
	Comment 32 • 10 years ago

Comment on attachment 8391097 [details] [diff] [review]
Bug 960426 - Part 2: Modification for IDL change. r=blassey

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

::: dom/network/interfaces/nsIMozNavigatorNetwork.idl
@@ +11,1 @@
>    readonly attribute nsISupports mozConnection;

OK, let's change this to the more specific interface.

	(no longer active)
	Comment 33 • 10 years ago

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #32)
> Comment on attachment 8391097 [details] [diff] [review]
> Bug 960426 - Part 2: Modification for IDL change. r=blassey
> 
> Review of attachment 8391097 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/interfaces/nsIMozNavigatorNetwork.idl
> @@ +11,1 @@
> >    readonly attribute nsISupports mozConnection;
> 
> OK, let's change this to the more specific interface.

Do we use this XPIDL property anywhere?

	John Shih Assignee
	Comment 34 • 10 years ago

Update based on previous review.
(Note: namespace mozilla:dom is for using ConnectionType directly, otherwise it requires mozilla:dom:ConnectionType)

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 35 • 10 years ago

I'd love to get this on the Firefox 31 train, which I think this means that it needs to land tomorrow.

	John Shih Assignee
	Comment 36 • 10 years ago

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #33)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #32)
> > Comment on attachment 8391097 [details] [diff] [review]
> > Bug 960426 - Part 2: Modification for IDL change. r=blassey
> > 
> > Review of attachment 8391097 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/network/interfaces/nsIMozNavigatorNetwork.idl
> > @@ +11,1 @@
> > >    readonly attribute nsISupports mozConnection;
> > 
> > OK, let's change this to the more specific interface.
> 
> Do we use this XPIDL property anywhere?

Not sure, but I think this is a special case.
NetworkProperties was originally designed as a builtin-class of nsIDOMMozConnection.
Then, nsIDOMMozConnection was not needed anymore after porting xpidl to webidl, so it is replaced by nsISupports.

	John Shih Assignee
	Comment 37 • 10 years ago

(In reply to Jonas Sicking (:sicking) from comment #35)
> I'd love to get this on the Firefox 31 train, which I think this means that
> it needs to land tomorrow.

Hi Jonas,
Since the final part (i.e., support this API in FFOS) is still under progress (almost done, but still need some testing & push try confirmation), I'm afraid it can not meet your expectation.. :(

	John Shih Assignee
	Comment 38 • 10 years ago

Comment on attachment 8391094 [details] [diff] [review]
Bug 960426 - Part 1: WebIDL changes for NetInfo API v3. r=sicking, marcosc

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

Since there already a pref for this API ("dom.network.enabled"), I think we don't need additional one.

	John Shih Assignee
	Comment 39 • 10 years ago

In this patch, nsAppShell.cpp is listening to "network-connection-state-changed" topic (notified by NetworkManager.js). After receiving nsINetworkInterface (carried in the notification), some data will be converted into the format of hal::NetworkInformation and then be sent through hal::NotifyNetworkChange().

	John Shih Assignee
	Comment 40 • 10 years ago

Small update: do not use dom::network::ConnectionType anymore.

	John Shih Assignee
	Comment 41 • 10 years ago

Hi Blassey,

Please re-check the patch again. 

There is an update for the patch. When I tried to build gecko, I encountered the error that related to ConnectionType serialization in Hal. The root cause was that I tried to serialize ConnectionType, which is an enum class generated from webidl. However, the serializer [1] only works for enum (not class). In order to solve the problem as well as avoid complicated type transformation, I decide to pure int in data transmission. Now the int value is only defined in sending site and receiving site.

Also, there is another question about using more specific interface instead of nsISupports. Since there is no nsIDOMMozConnectio anymore and NetworkProperties is merely a builtin-class. (please see Comment 36) Do you really think we need to create a new interface to replace nsISupports here?

Thanks!

[1] http://dxr.mozilla.org/mozilla-central/source/ipc/glue/IPCMessageUtils.h#96

	Brad Lassey [:blassey] (use needinfo?)
	Comment 42 • 10 years ago

(In reply to John Shih from comment #41)
> Created attachment 8394033 [details] [diff] [review]
> Bug 960426 - Part 2: Modification for IDL change. r=blassey
> 
> Hi Blassey,
> 
> Please re-check the patch again. 
> 
> There is an update for the patch. When I tried to build gecko, I encountered
> the error that related to ConnectionType serialization in Hal. The root
> cause was that I tried to serialize ConnectionType, which is an enum class
> generated from webidl. However, the serializer [1] only works for enum (not
> class). In order to solve the problem as well as avoid complicated type
> transformation, I decide to pure int in data transmission. Now the int value
> is only defined in sending site and receiving site.
Sorry, I'm not following. What change did you make? Can you referenced the previous patch?

> 
> Also, there is another question about using more specific interface instead
> of nsISupports. Since there is no nsIDOMMozConnectio anymore and
> NetworkProperties is merely a builtin-class. (please see Comment 36) Do you
> really think we need to create a new interface to replace nsISupports here?

I do prefer having a more specfic interface. I think the extra interface is worth it.

	John Shih Assignee
	Comment 43 • 10 years ago

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #42)
> (In reply to John Shih from comment #41)
> > Created attachment 8394033 [details] [diff] [review]
> > Bug 960426 - Part 2: Modification for IDL change. r=blassey
> > 
> > Hi Blassey,
> > 
> > Please re-check the patch again. 
> > 
> > There is an update for the patch. When I tried to build gecko, I encountered
> > the error that related to ConnectionType serialization in Hal. The root
> > cause was that I tried to serialize ConnectionType, which is an enum class
> > generated from webidl. However, the serializer [1] only works for enum (not
> > class). In order to solve the problem as well as avoid complicated type
> > transformation, I decide to pure int in data transmission. Now the int value
> > is only defined in sending site and receiving site.
> Sorry, I'm not following. What change did you make? Can you referenced the
> previous patch?

I removed the ConnectionType.cpp, which aimed at serializing enum class ConnectionType generated from NetworkInformation.webidl (the auto-geneated header was mozilla/dom/NetworkInformationBinding.h). And now in PHal.ipdl, the type is declared as int, instead of ConnectionType.

> 
> > 
> > Also, there is another question about using more specific interface insteadd
> > of nsISupports. Since there is no nsIDOMMozConnectio anymore and
> > NetworkProperties is merely a builtin-class. (please see Comment 36) Do you
> > really think we need to create a new interface to replace nsISupports here?
> 
> I do prefer having a more specfic interface. I think the extra interface is
> worth it.

Got it, thanks.

	Brad Lassey [:blassey] (use needinfo?)
	Comment 44 • 10 years ago

Comment on attachment 8394033 [details] [diff] [review]
Bug 960426 - Part 2: Modification for IDL change. r=blassey

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

no problem with the ConnectionType change, r- to get a patch with the nsIDOMMozConnection interface

	John Shih Assignee
	Comment 45 • 10 years ago

patch update.

	John Shih Assignee
	Comment 46 • 10 years ago

Fix the test failed in test_interfaces.html due to adding a new interface.

	(no longer active)
	Comment 47 • 10 years ago

Comment on attachment 8391094 [details] [diff] [review]
Bug 960426 - Part 1: WebIDL changes for NetInfo API v3. r=sicking, marcosc

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

r=me with the below addressed.

::: dom/webidl/Navigator.webidl
@@ +237,5 @@
>  
>  // nsIDOMMozNavigatorNetwork
>  partial interface Navigator {
>    [Pref="dom.network.enabled"]
> +  readonly attribute NetworkInformation? connection;

Not sure why you've made this property nullable.  That is not what the spec says, and if I'm reading the implementation correctly, it will never actually return null here.  So please drop the nullable part.

::: dom/webidl/NetworkInformation.webidl
@@ +1,4 @@
>  /* 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/.
>   */

Nit: please link to the spec in the comment section here, similarly to how we do this in other webidl files, for example, AudioContext.webidl.

@@ +15,5 @@
> +[Pref="dom.network.enabled"]
> +interface NetworkInformation : EventTarget {
> +  readonly attribute ConnectionType type;
> +  attribute EventHandler ontypechange;
> +};

Nit: We usually try to copy and paste the WebIDL from the spec and not change things such as the whitespace so that in the future we can paste the changed version of the IDL and do a git diff to see which parts of the API have changed.  So, please just paste the definition from the spec directly (without the [Exposed] part, of course.)

	John Shih Assignee
	Comment 48 • 10 years ago

Fix nit

	Brad Lassey [:blassey] (use needinfo?)
	Comment 49 • 10 years ago

Comment on attachment 8394871 [details] [diff] [review]
Bug 960426 - Part 2: Modification for IDL change. r=blassey

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

::: dom/network/interfaces/nsIConnection.idl
@@ +6,5 @@
> +
> +[scriptable, uuid(5bb9a486-30ee-4324-ac99-f9f84182fd7e)]
> +interface nsIConnection : nsISupports
> +{
> +};

Shouldn't this have a type attribute and the constants for the various types?

	(no longer active)
	Comment 50 • 10 years ago

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #49)
> Comment on attachment 8394871 [details] [diff] [review]
> Bug 960426 - Part 2: Modification for IDL change. r=blassey
> 
> Review of attachment 8394871 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/interfaces/nsIConnection.idl
> @@ +6,5 @@
> > +
> > +[scriptable, uuid(5bb9a486-30ee-4324-ac99-f9f84182fd7e)]
> > +interface nsIConnection : nsISupports
> > +{
> > +};
> 
> Shouldn't this have a type attribute and the constants for the various types?

No, I think we should remove this interface and make nsIMozNavigatorNetwork.connection be an nsISupports.

	John Shih Assignee
	Comment 51 • 10 years ago

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #49)
> Comment on attachment 8394871 [details] [diff] [review]
> Bug 960426 - Part 2: Modification for IDL change. r=blassey
> 
> Review of attachment 8394871 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/interfaces/nsIConnection.idl
> @@ +6,5 @@
> > +
> > +[scriptable, uuid(5bb9a486-30ee-4324-ac99-f9f84182fd7e)]
> > +interface nsIConnection : nsISupports
> > +{
> > +};
> 
> Shouldn't this have a type attribute and the constants for the various types?

Let me clarify this. Previously, NetInfo API is defined by xpidl, so there were nsIDOMMOzConnection.idl (including the attributes of API v2.0). Then, there was work [1] share the implementation with NetInfo API and created a new xpidl (aka nsINetworkProperties) in order to expose new attributes (isWifi, gateway) to necko. The way to obtain nsINetworkProperties in necko is: 1. get Navigator.mozConnection through nsIDOMNavigatorNetwork.idl 2. queryInterface through mozConnection (mozConnection is in the type of nsIDOMConnection).

After converting to webidl [2], nsIDOMConnection is replaced by dom::Connection. The API attribute is now defined in webidl. In external side (front-end), NetInfo API doesn't need to be obtained through nsIMozNavigatorNetwork.idl anymore. However, since it still needs to be used in internal side (necko), it is kept with only changing the type of mozConnection from nsIDOMMOzConnection to nsISupports. As a result, the logic in necko can consist with how it was before.

So if we want to not using nsISupports in nsIMozNavigatorNetwork.idl, the direct way is create a new (empty) xpidl and make it inherited by NetInfo. (That's what I do in the patch)

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=888268
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=952084

	Johnny Stenback (:jst)
	Comment 52 • 10 years ago

Can we not just use nsISupports in place of nsIConnection here? Or do we need something class specific to QI to in order to know that the object actually is what we expect it to be etc?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 53 • 10 years ago

Comment on attachment 8391094 [details] [diff] [review]
Bug 960426 - Part 1: WebIDL changes for NetInfo API v3. r=sicking, marcosc

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

r=me with Ehsans comments.

::: dom/webidl/NetworkInformation.webidl
@@ +13,3 @@
>  };
>  
> +[Pref="dom.network.enabled"]

Nit: Maybe call this dom.netinfo.enabled or dom.networkinfo.enabled. The pref isn't actually disabling access to the network :)

	Vincent Chang[:vchang][changyihsin]
	Comment 54 • 10 years ago

Comment on attachment 8393989 [details] [diff] [review]
Bug 960426 - Part 4: Support Network Information API in FFOS. r=vchange

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

I am not sure if it is right position to put network stuff in nsAppShell.cpp. Maybe we should put them in NetworkService.js ?

::: widget/gonk/nsAppShell.cpp
@@ +55,5 @@
>  #include "nsThreadUtils.h"
>  #include "nsWindow.h"
>  #include "OrientationObserver.h"
>  #include "GonkMemoryPressureMonitoring.h"
> +#include "nsINetworkManager.h" // For nsINetworkInterface

Nit: we don't need this comment here.

@@ +264,5 @@
>      return nsWindow::DispatchInputEvent(event, captured);
>  }
>  
> +static int32_t
> +getConnectionType(int32_t state, int32_t type)

s/getConnectionType/ConvertConnectType

@@ +277,5 @@
> +        case nsINetworkInterface::NETWORK_TYPE_MOBILE:
> +            return 0; // ConnectionType::Cellular
> +        default:
> +            return 4; // ConnectionType::Other
> +    }

Please enum these return values.

@@ +965,5 @@
> +        nsCOMPtr<nsINetworkInterface> network = do_QueryInterface(aSubject);
> +        if (network) {
> +            int32_t state;
> +            int32_t type;
> +            nsString gwAddress;

Nit: use nsAutoString for local variable.

@@ +977,5 @@
> +            }
> +
> +            hal::NotifyNetworkChange(hal::NetworkInformation(getConnectionType(state, type),
> +                                                             (type == nsINetworkInterface::NETWORK_TYPE_WIFI) ? true : false,
> +                                                             convertIpAddrtoInt(gwAddress)));

Nit: 80 characters or less per line.

	Brad Lassey [:blassey] (use needinfo?)
	Comment 55 • 10 years ago

John, can you explain why you don't want to add a connectionType attribute to the nsIConnection.idl?

	John Shih Assignee
	Comment 56 • 10 years ago

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #55)
> John, can you explain why you don't want to add a connectionType attribute
> to the nsIConnection.idl?

I was trying to explain in Comment 51, but may not that clear...:(

In short, connectionType attribute is already defined in NetworkInformation.webidl. The purpose of nsIConnection.idl is to replace nsISupports using in internal query (nsHttpHandler.cpp [1]), where only the attributes of nsINetworkProperties is needed.

[1] http://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpHandler.cpp#1927

	Brad Lassey [:blassey] (use needinfo?)
	Comment 57 • 10 years ago

An empty interface isn't useful to anyone. The return type of this should be the interface that's being used by callers. You seem to say that that is nsINetworkProperties now.

	John Shih Assignee
	Comment 59 • 10 years ago

patch update based on previous suggestion.

	Vincent Chang[:vchang][changyihsin]
	Comment 62 • 10 years ago

Comment on attachment 8397002 [details] [diff] [review]
Bug 960426 - Part 4: Support Network Information API in FFOS. r=vchange

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

Looks good. r=me with comments addressed.

::: dom/system/gonk/NetworkManager.js
@@ +661,5 @@
>  
> +  convertConnectionType: function(network) {
> +    if (network.type != Ci.nsINetworkInterface.NETWORK_TYPE_WIFI &&
> +        network.type != Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE) {
> +      return null;

Can you write the comments here to explain why null is return here ?

::: widget/gonk/nsAppShell.cpp
@@ +918,5 @@
>  {
> +    if (!strcmp(aTopic, "network-connection-state-changed")) {
> +        nsAutoString connectionType(aData);
> +        if (!connectionType.IsEmpty()) {
> +            const char* type = NS_ConvertUTF16toUTF8(connectionType).get();

You don't need nsAutoString here. You can use NS_ConvertUTF16toUTF8 type(aData).

	John Shih Assignee
	Comment 63 • 10 years ago

update based on Comment 62

	John Shih Assignee
	Comment 65 • 10 years ago

In the work, we changed the interface name of NetworkInformation API from original 'MozConnection' to 'NetworkInformation' based on the W3C spec. Unfortunately, it caused browser_dpg_variables-view-filter-03.js test failed due to 'NetworkInformation' contains the substring 'two'. When using 'two' to filter Global Variables, it would find one variable, which was unexpected in the test. 

The patch intends to fix the bug.
Past, are you the right guy to review the patch? If not, could you help on finding someone? Thanks!

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 66 • 10 years ago

Comment on attachment 8397776 [details] [diff] [review]
Bug 960426 - Part 6: Fix conflict in browser_dpg_variables-view-filter-03.js. r=past

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

::: browser/devtools/debugger/test/doc_with-frame.html
@@ +12,5 @@
>      <button onclick="test(10)">Click me!</button>
>  
>      <script type="text/javascript">
>        function test(aNumber) {
> +        var a, obj = { alpha: 1, beta: 2 };

Clever.

	John Shih Assignee
	Comment 67 • 10 years ago

try result : https://tbpl.mozilla.org/?tree=Try&rev=1822e649148d
Get some fail which looks unrelated to this bug. I think it's ready to go!

	Ryan VanderMeulen [:RyanVM]
	Comment 68 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae8a5c6d6340
https://hg.mozilla.org/integration/mozilla-inbound/rev/08c1d13ed194
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2a47d87402d
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed3ae51c434e
https://hg.mozilla.org/integration/mozilla-inbound/rev/d47b3f63c410
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f47bbd820a1

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 69 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/ae8a5c6d6340
https://hg.mozilla.org/mozilla-central/rev/08c1d13ed194
https://hg.mozilla.org/mozilla-central/rev/f2a47d87402d
https://hg.mozilla.org/mozilla-central/rev/ed3ae51c434e
https://hg.mozilla.org/mozilla-central/rev/d47b3f63c410
https://hg.mozilla.org/mozilla-central/rev/3f47bbd820a1

	Boris Zbarsky [:bzbarsky]
	Comment 70 • 10 years ago

Just making sure: the intent is to expose this to all web pages on fxos?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 71 • 10 years ago

Yes. I think we should expose this on both Fennec and FFOS for all pages. It's an improvement over what we're exposing now in that it seems to have better signals from other implementers that they are happy with the API.

	u279076
	Comment 72 • 10 years ago

Changing OS to Gonk since this is a FirefoxOS feature.

	(no longer active)
	Comment 73 • 10 years ago

Was this intentionally turned on for desktop and Fennec?  Currently navigator.connection.type returns "none" on desktop which violates the spec.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 74 • 10 years ago

Turning it on for Fennec was definitely intentional. Though I thought that we returned "mobile" or "wifi" there as appropriate (and maybe bluetooth?). Fennec was already shipping the old network information API, so this was an improvement over that.

For desktop it does indeed seem bad if we always return "none". We should either return a correct value ("wifi" or "ethernet" seems fine), or disable the API completely.

This is about to hit beta, so we should move sooner rather than later if any changes are needed.

	John Shih Assignee
	Comment 75 • 10 years ago

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #73)
> Was this intentionally turned on for desktop and Fennec?  Currently
> navigator.connection.type returns "none" on desktop which violates the spec.

The API currently is only supported on Fennec and B2G (previously only on Fennec). If we want to turned it on for desktop I can do the follow-up work.

	John Shih Assignee
	Comment 76 • 10 years ago

Note that supporting the API for desktop would be kind of complex because we need to consider about the back-end of different platforms. (That is, we need find way to obtain network information on Linux, Window, Mac OS)

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 77 • 10 years ago

John: The main problem is that the API *is* turned on on desktop right now!

We should turn it off since as you say, it isn't properly implemented there yet.

On desktop, |"connection" in navigator| should return false. I.e. the property should not be there at all.

	John Shih Assignee
	Comment 78 • 10 years ago

(In reply to Jonas Sicking (:sicking) from comment #77)
> John: The main problem is that the API *is* turned on on desktop right now!
> 
> We should turn it off since as you say, it isn't properly implemented there
> yet.
> 
> On desktop, |"connection" in navigator| should return false. I.e. the
> property should not be there at all.

Understood. I'll disable it on desktop in the follow-up bug.

	Marcos Caceres [:marcosc]
	Comment 79 • 10 years ago

Just a minor heads up that, after discussion with folks from Google, I've added an `unknown` connection type to the `ConnectionType` enum. The `unknown` connection type is defined as: "The user agent has established a network connection, but is unable to determine what the connection type is."

That is subtly different from `other`, in that the connection type is known by the UA, but doesn't fit into one of the definitions for the `ConnectionType` enum.

Should I open a new bug for this?

	John Shih Assignee
	Comment 80 • 10 years ago

(In reply to Marcos Caceres [:marcosc] from comment #79)
> Just a minor heads up that, after discussion with folks from Google, I've
> added an `unknown` connection type to the `ConnectionType` enum. The
> `unknown` connection type is defined as: "The user agent has established a
> network connection, but is unable to determine what the connection type is."
> 
> That is subtly different from `other`, in that the connection type is known
> by the UA, but doesn't fit into one of the definitions for the
> `ConnectionType` enum.
> 
> Should I open a new bug for this?

Can you give some cases about get type 'unknown' instead of 'other'?
From the implementation view, if connection could be established by the back-end, it usually means that such type of connection is supported by the system. Then, a type not listed in 'ConnectionType' enum can be classified into 'other'. Not sure what would the 'unknown' case like. Thanks!

	Marcos Caceres [:marcosc]
	Comment 81 • 10 years ago

It seems the person implementing in Chrome (Josh Karlin) wants to use "unknown" as a stopgap. This is what Josh said: 

"It's probably possible to get all of these types on all of the platforms that browsers run on, but there are several cases in Chromium (e.g., desktop) where bluetooth and cellular are not distinguished. We may fill them all out eventually but it will take time."

The discussion is at [1]. 

So, it might be the case that "unknown" never applies to us in Gecko/Gonk, as we might be able to reliably determine all the types. But it might also help with future-proofing the API as new networking types become available. 

[1] https://github.com/w3c/netinfo/issues/11

	John Shih Assignee
	Comment 82 • 10 years ago

(In reply to Marcos Caceres [:marcosc] from comment #81)
> It seems the person implementing in Chrome (Josh Karlin) wants to use
> "unknown" as a stopgap. This is what Josh said: 
> 
> "It's probably possible to get all of these types on all of the platforms
> that browsers run on, but there are several cases in Chromium (e.g.,
> desktop) where bluetooth and cellular are not distinguished. We may fill
> them all out eventually but it will take time."
> 
> The discussion is at [1]. 
> 
> So, it might be the case that "unknown" never applies to us in Gecko/Gonk,
> as we might be able to reliably determine all the types. But it might also
> help with future-proofing the API as new networking types become available. 
> 
> [1] https://github.com/w3c/netinfo/issues/11

Thanks for your information! So based my understanding, adding type 'unknown' might not be applied in both B2G/Fennc in current implementation. But since now it's a part of new spec and we may probably need it someday, I'll do a follow-up work to add 'unknown' into ConnectionType enum (instead of re-open this bug ;)). Does it work for you?

	Marcos Caceres [:marcosc]
	Comment 83 • 10 years ago

(In reply to John Shih from comment #82)
> Does it work for you?

Sounds great! Will be sure to let you know if there is any further feedback from the Chrome team. 

Please also don't hesitate to let me know if you find any issues with the API.

	Jean-Yves Perrier [:teoli]
	Comment 84 • 9 years ago

Updated Connection and renamed it to NetworkInformation:
https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation
and
https://developer.mozilla.org/en-US/Firefox/Releases/31#InterfacesAPIsDOM