This has been done for chrome in bug 667980 but we might want an API to expose to the content with some events when the connection changes. I didn't look closely at the API in bug 667980 so I don't have an opinion about keeping the same one or not. The Network Information API try to do that but isn't really satisfying: http://www.w3.org/TR/2011/WD-netinfo-api-20110607/

Yeah, I think we should add two more properties next to the current navigator.onLine property that we already have: One for telephony connectivity and one for type of data connectivity. So something like: navigator.onTele returns true if the device has connectivity to make/receive phone calls and text messages. Returns false otherwise. navigator.networktype returns "2g", "3g", "4g", "wifi", "wired", "other" and "none". The last option is return when, and only when, navigator.onLine returns false. We'd also need events firing when these properties change. Probably "ontele", "offtele" and "networktypechange". Also, the names of the properties/events above seriously needs better names :) We'd likely not want to expose the telephony connectivity except to privileged pages.

Exposing network connection type has some privacy issues, so either it should work only with privileged pages (whatever those are) or require explicit user permission. Maybe. Connection type sure can be used for fingerprinting, but I'm not sure how bad that case is. This feature should be security reviewed.

Absolutely agree on needing security review. I do think that we could expose network connectivity as it doesn't expose many bits of information that can be used for fingerprinting. The various wireless variants are very bad for fingerprinting as they vary a lot for a single user. What you can measure is wired vs. wireless, but you can get much of that same information from the OS type. Another option would be to collapse "wifi" and "wired". Being able to distinguish those doesn't seem terribly important.

I would prefer to return different value for .onTele (actually, I don't like that name...) to reflect the signal strength. Basically, an application might want to disable some features if the signal strength is "emergency call only" for example. Anyhow, this would require some privileges but that would probably be part of the WebAPI security story.

Network type sure gives you fingerprintable information. For example if you're always using 2G, you're probably somewhere in countryside, at least in Finland. But as I said, I'm not sure the issue is bad enough to worry about.

(In reply to Jonas Sicking (:sicking) from comment #1) > navigator.networktype returns "2g", "3g", "4g", "wifi", "wired", "other" and > "none". The last option is return when, and only when, navigator.onLine > returns false. Adding something like navigator.networkSpeed or .networkEstimatedSpeed might be a good idea to prevent authors to check for the connection type to assume the connection speed. In addition, it would be more future-proof than checking for .networkType to check if the user is using a fast connection or not.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #6) > Adding something like navigator.networkSpeed or .networkEstimatedSpeed might > be a good idea to prevent authors to check for the connection type to assume > the connection speed. In addition, it would be more future-proof than > checking for .networkType to check if the user is using a fast connection or > not. The problem with network speed is that unless I'm missing something it is hard to estimate well without regularly sending test payloads (you can't just use the page's initial load information, it may have been massively cached, and it may be long-running across radical network changes). That's not great in general, and when roaming it's just plain horrible. It can also be hard to distinguish your side being slow from the remote end being sluggish. And if you want to perform useful adaptation on the client, it's generally important to know latency as well as raw speed since it can make a huge difference. All of this plus the reasons cited above are why DAP has parked this spec while waiting for experimental input. Personally I'm not entirely convinced that you can actually do anything useful much with it. Android WebKit has supported it for a while now but I've had a hard time finding much in the way of real-world usage. PhoneGap seems to rely on it for some things, but it's unclear to me exactly why or if they're doing it for the right reasons.

The idea I had with network.speed isn't to give the actual network speed at a 1 kb/s precision but to give an estimate of it to prevent having this kind of code which wouldn't be future-proof at all: if (navigator.network.type == '3g') { // slow network code } or even worse: if (navigator.network.type != 'wifi' && navigator.network.type != 'ethernet') { // slow network code } I don't think it would hurt to have an implementation returning the maximal theoretical speed for the current connection type. Like 100Mbps for network.speed if the user is connected through an ethernet cable. It will be unlikely 100Mbps but I don't think anyone really cares to know if the connection is actually 10Mbps, 20Mbps or 100Mbps.

Backends implementation will be done in a follow-up. Also, when offline mozConnection.bandwidth should return 0, it will be done in a follow-up.

For the specification draft, see: https://wiki.mozilla.org/WebAPI/NetworkAPI

Comment on attachment 580852 [details] [diff] [review] Part 6 - Add NetworkInformation handling to Hal I'd like to review this patch after we agree on the interface changes in bug 709584, but it looks fine. > + * This method is semi-private in the sense of it is visible in the hal > + * namespace but should not be used. Calls to this method from the hal > + * namespace will produce a link error because it is not defined. Please indicate which namespaces this function *is* defined in. And rather than write this comment block three times, please reference the first one. > +++ b/dom/network/src/Types.h > +namespace mozilla { > +namespace hal { > +class NetworkInformation; > +} // namespace hal > + > +template <class T> > +class Observer; > + > +typedef Observer<hal::NetworkInformation> NetworkObserver; This is unfortunate. You can't get this by including Hal.h? Also, why is the Observer template declared in the mozilla namespace?

> + * If the connection is of a type that can be restricted. > + bool mCanBeRestricted; I have no idea what this means, so you should probably pick a different name.

(In reply to Justin Lebar [:jlebar] from comment #18) > Comment on attachment 580852 [details] [diff] [review] > Part 6 - Add NetworkInformation handling to Hal > > I'd like to review this patch after we agree on the interface changes in bug > 709584, but it looks fine. Why not reviewing it assuming the changes in bug 709584 are accepted? If they are not, I will have to change this patch anyway and ask for another review (if the changes are important enough). > > + * This method is semi-private in the sense of it is visible in the hal > > + * namespace but should not be used. Calls to this method from the hal > > + * namespace will produce a link error because it is not defined. > > Please indicate which namespaces this function *is* defined in. And rather > than write this comment block three times, please reference the first one. I've been thinking of a way to fix those horrible comments in my flight back from the work week. I will see if I can find the strenght to write the patch I've been thinking of. Basically, the idea is to have another header file that is not exported for internal methods. > > +++ b/dom/network/src/Types.h > > +namespace mozilla { > > +namespace hal { > > +class NetworkInformation; > > +} // namespace hal > > + > > +template <class T> > > +class Observer; > > + > > +typedef Observer<hal::NetworkInformation> NetworkObserver; > > This is unfortunate. You can't get this by including Hal.h? Also, why is > the Observer template declared in the mozilla namespace? I prefer to keep header file as much include-free as possible. And Observer is in the mozilla namespace for obvious reasons: that's a public API. (In reply to Justin Lebar [:jlebar] from comment #19) > > + * If the connection is of a type that can be restricted. > > + bool mCanBeRestricted; > > I have no idea what this means, so you should probably pick a different name. A connection can be restricted (see the draft spec). The backend can say to the DOM code if the current connection *can* technically be restricted. If yes, the DOM code use a preference to show it as restricted or not. Concretely, we might have an option in the settings saying "Do you have a quota or pay-per-use 3G connection?" If yes, it will be marked as restricted. I can add a comment but I don't see how to make that name clearer.

> Why not reviewing it assuming the changes in bug 709584 are accepted? If they are not, I will have > to change this patch anyway and ask for another review (if the changes are important enough). Right, I'm assuming the changes in bug 709584 will be accepted, and I'd like to review this patch after you change it to reflect those changes. > Basically, the idea is to have another header file that is not exported for internal methods. That would be great. > I prefer to keep header file as much include-free as possible. And Observer is in the mozilla > namespace for obvious reasons: that's a public API. I don't think you should define a class named mozilla::Observer. We already have ::nsObserver, and mozilla::Observer is not a generic observer. mozilla::hal::Observer would be fine, although per my comments in bug 709584, I think the name should be mozilla::hal::DeviceObserver, or something like that. I think |typedef Observer<hal::NetworkInformation> NetworkObserver| should be in the header where mozilla::Observer is defined and then you should include Hal.h. The templates are an implementation detail. (In my experience, a number of files "optimize" things by declaring nsTArray themselves rather than including nsTArray.h, and this makes your life unduly difficult if you want to add or remove a template parameter from nsTArray.) > I have no idea what this means, so you should probably pick a different name. > > A connection can be restricted (see the draft spec). I still don't understand what "restricted" means in this context, and the spec in comment 0 doesn't contain that word. It sounds like you might mean "metered", in the sense that "we care how much data goes over this connection." Could you please point me to the relevant definition?

(In reply to Justin Lebar [:jlebar] from comment #21) > I don't think you should define a class named mozilla::Observer. We already > have ::nsObserver, and mozilla::Observer is not a generic observer. > mozilla::hal::Observer would be fine, although per my comments in bug > 709584, I think the name should be mozilla::hal::DeviceObserver, or > something like that. > > I think |typedef Observer<hal::NetworkInformation> NetworkObserver| should > be in the header where mozilla::Observer is defined and then you should > include Hal.h. The templates are an implementation detail. (In my > experience, a number of files "optimize" things by declaring nsTArray > themselves rather than including nsTArray.h, and this makes your life unduly > difficult if you want to add or remove a template parameter from nsTArray.) Observer has nothing to do with hal. It's a generic API that has been added to our public API, see: xpcom/glue/Observer.h Basically, it's on observer/observed mechanism which is XPCOM free. > > I have no idea what this means, so you should probably pick a different name. > > > > A connection can be restricted (see the draft spec). > > I still don't understand what "restricted" means in this context, and the > spec in comment 0 doesn't contain that word. It sounds like you might mean > "metered", in the sense that "we care how much data goes over this > connection." Could you please point me to the relevant definition? 'restricted' or 'metered', as a non-native english speaker, I can't say. There is no relevant definition given that Jonas and I did wrote that draft. I think the attribute name will be bikesheded in the WG mailing list anyway. I'm okay to change the name to 'metered' but I don't care that much TBH. Jonas?

> I'm okay to change the name to 'metered' but I don't care that much TBH. Jonas? If "someone is keeping track of how much data is sent over this connection" is what you mean, you should use "metered" and not "restricted". If you mean something else, then "metered" may not be right. :) > Observer has nothing to do with hal. It's a generic API that has been added to our public API, see: > xpcom/glue/Observer.h Ah. Sorry; I should have looked it up before assuming you made it! I'm not really in favor of forward-declaring rather than #including things in header files (s/not really/really not/), but it's not a big deal.

Yeah, not sure what is the best word here. The idea is to tell the page when it's costing the user money to transfer bytes across the data connection.

(In reply to Jonas Sicking (:sicking) from comment #24) > Yeah, not sure what is the best word here. The idea is to tell the page when > it's costing the user money to transfer bytes across the data connection. It's not only about costing money. AFAICT, there are three kind of restrictions: - pay-per-mega - block after a certain monthly usage - reduce bandwidth after a certain monthly usage

"metered" seems to cover those three, because in all three cases, someone is keeping track of how much bandwidth you've used.

Mounir, have you tested an android implementation of checking for these values? I am worried that sometimes it can take time to query these values from native code through JNI. Since the API is synchronous, each call will block gecko (on android) as we reach into JNI to query their values. It would be good to know that this query is going to be fast enough to not be a problem. (slow web apis should be asynchronous).

This problem has already been encountered and solved with Battery API. We are going to follow the same idea. Basically, it is a observer/observable mechanism: the Android backend will listen to changes and notify them. This notification will go up to the DOM and will be saved by the DOM object and an object per process in hal/. When a DOM object needs a value, it will read it from it's saved information. When created, the DOM object will have to retrieve those information by requesting them trough hal. If hal have them stored, it will just return them. Otherwise hal will have to ask them to the backend. This is the only blocking call. To summarize, there will be only one blocking call: when we go from 0 to 1 Connection object in a process. We could even try to prevent that but that would go with some other costs.

Comment on attachment 583139 [details] [diff] [review] Part 6 - Add NetworkInformation handling to Hal >+/** >+ * A set of constants that might need to be used by network backends. >+ */ s/that might need to be // >+namespace mozilla { >+namespace dom { >+namespace network { >+ >+ static const double kDefaultBandwidth = -1.0; >+ static const bool kDefaultCanBeMetered = false; s/CanBe/Is, here and in the structure definition. >+#ifndef mozilla_dom_network_Types_h >+#define mozilla_dom_network_Types_h >+ >+namespace mozilla { >+namespace hal { >+class NetworkInformation; >+} // namespace hal >+ >+template <class T> >+class Observer; >+ >+typedef Observer<hal::NetworkInformation> NetworkObserver; >+ >+} // namespace mozilla >+ >+#endif // mozilla_dom_network_Types_h >diff --git a/hal/Hal.h b/hal/Hal.h >--- a/hal/Hal.h >+++ b/hal/Hal.h >@@ -41,16 +41,17 @@ > #define mozilla_Hal_h 1 > > #include "mozilla/hal_sandbox/PHal.h" > #include "base/basictypes.h" > #include "mozilla/Types.h" > #include "nsTArray.h" > #include "prlog.h" > #include "mozilla/dom/battery/Types.h" >+#include "mozilla/dom/network/Types.h" The only relevant declaration in dom/network/Types.h that we can bring in is |typedef Observer<hal::NetworkInformation> NetworkObserver|. It seems to me that this (and dom/battery/Types.h) shouldn't be included here. Rather, that typedef and the corresponding BatteryObserver typedef should be declared here (because logically, they're hal types, not dom types). If it were me, I'd then #include Hal.h where you currently include either Types.h. I don't see the point of these small headers which forward-declare things from Hal.h -- it seems like a premature optimization for compilation time, and it's confusing, because it's not clear on face what's in Types.h or where those types are actually defined. But if you really think Types.h is a good idea, you could still move the typedefs into Hal.h, which would make sense to me. (Notice that this would put them into the hal namespace, which I think is fine.) >+namespace hal { >+ struct NetworkInformation { >+ double bandwidth; >+ bool canBeMetered; isMetered

Comment on attachment 583133 [details] [diff] [review] Part 2 - Add .mozConnection to navigator Review of attachment 583133 [details] [diff] [review]: ----------------------------------------------------------------- Why not just add the new property to nsIDOMNavigator?

(In reply to Jonas Sicking (:sicking) from comment #38) > Comment on attachment 583133 [details] [diff] [review] > Part 2 - Add .mozConnection to navigator > > Review of attachment 583133 [details] [diff] [review]: > ----------------------------------------------------------------- > > Why not just add the new property to nsIDOMNavigator? So we can disable the feature with a pref, see part 4.

Comment on attachment 583140 [details] [diff] [review] Part 7 - Full DOM implementation Review of attachment 583140 [details] [diff] [review]: ----------------------------------------------------------------- Looks good otherwise ::: dom/network/src/Connection.cpp @@ +129,2 @@ > *aMetered = Preferences::GetBool(sMeteredPrefName, > sMeteredDefaultValue); I think it'd be worth having a value to set this pref to which make .metered always return true, no matter what connection you're on.

Comment on attachment 583138 [details] [diff] [review] Part 5 - Use a pref for connection.metered Review of attachment 583138 [details] [diff] [review]: ----------------------------------------------------------------- Given the changes in the followup, r=me

https://hg.mozilla.org/mozilla-central/rev/c199854a52b7 https://hg.mozilla.org/mozilla-central/rev/e1cf27289600 https://hg.mozilla.org/mozilla-central/rev/4f8fb99e8e86 https://hg.mozilla.org/mozilla-central/rev/5c6dc5015a9a https://hg.mozilla.org/mozilla-central/rev/4a9f98d2f07a https://hg.mozilla.org/mozilla-central/rev/ae007b96ebf0 https://hg.mozilla.org/mozilla-central/rev/778597344568

Documented https://developer.mozilla.org/en/DOM/window.navigator.connection added to https://developer.mozilla.org/en/DOM/window.navigator https://developer.mozilla.org/en/Firefox_12_for_developers