Andrea Marchesini [:baku] Assignee
	Description • 11 years ago

      No description provided.

	Andrea Marchesini [:baku] Assignee
	Comment 2 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=e5ee5afebe14

	Andrea Marchesini [:baku] Assignee
	Comment 3 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=fdb8357759e3

I had to use: 
NS_IMPL_ADDREF_INHERITED(network::Connection, nsDOMEventTargetHelper)

because otherwise the hashtable used by GetBloatEntry gets confused.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 4 • 11 years ago

Comment on attachment 8350022 [details] [diff] [review]
mozconnection.patch

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

Same comment about namespacing.

f- for the unrestricted double bit; please write a test that would have asserted in the case that returns infinity. (If we don't assert in that case, please make sure to make us assert.)

::: dom/base/Navigator.cpp
@@ +1269,4 @@
>  {
> +  nsRefPtr<network::Connection> connection = GetMozConnection();
> +  nsCOMPtr<nsINetworkProperties> properties =
> +    static_cast<nsINetworkProperties*>(connection.get());

Shouldn't need the cast, and you should be able to directly assign the result of GetMozConnection() into this.

::: dom/network/src/Connection.cpp
@@ +30,5 @@
>  
>  // Don't use |Connection| alone, since that confuses nsTraceRefcnt since
>  // we're not the only class with that name.
> +NS_IMPL_ADDREF_INHERITED(network::Connection, nsDOMEventTargetHelper)
> +NS_IMPL_RELEASE_INHERITED(network::Connection, nsDOMEventTargetHelper)

Why change this?

::: dom/webidl/MozConnection.webidl
@@ +2,5 @@
> + * 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/.
> + */
> +
> +interface MozConnection : EventTarget {

Do we want to pref this on dom.network.enabled?

@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +interface MozConnection : EventTarget {
> +  readonly attribute double  bandwidth;

Needs to be 'unrestricted double'

	Masatoshi Kimura [:emk]
	Comment 5 • 11 years ago

(In reply to :Ms2ger from comment #4)
> ::: dom/webidl/MozConnection.webidl
> @@ +2,5 @@
> > + * 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/.
> > + */
> > +
> > +interface MozConnection : EventTarget {
> 
> Do we want to pref this on dom.network.enabled?

Yes. We shouldn't pollute the global name space blindly.

	Andrea Marchesini [:baku] Assignee
	Comment 6 • 11 years ago

I don't understand what you meant with the assertion for bandwidth value when infinity. Infinity is an acceptable value for this property.
Is the test we have good enough?

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 7 • 11 years ago

Comment on attachment 8355181 [details] [diff] [review]
mozconnection.patch

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

Looks good, I think, but I don't have time for a full review, sorry.

::: dom/base/Navigator.cpp
@@ +1269,3 @@
>  {
> +  nsRefPtr<network::Connection> connection = GetMozConnection();
> +  nsCOMPtr<nsINetworkProperties> properties = connection.get();

Should be able to just assign GetMozConnection() into this

	Andrea Marchesini [:baku] Assignee
	Comment 8 • 11 years ago

Ehsan, can you review this patch?

	(no longer active)
	Comment 9 • 11 years ago

Comment on attachment 8359711 [details] [diff] [review]
mozconnection.patch

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

r=me

::: dom/network/src/Connection.cpp
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include <limits>
>  #include "mozilla/Hal.h"
>  #include "Connection.h"

Nit: mozilla/dom/network/Connection.h

::: dom/webidl/MozConnection.webidl
@@ +2,5 @@
> + * 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/.
> + */
> +
> +[Pref="dom.network.enabled"]

Because of this change, you need to modify the MozConnection entry in test_interfaces.html and get r+ from a DOM peer on that.

	Andrea Marchesini [:baku] Assignee
	Comment 10 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=09dfdff4250f

Green on try.

	Masatoshi Kimura [:emk]
	Comment 12 • 11 years ago

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #9)
> Because of this change, you need to modify the MozConnection entry in
> test_interfaces.html and get r+ from a DOM peer on that.

Actually it will not be needed because dom.network.enabled is enabled everywhere.

	(no longer active)
	Comment 13 • 11 years ago

(In reply to comment #12)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #9)
> > Because of this change, you need to modify the MozConnection entry in
> > test_interfaces.html and get r+ from a DOM peer on that.
> 
> Actually it will not be needed because dom.network.enabled is enabled
> everywhere.

That is probably a bug.  We should disable it on non-b2g I think.  Filed bug 959845 for that.

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 11 years ago

Comment on attachment 8360054 [details] [diff] [review]
mozconnection.patch

r=me

	Andrea Marchesini [:baku] Assignee
	Comment 15 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=43cf32cba6c2


https://hg.mozilla.org/integration/mozilla-inbound/rev/5302ee051b86

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/5302ee051b86

	(no longer active)
	Comment 17 • 11 years ago

I think I missed this before in the review, but the spec says that the Connection interface should be [NoInterfaceObject].  Andrea, can you please do that in a follow-up?  Thanks!

	Masatoshi Kimura [:emk]
	Comment 18 • 11 years ago

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #17)
> I think I missed this before in the review, but the spec says that the
> Connection interface should be [NoInterfaceObject].  Andrea, can you please
> do that in a follow-up?  Thanks!

See the duped bug 916606.  Mounir (the spec editor of the Network Information API) said that the spec is simply wrong.

	Andrea Marchesini [:baku] Assignee
	Comment 19 • 11 years ago

emk, so can I just close bug 960945 as invalid?
BTW: sorry for this patch, I didn't see your patches when I started.

	Masatoshi Kimura [:emk]
	Comment 20 • 11 years ago

(In reply to Andrea Marchesini (:baku) from comment #19)
> emk, so can I just close bug 960945 as invalid?
> BTW: sorry for this patch, I didn't see your patches when I started.

Please ask Mounir. I just passed on a message.

	Andrea Marchesini [:baku] Assignee
	Comment 21 • 11 years ago

Jonas?

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 22 • 11 years ago

What happened to the assertion I requested in comment #4?

	Andrea Marchesini [:baku] Assignee
	Comment 23 • 11 years ago

(In reply to :Ms2ger from comment #22)
> What happened to the assertion I requested in comment #4?

Almost forgot. follow up bug 964041.

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

I think I'd prefer to make this a [NoInterfaceObject] on this interface. Simply because this API is heavily in flux and the less we expose to the web the better.

However I don't think it's particularly important. This API is only implemented on Android and should be completely disabled everywhere else (that's a separate bug), so no matter what it won't have a large effect what we do here.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 25 • 11 years ago

Disabling this API on non-Android platforms is bug 959845.