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

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

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

(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

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

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).

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."

(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.

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.

Status update: The implementation is going to follow the new API proposal [1]. [1] http://w3c.github.io/netinfo/

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 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".

Fix Nits.

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

(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.

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

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

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 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 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.

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.

(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

Patch update based on Comment 22.

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

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 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?

(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.

(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?

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

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

(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.

(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 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.

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

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

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

(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.

(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

patch update.

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

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.)

Fix nit

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.

(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 :)

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.

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

(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

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.

patch update based on previous suggestion.

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).

update based on Comment 62

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 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.

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!

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

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

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

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.

Changing OS to Gonk since this is a FirefoxOS feature.

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

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.

(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.

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.

(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?

(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

(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.

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