The default bug view has changed. See this FAQ.

Support Network Information API in Firefox OS

RESOLVED FIXED in mozilla31

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: John Shih, Assigned: John Shih)

Tracking

(Blocks: 3 bugs, {dev-doc-complete, feature, relnote})

unspecified
mozilla31
x86_64
Gonk (Firefox OS)
dev-doc-complete, feature, relnote
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(relnote-b2g ?)

Details

(Whiteboard: [dependency: marketplace][dependency: marketplace-partners])

Attachments

(6 attachments, 14 obsolete attachments)

24.52 KB, patch
John Shih
: review+
Details | Diff | Splinter Review
3.19 KB, patch
John Shih
: review+
Details | Diff | Splinter Review
1.42 KB, patch
John Shih
: review+
Details | Diff | Splinter Review
5.97 KB, patch
John Shih
: review+
Details | Diff | Splinter Review
17.34 KB, patch
blassey
: review+
Details | Diff | Splinter Review
9.29 KB, patch
past
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 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
(Assignee)

Updated

3 years ago
Assignee: nobody → jshih
(Assignee)

Comment 1

3 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
Flags: needinfo?
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
Flags: needinfo?(mcaceres)
Flags: needinfo?(jonas)
Component: RIL → DOM: Device Interfaces
Flags: needinfo?
Product: Firefox OS → Core
(Assignee)

Comment 3

3 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!
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
Flags: needinfo?(mcaceres)
(Assignee)

Comment 5

3 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
Flags: needinfo?(mcaceres)
Flags: needinfo?(ferjmoreno)
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).
Flags: needinfo?(mcaceres)
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."
Flags: needinfo?(ferjmoreno)
(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.
Flags: needinfo?(jonas)
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.

Updated

3 years ago
Duplicate of this bug: 796539
Keywords: dev-doc-needed

Updated

3 years ago
Blocks: 971166
(Assignee)

Updated

3 years ago
No longer blocks: 796539
(Assignee)

Updated

3 years ago
Depends on: 952048
(Assignee)

Updated

3 years ago
Depends on: 952084
No longer depends on: 952048
(Assignee)

Comment 11

3 years ago
Status update:

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

[1] http://w3c.github.io/netinfo/
(Assignee)

Comment 12

3 years ago
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.
Attachment #8385965 - Flags: review?(mcaceres)
Attachment #8385965 - Flags: review?(jonas)
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".
(Assignee)

Comment 14

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

Fix Nits.
Attachment #8385965 - Attachment is obsolete: true
Attachment #8385965 - Flags: review?(mcaceres)
Attachment #8385965 - Flags: review?(jonas)
Attachment #8386006 - Flags: review?(mcaceres)
Attachment #8386006 - Flags: review?(jonas)
Attachment #8386006 - Flags: review?(mcaceres) → review+
Attachment #8386006 - Flags: review?(jonas) → review+
But keep in mind that this can't land until we've changed the Android implementation which currently uses the old webidl.
(Assignee)

Comment 16

3 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.
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.
(Assignee)

Comment 18

3 years ago
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.
(Assignee)

Comment 19

3 years ago
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.
Attachment #8387484 - Flags: review?(dougt)
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?
Attachment #8387484 - Flags: review?(dougt) → review?(blassey.bugs)
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?
Attachment #8387484 - Flags: review?(blassey.bugs) → review+

Updated

3 years ago
Whiteboard: [apps watch list]
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>
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.
Flags: needinfo?(jonas)
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.
Flags: needinfo?(jonas)
(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.
(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
(Assignee)

Comment 27

3 years ago
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.
Attachment #8386006 - Attachment is obsolete: true
Attachment #8391094 - Flags: review?(jonas)
Attachment #8391094 - Flags: review?(ehsan)
(Assignee)

Comment 28

3 years ago
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.
Attachment #8387480 - Attachment is obsolete: true
Attachment #8391097 - Flags: review?(blassey.bugs)
(Assignee)

Comment 29

3 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.
Attachment #8391094 - Flags: review?(jonas)
Attachment #8391094 - Flags: review?(ehsan)
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?
(Assignee)

Comment 31

3 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
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.
Attachment #8391097 - Flags: review?(blassey.bugs) → review+
(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?
(Assignee)

Comment 34

3 years ago
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)
Attachment #8387484 - Attachment is obsolete: true
Attachment #8392013 - Flags: review?(blassey.bugs)
I'd love to get this on the Firefox 31 train, which I think this means that it needs to land tomorrow.
(Assignee)

Comment 36

3 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.
(Assignee)

Comment 37

3 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.. :(
Attachment #8392013 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 38

3 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.
Attachment #8391094 - Flags: review?(jonas)
Attachment #8391094 - Flags: review?(ehsan)
(Assignee)

Comment 39

3 years ago
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().
Attachment #8393989 - Flags: review?(vchang)
(Assignee)

Comment 40

3 years ago
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.
Attachment #8392013 - Attachment is obsolete: true
Attachment #8394013 - Flags: review+
(Assignee)

Comment 41

3 years ago
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
Attachment #8391097 - Attachment is obsolete: true
Attachment #8394033 - Flags: review?(blassey.bugs)
(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.
(Assignee)

Comment 43

3 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.
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
Attachment #8394033 - Flags: review?(blassey.bugs) → review-
(Assignee)

Comment 45

3 years ago
Created attachment 8394697 [details] [diff] [review]
Bug 960426 - Part 2: Modification for IDL change. r=blassey

patch update.
Attachment #8394033 - Attachment is obsolete: true
Attachment #8394697 - Flags: review?(blassey.bugs)
(Assignee)

Comment 46

3 years ago
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.
Attachment #8394702 - Flags: review?(jonas)
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.)
Attachment #8391094 - Flags: review?(ehsan) → review+
(Assignee)

Comment 48

3 years ago
Created attachment 8394871 [details] [diff] [review]
Bug 960426 - Part 2: Modification for IDL change. r=blassey

Fix nit
Attachment #8394697 - Attachment is obsolete: true
Attachment #8394697 - Flags: review?(blassey.bugs)
Attachment #8394871 - Flags: review?(blassey.bugs)
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?
(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.
(Assignee)

Comment 51

3 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
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 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 :)
Attachment #8391094 - Flags: review?(jonas) → review+
Attachment #8394702 - Flags: review?(jonas) → review+
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.
Attachment #8393989 - Flags: review?(vchang)
John, can you explain why you don't want to add a connectionType attribute to the nsIConnection.idl?
Flags: needinfo?(jshih)
(Assignee)

Comment 56

3 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
Flags: needinfo?(jshih)
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.
Attachment #8394871 - Flags: review?(blassey.bugs) → review-
(Assignee)

Comment 58

3 years ago
Created attachment 8396999 [details] [diff] [review]
Bug 960426 - Part 2: Modification for IDL change. r=blassey
Attachment #8394871 - Attachment is obsolete: true
Attachment #8396999 - Flags: review?(blassey.bugs)
(Assignee)

Comment 59

3 years ago
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.
Attachment #8391094 - Attachment is obsolete: true
Attachment #8397001 - Flags: review+
(Assignee)

Comment 60

3 years ago
Created attachment 8397002 [details] [diff] [review]
Bug 960426 - Part 4: Support Network Information API in FFOS. r=vchange
Attachment #8393989 - Attachment is obsolete: true
Attachment #8397002 - Flags: review?(vchang)
(Assignee)

Comment 61

3 years ago
Created attachment 8397575 [details] [diff] [review]
Bug 960426 - Part 5: Add check of NetworkInformation into test_interfaces.html. r=sicking
Attachment #8394702 - Attachment is obsolete: true
Attachment #8397575 - Flags: review+
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).
Attachment #8397002 - Flags: review?(vchang) → review+
(Assignee)

Comment 63

3 years ago
Created attachment 8397666 [details] [diff] [review]
Bug 960426 - Part 4: Support Network Information API in FFOS. r=vchange

update based on Comment 62
Attachment #8397002 - Attachment is obsolete: true
Attachment #8397666 - Flags: review+
(Assignee)

Comment 64

3 years ago
Created attachment 8397775 [details] [diff] [review]
Bug 960426 - Part 2: Modification for IDL change. r=blassey
Attachment #8396999 - Attachment is obsolete: true
Attachment #8396999 - Flags: review?(blassey.bugs)
Attachment #8397775 - Flags: review?(blassey.bugs)
(Assignee)

Comment 65

3 years ago
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!
Attachment #8397776 - Flags: review?(past)
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.
Attachment #8397776 - Flags: review?(past) → review+
Attachment #8397775 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 67

3 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!
Keywords: checkin-needed
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
Flags: in-testsuite?
Keywords: checkin-needed
Whiteboard: [apps watch list] → [apps watch list]
(Assignee)

Updated

3 years ago
Blocks: 673923
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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Just making sure: the intent is to expose this to all web pages on fxos?
Flags: needinfo?(jonas)
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.
Flags: needinfo?(jonas)
Depends on: 993435

Updated

3 years ago
relnote-b2g: --- → ?
Keywords: feature, relnote
Whiteboard: [apps watch list] → [apps watch list] [dependency: marketplace]
Changing OS to Gonk since this is a FirefoxOS feature.
OS: Linux → Gonk (Firefox OS)
Was this intentionally turned on for desktop and Fennec?  Currently navigator.connection.type returns "none" on desktop which violates the spec.
Flags: needinfo?(jshih)
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.
(Assignee)

Comment 75

3 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.
Flags: needinfo?(jshih)
(Assignee)

Updated

3 years ago
Blocks: 1020758
(Assignee)

Comment 76

3 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)
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.
(Assignee)

Comment 78

3 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.
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?
(Assignee)

Comment 80

3 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!
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
(Assignee)

Comment 82

3 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?
(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.
(Assignee)

Updated

3 years ago
Blocks: 1023029
Whiteboard: [apps watch list] [dependency: marketplace] → [dependency: marketplace][dependency: marketplace-partners]
Blocks: 793729
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.