Closed Bug 746069 Opened 12 years ago Closed 12 years ago

Expose coarse-grained network usage stats to (privileged) web content

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: cjones, Assigned: albert)

References

Details

(Keywords: dev-doc-complete, feature, Whiteboard: [LOE:S] [WebAPI:P3])

Attachments

(4 files, 50 obsolete files)

6.22 KB, patch
Details | Diff | Splinter Review
4.04 KB, patch
Details | Diff | Splinter Review
37.38 KB, patch
Details | Diff | Splinter Review
31.50 KB, patch
Details | Diff | Splinter Review
This bug entails two work items
 - metering network bytes sent/received per interface
 - expose that data to privileged content

The use case here is showing UI like the following in a "settings" screen or something

 Wifi: Sent 10MB | Received 100MB
 3g:   Sent 1MB | Received 20MB

I don't have much of an opinion on the API we expose, but I bet our WebAPI team does! :)  It should probably play well with the network manager interface, and in fact exposing this info on a network manager seems fine to me.

To collect this data, we need to find the read()/write() calls in necko that talk to the raw sockets and meter the data sent.  We need a way to associate those sockets with the interface on which they're opened.

The right thing to stick this on is probably SocketTransport, but necko folks could advise better.
SocketTransport and its input and output streams have information about how many bytes has been sent out and received in.  

Since SocketTransport is the only class that low that can know which interface and network type the IP connection is using, then it is the right place.  

However, SocketTransport has only information about application data.  It doesn't count SSL overhead.  We may however expose this information someway as well from within NSS.
What's a rough back-of-the-envelope guess at the bandwidth consumed by SSL handshaking?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> What's a rough back-of-the-envelope guess at the bandwidth consumed by SSL
> handshaking?

Cannot say.  It depends on protocol version used, how the server is configured (many parameters).  It may be some 10-15% roughly speaking.  

I've checked whether the TCP/UDP NSPR layer counts bytes going through, but it doesn't.  As well as NSS.  So we have to add a layer to be exact (needs to modify NSS and NSPR) or rather just add and expose byte counters at NSPR for TCP and UDP (modify just NSPR). (no bug for it so far AFAIK, Wan-Teh?)
FYI: Also this information is available in the file /proc/net/dev which also includes traffic generated by external processes since is maintained by the kernel.

$ cat /proc/net/dev
Inter-|   Receive                                                |  Transmit
 face |bytes    packets errs drop fifo frame compressed multicast|bytes    packets errs drop fifo colls carrier compressed
    lo:  569124   12441    0    0    0     0          0         0   569124   12441    0    0    0     0       0          0
svnet0: 2703862  117892 154812    0    0     0          0         0 14075061   77166    0    0    0     0       0          0
  usb0:       0       0    0    0    0     0          0         0        0       0    0    0    0     0       0          0
  sit0:       0       0    0    0    0     0          0         0        0       0    0    0    0     0       0          0
  eth0:     720      12    0    0    0     0          0         0      492       6    0    0    0     0       0          0
  pdp0:  179229     439    0    0    0     0          0         0    79735     508    0    0    0     0       0          0
Getting traffic information from /proc/net/dev is also the way used by Android.

Consider traffic could be also generated by other processes than b2g, should we gather information from here?
We can get system network usage from below system files, same as /proc/net/dev, just more direct.

3G:
/sys/class/net/rmnet0/statistics/rx_bytes
/sys/class/net/rmnet0/statistics/tx_bytes

Wifi:
/sys/class/net/wlan0/statistics/rx_bytes
/sys/class/net/wlan0/statistics/tx_bytes

These information are not persist, so it brings up one question, do we need network usage stats to be persistence?
If we need to persist the information, then polling and saving to disk periodically will be needed.
Attached patch Created (obsolete) — Splinter Review
I created a patch implementing the proposal of Shian-Yow Wu. In addition to the rx and tx bytes parameters, there are more interesting parameters like crc error, drops, etc.

Regarding to persistence it can be achieved using indexedDB, what will help to allow users to get custom stats depending on time intervals.

On the other hand, I will try to extract this data from the netd daemon using a socket connection, does anyone know if it is possible?
We need to persist this data.

If netd is already doing this for us, awesome!  Let's use that.
I managed to get the data from the netd daemon, only rx and tx bytes, not the other parameters. It is just a PoC because code is dirty, when I clean the code I'll upload the patch.

BTW, netd also allows to configure routes, configure ifaces, throttling bandwidth, enable tethering, set DNS, etc.
Attached file Web API NetworkUsageStats idl (obsolete) —
As discussed in the WebAPIs mailing list here we have the IDL corresponding to the expose the usage stats per interface
Attached file idl (obsolete) —
idl updated with correct format, license, etc
Attachment #630128 - Flags: feedback?(jones.chris.g)
Comment on attachment 630128 [details]
idl

I'm not the best guy for this, roping in Mounir and Jonas.
Attachment #630128 - Flags: feedback?(mounir)
Attachment #630128 - Flags: feedback?(jones.chris.g)
Attachment #630128 - Flags: feedback?(jonas)
Comment on attachment 630128 [details]
idl

So ConnectionUsageStats is going to have the same connectType value than the one passed to NetStatsOptions, right? I know this is used in a few APIs but I usually don't like that. Is that the same thing for startDate/endDate?

I'm not sure what NetworkUsageStats.types is for. Does that list all available types?

Maybe you could make the parameter optional in getNetworkStats().

I wonder if clearData() could get a NetStatsOptions so you can clear data for some connection types and for a specified time range. Might be useful if you just want to clear the previous month stats, right?
Passing no NetStatsOptions would clear everything.
Attachment #630128 - Flags: feedback?(mounir)
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #14)
> Comment on attachment 630128 [details]
> idl
> 
> So ConnectionUsageStats is going to have the same connectType value than the
> one passed to NetStatsOptions, right? I know this is used in a few APIs but
> I usually don't like that. Is that the same thing for startDate/endDate?

Yes, connectionType, startDate and endDate of ConnectionUsageStats are the same values than the ones passed to NetStatOptions.

> I'm not sure what NetworkUsageStats.types is for. Does that list all
> available types?

Yes, it does list all connectionTypes.

> Maybe you could make the parameter optional in getNetworkStats().

Do you mean that getNetwoekStats called with no parameters should return total bytes through all connections?

As discussed in the mailing list, the first proposal included the per app statistics and NetstatOptions had a "DOMString app" to get these stats. Then, is necessary to pass a NetstatOptions to the getNetworkStats function.

Finnally, we decided that until the per-app stats feature is not available we will not include the 'app' option. This is the reason to make this parameter mandatory, waiting to have the per-app stats feature implemented.

> I wonder if clearData() could get a NetStatsOptions so you can clear data
> for some connection types and for a specified time range. Might be useful if
> you just want to clear the previous month stats, right?
> Passing no NetStatsOptions would clear everything.

It makes sense, good point.
Comment on attachment 630128 [details]
idl

>[scriptable, uuid(037435a6-f563-48f3-99b3-a0106d8ba5bd)]
>interface ConnectionUsageStats
>{
>  // "mobile", "wifi"
>  readonly attribute DOMString connectionType;
>
>  readonly attribute double rxBytes;
>  readonly attribute double txBytes;
>
>  Date?         startDate;
>  Date?         endDate;

XPIDL doesn't support the Date type. So you have to change this to something like:

[implicit_jscontext] readonly attribute jsval startDate;
[implicit_jscontext] readonly attribute jsval endDate;

Also, I don't think startDate can ever be null.

>[scriptable, uuid(6b671589-7bd7-45b1-b8b6-af377aaef26f)]
>interface NetStatsOptions {

Make this a "dictionary" rather than an "interface". See for example
http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIDOMFile.idl#87

>   DOMString?    connectionType;
>   Date?         startDate;
>   Date?         endDate;

Same here, xpidl doesn't have support for Date objects, even in dictionaries, so use jsval here.

>interface NetworkUsageStats : nsISupports
>{
>  DOMRequest getNetworkStats(in NetStatsOptions options);
>  // If connectionType != null => request.result = InterfaceStats
>  // If connectionType == null => request.result = InterfaceStats (total bytes through all connections, connectionType == null)
>
>  readonly attribute nsIArray     types; /* an array of strings */
>
>  void clearAllData();
>};

Looks good.
Attachment #630128 - Flags: feedback?(jonas) → feedback+
(In reply to Jonas Sicking (:sicking) Vacation June 11-22 from comment #16)

> >  readonly attribute nsIArray     types; /* an array of strings */

Do we want to expose an nsIArray to web content?
(In reply to Fabrice Desré [:fabrice] from comment #17)
> (In reply to Jonas Sicking (:sicking) Vacation June 11-22 from comment #16)
> 
> > >  readonly attribute nsIArray     types; /* an array of strings */
> 
> Do we want to expose an nsIArray to web content?

It was the initial idea, but finally I changed to nsDOMStringList
Attached patch Feature implemented (obsolete) — Splinter Review
The patch implements the feature for b2g. This patch does not work for desktop version, under desktop functions will return 'METHOD_NOT_IMPLEMENTED".

It is not working for desktop because interfaces are obtained using the system properties "wifi.interface" and "mobiledata.interfaces". I think it can be solved by implementing the function to get interfaces in HAL or using 'ifdef' directives. Which option do you think is the best approach?

Also, I tested with otoro but I see that "mobiledata.interfaces" is not defined. Is it possible to define it like it is in galaxy s2? Otherwise, how can I map interfaces with mobile connection?
Attachment #636856 - Flags: review?(mounir)
Attachment #636856 - Flags: review?(jones.chris.g)
Attachment #636856 - Flags: review?(jonas)
Comment on attachment 636856 [details] [diff] [review]
Feature implemented

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

Could you split the patch in smaller patches that represent logical unit? It will ease the review process.
Attachment #636856 - Flags: review?(mounir)
Attached patch Expose IDL to DOM impl (obsolete) — Splinter Review
Implementation to expose methods and attributes to DOM
Attachment #637018 - Flags: review?(mounir)
Attachment #637018 - Flags: review?(jones.chris.g)
Attachment #637018 - Flags: review?(jonas)
Nomming for basecamp.
blocking-basecamp: --- → ?
Is this currently needed for anything other than Gaia apps?  We already have the Network Information API.. is this an addition to that or separate?
Comment on attachment 636856 [details] [diff] [review]
Feature implemented

This is pretty far away from what I'm comfortable reviewing.

We have some code to talk to netd for tethering.  We should share that if possible.  philikon can advise.
Attachment #636856 - Flags: review?(jones.chris.g) → review?(philipp)
Comment on attachment 637018 [details] [diff] [review]
Expose IDL to DOM impl

(Similarly here, not code I can review.)
Attachment #637018 - Flags: review?(jones.chris.g)
You may look at gecko/dom/system/gonk/VolumeManager.cpp. In particular the WriteCommandData, OnFileCanReadWithoutBlocking and OnFileCanWriteWithoutBlocking functions. That's asynchronous I/O functions used to talk to vold over a unix domain socket interface. 

To do asynchronous I/O with netd, you may also look at my patch in bug 735547. 
https://bugzilla.mozilla.org/show_bug.cgi?id=735547

Using asynchronous I/O can prevent main thread from being blocked.
Comment on attachment 636856 [details] [diff] [review]
Feature implemented

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

::: dom/network/src/NetdClient.h
@@ +20,5 @@
> +#include <sys/types.h>
> +#include <sys/un.h>
> +
> +class NetdClient
> +{

I suggest reusing the netd-socket bridge that Vincent Chang wrote in bug 735547 rather than introducing main thread I/O like this.

Also, I highly recommend you break out your patch into several parts and enumerate them, e.g.:

Part 1: IDLs
Part 2: DOM implementation
Part 3: netd backend implementation
etc.
Attachment #636856 - Flags: feedback-
Comment on attachment 637018 [details] [diff] [review]
Expose IDL to DOM impl

I do not have the bandwidth to review that for the moment. Re-assign later if you can't find a proper reviewer.
Attachment #637018 - Flags: review?(mounir)
Comment on attachment 636856 [details] [diff] [review]
Feature implemented

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

Clearing review flags as per previous comments.
Attachment #636856 - Flags: review?(philipp)
Attachment #636856 - Flags: review?(jonas)
Comment on attachment 637018 [details] [diff] [review]
Expose IDL to DOM impl

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

r=me, but don't land this until there's a complete implementation. And security checks of course.

::: dom/base/Navigator.cpp
@@ +1191,5 @@
> +Navigator::GetMozNetworkStats(nsIDOMMozNetworkStatsManager** aNetworkStatsManager)
> +{
> +   *aNetworkStatsManager = nsnull;
> +
> +  if (!mNetworkStatsManager) {

You need to add security checks here.

::: dom/network/src/NetworkStatsCommon.h
@@ +23,5 @@
> +#define POST_ERROR_EVENT_INVALID_DATE           "Invalid dates: endDate smaller than startDate"
> +#define POST_ERROR_EVENT_ILLEGAL_TYPE           "Invalid connectionType value"
> +#define POST_ERROR_EVENT_UNKNOWN_TYPE           "ConnectionType value does not match with an existing connection"
> +#define POST_ERROR_EVENT_DATABASE_ERROR         "Error removing data"
> +#define POST_ERROR_EVENT_UNKNOWN_ERROR          "Unknown error"

Make sure you don't set the .name property of a DOMError to any of these values. The .name property should generally be one of the values from this table:

http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#error-names-0

::: dom/network/src/NetworkStatsManager.cpp
@@ +63,5 @@
> +  }
> +
> +  if (options.startDate != JSVAL_NULL) {
> +    if (!options.startDate.isObject()) {
> +      NS_ERROR(POST_ERROR_EVENT_ILLEGAL_DATE);

You shouldn't assert just because someone passed an invalid argument. NS_WARNING is ok IMO.

@@ +69,5 @@
> +    }
> +
> +    JSObject& obj = options.startDate.toObject();
> +    if (!JS_ObjectIsDate(aCx, &obj)) {
> +      NS_ERROR(POST_ERROR_EVENT_ILLEGAL_DATE);

Same here.

@@ +78,5 @@
> +  }
> +
> +  if (options.endDate != JSVAL_NULL) {
> +    if (!options.endDate.isObject()) {
> +      NS_ERROR(POST_ERROR_EVENT_ILLEGAL_DATE);

And here.

@@ +84,5 @@
> +    }
> +
> +    JSObject& obj = options.endDate.toObject();
> +    if (!JS_ObjectIsDate(aCx, &obj)) {
> +      NS_ERROR(POST_ERROR_EVENT_ILLEGAL_DATE);

And here.

@@ +88,5 @@
> +      NS_ERROR(POST_ERROR_EVENT_ILLEGAL_DATE);
> +      return NS_ERROR_INVALID_ARG;
> +    }
> +
> +    aParams->endDate = js_DateGetMsecSinceEpoch(aCx, &obj);

Also check that endDate > startDate if startDate is set.
Attachment #637018 - Flags: review?(jonas) → review+
Attached file Updated interface definition V2 (obsolete) —
Minor changes to idl
Attachment #630128 - Attachment is obsolete: true
Attachment #641387 - Flags: review?(jonas)
Applied proposed changes
Attachment #637018 - Attachment is obsolete: true
Attachment #641391 - Flags: review?(philipp)
(In reply to Jonas Sicking (:sicking) from comment #30)
> Comment on attachment 637018 [details] [diff] [review]
> Expose IDL to DOM impl
> 
> Review of attachment 637018 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, but don't land this until there's a complete implementation. And
> security checks of course.
> 
> ::: dom/base/Navigator.cpp
> @@ +1191,5 @@
> > +Navigator::GetMozNetworkStats(nsIDOMMozNetworkStatsManager** aNetworkStatsManager)
> > +{
> > +   *aNetworkStatsManager = nsnull;
> > +
> > +  if (!mNetworkStatsManager) {
> 
> You need to add security checks here.

I add a check to the new "dom.networkStats.enabled" preference. I thought this feature is not restricted, do you think that it has to be restricted to specific apps?

> ::: dom/network/src/NetworkStatsCommon.h
> @@ +23,5 @@
> > +#define POST_ERROR_EVENT_INVALID_DATE           "Invalid dates: endDate smaller than startDate"
> > +#define POST_ERROR_EVENT_ILLEGAL_TYPE           "Invalid connectionType value"
> > +#define POST_ERROR_EVENT_UNKNOWN_TYPE           "ConnectionType value does not match with an existing connection"
> > +#define POST_ERROR_EVENT_DATABASE_ERROR         "Error removing data"
> > +#define POST_ERROR_EVENT_UNKNOWN_ERROR          "Unknown error"
> 
> Make sure you don't set the .name property of a DOMError to any of these
> values. The .name property should generally be one of the values from this
> table:
> 
> http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#error-names-0

I use these definitions to provide a descriptive error through DOMError, but according to your comment I'll use standard values
 
> ::: dom/network/src/NetworkStatsManager.cpp
> @@ +63,5 @@
> > +  }
> > +
> > +  if (options.startDate != JSVAL_NULL) {
> > +    if (!options.startDate.isObject()) {
> > +      NS_ERROR(POST_ERROR_EVENT_ILLEGAL_DATE);
> 
> You shouldn't assert just because someone passed an invalid argument.
> NS_WARNING is ok IMO.
> 
> @@ +69,5 @@
> > +    }
> > +
> > +    JSObject& obj = options.startDate.toObject();
> > +    if (!JS_ObjectIsDate(aCx, &obj)) {
> > +      NS_ERROR(POST_ERROR_EVENT_ILLEGAL_DATE);
> 
> Same here.
> 
> @@ +78,5 @@
> > +  }
> > +
> > +  if (options.endDate != JSVAL_NULL) {
> > +    if (!options.endDate.isObject()) {
> > +      NS_ERROR(POST_ERROR_EVENT_ILLEGAL_DATE);
> 
> And here.
> 
> @@ +84,5 @@
> > +    }
> > +
> > +    JSObject& obj = options.endDate.toObject();
> > +    if (!JS_ObjectIsDate(aCx, &obj)) {
> > +      NS_ERROR(POST_ERROR_EVENT_ILLEGAL_DATE);
> 
> And here.
> 
> @@ +88,5 @@
> > +      NS_ERROR(POST_ERROR_EVENT_ILLEGAL_DATE);
> > +      return NS_ERROR_INVALID_ARG;
> > +    }
> > +
> > +    aParams->endDate = js_DateGetMsecSinceEpoch(aCx, &obj);
> 
> Also check that endDate > startDate if startDate is set.

I check this condition in the request manager implementation to be able to return a descriptive literal error message. Since definitions now are not used, I check the condition where you propose.
Please mark patches and attachments that are no longer valid as obsolete. Thanks!
Comment on attachment 641391 [details] [diff] [review]
Updated interface implementation V2

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

::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
@@ +19,5 @@
> +
> +[scriptable, builtinclass,  uuid(87529a6c-aef6-11e1-a595-4f034275cfa6)]
> +interface nsIDOMMozNetworkStatsManager : nsIDOMEventTarget
> +{
> +  /* If successful, the request's onsuccess will be called, and the request's

Nit: please use Javadoc-style comments:

  /**
   * xyz
   */

@@ +32,5 @@
> +   */
> +  [implicit_jscontext]
> +  nsIDOMDOMRequest               getNetworkStats(in jsval options);
> +
> +  // An array of DOMStrings that can return null.

Same as above (Javadoc style please). Also would be good to actually describe what this attribute does.

Lastly, grammar nit: s/can return null/can be null.

::: dom/network/src/NetworkStatsManager.h
@@ +14,5 @@
> +namespace dom {
> +namespace network {
> +
> +class NetworkStatsManager : public nsDOMEventTargetHelper
> +                          , public nsIDOMMozNetworkStatsManager

I'm not sure why you're implementing this in C++. The DOM-facing API is simple enough for this to be in JS and it makes IPC support a bit easier. Everything would definitely be much less verbose. A good example to use as inspiration is the navigator.mozApps implementation: https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/

Factually I could give this patch an r+ from my side (a DOM peer would have to double check all the macros, though). f- from me because I think we should just do it in JS.
Attachment #641391 - Flags: review?(philipp) → feedback-
Changed C++ implementation to JS
Attachment #625633 - Attachment is obsolete: true
Attachment #626407 - Attachment is obsolete: true
Attachment #630101 - Attachment is obsolete: true
Attachment #636856 - Attachment is obsolete: true
Attachment #641391 - Attachment is obsolete: true
Attachment #642247 - Flags: review?(philipp)
Comment on attachment 641387 [details]
Updated interface definition V2

Can you submit this in form of a patch, please? Thanks!
(In reply to Philipp von Weitershausen [:philikon] from comment #37)
> Can you submit this in form of a patch, please? Thanks!

Oh never mind, I see the XPIDLs are part of the implementation patch. Normally we split those out for review purposes, but I guess that works.
Comment on attachment 642247 [details] [diff] [review]
Updated interface implementation V2

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

::: browser/installer/package-manifest.in
@@ +483,5 @@
>  @BINPATH@/components/AlarmsManager.js
>  @BINPATH@/components/AlarmsManager.manifest
> +
> +@BINPATH@/components/NetworkStatsManager.js
> +@BINPATH@/components/NetworkStatsManager.manifest

Why are you adding these to Firefox?

::: dom/network/interfaces/nsIDOMNetworkStats.idl
@@ +19,5 @@
> +   */
> +  attribute DOMString    connectionType;
> +
> +  /**
> +   * Return number of received / transmitted bytes

Attributes don't really "return" anything.

::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
@@ +11,5 @@
> +
> +
> +dictionary NetworkStatsOptions
> +{
> +  DOMString connectionType;

You should document what this means. What are some valid inputs here?

@@ +13,5 @@
> +dictionary NetworkStatsOptions
> +{
> +  DOMString connectionType;
> +  jsval startDate;
> +  jsval endDate;

Suggest adding a // Date to the end of these lines. Also, from reading the implementation it seems these fields are mandatory. Why?

@@ +25,5 @@
> +   * If successful, the request's onsuccess will be called, and the request's
> +   * result will be a nsIDOMMozNetworkStats.
> +   *
> +   * Otherwise, the request's onerror will be called, and the request's error
> +   * will be either 'RadioNotAvailable', 'RequestNotSupported', or 'GenericFailure'.

I don't see these errors mentioned anywhere else in your patch.

@@ +29,5 @@
> +   * will be either 'RadioNotAvailable', 'RequestNotSupported', or 'GenericFailure'.
> +   *
> +   * If connectionType != null => request.result = InterfaceStats
> +   * If connectionType == null => request.result = InterfaceStats (total bytes
> +   *                              through all connections, connectionType == null)

It's more correct to say "the request's success event will be notified". But the whole "if success, the success event will be notified" sounds very redundant. It's kind of assumed as part of DOMRequest. Here's a suggestion for an improved phrasing, including an introductory text.

  /**
   * Query network interface statistics.
   *
   * If options.connectionType is not provided, return statistics for all known
   * network interfaces.
   *
   * If successful, the request result will be an nsIDOMMozNetworkStats object.
   */

::: dom/network/interfaces/nsINetworkStatsService.idl
@@ +6,5 @@
> +
> +interface nsIDOMMozNetworkStats;
> +
> +[scriptable, uuid(973ca911-2c23-4586-8963-c129d5f0ff98)]
> +interface nsINetworkStatsService : nsISupports

Why is this interface needed?

@@ +12,5 @@
> +  readonly attribute jsval connectionTypes;
> +
> +  void updateStats();
> +
> +  void getStats( in DOMString aConnectionType,

Nit: No space between parenthesis and parameters.

::: dom/network/src/NetworkStatsManager.js
@@ +1,5 @@
> +/* 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/. */
> +
> +"use strict"

Semicolon needed at the end of this line.

@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict"
> +
> +/* static functions */

JS doesn't have "static" functions. You can remove this comment.

@@ +4,5 @@
> +
> +"use strict"
> +
> +/* static functions */
> +let DEBUG = 1;

s/1/true/

@@ +8,5 @@
> +let DEBUG = 1;
> +if (DEBUG)
> +  debug = function (s) { dump("-*- NetworkStatsManager: " + s + "\n"); }
> +else
> +  debug = function (s) {}

Please always use braces for blocks, even one-liners. Please fix this here and everywhere else in your patch.

@@ +66,5 @@
> +
> +    if (this.hasPrivileges) {
> +      if(!aOptions.startDate || !aOptions.endDate ||
> +         Object.prototype.toString.call(aOptions.startDate) !== "[object Date]" ||
> +         Object.prototype.toString.call(aOptions.endDate) !== "[object Date]" ||

Use `xyz instanceof Date`

@@ +109,5 @@
> +          debug("no request stored!" + msg.id);
> +        }
> +        break;
> +      case "NetworkStats:Clear:Return:OK":
> +        req = this.getRequest(msg.id);

s/getRequest/takeRequest/ to avoid leaking!

@@ +116,5 @@
> +
> +        break;
> +      case "NetworkStats:Get:Return:KO":
> +      case "NetworkStats:Clear:Return:KO":
> +        req = this.getRequest(msg.id);

Ditto.

@@ +141,5 @@
> +    let perm = principal == secMan.getSystemPrincipal() ?
> +                 Ci.nsIPermissionManager.ALLOW_ACTION :
> +                 Services.perms.testExactPermission(principal.URI, "networkstats-manage");
> +
> +    //only pages with perm set can use the netstats

Coding style nit: space after //, begin sentence with capital letter, and end with a period.

::: dom/network/src/NetworkStatsManager.manifest
@@ +1,3 @@
> +component {037435a6-f563-48f3-99b3-a0106d8ba5bd} NetworkStatsManager.js
> +contract @mozilla.org/networkStats;1 {037435a6-f563-48f3-99b3-a0106d8ba5bd}
> +category JavaScript-global-constructor mozNetworkStatsData @mozilla.org/networkStats;1

As a constructor it should be called `MozNetworkStatsData`. But I don't quite understand why we even need to expose this constructor. It's not like content needs to create these objects.

::: dom/network/src/NetworkStatsService.js
@@ +13,5 @@
> +const NET_NETWORKSTATSSERVICE_CID = Components.ID("{18725604-e9ac-488a-8aa0-2471e7f6c0a4}");
> +
> +const DEBUG = true;
> +
> +const SAMPLE_RATE = 1000*60*60*24; // day

How do you arrive at this number? Why only update the db once a day? Why use a hard timer and not an idle notification so we can do it when the phone is idle? Also might make sense to tally up numbers before a network interface is shutdown (so we'd want to hook into the network manager for that).

@@ +24,5 @@
> +  debug("Service started");
> +
> +  Services.obs.addObserver(this, "xpcom-shutdown", false);
> +
> +  if (this.timer == null) {

Generally `if (!this.timer)` is preferred, but I wonder why you're even doing this check. It should always be `null`.

@@ +34,5 @@
> +
> +  this.messages = ["NetworkStats:Get", "NetworkStats:Clear"];
> +  this.messages.forEach((function(msgName) {
> +    ppmm.addMessageListener(msgName, this);
> +  }).bind(this));

`array.forEach(func.bind(this))` can be written simpler as `array.forEach(func, this)`.

@@ +39,5 @@
> +
> +  try {
> +    let hosts = Services.prefs.getCharPref("dom.mozNetworkStats.whitelist")
> +    hosts.split(",").forEach(function(aHost) {
> +      if (aHost.length > 0)

if (aHost)

@@ +41,5 @@
> +    let hosts = Services.prefs.getCharPref("dom.mozNetworkStats.whitelist")
> +    hosts.split(",").forEach(function(aHost) {
> +      if (aHost.length > 0)
> +        Services.perms.add(Services.io.newURI(aHost, null, null), "networkstats-manage",
> +                             Ci.nsIPermissionManager.ALLOW_ACTION);

Coding style nit: please align arguments.

@@ +72,5 @@
> +
> +  // nsIObserver
> +
> +  observe: function observe(subject, topic, data) {
> +    if(topic == "xpcom-shutdown") {

if (topic != "xpcom-shutdown") {
  return;
}

(Also, coding style nit: space between `if`/`for/etc. and parenthesis. Here and everywhere else.)

@@ +77,5 @@
> +      if(DEBUG) debug("Service shutdown -------------");
> +
> +      this.messages.forEach((function(msgName) {
> +        ppmm.removeMessageListener(msgName, this);
> +      }).bind(this));

See above.

@@ +94,5 @@
> +  },
> +
> +  updateStats: function updateStats(connType, ifaces) {
> +    // TODO: get stats and update DB
> +    debug("Update Stats");

Clearly this patch isn't done yet. Feel free to request feedback until it is, but review should only be requested for patches that are complete.

@@ +102,5 @@
> +    let options = msg.data;
> +    if(DEBUG) debug("getstats for:-" + options.connectionType + "-");
> +    let filter = {connectionType: options.connectionType,
> +                  startDate: options.startDate,
> +                  endDate: options.endDate};

Unused variable (also superfluous since it just copies stuff from `options`.

::: js/xpconnect/src/dictionary_helper_gen.conf
@@ +28,5 @@
>       [ 'MozApplicationEventInit', 'nsIDOMApplicationRegistry.idl' ],
>       [ 'DOMFileMetadataParameters', 'nsIDOMLockedFile.idl' ],
>       [ 'XMLHttpRequestParameters', 'nsIXMLHttpRequest.idl' ],
> +     [ 'DeviceStorageEnumerationParameters', 'nsIDOMDeviceStorage.idl' ],
> +     [ 'NetworkStatsOptions', 'nsIDOMNetworkStatsManager.idl' ]

Pretty sure you no longer need this change. This was only needed when you were trying to access the options object from C++.
Attachment #642247 - Flags: review?(philipp)
Whiteboard: [blocked-on-input]
Attached patch Update V3 (obsolete) — Splinter Review
Attachment #641387 - Attachment is obsolete: true
Attachment #642247 - Attachment is obsolete: true
Attachment #646945 - Flags: review?(philipp)
Attachment #646946 - Flags: review?(philipp)
(In reply to Philipp von Weitershausen [:philikon] from comment #39)
> Comment on attachment 642247 [details] [diff] [review]
> Updated interface implementation V2
> 
> Review of attachment 642247 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +66,5 @@
> > +
> > +    if (this.hasPrivileges) {
> > +      if(!aOptions.startDate || !aOptions.endDate ||
> > +         Object.prototype.toString.call(aOptions.startDate) !== "[object Date]" ||
> > +         Object.prototype.toString.call(aOptions.endDate) !== "[object Date]" ||
> 
> Use `xyz instanceof Date`
>

'xzy instanceof Date' does not work, it returns false.
 
> @@ +13,5 @@
> > +const NET_NETWORKSTATSSERVICE_CID = Components.ID("{18725604-e9ac-488a-8aa0-2471e7f6c0a4}");
> > +
> > +const DEBUG = true;
> > +
> > +const SAMPLE_RATE = 1000*60*60*24; // day
> 
> How do you arrive at this number? Why only update the db once a day? Why use
> a hard timer and not an idle notification so we can do it when the phone is
> idle? Also might make sense to tally up numbers before a network interface
> is shutdown (so we'd want to hook into the network manager for that).
> 

This number comes from the discussion of the IDL proposal in the dev-webapi mailing list. For users rx/tx use is interesting to see have control of their network usage according to the data plan they have, which is paid monthly usually. So having daily samples seems a good approach. Furthermore, an estimation of having daily samples for 3 or 4 months of data usage per interface and data usage per app using the same schema showed that it fits in the database consuming an acceptable amount of disk space.

Hard timer is used to ensure that samples are taken daily.

Is not necessary to get numbers when an interface is going to shutdown because physical interface is always up, even if mobile connection or wifi are down, and netd can retrieve rx/tx bytes.
Assignee: nobody → acperez
Comment on attachment 646946 [details] [diff] [review]
Netd integration V1 based on patch for bug 735547

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

What is this patch supposed to do? Is this just the sum of the patches from bug 735547? In that case, it is not necessary to attach this patch to the bug and ask for review. (Also, pretty sure you didn't mean to include WifiWorker.js.orig and .rej in the patch, right?)

If you have any changes to make on top of bug 735547, please submit those (and *only* those) in a separate patch. If you're using git, you can use interactive rebase to maintain and edit a series of separate commits. If you're using Mercurial, you can use MQ (https://developer.mozilla.org/en/Mercurial_Queues).
Attachment #646946 - Flags: review?(philipp)
(In reply to Albert from comment #42)
> > > +    if (this.hasPrivileges) {
> > > +      if(!aOptions.startDate || !aOptions.endDate ||
> > > +         Object.prototype.toString.call(aOptions.startDate) !== "[object Date]" ||
> > > +         Object.prototype.toString.call(aOptions.endDate) !== "[object Date]" ||
> > 
> > Use `xyz instanceof Date`
> >
> 
> 'xzy instanceof Date' does not work, it returns false.

Not sure how you're testing this, but it works here (using the Firefox web developer console):

[17:58:57.929] < x = new Date();
[17:58:57.937] > Mon Jul 30 2012 17:58:57 GMT-0700 (PDT)
[17:59:01.528] < x instanceof Date
[17:59:01.536] > true

> Is not necessary to get numbers when an interface is going to shutdown
> because physical interface is always up,

Are you positive on that? At least the Wifi manager has to enable the interface the very first time [1].

Anyway, just doing it once a day based on a timer seems a bit too simplistic to me. What if the phone crashes or runs out of battery? Right now we don't seem to remember any stats when the phone shuts down normally. Will netd restore them magically?

Also, from a UX point of view, it seems that we should at least trigger an update if the stats are requested through the API (using a debouncer to ensure we don't talk to `netd` multiple times within a short period of time.) Because if I just watched a YouTube video via 3G, I'd expect the network stats to reflect that!

[1] https://mxr.mozilla.org/mozilla-central/source/dom/wifi/WifiWorker.js#977
(In reply to Philipp von Weitershausen [:philikon] from comment #44)
> (In reply to Albert from comment #42)
> > > > +    if (this.hasPrivileges) {
> > > > +      if(!aOptions.startDate || !aOptions.endDate ||
> > > > +         Object.prototype.toString.call(aOptions.startDate) !== "[object Date]" ||
> > > > +         Object.prototype.toString.call(aOptions.endDate) !== "[object Date]" ||
> > > 
> > > Use `xyz instanceof Date`
> > >
> > 
> > 'xzy instanceof Date' does not work, it returns false.
> 
> Not sure how you're testing this, but it works here (using the Firefox web
> developer console):
> 
> [17:58:57.929] < x = new Date();
> [17:58:57.937] > Mon Jul 30 2012 17:58:57 GMT-0700 (PDT)
> [17:59:01.528] < x instanceof Date
> [17:59:01.536] > true
> 

I know, I did same test and it works but when testing the patch it doesn't. If I call:

navigator.mozNetworkStats.getNetworkStats({startDate: new Date("July 3, 2012 19:00:00"), endDate: new Date(), connectionType: "wifi"})

When I get startDate and endDate in my 'NetworkStatsManager.js', then 'startDate instanceof Date' return false. Can it be a cross-frame instanceof scope problem?

http://julianjelfs.wordpress.com/2010/01/15/solution-to-the-cross-frame-instanceof-scope-problem/

> > Is not necessary to get numbers when an interface is going to shutdown
> > because physical interface is always up,
> 
> Are you positive on that? At least the Wifi manager has to enable the
> interface the very first time [1].

I mean that netd doesn't care if interface is up or down as it get stats from /sys/class/<interface>/statistics
> 
> Anyway, just doing it once a day based on a timer seems a bit too simplistic
> to me. What if the phone crashes or runs out of battery? Right now we don't
> seem to remember any stats when the phone shuts down normally. Will netd
> restore them magically?

If the phone crashes or runs out of battery, when it is started again it will trigger the timer again and when some interface is registered, the NetworkStatsService will save the new stats because it is listening for NetworkManager events.

With this approach the network usage between last sample and the time of the crash will be stored when the phone is started next time, the amount of data transferred or received is not lost, just stored with an incorrect timestamp if the restart is performed after the sample period (one day). Do you have any proposal to solve this?.

Netd return an absolute counter, so stats from last time are obtained subtracting samples.
> 
> Also, from a UX point of view, it seems that we should at least trigger an
> update if the stats are requested through the API (using a debouncer to
> ensure we don't talk to `netd` multiple times within a short period of
> time.) Because if I just watched a YouTube video via 3G, I'd expect the
> network stats to reflect that!

You are right, I will update stats when a request to the API is done and when phone shuts down normally.

> 
> [1] https://mxr.mozilla.org/mozilla-central/source/dom/wifi/WifiWorker.js#977
Attachment #646945 - Attachment is obsolete: true
Attachment #646945 - Flags: review?(philipp)
Attachment #649265 - Flags: review?(philipp)
Attachment #646946 - Attachment is obsolete: true
Attachment #649266 - Flags: review?(philipp)
Comment on attachment 649266 [details] [diff] [review]
Netd integration based on bug 735547

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

General note: Please make sure you follow the Mozilla coding style everywhere. This especially concerns your use of whitespace (or lack thereof) in the right places. Apart from that and the two comments below, this looks pretty good!

::: dom/system/gonk/NetworkManager.js
@@ +200,5 @@
> +          }
> +      };
> +
> +      this.controlMessage(options, function(obj, data) {
> +        if(data.result == TETHERING_COMMAND_SUCCESS){

Looks like we should rename this constant to NETD_COMMAND_SUCCESS or something.

::: dom/system/gonk/net_worker.js
@@ +569,5 @@
>    }
>    return true;
>  }
>  
> +let gNetworkInterfaceStats = [getRxBytes,

Nit: call this `gNetworkInterfaceStatsChain`.
Attachment #649266 - Flags: review?(philipp) → feedback+
Comment on attachment 649265 [details] [diff] [review]
Added update stats before get request is processed

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

General note: Please make sure you follow the Mozilla coding style everywhere. This especially concerns your use of whitespace (or lack thereof) in the right places.

I need to read over your db querying and updating code again more closely, but you can work on all the other comments I added below in the mean time.

::: dom/network/src/NetworkStatsDB.jsm
@@ +14,5 @@
> +}
> +
> +const Cu = Components.utils;
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;

const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;

@@ +26,5 @@
> +
> +const VALUES_MAX_LENGTH = 6*30;
> +
> +//const SAMPLE_RATE = 1000*60*60*24; // day
> +const SAMPLE_RATE = 1000*60; // minute

Coding style nit: Spaces between operators, please.

@@ +30,5 @@
> +const SAMPLE_RATE = 1000*60; // minute
> +
> +function NetworkStatsDB(aGlobal) {
> +  debug("Constructor");
> +  this._global = aGlobal;

Don't need that. (See below for IDB interfaces.)

@@ +46,5 @@
> +        /**
> +         * Create the initial database schema.
> +         */
> +        objectStore = db.createObjectStore(this.dbStoreName, { keyPath: "id" });
> +        objectStore.createIndex("id", ["connectionType", "timestamp"], { unique: true});

I don't see `this.dbStoreName` being defined anywhere. Do you mean STORE_NAME? Also, there's no need to do this in two steps, you can just say:

  objectStore = db.createObjectStore(this.dbStoreName, { keyPath: ["connectionType", "timestamp"] });

@@ +61,5 @@
> +
> +  saveStats: function saveStats(stats, successCb, errorCb) {
> +    let timestamp = Math.floor(stats.date.getTime() / (SAMPLE_RATE)) * SAMPLE_RATE;
> +    stats = {id:             [stats.connectionType,
> +                              timestamp],

No need for this.

@@ +73,5 @@
> +    this.newTxn("readwrite", function(txn, store) {
> +      debug("Going to store " + JSON.stringify(stats));
> +
> +      let request = store.index("connectionType").openCursor(stats.connectionType, "prev");
> +      request.onsuccess = function(event) {

Please always name your functions, e.g.

  request.onsuccess = function onsuccess(event) { ...

@@ +75,5 @@
> +
> +      let request = store.index("connectionType").openCursor(stats.connectionType, "prev");
> +      request.onsuccess = function(event) {
> +        let cursor = event.target.result;
> +        if (cursor){

Coding style nit: space before {

@@ +95,5 @@
> +          if (diff > 1){
> +            // Some samples lost. Device off during one or more samplerate periods
> +            // Add lost samples and the actual one.
> +            if(diff > VALUES_MAX_LENGTH)
> +              diff = VALUES_MAX_LENGTH;

Coding style nit: always need braces.

@@ +132,5 @@
> +            debug("This stat record older than last one");
> +          }
> +        }
> +        else{
> +          // First element.

itym "No more elements."

Anyway, please convert this to bail-out-early style. Instead of writing

  if (foo) {
    giant
    block
    of
    code
    here
  } else {
    doSomethingReallyShort();
  }
  return;

it's much more readable to write

  if (!foo) {
    doSomethingReallShort();
    return;
  }
  giant
  block
  of
  code
  here

Also, coding style nit: cuddle the else (`} else {`)

@@ +149,5 @@
> +    }
> +
> +    let request;
> +
> +    if (networkStats instanceof Array) {

Better: if (Array.isArray(networkStats))

@@ +152,5 @@
> +
> +    if (networkStats instanceof Array) {
> +      let len = networkStats.length - 1;
> +      for (let i = 0; i < len; i++) {
> +        request = store.put(networkStats[i]).onerror = function(event) {

That `request` variable makes no sense. It will be the function.

@@ +160,5 @@
> +      }
> +      request = store.put(networkStats[len]);
> +    }
> +    else{
> +      request = store.put(networkStats);

You don't need to assign to request if you're not using it later. Also, bail-out-early style please!

@@ +169,5 @@
> +    // Callback function to remove old items when new ones are added.
> +    let filterDate = date - (SAMPLE_RATE * VALUES_MAX_LENGTH - 1);
> +    let lowFilter = [connType, 0];
> +    let upFilter = [connType, filterDate];
> +    let range = this._global.IDBKeyRange.bound(lowFilter, upFilter, false, false);

I would prefer you accessed the interfaces using `Ci.nsIIDBKeyRange` etc.

@@ +209,5 @@
> +          txBytes.push(cursor.value.txBytes);
> +          dates.push(cursor.value.timestamp);
> +          cursor.continue();
> +        } 
> +        else{

Bail-out-early style please: return after the `cursor.continue()` above, then you don't have to do the `else` block.

@@ +306,5 @@
> +    }, aSuccessCb, aFailureCb);
> +  },
> +
> +  init: function init(aGlobal) {
> +      this.initDBHelper(DB_NAME, DB_VERSION, STORE_NAME, aGlobal);

Coding style nit: 2 space indentation, please.

::: dom/network/src/NetworkStatsManager.js
@@ +15,5 @@
> +}
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;

const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;

@@ +20,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/DOMRequestHelper.jsm");
> +Cu.import("resource://gre/modules/NetworkStatsService.jsm");

You're only importing this file to have access to a single value. That seems like overkill. Remember, NetworkStatsManager.js lives in the content process, where as your NetworkStatsService and NetworkStatsDB live in the main process. By importing it here you're also creating another instance of the NetworkStatsService in the content process. I don't think you want that.

@@ +142,5 @@
> +    }
> +  },
> +
> +  get types(){
> +    return NetworkStatsService.connectionTypes;

I'm not sure I agree with the way you created `connectionTypes` (as a mapping) and the promise of returning an array (a list) here.

@@ +168,5 @@
> +        }
> +        break;
> +      case "NetworkStats:Get:Return:KO":
> +      case "NetworkStats:Clear:Return:KO":
> +        req = this.getRequest(msg.id);

takeRequest!

@@ +216,5 @@
> +                                     flags: nsIClassInfo.DOM_OBJECT})
> +}
> +
> +const NSGetFactory = XPCOMUtils.generateNSGetFactory(
> +                       [NetworkStats, NetworkStatsManager])

Coding style nit: indent by 2 spaces, please.

::: dom/network/src/NetworkStatsService.jsm
@@ +33,5 @@
> +                                   "nsIIndexedDatabaseManager");
> +
> +XPCOMUtils.defineLazyGetter(this, "ppmm", function() {
> +  return Cc["@mozilla.org/parentprocessmessagemanager;1"].getService(Ci.nsIFrameMessageManager);
> +});

Why not use `defineLazyServiceGetter` here as well?

@@ +45,5 @@
> +    Services.obs.addObserver(this, "xpcom-shutdown", false);
> +    Services.obs.addObserver(this, TOPIC_INTERFACE_REGISTERED, false);
> +    Services.obs.addObserver(this, "profile-after-change", false);
> +
> +    this.networkManager = Cc["@mozilla.org/network/manager;1"].getService(Ci.nsINetworkManager);

Coding style nit: please wrap this overlong line (80 chars max!):

  this.networkManager = Cc["@mozilla.org/network/manager;1"]
                          .getService(Ci.nsINetworkManager);

@@ +51,5 @@
> +    this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +
> +    this.connections = [];
> +    this.connections[NETWORK_TYPE_WIFI] = "wifi";
> +    this.connections[NETWORK_TYPE_MOBILE] = "mobile";

Better name: `this.connectionTypes`. But, either you want an array which is a list, or you want a mapping from one value to another, in which case you want to use

  this.connections = Object.create(null);

@@ +58,5 @@
> +    this.messages.forEach(function(msgName) {
> +      ppmm.addMessageListener(msgName, this);
> +    }, this);
> +
> +    var idbManager = Components.classes["@mozilla.org/dom/indexeddb/manager;1"].getService(Ci.nsIIndexedDatabaseManager);

Coding style nits: s/var/let/, s/Components.classes/Cc/, wrap overlong line.

@@ +71,5 @@
> +      let hosts = Services.prefs.getCharPref("dom.mozNetworkStats.whitelist")
> +      hosts.split(",").forEach(function(aHost) {
> +        if (aHost)
> +          Services.perms.add(Services.io.newURI(aHost, null, null), "networkstats-manage",
> +                             Ci.nsIPermissionManager.ALLOW_ACTION);

Uh, no. Please get rid of this and prime your permissions database as part of the Gaia build.

Also, coding style: You always need braces for a block.

@@ +93,5 @@
> +    }
> +  },
> +
> +  observe: function observe(subject, topic, data) {
> +    if (topic == TOPIC_INTERFACE_REGISTERED) {

Use a `switch` statement here, please.

@@ +104,5 @@
> +      debug("Service shutdown");
> +
> +      this.messages.forEach((function(msgName) {
> +        ppmm.removeMessageListener(msgName, this);
> +      }).bind(this));

Do

  array.forEach(func, this);

instead of

  array.forEach(func.bind(this));

@@ +116,5 @@
> +        if (this._db){
> +          this._db.close();
> +        }
> +        this._db = null;
> +      }.bind(this));

You're not going to get there. "xpcom-shutdown" means that on the next event loop the process is ending. Your callback will never be called. There's no need to close the DB manually anyway.

@@ +132,5 @@
> +    for (let index in this.connections){
> +      connTypes.push(this.connections[index]);
> +    }
> +
> +    return connTypes;

Much simpler (and faster):

  get connectionTypes() {
    return this.connections.slice();
  }

Also, *if* you have to iterate over an array, please use a traditional for-i loop:

  for (let i = 0; i < this.connections.length; i++)

@@ +144,5 @@
> +
> +      if (!options.connectionType || options.connectionType.length == 0){
> +        this._db.findAll(
> +          function(result) { ppmm.sendAsyncMessage("NetworkStats:Get:Return:OK", { id: msg.id, stats: result }); }.bind(this),
> +          function(aErrorMsg) { ppmm.sendAsyncMessage("NetworkStats:Get:Return:KO", { id: msg.id, errorMsg: aErrorMsg }); }.bind(this),

No need for the `bind(this)` since you're not using `this`.

Also: please wrap overlong lines.

@@ +151,5 @@
> +        return;
> +      }
> +
> +      for (let i in this.connections){
> +        if (this.connections[i] == options.connectionType){

You can replace this for loop + if statement with a simple

  if (this.connections.indexOf(options.connectionType) != -1)
Attachment #649265 - Flags: review?(philipp) → feedback?(philipp)
Attached patch Network Stats V4 (obsolete) — Splinter Review
Attachment #649265 - Attachment is obsolete: true
Attachment #649265 - Flags: feedback?(philipp)
Attachment #649739 - Flags: review?(philipp)
(In reply to Philipp von Weitershausen [:philikon] from comment #49)
> Comment on attachment 649265 [details] [diff] [review]
> Added update stats before get request is processed
> 
> Review of attachment 649265 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/src/NetworkStatsDB.jsm
> @@ +30,5 @@
> > +const SAMPLE_RATE = 1000*60; // minute
> > +
> > +function NetworkStatsDB(aGlobal) {
> > +  debug("Constructor");
> > +  this._global = aGlobal;
> 
> Don't need that. (See below for IDB interfaces.)

(See below)

> 
> @@ +61,5 @@
> > +
> > +  saveStats: function saveStats(stats, successCb, errorCb) {
> > +    let timestamp = Math.floor(stats.date.getTime() / (SAMPLE_RATE)) * SAMPLE_RATE;
> > +    stats = {id:             [stats.connectionType,
> > +                              timestamp],
> 
> No need for this.

Do you refer to Math.floor ... ? This is done to filter dates to the defined period precision with SAMPLE_RATE constant. If date is not filtered, when comparing with last sample there is an error due to samples are not exactly taken in the same millisecond.

> 
> @@ +169,5 @@
> > +    // Callback function to remove old items when new ones are added.
> > +    let filterDate = date - (SAMPLE_RATE * VALUES_MAX_LENGTH - 1);
> > +    let lowFilter = [connType, 0];
> > +    let upFilter = [connType, filterDate];
> > +    let range = this._global.IDBKeyRange.bound(lowFilter, upFilter, false, false);
> 
> I would prefer you accessed the interfaces using `Ci.nsIIDBKeyRange` etc.

I tried to use Ci.nsIIDBKeyRange but it gives an error:

E/GeckoConsole(  105): [JavaScript Error: "[Exception... "'TypeError: Ci.nsIIDBKeyRange.bound is not a function' when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: no]"]

Having a look to dom/indexedDB/nsIIDBKeyRange.idl I see there is no 'bound' method defined.

> ::: dom/network/src/NetworkStatsManager.js
> @@ +20,5 @@
> > +
> > +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +Cu.import("resource://gre/modules/DOMRequestHelper.jsm");
> > +Cu.import("resource://gre/modules/NetworkStatsService.jsm");
> 
> You're only importing this file to have access to a single value. That seems
> like overkill. Remember, NetworkStatsManager.js lives in the content
> process, where as your NetworkStatsService and NetworkStatsDB live in the
> main process. By importing it here you're also creating another instance of
> the NetworkStatsService in the content process. I don't think you want that.
> 

I see what you mean, but i don't know how to access the value without importing. Some recommendation?

> @@ +142,5 @@
> > +    }
> > +  },
> > +
> > +  get types(){
> > +    return NetworkStatsService.connectionTypes;
> 
> I'm not sure I agree with the way you created `connectionTypes` (as a
> mapping) and the promise of returning an array (a list) here.
> 

connectionTypes should be created as a list.

> ::: dom/network/src/NetworkStatsService.jsm
> @@ +51,5 @@
> > +    this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> > +
> > +    this.connections = [];
> > +    this.connections[NETWORK_TYPE_WIFI] = "wifi";
> > +    this.connections[NETWORK_TYPE_MOBILE] = "mobile";
> 
> Better name: `this.connectionTypes`. But, either you want an array which is
> a list, or you want a mapping from one value to another, in which case you
> want to use
>
>   this.connections = Object.create(null);

My intention was to define connections as a list. NETWORK_TYPE_WIFI and NETWORK_TYPE_MOBILE are numbers (0 and 1). The reason to initialize in this way was to ensure that if there are more connection types that are not sequential the relation will be correct.
 
> 
> @@ +132,5 @@
> > +    for (let index in this.connections){
> > +      connTypes.push(this.connections[index]);
> > +    }
> > +
> > +    return connTypes;
> 
> Much simpler (and faster):
> 
>   get connectionTypes() {
>     return this.connections.slice();
>   }
> 
> Also, *if* you have to iterate over an array, please use a traditional for-i
> loop:
> 
>   for (let i = 0; i < this.connections.length; i++)
> 

The problem is that if constants (NETWORK_TYPE_WIFI, NETWORK_TYPE_MOBILE and maybe others in future) are not sequential, gaps should be removed. Clients doesn't care of the relation between the constant number and the connection type string
(In reply to Albert from comment #51)
> > ::: dom/network/src/NetworkStatsDB.jsm
> > @@ +30,5 @@
> > > +const SAMPLE_RATE = 1000*60; // minute
> > > +
> > > +function NetworkStatsDB(aGlobal) {
> > > +  debug("Constructor");
> > > +  this._global = aGlobal;
> > 
> > Don't need that. (See below for IDB interfaces.)
> 
> (See below)
...
> > > +    let range = this._global.IDBKeyRange.bound(lowFilter, upFilter, false, false);
> > 
> > I would prefer you accessed the interfaces using `Ci.nsIIDBKeyRange` etc.
> 
> I tried to use Ci.nsIIDBKeyRange but it gives an error:
> 
> E/GeckoConsole(  105): [JavaScript Error: "[Exception... "'TypeError:
> Ci.nsIIDBKeyRange.bound is not a function' when calling method:
> [nsIDOMEventListener::handleEvent]"  nsresult: "0x8057001c
> (NS_ERROR_XPC_JS_THREW_JS_OBJECT)"  location: "native frame :: <unknown
> filename> :: <TOP_LEVEL> :: line 0"  data: no]"]
> 
> Having a look to dom/indexedDB/nsIIDBKeyRange.idl I see there is no 'bound'
> method defined.

Ah, hrm, right. I see. But, still no need for `this._global` because you're doing

  this.initDBHelper(DB_NAME, DB_VERSION, STORE_NAME, aGlobal);

and this will make your global object available as `this.dbGlobal`. In fact, I see you're using the same values here:

  this._db = new NetworkStatsDB(myGlobal);
  this._db.init(myGlobal);

I don't quite understand why `init` is even needed as a separate method. You can just make that part of the constructor.

> > > +  saveStats: function saveStats(stats, successCb, errorCb) {
> > > +    let timestamp = Math.floor(stats.date.getTime() / (SAMPLE_RATE)) * SAMPLE_RATE;
> > > +    stats = {id:             [stats.connectionType,
> > > +                              timestamp],
> > 
> > No need for this.
> 
> Do you refer to Math.floor ... ? This is done to filter dates to the defined
> period precision with SAMPLE_RATE constant. If date is not filtered, when
> comparing with last sample there is an error due to samples are not exactly
> taken in the same millisecond.

No, i'm referring to the `id` field. It's unnecessary, as I explained in my review comment of your index creations. You can define a combined primary key of `connectionType` and `timestamp`.

> > > +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > > +Cu.import("resource://gre/modules/Services.jsm");
> > > +Cu.import("resource://gre/modules/DOMRequestHelper.jsm");
> > > +Cu.import("resource://gre/modules/NetworkStatsService.jsm");
> > 
> > You're only importing this file to have access to a single value. That seems
> > like overkill. Remember, NetworkStatsManager.js lives in the content
> > process, where as your NetworkStatsService and NetworkStatsDB live in the
> > main process. By importing it here you're also creating another instance of
> > the NetworkStatsService in the content process. I don't think you want that.
> > 
> 
> I see what you mean, but i don't know how to access the value without
> importing. Some recommendation?

Right now you hardcode the list of supported connection types in the `NetworkStatsService` constructor. You might as well also hardcode it in the `NetworkStatsManager` code for now, adding comments in both places explaining the duplication. It's not like we currently support dynamically adding connection types at runtime.

> > > +  get types(){
> > > +    return NetworkStatsService.connectionTypes;
> > 
> > I'm not sure I agree with the way you created `connectionTypes` (as a
> > mapping) and the promise of returning an array (a list) here.
> > 
> 
> connectionTypes should be created as a list.
> 
> > ::: dom/network/src/NetworkStatsService.jsm
> > @@ +51,5 @@
> > > +    this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> > > +
> > > +    this.connections = [];
> > > +    this.connections[NETWORK_TYPE_WIFI] = "wifi";
> > > +    this.connections[NETWORK_TYPE_MOBILE] = "mobile";
> > 
> > Better name: `this.connectionTypes`. But, either you want an array which is
> > a list, or you want a mapping from one value to another, in which case you
> > want to use
> >
> >   this.connections = Object.create(null);

This sentence:

> My intention was to define connections as a list.

and this sentence:

> The reason to initialize in this
> way was to ensure that if there are more connection types that are not
> sequential the relation will be correct.

contradict each other. I think what you want is a mapping. `Object.create(null)` is your friend.

> The problem is that if constants (NETWORK_TYPE_WIFI, NETWORK_TYPE_MOBILE and
> maybe others in future) are not sequential, gaps should be removed. Clients
> doesn't care of the relation between the constant number and the connection
> type string

That is correct.
Attached patch Network Stats V5 (obsolete) — Splinter Review
Attachment #649739 - Attachment is obsolete: true
Attachment #649739 - Flags: review?(philipp)
Attachment #649982 - Flags: review?(philipp)
(In reply to Philipp von Weitershausen [:philikon] from comment #52)
> (In reply to Albert from comment #51)
> > > > +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > > > +Cu.import("resource://gre/modules/Services.jsm");
> > > > +Cu.import("resource://gre/modules/DOMRequestHelper.jsm");
> > > > +Cu.import("resource://gre/modules/NetworkStatsService.jsm");
> > > 
> > > You're only importing this file to have access to a single value. That seems
> > > like overkill. Remember, NetworkStatsManager.js lives in the content
> > > process, where as your NetworkStatsService and NetworkStatsDB live in the
> > > main process. By importing it here you're also creating another instance of
> > > the NetworkStatsService in the content process. I don't think you want that.
> > > 
> > 
> > I see what you mean, but i don't know how to access the value without
> > importing. Some recommendation?
> 
> Right now you hardcode the list of supported connection types in the
> `NetworkStatsService` constructor. You might as well also hardcode it in the
> `NetworkStatsManager` code for now, adding comments in both places
> explaining the duplication. It's not like we currently support dynamically
> adding connection types at runtime.
> 

From the beginning I expected to get the available connection types from the NetworkManager, but it only defines constants for NETWORK_TYPE_WIFI, NETWORK_TYPE_MOBILE and NETWORK_TYPE_MOBILE_MMS; it doesn't provide a method to get all available connection types, so it needs to be hardcoded.

I have duplicated hardcoded constants as you adviced me, although I don't like.
Whiteboard: [blocked-on-input] → [blocked-on-input Chris Lee]
Whiteboard: [blocked-on-input Chris Lee]
Updated to last version of netd at bug 735547
Attachment #649266 - Attachment is obsolete: true
Attachment #652736 - Flags: review?(philipp)
Attached patch NetworkStats IDL v6 (obsolete) — Splinter Review
Updated to remove the use of long arrays following the comment 46 on bug 746073
Attachment #652738 - Flags: review?(jonas)
Attached patch NetworkStats Implementation v6 (obsolete) — Splinter Review
Attachment #649982 - Attachment is obsolete: true
Attachment #649982 - Flags: review?(philipp)
Attachment #652739 - Flags: review?(philipp)
Comment on attachment 652736 [details] [diff] [review]
Netd integration based on V9 of bug 735547

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

Looks good. Just a few things here and there. Also, please remove all trailing whitespace.

::: dom/system/gonk/NetworkManager.js
@@ +229,5 @@
>    },
>  
> +  getNetworkInterfaceStats: function getNetworkInterfaceStats(connectionType, callback) {
> +    if(this.getNetworkInterface(connectionType)){
> +      let iface = this.getNetworkInterface(connectionType).name;

Normalize and bail out early, please:

  let iface = this.getNetworkInterface(connectionType);
  if (!iface) {
    debug("There is no interface registered for network type " + connectionType);
    return;
  }
  // ...
  //   ifname: iface.name

(Nit: space between `if` and parenthesis.)

@@ +238,5 @@
> +        ifname: iface,
> +        connType: connectionType,
> +        rxBytes: -1,
> +        txBytes: -1,
> +        date: new Date()

I would add these default values in the worker.

@@ +249,5 @@
> +        let success = false;
> +
> +        if (result.resultCode >= NETD_COMMAND_OKAY && result.resultCode < NETD_COMMAND_ERROR) {
> +          success = true;
> +        }

let success = result.resultCode >= NETD_COMMAND_OKAY && result.resultCode < NETD_COMMAND_ERROR;

@@ +252,5 @@
> +          success = true;
> +        }
> +        callback.networkStatsAvailable(success, result.connType, result.rxBytes, result.txBytes, result.date);
> +      });
> +      return true;

I don't understand why this returns booleans.

::: dom/system/gonk/net_worker.js
@@ +100,5 @@
>  
> +function networkInterfaceStatsFail(params) {
> +  // Notify the main thread. 
> +  postMessage(params);
> +  return true;

Why the `return true`? Remove please.

@@ +524,5 @@
>    }
>    return true;
>  }
>  
> +let gNetworkInterfaceStats = [getRxBytes,

gNetworkInterfaceStatsChain, please

::: dom/system/gonk/nsINetworkManager.idl
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "nsISupports.idl"
>  
> +interface nsIDOMMozNetworkStats;

Not used. Remove please.

@@ +90,5 @@
>  
> +[scriptable, function, uuid(9a887e18-b879-4bb1-8663-238bc4234ba0)]
> +interface nsINetworkStatsCallback : nsISupports
> +{
> +  void networkStatsAvailable(in boolean result,

s/result/success/
Attachment #652736 - Flags: review?(philipp)
Comment on attachment 652738 [details] [diff] [review]
NetworkStats IDL v6

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

r=me either way.

::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
@@ +17,5 @@
> +   * mobile, wifi or null (if null -> stats for both mobile and wifi)
> +   */
> +  DOMString connectionType;
> +  jsval startDate;           // date
> +  jsval endDate;             // date

Nit: You could rename these to simply "start"/"end". Same in nsIDOMMozNetworkStats.
Attachment #652738 - Flags: review?(jonas) → review+
(In reply to Philipp von Weitershausen [:philikon] from comment #58)
> Comment on attachment 652736 [details] [diff] [review]
> Netd integration based on V9 of bug 735547
> 
> Review of attachment 652736 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +252,5 @@
> > +          success = true;
> > +        }
> > +        callback.networkStatsAvailable(success, result.connType, result.rxBytes, result.txBytes, result.date);
> > +      });
> > +      return true;
> 
> I don't understand why this returns booleans.

This returns booleans to notify an error of invalid network interface, otherwise callback error notifies error during netd operation. Do you think that is better to unify errors and notify them using the callback? 
 
> ::: dom/system/gonk/net_worker.js
> @@ +100,5 @@
> >  
> > +function networkInterfaceStatsFail(params) {
> > +  // Notify the main thread. 
> > +  postMessage(params);
> > +  return true;
> 
> Why the `return true`? Remove please.
> 

I followed the structure of the net_worker.js xxxFail/xxxSuccess functions, all are returning true.
Attachment #652736 - Attachment is obsolete: true
Attached patch NetworkStats IDL v7 (obsolete) — Splinter Review
Changes based on comment 59
Attachment #652738 - Attachment is obsolete: true
Attachment #653333 - Flags: review?(jonas)
Attached patch NetworkStats Implementation v7 (obsolete) — Splinter Review
Minor changes to adapt implementation to new version of the idl
Attachment #652739 - Attachment is obsolete: true
Attachment #652739 - Flags: review?(philipp)
Attachment #653337 - Flags: review?(philipp)
Comment on attachment 653333 [details] [diff] [review]
NetworkStats IDL v7

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

Drive by comments on IDL.

::: dom/network/interfaces/nsIDOMNetworkStats.idl
@@ +22,5 @@
> +
> +  /**
> +   * Stats for connectionType
> +   */
> +  readonly attribute jsval        data;      // array of NetworkStatsData

what sort of array?  I assume one element per day?   Say so in comment.

::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
@@ +13,5 @@
> +dictionary NetworkStatsOptions
> +{
> +  /**
> +   * Connection type used to filter which network stats will be returned:
> +   * mobile, wifi or null (if null -> stats for both mobile and wifi)

put 'mobile' and 'wifi' in quotes to make it clear they're strings?   And "If null, stats for both mobile and wifi are returned" rather then "->" :)

@@ +25,5 @@
> +interface nsIDOMMozNetworkStatsManager : nsISupports
> +{
> +  /**
> +   * Query network interface statistics.
> +   *

"Takes a NetworkStatsOptions and returns a nsIDOMMozNetworkStats object for the given connectionType for the start->end data range."

@@ +34,5 @@
> +   *
> +   * If network stats are not available for requested dates rxBytes & txBytes are undefined.
> +   *
> +   * If network stats are not available for some period of requested dates,
> +   * start/end is/are modified.

"are modified to the date range for which data was available"

But you seem to be saying different things here.  A few lines before this you say "If network stats are not available for requested dates rxBytes & txBytes are undefined.", but then here you're saying we'll modify start/end to match the range of dates for which we do have data.  Do we only have nsIDOMMozNetworkStats with undefined rxBytes when there's a "hole" where some day in between valid start...end doesn't have data?

I think modifying start/end is too complicated, and it would be simpler to just always return an array of nsIDOMMozNetworkStats with the length of the date range requested, and just have undefined txBytes for days where we lack data.

@@ +39,5 @@
> +   */
> +  nsIDOMDOMRequest               getNetworkStats(in jsval options);
> +
> +  /**
> +   * An array of DOMStrings that can be null.

If they're not null, they're what?  The list of allowable connectionTypes?  If so please document, and maybe rename to connectionTypes?
Comment on attachment 653332 [details] [diff] [review]
Netd integration based on V9 of bug 735547

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

::: dom/system/gonk/nsINetworkManager.idl
@@ +176,5 @@
> +   *
> +   * @param callback
> +   *        Callback to notify result.
> +   */
> +  boolean getNetworkInterfaceStats(in short networkType, in nsINetworkStatsCallback callback);

So this doesn't take a date range, but callback includes a date.  I assume it's getting today's data?  Document.
(In reply to Jason Duell (:jduell) from comment #64)
> Comment on attachment 653333 [details] [diff] [review]
> NetworkStats IDL v7
> 
> Review of attachment 653333 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Drive by comments on IDL.
> 
> ::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
> @@ +25,5 @@
> > +interface nsIDOMMozNetworkStatsManager : nsISupports
> > +{
> > +  /**
> > +   * Query network interface statistics.
> > +   *
> 
> "Takes a NetworkStatsOptions and returns a nsIDOMMozNetworkStats object for
> the given connectionType for the start->end data range."
> 
> @@ +34,5 @@
> > +   *
> > +   * If network stats are not available for requested dates rxBytes & txBytes are undefined.
> > +   *
> > +   * If network stats are not available for some period of requested dates,
> > +   * start/end is/are modified.
> 
> "are modified to the date range for which data was available"
> 
> But you seem to be saying different things here.  A few lines before this
> you say "If network stats are not available for requested dates rxBytes &
> txBytes are undefined.", but then here you're saying we'll modify start/end
> to match the range of dates for which we do have data.  Do we only have
> nsIDOMMozNetworkStats with undefined rxBytes when there's a "hole" where
> some day in between valid start...end doesn't have data?

Current implementation fill gaps between samples when updating stats, so there will be samples stored for all dates during time defined by VALUES_MAX_LENGTH (6 months). Then, depending on dates coming from dictionary:

1 - date range is inside now && (now - VALUES_MAX_LENGTH).
    No problem in this case.

2 - date range exceeds lower, upper or both limits defined by now && (now - VALUES_MAX_LENGTH).
    Here is where I'm saying to modify the date range.

3 - date range is before (now - VALUES_MAX_LENGTH) or after now.
    Here is where I'm saying to return null or undefined.

I'm agree that this way of processing a request is a strange solution, so I'll change it as you advised.
Attached patch NetworkStats IDL v8 (obsolete) — Splinter Review
Changes based on comment 64
Attachment #653333 - Attachment is obsolete: true
Attachment #654213 - Flags: review?(jduell.mcbugs)
Changes based on comment 65
Attachment #653332 - Attachment is obsolete: true
Attachment #654236 - Flags: review?(jduell.mcbugs)
Attached patch NetworkStats Implementation v8 (obsolete) — Splinter Review
Changes according to IDL v8
Attachment #653337 - Attachment is obsolete: true
Attachment #653337 - Flags: review?(philipp)
Attachment #654301 - Flags: review?(jduell.mcbugs)
Flags: sec-review?(ptheriault)
Hey Jason, it's been 5 days since these were posted - do you have an ETA for when you can review?
Comment on attachment 654236 [details] [diff] [review]
Netd integration based on V9 of bug 735547

[scriptable, uuid(3bc29392-2fba-11e1-80fd-0010183a41af)]
- In interface nsINetworkManager:

+  boolean getNetworkInterfaceStats(in short networkType, in nsINetworkStatsCallback callback);

This is a binary incompatible change so you need to update the uuid for this interface.
Comment on attachment 654213 [details] [diff] [review]
NetworkStats IDL v8

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

Changes are good with one minor typo fix:

jonas: the only v7->v8 semantic difference is we no longer trim the client's start/end dates if there's no data for some points: we now return undefined values for those days. See comment 64 and 66.

::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
@@ +29,5 @@
> +   *
> +   * If successful, the request result will be an nsIDOMMozNetworkStats object.
> +   *
> +   * If network stats are not available for some dates, then rxBytes &
> +   * txBytes are undefined for that dates.

"for those dates", not "that dates"
Attachment #654213 - Flags: review?(jonas)
Attachment #654213 - Flags: review?(jduell.mcbugs)
Attachment #654213 - Flags: feedback+
Comment on attachment 654236 [details] [diff] [review]
Netd integration based on V9 of bug 735547

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

This seems ok to me, but philikon knows this code better.
Attachment #654236 - Flags: review?(jduell.mcbugs) → review?(philipp)
Attachment #654301 - Flags: review?(jduell.mcbugs) → review?(philipp)
Comment on attachment 654301 [details] [diff] [review]
NetworkStats Implementation v8

sr=jst insofar as I'm ok with this code being added to the DOM module, and generally these changes look fine with me. philikon should still review though.
Attachment #654301 - Flags: superreview+
Comment on attachment 654213 [details] [diff] [review]
NetworkStats IDL v8

What was changed here? You really don't need to ask for another sr if there is no substantive changes since the last patch. Especially without letting the reviewer know what changed it means that the review has to be done from scratch.
Attachment #654213 - Flags: review?(jonas) → review+
> Especially without letting the reviewer know what changed

b-b-but--comment 72?   

It probably could have skipped sr, but I figured you'd wave it through...
Crap! Sorry about that. Entirely my bad. Totally ok to ask for sr with that clarification.
Comment on attachment 654236 [details] [diff] [review]
Netd integration based on V9 of bug 735547

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

Sorry for the review lag. Looks good! Just a small style nit in the comment:

::: dom/system/gonk/nsINetworkManager.idl
@@ +168,5 @@
>    long overrideActive(in nsINetworkInterface network);
>  
> +  /**
> +   * Function to retrieve network interface stats. Return false if there is no
> +   * interface registered for the networkType param.

Nit:

 /**
  * Retrieve network interface stats.
  *
  * @param ...
  *
  * @param ...
  *
  * @return false if there is no interface registered for the networkType param.
  */
Attachment #654236 - Flags: review?(philipp) → review+
Comment on attachment 654301 [details] [diff] [review]
NetworkStats Implementation v8

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

This patch shares a lot of problems with the implementation in bug 746074. Please see bug 746074 comment 34. Nearly all those review comments also apply to your patch. In fact, you might want to work with frsela to get some of the helpers (e.g. for the unified callback) uplifted into IndexedDBHelper.

::: dom/network/src/Makefile.in
@@ +33,5 @@
>  LOCAL_INCLUDES = \
>    -I$(topsrcdir)/content/events/src \
>    $(NULL)
>  
> +ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))

The problem with this is that this won't be true for B2G desktop builds. The NetworkManager and other stuff in dom/system/gonk is keyed off of `ifdef MOZ_B2G_RIL`, so I think you should the same here.

::: dom/network/src/NetworkStatsDB.jsm
@@ +7,5 @@
> +const EXPORTED_SYMBOLS = ['NetworkStatsDB'];
> +
> +let DEBUG = true;
> +if (DEBUG) {
> +  debug = function (s) { dump("-*- NetworkStatsDB: " + s + "\n"); }

You use debug() on a couple of JSON.stringify'ed values. Even if DEBUG is false, we will stringify the object which creates a lot of overhead. I suggest you do the same thing we did in bug 784746 for the contacts database and prefix every debug() call with `if DEBUG` instead of doing it here.

@@ +24,5 @@
> +
> +const VALUES_MAX_LENGTH = 6 * 30;
> +
> +//const SAMPLE_RATE = 1000 * 60 * 60 * 24; // day
> +const SAMPLE_RATE = 1000 * 60; // minute

Which one is it now? :)

@@ +85,5 @@
> +        } else {
> +          // Empty, so save first element.
> +          this._saveStats(txn, store, stats);
> +        }
> +        return;

Superfluous `return`. Also, I'd prefer bail-out-early style here:

  if (!cursor) {
    this._saveStats(txn, store, stats);
    return;
  }
  ...

@@ +156,5 @@
> +      for (let i = 0; i < len; i++) {
> +        store.put(networkStats[i]).onerror = function onerror(event) {
> +          debug("Error saving data: " + event.message);
> +          event.transaction.abort();
> +        }

Errors automatically bubble and will abort the transaction. You can get rid of this event handler. That also means you can make the loop over the entire array.

@@ +286,5 @@
> +  get sampleRate () {
> +    return SAMPLE_RATE;
> +  },
> +
> +  dataLog: function dataLog(aSuccessCb, aFailureCb) {

Maybe a slightly more descriptive name would be better, e.g. `fetchAllRecords()`

@@ +291,5 @@
> +    this.newTxn("readonly", function (txn, store) {
> +      if (!txn.result)
> +        txn.result = {};
> +
> +      let result = [];

There's some inconsistency here... You're creating the result as an object above, but then create a local result variable. (Also, you always need braces for ifs). I think you can just do

  let result = txn.result = [];

But even that is unnecessary because you don't really have to fetch all entries manually. You can just do

  store.mozGetAll().onsuccess = function onsuccess(event) {
    txn.result = evnet.target.result;
  };
Attachment #654301 - Flags: review?(philipp) → review-
Attached patch NetworkStats Implementation v9 (obsolete) — Splinter Review
Changes based on comment 79
Attachment #654301 - Attachment is obsolete: true
Attachment #657833 - Flags: review?(philipp)
Attached patch NetworkStats IDL v9 (obsolete) — Splinter Review
Modified IDL to include sampleRate and storageDateLimit to nsIDOMMozNetworkStatsManager.

These parameters are included because the people of costcontrol webapp requested them.
Attachment #654213 - Attachment is obsolete: true
Attachment #658440 - Flags: review?(jonas)
Comment on attachment 658440 [details] [diff] [review]
NetworkStats IDL v9

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

r=me, but we have to stop tweaking these patches and do the extra functionality as followups instead.

::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
@@ +44,5 @@
> +   */
> +  nsIDOMDOMRequest               clearAllData();
> +
> +  /**
> +   * Return sampleRate. Time in milis between samples stored in database.

"milliseconds". And you can remove the "Return sampleRate." part.

Might be worth simply using seconds rather than milliseconds though. Seems unlikely that we'll ever store data with sub-second precision. I'm fine either way though.

@@ +49,5 @@
> +   */
> +  readonly attribute long        sampleRate;
> +
> +  /**
> +   * Return storageDateLimit. Time in millis of the maximum period of dates

Same here.
Attachment #658440 - Flags: review?(jonas) → review+
Attached patch NetworkStats Implementation v10 (obsolete) — Splinter Review
Changes based on comment 79 and following changes:

- NetworkStatsDB: fixed fillResultSamples when data array parameter is empty.

- NetworkStatsManager: added __exposedProps__ to expose properties to webapps.
    Fill NetworkStats result object with attribute data as a NetworkStatsData instead of a javascript object.

- NetworkStatsService -> now if interface is not registered for a valid connection type, new sample is saved in indexedDb with values = 0.

- Added two parameters exposed in the IDL (sampleRate and maxStorageSamples).
Attachment #657833 - Attachment is obsolete: true
Attachment #657833 - Flags: review?(philipp)
Attachment #658886 - Flags: review?(philipp)
Attached patch NetworkStats IDL v9 (obsolete) — Splinter Review
Applied comments of Comment 82
Attached patch NetworkStats IDL v10 (obsolete) — Splinter Review
Applied comments of Comment 82

Last comment 84 has wrong patch, corrected.
Sorry about that..
Attachment #658440 - Attachment is obsolete: true
Attachment #658887 - Attachment is obsolete: true
Whiteboard: [WebAPI:P3]
Keywords: feature
Comment on attachment 658886 [details] [diff] [review]
NetworkStats Implementation v10

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

Sorry for taking so long to review this. This looks really good! I just have a few smaller points that I'd like to see addressed before we land it. Thanks!

::: dom/network/src/NetworkStatsDB.jsm
@@ +37,5 @@
> +      txnCb(true, result);
> +    }
> +    function errorCb(error) {
> +      txnCb(false, error);
> +    }

I'd prefer the node convention here of using (error, result) for the callback signature:

  function successCb(result) {
    txnCb(null, result);
  }
  function errorCb(error) {
    txnCb(error, null);
  }

You don't lose any information compared to your case, but the parameter naming and usage is *much* clearer.

@@ +174,5 @@
> +    }
> +
> +    if (!txn.result) {
> +      txn.result = {};
> +    }

I don't think this is necessary, is it?

@@ +241,5 @@
> +        this.fillResultSamples(aOptions, data);
> +
> +        let result = {};
> +        result.connectionType = aOptions.connectionType;
> +        result.start = new Date(aOptions.start);

Instead of creating a new `result` object, just fill the `txn.result` object you already created.

@@ +280,5 @@
> +                        txBytes: cursor.value.txBytes,
> +                        date: new Date(cursor.value.timestamp) });
> +          }
> +          cursor.continue();
> +        } else {

Return after `cursor.continue()` and save the `else` block, like in the `find()` method above.

@@ +286,5 @@
> +          this.fillResultSamples(aOptions, data);
> +
> +          let result = {};
> +          result.connectionType = aOptions.connectionType;
> +          result.start = new Date(aOptions.start);

Same as in `find()`: fill `txn.result`

@@ +335,5 @@
> +        var cursor = event.target.result;
> +        if (cursor) {
> +            txn.result.push(cursor.value);
> +            cursor.continue();
> +        }

Why not

  store.mozGetAll().onsuccess = function onsuccess(event) {
    txn.result = event.target.result;
  };

like I suggested in comment 79?

::: dom/network/src/NetworkStatsManager.js
@@ +15,5 @@
> +
> +XPCOMUtils.defineLazyGetter(Services, "DOMRequest", function() {
> +  return Cc["@mozilla.org/dom/dom-request-service;1"]
> +                .getService(Ci.nsIDOMRequestService);
> +});

Remove this please (was mentioned in bug 746074 comment 34)

@@ +20,5 @@
> +
> +XPCOMUtils.defineLazyGetter(this, "cpmm", function() {
> +  return Cc["@mozilla.org/childprocessmessagemanager;1"]
> +                .getService(Ci.nsISyncMessageSender);
> +});

Use XPCOMUtils.defineLazyServiceGetter, please.

@@ +42,5 @@
> +                     },
> +
> +  get rxBytes () {
> +    return this._rxBytes;
> +  },

These getters and setters are unnecessary, please remove them (was mentioned in bug 746074 comment 34)

@@ +171,5 @@
> +    this.checkPrivileges();
> +
> +    if(!aOptions.start || !aOptions.end ||
> +      aOptions.start > aOptions.end)
> +      throw Components.results.NS_ERROR_INVALID_ARG;

Always need braces.

@@ +227,5 @@
> +          Services.DOMRequest.fireSuccess(req, result);
> +        } else {
> +          if (DEBUG) {
> +            debug("No request stored with id " + msg.id);
> +          }

Prefer bail out early style:

  if (!req) {
    if (DEBUG) debug("No request stored with id " + msg.id);
    return;
  }
  ...

@@ +248,5 @@
> +        if (DEBUG) {
> +          debug("Wrong message: " + aMessage.name);
> +        }
> +    }
> +    this.removeRequest(msg.id);

This should be unnecessary since you're using `takeRequest()` above. Please remove.

::: dom/network/src/NetworkStatsService.jsm
@@ +153,5 @@
> +          function(success, result) {
> +            if (!success) {
> +              mm.sendAsyncMessage("NetworkStats:Get:Return:KO",
> +                                  { id: msg.id, errorMsg: result });
> +              return;

You could just have one message for both success and failure cases, e.g.:

  mm.sendAsyncMessage("NetworkStats:Get:Return",
                      { id: msg.id, error: error, result: result });

and then on the DOM side you look at msg.json.error to decide whether you fire error or success events. I think that'd be somewhat simpler. Simpler = better. :)

@@ +294,5 @@
> +      debug("Update stats for " + this._connectionTypes[connectionType]);
> +    }
> +
> +    if (!this._connectionTypes[connectionType]) {
> +      if (callback){

Nit: missing space between ){

@@ +335,5 @@
> +                           }});
> +    }
> +    else{
> +      if (callback) { callback(false, "Netd IPC error"); }
> +    }

Coding style: please honour the whitespace and bracing rules. Also, bail out early!

  if (!result) {
    if (callback) {
      callback(false, "Netd IPC error");
    }
    return;
  }
  ...
Attachment #658886 - Flags: review?(philipp)
Attached patch Netd integration (obsolete) — Splinter Review
Changed uuid of NetworkManager idl
Attachment #654236 - Attachment is obsolete: true
Attachment #659703 - Attachment is obsolete: true
Attached patch NetworkStats Implementation v11 (obsolete) — Splinter Review
Changes from comment 86
Attachment #658886 - Attachment is obsolete: true
Attachment #659705 - Flags: review?(philipp)
(In reply to Philipp von Weitershausen [:philikon] from comment #86)
> Comment on attachment 658886 [details] [diff] [review]
> NetworkStats Implementation v10
> 
> Review of attachment 658886 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +174,5 @@
> > +    }
> > +
> > +    if (!txn.result) {
> > +      txn.result = {};
> > +    }
> 
> I don't think this is necessary, is it?
> 

The if statement is not necessary, but the object creation is.
(In reply to Albert from comment #90)
> > > +    if (!txn.result) {
> > > +      txn.result = {};
> > > +    }
> > 
> > I don't think this is necessary, is it?
> > 
> 
> The if statement is not necessary, but the object creation is.

Can you explain why?
(In reply to Philipp von Weitershausen [:philikon] from comment #91)
> (In reply to Albert from comment #90)
> > > > +    if (!txn.result) {
> > > > +      txn.result = {};
> > > > +    }
> > > 
> > > I don't think this is necessary, is it?
> > > 
> > 
> > The if statement is not necessary, but the object creation is.
> 
> Can you explain why?

Because 'txn.result' doesn't exist, it is undefined. when you try to set attributes an exception is thrown:

  txn.result.connectionType = aOptions.connectionType;
  txn.result.start = new Date(aOptions.start);
  txn.result.end = new Date(aOptions.end);
  txn.result.data = data;

E/GeckoConsole( 1317): [JavaScript Error: "[Exception... "'TypeError: txn.result is undefined' when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT)"  location: "<unknown>"  data: no]"]
Comment on attachment 659705 [details] [diff] [review]
NetworkStats Implementation v11

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

Getting there! Lots of minor issues, and a general note about documentation: your patch is almost entirely devoid of comments. Please document tricky parts of the code and choices you made in comments. Think of a developer who doesn't know anything about what's going on.

Also, keep in mind that we have an automated testing policy. The DOM API will likely need a combination mochitests and Marionette tests, and the service/DB could use some unit tests (xpcshell tests).

::: dom/network/src/NetworkStatsService.jsm
@@ +42,5 @@
> +    Services.obs.addObserver(this, TOPIC_INTERFACE_REGISTERED, false);
> +    Services.obs.addObserver(this, "profile-after-change", false);
> +
> +    this.networkManager = Cc["@mozilla.org/network/manager;1"]
> +                            .getService(Ci.nsINetworkManager);

Please convert this to a lazy service getter, like the other ones at the top of the file. Thanks!

@@ +63,5 @@
> +    }, this);
> +
> +    let idbManager = Cc["@mozilla.org/dom/indexeddb/manager;1"]
> +                       .getService(Ci.nsIIndexedDatabaseManager);
> +    idbManager.initWindowless(myGlobal);

You already defined `gIDBManager` above. Just use that here, please.

@@ +77,5 @@
> +  receiveMessage: function(aMessage) {
> +    if (DEBUG) {
> +      debug("receiveMessage " + aMessage.name);
> +    }
> +    let mm = aMessage.target.QueryInterface(Ci.nsIMessageSender);

The `.QueryInterface(Ci.nsIMessageSender)` is no longer needed. Please remove.

@@ +192,5 @@
> +      lastElement = { type: i,
> +                      queueIndex: this.isUpdateQueued(i)};
> +      if (lastElement.queueIndex == -1) {
> +          elements.push({type: lastElement.type, callbacks: []});
> +      }

Nit: wrong indentation.

@@ +199,5 @@
> +    if (elements.length > 0) {
> +      elements[elements.length - 1].callbacks.push(callback);
> +      this.updateQueue = this.updateQueue.concat(elements);
> +    }
> +    else {

Coding style nit: cuddle the else (please do this here and everywhere else)

@@ +230,5 @@
> +
> +    this.controller();
> +  },
> +
> +  isUpdateQueued: function isUpdateQueued(type) {

The name of the function suggests a Boolean return value, but you're returning an index.

@@ +239,5 @@
> +    }
> +    return -1;
> +  },
> +
> +  controller: function controller(aResult, aMessage) {

This is a pretty non-descriptive method name. Usually method names should be verbs, not nouns. On top of that, a short comment text about the purpose and design of this set of methods would be good for the next developer.

Also: lots of coding style nits in this method that I have already pointed out in other reviews. I don't like having to repeat myself for trivial stuff like that. Please address them.

@@ +243,5 @@
> +  controller: function controller(aResult, aMessage) {
> +    if (aResult != undefined) {
> +      let item = this.updateQueue.shift();
> +      for (let i in item.callbacks) {
> +        let callback = item.callbacks[i];

for (let callback of item.callbacks)

@@ +309,5 @@
> +    }
> +    this._db.saveStats(stats,
> +                       function(error, result) {
> +                         if (callback) {
> +                           if (error) {

Incorrect indentation. Please always indent JS code by 2 spaces.

  this._db.saveStats(stats, function (error, result) {
    ...
  });

Also it's a really good idea to name *all* functions, so in this case

  this._db.saveStats(stats, function onStatsSaved(error, result) {
    ...
  });

or something like that.

@@ +318,5 @@
> +                           callback(true, "OK");
> +                         }});
> +  },
> +
> +  fetchAllRecords: function fetchAllRecords() {

logAllRecords?
Attachment #659705 - Flags: review?(philipp)
Attached patch NetworkStats IDL v11 (obsolete) — Splinter Review
Changed interface name 'nsINetworkStatsData' by 'nsIDOMMozNetworkStatsData'.
Attachment #658888 - Attachment is obsolete: true
Attached patch NetworkStats Implementation v12 (obsolete) — Splinter Review
Changes from comment 93.

Still working on implementing mochitest for idl and unit tests for service
Attachment #659705 - Attachment is obsolete: true
Attachment #660882 - Flags: review?(philipp)
Whiteboard: [WebAPI:P3] → [LOE:S] [WebAPI:P3]
Attached patch Tests V1 (obsolete) — Splinter Review
Added tests
Attachment #662182 - Flags: review?(philipp)
Attached patch NetworkStats Implementation v12 (obsolete) — Splinter Review
Changes from Comment 93 and changes from bugs detected when implementing tests.
Attachment #660882 - Attachment is obsolete: true
Attachment #660882 - Flags: review?(philipp)
Attachment #662200 - Flags: review?(philipp)
(In reply to Albert from comment #97)
> and changes from bugs detected when implementing tests.

That's why we require tests. :)
Comment on attachment 662200 [details] [diff] [review]
NetworkStats Implementation v12

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

Great! r=me with the minor nits below addressed.

::: dom/network/src/NetworkStatsDB.jsm
@@ +72,5 @@
> +  },
> +
> +  saveStats: function saveStats(stats, aResultCb) {
> +    // Filter timestamp to get SAMPLE_RATE precission
> +    let timestamp = Math.floor(stats.date.getTime() / (SAMPLE_RATE)) * SAMPLE_RATE;

No need to put parentheses around `SAMPLE_RATE`

@@ +220,5 @@
> +
> +  find: function find(aResultCb, aOptions) {
> +    // Filter end and start date to adapt them to SAMPLE_RATE precision.
> +    let start = Math.floor(aOptions.start / (SAMPLE_RATE)) * SAMPLE_RATE;
> +    let end = Math.floor(aOptions.end / (SAMPLE_RATE)) * SAMPLE_RATE;

See above: no need for parens around `SAMPLE_RATE`

::: dom/network/src/NetworkStatsService.jsm
@@ +266,5 @@
> +    return -1;
> +  },
> +
> +  /*
> +   * Function responsible of process all requests of the queue.

Simpler:

  Process all requests in the queue.

Which begs the question: why not call it `processQueue` or `processAllRequestsInQueue`. `manageQueue` is pretty non-descriptive.

@@ +332,5 @@
> +   * Callback of request stats. Store stats in database.
> +   */
> +  networkStatsAvailable: function networkStatsAvailable(callback, result, connType, rxBytes, txBytes, date) {
> +    if (!result) {
> +      if (callback) { callback(false, "Netd IPC error"); }

Coding style nit: callback invocation should be on its own line.
Attachment #662200 - Flags: review?(philipp) → review+
Attached patch NetworkStats Implementation v14 (obsolete) — Splinter Review
Changes from comment 99 and rebase
Attachment #662200 - Attachment is obsolete: true
Attached patch Tests V2 (obsolete) — Splinter Review
Added xpcshell tests
Attachment #662182 - Attachment is obsolete: true
Attachment #662182 - Flags: review?(philipp)
Attachment #663037 - Flags: review?(philipp)
Comment on attachment 663037 [details] [diff] [review]
Tests V2

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

::: dom/network/tests/test_networkstats_basics.html
@@ +17,5 @@
> +  ok(!(aInterface in window), aInterface + " should be prefixed");
> +  ok(("Moz" + aInterface) in window, aInterface + " should be prefixed");
> +}
> +
> +function testPermissions() {

nit: better name would be setUpPermissions() or something like that. A function named something like `testFoobar()` suggests it's an actual test.

@@ +113,5 @@
> +        return;
> +      }
> +
> +      ok(false, "getNetworkStats launch exceptionwhen start is greater than end");
> +    }, 1000);

Why the `setTimeout(..., 1000)`? In total you're adding 3 seconds of delay to this test but I can't figure out why. Fixed delays in tests are not a good idea, they're a great way to either waste time or to introduce random failures.

::: dom/network/tests/unit/test_networkstats_service.js
@@ +1,1 @@
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;

Nit: add public domain header to test files: https://www.mozilla.org/MPL/boilerplate-1.1/pd-c

@@ +92,5 @@
> +  do_check_eq(NetworkStatsService.updateQueue[0].callbacks[0], null);
> +  do_check_neq(NetworkStatsService.updateQueue[0].callbacks[1], null);
> +}
> +
> +add_test(clearDB);

Just do this inline for each test function above:

  add_test(function test_clearDB() {
    ...
  });

Also, to follow a convention that's used in most other places, please prefix your test functions with `test_`. Both things make it easier to understand which functions are actual tests and which are just helpers.
Attachment #663037 - Flags: review?(philipp)
Attached patch Tests V3 (obsolete) — Splinter Review
Changes from comment 102.
Attachment #663037 - Attachment is obsolete: true
Attachment #664937 - Flags: review?(philipp)
Comment on attachment 664937 [details] [diff] [review]
Tests V3

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

Please be sure to get a try build before marking as checkin-needed.
Attachment #664937 - Flags: review?(philipp) → review+
Attached patch NetworkStats IDL v12 (obsolete) — Splinter Review
Rebased
Attachment #660881 - Attachment is obsolete: true
Attachment #665849 - Flags: checkin?(philipp)
Try run for a8ba818ea5cf is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a8ba818ea5cf
Results (out of 10 total builds):
    exception: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ferjmoreno@gmail.com-a8ba818ea5cf
Attachment #665849 - Flags: checkin?(philipp)
Try run for ad1c3f319b74 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ad1c3f319b74
Results (out of 187 total builds):
    success: 162
    warnings: 23
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ferjmoreno@gmail.com-ad1c3f319b74
Attached patch Tests V4 (obsolete) — Splinter Review
As NetworkStats are only available for b2g, modified dom/network/tests/Makefile.in to enable mochitest and xpcshell tests if MOZ_B2G_RIL is defined. Also xpcshell tests are moved to a new unit_stats folder because unit folder contains tests for all platforms. Thus a new line in testing/xpcshell/xpcshell.ini is added to the new unit_stats folder
Attachment #664937 - Attachment is obsolete: true
Attachment #666719 - Flags: review?(marshall)
Comment on attachment 666719 [details] [diff] [review]
Tests V4

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

Wow, great tests :). I'm admittedly new to this bug, so I probably don't fully understand all that's being tested here. Pinging vicamo for r? as well..

::: dom/network/tests/test_networkstats_basics.html
@@ +31,5 @@
> +  checkInterface("NetworkStats");
> +  checkInterface("NetworkStatsData");
> +
> +  ok('mozNetworkStats' in navigator, "navigator.mozMozNetworkStats should exist");
> +  ok(navigator.mozNetworkStats, "navigator.mozNetworkStats returns an object");

Hrm, shouldn't mozNetworkStats be undefined / null if the "networkstats-manage" permission hasn't been granted yet?
Attachment #666719 - Flags: review?(vyang)
Attachment #666719 - Flags: review?(marshall)
Attachment #666719 - Flags: review+
Comment on attachment 666719 [details] [diff] [review]
Tests V4

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

I know little about the API here, and can only pickup some space errors. I think I should not place any review comment here (<= yeah, I've already done.).

::: dom/network/tests/unit_stats/test_networkstats_db.js
@@ +38,5 @@
> +
> +  var aux = start;
> +  var success = true;
> +  for (var i = 0; i <= samples; i++) {
> +    if (data[i].date.getTime() != aux|| data[i].rxBytes != undefined || data[i].txBytes != undefined) {

"... aux || ..."

@@ +64,5 @@
> +  var aux = start;
> +  var success = true;
> +  for (var i = 0; i <= samples; i++) {
> +    if (i == 1) {
> +      if (data[i].date.getTime() != aux|| data[i].rxBytes != 0 || data[i].txBytes != 0) {

ditto
Attachment #666719 - Flags: review?(vyang)
Attached patch NetworkStats IDL v13 (obsolete) — Splinter Review
Rebased
Attachment #665849 - Attachment is obsolete: true
Attachment #667544 - Flags: checkin?(marshall)
Attached patch NetworkStats Implementation v15 (obsolete) — Splinter Review
rebased
Attachment #663036 - Attachment is obsolete: true
Attachment #667545 - Flags: checkin?(marshall)
Attached patch Netd integration V7 (obsolete) — Splinter Review
rebased
Attachment #659704 - Attachment is obsolete: true
Attachment #667548 - Flags: checkin?(marshall)
Attached patch Tests V5 (obsolete) — Splinter Review
Changes from comment 112 and rebased
Attachment #666719 - Attachment is obsolete: true
Attachment #667550 - Flags: checkin?(marshall)
Albert, these patches look like plain git diffs. You'll need to format these with HG commit messages. If you're working with gecko from git, you can use jlebar's moz-git-tools to help you format your patches:

https://github.com/jlebar/moz-git-tools

Can you also make sure to label each patch / commit message with a part number? That will be the order that I apply them in.

Your commit messages should look something like:

Bug 746069: Part 1: DOM interfaces for NetworkStats. r=philikon r=sicking

For more about the commit / patch guidelines, see:
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment
Attached patch Part 1: Netd integration (v7) (obsolete) — Splinter Review
Attachment #667548 - Attachment is obsolete: true
Attachment #667548 - Flags: checkin?(marshall)
Attachment #667675 - Flags: checkin?(marshall)
Attached patch Part 2: NetworkStats IDL (v13) (obsolete) — Splinter Review
Attachment #667544 - Attachment is obsolete: true
Attachment #667544 - Flags: checkin?(marshall)
Attachment #667678 - Flags: checkin?(marshall)
Attachment #667545 - Attachment is obsolete: true
Attachment #667545 - Flags: checkin?(marshall)
Attachment #667681 - Flags: checkin?(marshall)
Attached patch Part 4: NetworkStats tests (v5) (obsolete) — Splinter Review
Attachment #667550 - Attachment is obsolete: true
Attachment #667550 - Flags: checkin?(marshall)
Attachment #667684 - Flags: checkin?(marshall)
(In reply to Albert from comment #120)
> Created attachment 667681 [details] [diff] [review]
> Part 3: NetworkStats Implementation (v15)

This patch isn't applying cleanly for me against the latest m-c. Can you rebase it?
Rebase
Attachment #667681 - Attachment is obsolete: true
Attachment #667681 - Flags: checkin?(marshall)
Attachment #667997 - Flags: checkin?(marshall)
Ergh, Part 4 will need a rebase too :( It applies cleanly on mozilla-central but not mozilla-inbound. Here's the error:

patching file dom/tests/mochitest/general/test_interfaces.html
Hunk #1 FAILED at 536
1 out of 1 hunks FAILED -- saving rejects to file dom/tests/mochitest/general/test_interfaces.html.rej

Can you also make sure you have the same author in all of your commits? I noticed you have "Albert Crespell" in Part 3, and just "Albert" in Parts 1, 2, and 4.
Changed author
Attachment #667675 - Attachment is obsolete: true
Attachment #667675 - Flags: checkin?(marshall)
Attachment #668033 - Flags: checkin?(marshall)
Changed author
Attachment #667678 - Attachment is obsolete: true
Attachment #667678 - Flags: checkin?(marshall)
Attachment #668034 - Flags: checkin?(marshall)
Attachment #667997 - Attachment is obsolete: true
Attachment #667997 - Flags: checkin?(marshall)
Attachment #668035 - Flags: checkin?(marshall)
rebase
Attachment #667684 - Attachment is obsolete: true
Attachment #667684 - Flags: checkin?(marshall)
Attachment #668037 - Flags: checkin?(marshall)
Required a follow-up because of a missing comma in test_interfaces.html:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61dbc8b4ca76
Depends on: 798740
No longer blocks: 799547
Depends on: 799547
Attachment #668033 - Flags: checkin?(marshall)
Attachment #668034 - Flags: checkin?(marshall)
Attachment #668035 - Flags: checkin?(marshall)
Comment on attachment 668037 [details] [diff] [review]
Part 4: NetworkStats tests (v6)

Clearing these checkin flags, this has landed.
Attachment #668037 - Flags: checkin?(marshall)
Clearing sec-review flag here based on:
- API restricted to certified apps only (based on a check in a parent process, here: https://mxr.mozilla.org/mozilla-central/source/dom/network/src/NetworkStatsService.jsm#77)
- API only allows read-only access to relatively low risk information (not much benefit beyond finger printing)

As such this is a very low risk API and no further review is required.
Flags: sec-review?(ptheriault) → sec-review+
You need to log in before you can comment on or make changes to this bug.