Last Comment Bug 960426 - Support Network Information API in Firefox OS
: Support Network Information API in Firefox OS
Status: RESOLVED FIXED
[dependency: marketplace][dependency:...
: dev-doc-complete, feature, relnote
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: x86_64 Gonk (Firefox OS)
-- normal (vote)
: mozilla31
Assigned To: John Shih
:
: Andrew Overholt [:overholt]
Mentors:
: 796539 (view as bug list)
Depends on: 677166 952084 993435
Blocks: webapi 793729 971166 1020758 1023029
  Show dependency treegraph
 
Reported: 2014-01-16 00:04 PST by John Shih
Modified: 2015-10-04 07:02 PDT (History)
30 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
?


Attachments
Bug 960426 - Part 1: WebIDL changes for NetInfo API v3. r=sicking, marcosc (1.05 KB, patch)
2014-03-05 01:18 PST, John Shih
no flags Details | Diff | Splinter Review
Bug 960426 - Part 1: WebIDL changes for NetInfo API v3. r=sicking, marcosc (1.12 KB, patch)
2014-03-05 02:34 PST, John Shih
jonas: review+
mcaceres: review+
Details | Diff | Splinter Review
(wip) Bug 960426 - Part 2: Implementation. r=vchang (10.84 KB, patch)
2014-03-07 02:26 PST, John Shih
no flags Details | Diff | Splinter Review
Bug 960426 - Part 3: Related change in Fennec. r=dougt (25.54 KB, patch)
2014-03-07 02:28 PST, John Shih
blassey.bugs: review+
Details | Diff | Splinter Review
Bug 960426 - Part 1: WebIDL changes for NetInfo API v3. r=sicking, marcosc (1.92 KB, patch)
2014-03-14 02:32 PDT, John Shih
jonas: review+
ehsan: review+
Details | Diff | Splinter Review
Bug 960426 - Part 2: Modification for IDL change. r=blassey (16.31 KB, patch)
2014-03-14 02:38 PDT, John Shih
blassey.bugs: review+
Details | Diff | Splinter Review
Bug 960426 - Part 3: Related change in Fennec. r=blassey (25.51 KB, patch)
2014-03-16 19:51 PDT, John Shih
blassey.bugs: review+
Details | Diff | Splinter Review
Bug 960426 - Part 4: Support Network Information API in FFOS. r=vchange (5.54 KB, patch)
2014-03-20 00:35 PDT, John Shih
no flags Details | Diff | Splinter Review
Bug 960426 - Part 3: Related change in Fennec. r=blassey (24.52 KB, patch)
2014-03-20 02:33 PDT, John Shih
johnshih.bugs: review+
Details | Diff | Splinter Review
Bug 960426 - Part 2: Modification for IDL change. r=blassey (15.66 KB, patch)
2014-03-20 03:23 PDT, John Shih
blassey.bugs: review-
Details | Diff | Splinter Review
Bug 960426 - Part 2: Modification for IDL change. r=blassey (19.31 KB, patch)
2014-03-21 03:39 PDT, John Shih
no flags Details | Diff | Splinter Review
Bug 960426 - Part 5: Add check of NetworkInformation into test_interfaces.html. r=sicking (1.42 KB, patch)
2014-03-21 03:42 PDT, John Shih
jonas: review+
Details | Diff | Splinter Review
Bug 960426 - Part 2: Modification for IDL change. r=blassey (14.42 KB, patch)
2014-03-21 09:11 PDT, John Shih
blassey.bugs: review-
Details | Diff | Splinter Review
Bug 960426 - Part 2: Modification for IDL change. r=blassey (17.36 KB, patch)
2014-03-26 03:07 PDT, John Shih
no flags Details | Diff | Splinter Review
Bug 960426 - Part 1: WebIDL changes for NetInfo API v3. r=sicking, marcosc (3.19 KB, patch)
2014-03-26 03:08 PDT, John Shih
johnshih.bugs: review+
Details | Diff | Splinter Review
Bug 960426 - Part 4: Support Network Information API in FFOS. r=vchange (5.86 KB, patch)
2014-03-26 03:09 PDT, John Shih
changyihsin: review+
Details | Diff | Splinter Review
Bug 960426 - Part 5: Add check of NetworkInformation into test_interfaces.html. r=sicking (1.42 KB, patch)
2014-03-26 20:20 PDT, John Shih
johnshih.bugs: review+
Details | Diff | Splinter Review
Bug 960426 - Part 4: Support Network Information API in FFOS. r=vchange (5.97 KB, patch)
2014-03-27 01:11 PDT, John Shih
johnshih.bugs: review+
Details | Diff | Splinter Review
Bug 960426 - Part 2: Modification for IDL change. r=blassey (17.34 KB, patch)
2014-03-27 04:35 PDT, John Shih
blassey.bugs: review+
Details | Diff | Splinter Review
Bug 960426 - Part 6: Fix conflict in browser_dpg_variables-view-filter-03.js. r=past (9.29 KB, patch)
2014-03-27 04:44 PDT, John Shih
past: review+
Details | Diff | Splinter Review

Description User image John Shih 2014-01-16 00:04:52 PST
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
Comment 1 User image John Shih 2014-01-16 01:33:17 PST
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
Comment 2 User image Fernando Jiménez Moreno [:ferjm] 2014-01-16 02:15:49 PST
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
Comment 3 User image John Shih 2014-01-16 02:33:53 PST
(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!
Comment 4 User image Marcos Caceres [:marcosc] 2014-01-16 05:03:37 PST
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
Comment 5 User image John Shih 2014-01-20 18:40:25 PST
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
Comment 6 User image Marcos Caceres [:marcosc] 2014-01-21 01:59:13 PST
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).
Comment 7 User image Fernando Jiménez Moreno [:ferjm] 2014-01-22 07:29:38 PST
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."
Comment 8 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2014-01-27 18:08:13 PST
(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.
Comment 9 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2014-01-27 18:16:57 PST
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.
Comment 10 User image Fred Lin [:gasolin] 2014-02-05 01:51:28 PST
*** Bug 796539 has been marked as a duplicate of this bug. ***
Comment 11 User image John Shih 2014-03-03 19:38:05 PST
Status update:

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

[1] http://w3c.github.io/netinfo/
Comment 12 User image John Shih 2014-03-05 01:18:43 PST
Created attachment 8385965 [details] [diff] [review]
Bug 960426 - Part 1: WebIDL changes for NetInfo API v3. r=sicking, marcosc

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.
Comment 13 User image Marcos Caceres [:marcosc] 2014-03-05 02:09:07 PST
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".
Comment 14 User image John Shih 2014-03-05 02:34:46 PST
Created attachment 8386006 [details] [diff] [review]
Bug 960426 - Part 1: WebIDL changes for NetInfo API v3. r=sicking, marcosc

Fix Nits.
Comment 15 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2014-03-05 10:06:26 PST
But keep in mind that this can't land until we've changed the Android implementation which currently uses the old webidl.
Comment 16 User image John Shih 2014-03-05 17:40:17 PST
(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.
Comment 17 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2014-03-05 22:54:15 PST
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.
Comment 18 User image John Shih 2014-03-07 02:26:22 PST
Created attachment 8387480 [details] [diff] [review]
(wip) Bug 960426 - Part 2: Implementation. r=vchang

(WIP) Do the corresponding modification for interface change. Next step will be supporting this API in FFOS.
Comment 19 User image John Shih 2014-03-07 02:28:28 PST
Created attachment 8387484 [details] [diff] [review]
Bug 960426 - Part 3: Related change in Fennec. r=dougt

Corresponding modification in fennec. I've verified it works on my testing Android device.
Comment 20 User image Doug Turner (:dougt) 2014-03-08 03:15:59 PST
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?
Comment 21 User image Brad Lassey [:blassey] (use needinfo?) 2014-03-10 16:19:10 PDT
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?
Comment 22 User image :Ehsan Akhgari 2014-03-13 17:45:14 PDT
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>
Comment 23 User image :Ehsan Akhgari 2014-03-13 17:46:11 PDT
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.
Comment 24 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2014-03-13 19:16:13 PDT
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.
Comment 25 User image :Ehsan Akhgari 2014-03-13 19:40:11 PDT
(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.
Comment 26 User image Masatoshi Kimura [:emk] 2014-03-13 20:06:21 PDT
(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
Comment 27 User image John Shih 2014-03-14 02:32:10 PDT
Created attachment 8391094 [details] [diff] [review]
Bug 960426 - Part 1: WebIDL changes for NetInfo API v3. r=sicking, marcosc

Patch update based on Comment 22.
Comment 28 User image John Shih 2014-03-14 02:38:40 PDT
Created attachment 8391097 [details] [diff] [review]
Bug 960426 - Part 2: Modification for IDL change. r=blassey

Separated the modification for IDL change from implementation (support API in FFOS). Gecko support will be in another part.
Comment 29 User image John Shih 2014-03-14 02:50:31 PDT
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.
Comment 30 User image Brad Lassey [:blassey] (use needinfo?) 2014-03-14 08:38:26 PDT
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?
Comment 31 User image John Shih 2014-03-14 09:02:10 PDT
(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
Comment 32 User image Brad Lassey [:blassey] (use needinfo?) 2014-03-14 12:56:13 PDT
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.
Comment 33 User image :Ehsan Akhgari 2014-03-14 13:09:12 PDT
(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?
Comment 34 User image John Shih 2014-03-16 19:51:42 PDT
Created attachment 8392013 [details] [diff] [review]
Bug 960426 - Part 3: Related change in Fennec. r=blassey

Update based on previous review.
(Note: namespace mozilla:dom is for using ConnectionType directly, otherwise it requires mozilla:dom:ConnectionType)
Comment 35 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2014-03-16 22:29:48 PDT
I'd love to get this on the Firefox 31 train, which I think this means that it needs to land tomorrow.
Comment 36 User image John Shih 2014-03-17 01:30:18 PDT
(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.
Comment 37 User image John Shih 2014-03-17 01:34:47 PDT
(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.. :(
Comment 38 User image John Shih 2014-03-20 00:23:18 PDT
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.
Comment 39 User image John Shih 2014-03-20 00:35:30 PDT
Created attachment 8393989 [details] [diff] [review]
Bug 960426 - Part 4: Support Network Information API in FFOS. r=vchange

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().
Comment 40 User image John Shih 2014-03-20 02:33:01 PDT
Created attachment 8394013 [details] [diff] [review]
Bug 960426 - Part 3: Related change in Fennec. r=blassey

Small update: do not use dom::network::ConnectionType anymore.
Comment 41 User image John Shih 2014-03-20 03:23:33 PDT
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.

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
Comment 42 User image Brad Lassey [:blassey] (use needinfo?) 2014-03-20 14:59:42 PDT
(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.
Comment 43 User image John Shih 2014-03-20 18:52:13 PDT
(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.
Comment 44 User image Brad Lassey [:blassey] (use needinfo?) 2014-03-20 19:04:45 PDT
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
Comment 45 User image John Shih 2014-03-21 03:39:02 PDT
Created attachment 8394697 [details] [diff] [review]
Bug 960426 - Part 2: Modification for IDL change. r=blassey

patch update.
Comment 46 User image John Shih 2014-03-21 03:42:30 PDT
Created attachment 8394702 [details] [diff] [review]
Bug 960426 - Part 5: Add check of NetworkInformation into test_interfaces.html. r=sicking

Fix the test failed in test_interfaces.html due to adding a new interface.
Comment 47 User image :Ehsan Akhgari 2014-03-21 08:22:18 PDT
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.)
Comment 48 User image John Shih 2014-03-21 09:11:47 PDT
Created attachment 8394871 [details] [diff] [review]
Bug 960426 - Part 2: Modification for IDL change. r=blassey

Fix nit
Comment 49 User image Brad Lassey [:blassey] (use needinfo?) 2014-03-21 09:50:51 PDT
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?
Comment 50 User image :Ehsan Akhgari 2014-03-21 09:56:05 PDT
(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.
Comment 51 User image John Shih 2014-03-21 10:53:54 PDT
(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
Comment 52 User image Johnny Stenback (:jst, jst@mozilla.com) 2014-03-21 13:57:09 PDT
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?
Comment 53 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2014-03-21 15:04:09 PDT
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 :)
Comment 54 User image Vincent Chang[:vchang][changyihsin] 2014-03-22 21:23:11 PDT
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.
Comment 55 User image Brad Lassey [:blassey] (use needinfo?) 2014-03-24 10:17:00 PDT
John, can you explain why you don't want to add a connectionType attribute to the nsIConnection.idl?
Comment 56 User image John Shih 2014-03-24 18:59:51 PDT
(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
Comment 57 User image Brad Lassey [:blassey] (use needinfo?) 2014-03-25 06:11:11 PDT
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.
Comment 58 User image John Shih 2014-03-26 03:07:44 PDT
Created attachment 8396999 [details] [diff] [review]
Bug 960426 - Part 2: Modification for IDL change. r=blassey
Comment 59 User image John Shih 2014-03-26 03:08:43 PDT
Created attachment 8397001 [details] [diff] [review]
Bug 960426 - Part 1: WebIDL changes for NetInfo API v3. r=sicking, marcosc

patch update based on previous suggestion.
Comment 60 User image John Shih 2014-03-26 03:09:18 PDT
Created attachment 8397002 [details] [diff] [review]
Bug 960426 - Part 4: Support Network Information API in FFOS. r=vchange
Comment 61 User image John Shih 2014-03-26 20:20:49 PDT
Created attachment 8397575 [details] [diff] [review]
Bug 960426 - Part 5: Add check of NetworkInformation into test_interfaces.html. r=sicking
Comment 62 User image Vincent Chang[:vchang][changyihsin] 2014-03-26 23:59:24 PDT
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).
Comment 63 User image John Shih 2014-03-27 01:11:18 PDT
Created attachment 8397666 [details] [diff] [review]
Bug 960426 - Part 4: Support Network Information API in FFOS. r=vchange

update based on Comment 62
Comment 64 User image John Shih 2014-03-27 04:35:12 PDT
Created attachment 8397775 [details] [diff] [review]
Bug 960426 - Part 2: Modification for IDL change. r=blassey
Comment 65 User image John Shih 2014-03-27 04:44:22 PDT
Created attachment 8397776 [details] [diff] [review]
Bug 960426 - Part 6: Fix conflict in browser_dpg_variables-view-filter-03.js. r=past

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!
Comment 66 User image Panos Astithas [:past] 2014-03-27 06:58:16 PDT
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.
Comment 67 User image John Shih 2014-03-27 07:17:48 PDT
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!
Comment 70 User image Boris Zbarsky [:bz] (still a bit busy) 2014-03-27 20:23:22 PDT
Just making sure: the intent is to expose this to all web pages on fxos?
Comment 71 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2014-03-27 22:57:08 PDT
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.
Comment 72 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2014-05-07 13:31:42 PDT
Changing OS to Gonk since this is a FirefoxOS feature.
Comment 73 User image :Ehsan Akhgari 2014-06-04 14:20:37 PDT
Was this intentionally turned on for desktop and Fennec?  Currently navigator.connection.type returns "none" on desktop which violates the spec.
Comment 74 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2014-06-04 16:16:21 PDT
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.
Comment 75 User image John Shih 2014-06-04 18:49:06 PDT
(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.
Comment 76 User image John Shih 2014-06-05 01:27:28 PDT
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)
Comment 77 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2014-06-05 17:57:23 PDT
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.
Comment 78 User image John Shih 2014-06-06 00:46:00 PDT
(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.
Comment 79 User image Marcos Caceres [:marcosc] 2014-06-06 07:50:05 PDT
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?
Comment 80 User image John Shih 2014-06-06 08:30:11 PDT
(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!
Comment 81 User image Marcos Caceres [:marcosc] 2014-06-06 11:58:20 PDT
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
Comment 82 User image John Shih 2014-06-06 18:13:17 PDT
(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?
Comment 83 User image Marcos Caceres [:marcosc] 2014-06-09 11:00:22 PDT
(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.
Comment 84 User image Jean-Yves Perrier [:teoli] 2015-10-04 07:02:26 PDT
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

Note You need to log in before you can comment on or make changes to this bug.