Last Comment Bug 746069 - Expose coarse-grained network usage stats to (privileged) web content
: Expose coarse-grained network usage stats to (privileged) web content
Status: RESOLVED FIXED
[LOE:S] [WebAPI:P3]
: dev-doc-complete, feature
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Albert [:albert]
:
Mentors:
Depends on: 746074 798740 799547
Blocks: webapi b2g-metering-usage 746073
  Show dependency treegraph
 
Reported: 2012-04-16 23:23 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2013-08-29 09:26 PDT (History)
35 users (show)
ptheriault: sec‑review+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Created (6.07 KB, patch)
2012-05-21 07:10 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Here is the patch getting info from netd daemon (12.03 KB, patch)
2012-05-23 06:02 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Web API NetworkUsageStats idl (734 bytes, text/plain)
2012-06-05 01:49 PDT, Albert [:albert]
no flags Details
idl (1.30 KB, text/plain)
2012-06-05 04:05 PDT, Albert [:albert]
jonas: feedback+
Details
Feature implemented (75.43 KB, patch)
2012-06-26 13:33 PDT, Albert [:albert]
philipp: feedback-
Details | Diff | Splinter Review
Expose IDL to DOM impl (17.78 KB, patch)
2012-06-27 00:43 PDT, Albert [:albert]
jonas: review+
Details | Diff | Splinter Review
Updated interface definition V2 (988 bytes, text/plain)
2012-07-12 01:36 PDT, Albert [:albert]
jonas: review+
Details
Updated interface implementation V2 (18.14 KB, patch)
2012-07-12 01:49 PDT, Albert [:albert]
philipp: feedback-
Details | Diff | Splinter Review
Updated interface implementation V2 (21.69 KB, patch)
2012-07-14 09:56 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Update V3 (34.18 KB, patch)
2012-07-28 23:17 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Netd integration V1 based on patch for bug 735547 (156.28 KB, patch)
2012-07-28 23:19 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Added update stats before get request is processed (36.24 KB, patch)
2012-08-06 07:21 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Netd integration based on bug 735547 (9.32 KB, patch)
2012-08-06 07:23 PDT, Albert [:albert]
philipp: feedback+
Details | Diff | Splinter Review
Network Stats V4 (36.19 KB, patch)
2012-08-07 12:17 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Network Stats V5 (36.01 KB, patch)
2012-08-08 00:26 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Netd integration based on V9 of bug 735547 (6.61 KB, patch)
2012-08-17 05:51 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
NetworkStats IDL v6 (2.97 KB, patch)
2012-08-17 05:56 PDT, Albert [:albert]
jonas: review+
Details | Diff | Splinter Review
NetworkStats Implementation v6 (36.07 KB, patch)
2012-08-17 05:57 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Netd integration based on V9 of bug 735547 (5.95 KB, patch)
2012-08-20 04:36 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
NetworkStats IDL v7 (3.52 KB, patch)
2012-08-20 04:40 PDT, Albert [:albert]
jonas: review+
Details | Diff | Splinter Review
NetworkStats Implementation v7 (32.45 KB, patch)
2012-08-20 04:51 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
NetworkStats IDL v8 (3.54 KB, patch)
2012-08-22 08:15 PDT, Albert [:albert]
jonas: review+
jduell.mcbugs: feedback+
Details | Diff | Splinter Review
Netd integration based on V9 of bug 735547 (6.03 KB, patch)
2012-08-22 09:22 PDT, Albert [:albert]
philipp: review+
Details | Diff | Splinter Review
NetworkStats Implementation v8 (33.25 KB, patch)
2012-08-22 11:55 PDT, Albert [:albert]
philipp: review-
jst: superreview+
Details | Diff | Splinter Review
NetworkStats Implementation v9 (34.17 KB, patch)
2012-09-03 05:21 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
NetworkStats IDL v9 (3.90 KB, patch)
2012-09-05 04:03 PDT, Albert [:albert]
jonas: review+
Details | Diff | Splinter Review
NetworkStats Implementation v10 (36.08 KB, patch)
2012-09-06 08:05 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
NetworkStats IDL v9 (3.86 KB, patch)
2012-09-06 08:07 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
NetworkStats IDL v10 (3.86 KB, patch)
2012-09-06 08:10 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Netd integration based on V9 of bug 735547 (6.09 KB, patch)
2012-09-10 07:20 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Netd integration (6.09 KB, patch)
2012-09-10 07:22 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
NetworkStats Implementation v11 (33.86 KB, patch)
2012-09-10 07:24 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
NetworkStats IDL v11 (3.86 KB, patch)
2012-09-13 10:10 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
NetworkStats Implementation v12 (37.11 KB, patch)
2012-09-13 10:12 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Tests V1 (12.15 KB, patch)
2012-09-18 08:45 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
NetworkStats Implementation v12 (37.18 KB, patch)
2012-09-18 09:44 PDT, Albert [:albert]
philipp: review+
Details | Diff | Splinter Review
NetworkStats Implementation v14 (37.20 KB, patch)
2012-09-20 08:59 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Tests V2 (30.36 KB, patch)
2012-09-20 09:01 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Tests V3 (30.04 KB, patch)
2012-09-26 06:59 PDT, Albert [:albert]
philipp: review+
Details | Diff | Splinter Review
NetworkStats IDL v12 (3.82 KB, patch)
2012-09-28 03:56 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Tests V4 (31.07 KB, patch)
2012-10-01 14:44 PDT, Albert [:albert]
marshall: review+
Details | Diff | Splinter Review
NetworkStats IDL v13 (3.82 KB, patch)
2012-10-03 10:48 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
NetworkStats Implementation v15 (37.40 KB, patch)
2012-10-03 10:51 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Netd integration V7 (6.28 KB, patch)
2012-10-03 10:52 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Tests V5 (31.43 KB, patch)
2012-10-03 10:55 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Part 1: Netd integration (v7) (6.21 KB, patch)
2012-10-03 15:05 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Part 2: NetworkStats IDL (v13) (4.03 KB, patch)
2012-10-03 15:07 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Part 3: NetworkStats Implementation (v15) (37.37 KB, patch)
2012-10-03 15:08 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Part 4: NetworkStats tests (v5) (31.54 KB, patch)
2012-10-03 15:09 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
NetworkStats Implementation (v16) (37.38 KB, patch)
2012-10-04 08:24 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Part 1: Netd integration (v7) (6.22 KB, patch)
2012-10-04 09:49 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Part 2: NetworkStats IDL (v13) (4.04 KB, patch)
2012-10-04 09:52 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Part 3: NetworkStats Implementation (v16) (37.38 KB, patch)
2012-10-04 09:53 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review
Part 4: NetworkStats tests (v6) (31.50 KB, patch)
2012-10-04 09:54 PDT, Albert [:albert]
no flags Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-16 23:23:49 PDT
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.
Comment 1 Honza Bambas (:mayhemer) 2012-04-17 07:48:09 PDT
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.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-17 11:49:04 PDT
What's a rough back-of-the-envelope guess at the bandwidth consumed by SSL handshaking?
Comment 3 Honza Bambas (:mayhemer) 2012-04-25 12:28:54 PDT
(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?)
Comment 4 Fernando R. Sela (no CC, needinfo please) [:frsela] 2012-04-26 08:51:19 PDT
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
Comment 5 Shian-Yow Wu [:swu] 2012-04-26 19:29:58 PDT
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?
Comment 6 Shian-Yow Wu [:swu] 2012-05-21 06:01:31 PDT
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.
Comment 7 Albert [:albert] 2012-05-21 07:10:13 PDT
Created attachment 625633 [details] [diff] [review]
Created

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?
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-21 12:40:22 PDT
We need to persist this data.

If netd is already doing this for us, awesome!  Let's use that.
Comment 9 Albert [:albert] 2012-05-21 16:48:29 PDT
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.
Comment 10 Albert [:albert] 2012-05-23 06:02:07 PDT
Created attachment 626407 [details] [diff] [review]
Here is the patch getting info from netd daemon
Comment 11 Albert [:albert] 2012-06-05 01:49:54 PDT
Created attachment 630101 [details]
Web API NetworkUsageStats idl

As discussed in the WebAPIs mailing list here we have the IDL corresponding to the expose the usage stats per interface
Comment 12 Albert [:albert] 2012-06-05 04:05:28 PDT
Created attachment 630128 [details]
idl

idl updated with correct format, license, etc
Comment 13 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-06 01:48:06 PDT
Comment on attachment 630128 [details]
idl

I'm not the best guy for this, roping in Mounir and Jonas.
Comment 14 Mounir Lamouri (:mounir) 2012-06-06 03:43:22 PDT
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.
Comment 15 Albert [:albert] 2012-06-06 08:25:24 PDT
(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 16 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-06-06 17:36:17 PDT
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.
Comment 17 [:fabrice] Fabrice Desré 2012-06-14 22:16:55 PDT
(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?
Comment 18 Albert [:albert] 2012-06-15 00:34:18 PDT
(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
Comment 19 Albert [:albert] 2012-06-26 13:33:43 PDT
Created attachment 636856 [details] [diff] [review]
Feature implemented

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?
Comment 20 Mounir Lamouri (:mounir) 2012-06-26 15:29:06 PDT
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.
Comment 21 Albert [:albert] 2012-06-27 00:43:11 PDT
Created attachment 637018 [details] [diff] [review]
Expose IDL to DOM impl

Implementation to expose methods and attributes to DOM
Comment 22 Andrew Overholt [:overholt] (back Aug 31) 2012-06-27 08:42:28 PDT
Nomming for basecamp.
Comment 23 Lucas Adamski [:ladamski] 2012-06-28 01:48:52 PDT
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 24 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-29 08:34:37 PDT
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.
Comment 25 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-29 08:35:50 PDT
Comment on attachment 637018 [details] [diff] [review]
Expose IDL to DOM impl

(Similarly here, not code I can review.)
Comment 26 Vincent Chang[:vchang][changyihsin] 2012-07-03 01:00:23 PDT
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 27 Philipp von Weitershausen [:philikon] 2012-07-03 15:51:06 PDT
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.
Comment 28 Mounir Lamouri (:mounir) 2012-07-03 16:41:49 PDT
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.
Comment 29 Philipp von Weitershausen [:philikon] 2012-07-06 16:24:52 PDT
Comment on attachment 636856 [details] [diff] [review]
Feature implemented

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

Clearing review flags as per previous comments.
Comment 30 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-10 21:02:45 PDT
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.
Comment 31 Albert [:albert] 2012-07-12 01:36:31 PDT
Created attachment 641387 [details]
Updated interface definition V2

Minor changes to idl
Comment 32 Albert [:albert] 2012-07-12 01:49:21 PDT
Created attachment 641391 [details] [diff] [review]
Updated interface implementation V2

Applied proposed changes
Comment 33 Albert [:albert] 2012-07-12 02:00:12 PDT
(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.
Comment 34 Philipp von Weitershausen [:philikon] 2012-07-12 16:37:35 PDT
Please mark patches and attachments that are no longer valid as obsolete. Thanks!
Comment 35 Philipp von Weitershausen [:philikon] 2012-07-12 16:46:45 PDT
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.
Comment 36 Albert [:albert] 2012-07-14 09:56:31 PDT
Created attachment 642247 [details] [diff] [review]
Updated interface implementation V2

Changed C++ implementation to JS
Comment 37 Philipp von Weitershausen [:philikon] 2012-07-16 14:54:50 PDT
Comment on attachment 641387 [details]
Updated interface definition V2

Can you submit this in form of a patch, please? Thanks!
Comment 38 Philipp von Weitershausen [:philikon] 2012-07-16 14:55:59 PDT
(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 39 Philipp von Weitershausen [:philikon] 2012-07-16 16:12:16 PDT
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++.
Comment 40 Albert [:albert] 2012-07-28 23:17:47 PDT
Created attachment 646945 [details] [diff] [review]
Update V3
Comment 41 Albert [:albert] 2012-07-28 23:19:39 PDT
Created attachment 646946 [details] [diff] [review]
Netd integration V1 based on patch for bug 735547
Comment 42 Albert [:albert] 2012-07-28 23:43:24 PDT
(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.
Comment 43 Philipp von Weitershausen [:philikon] 2012-07-30 18:13:13 PDT
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).
Comment 44 Philipp von Weitershausen [:philikon] 2012-07-30 18:14:21 PDT
(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
Comment 45 Albert [:albert] 2012-07-30 23:41:54 PDT
(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
Comment 46 Albert [:albert] 2012-08-06 07:21:45 PDT
Created attachment 649265 [details] [diff] [review]
Added update stats before get request is processed
Comment 47 Albert [:albert] 2012-08-06 07:23:58 PDT
Created attachment 649266 [details] [diff] [review]
Netd integration based on bug 735547
Comment 48 Philipp von Weitershausen [:philikon] 2012-08-06 16:58:38 PDT
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`.
Comment 49 Philipp von Weitershausen [:philikon] 2012-08-06 18:43:32 PDT
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)
Comment 50 Albert [:albert] 2012-08-07 12:17:01 PDT
Created attachment 649739 [details] [diff] [review]
Network Stats V4
Comment 51 Albert [:albert] 2012-08-07 12:38:21 PDT
(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
Comment 52 Philipp von Weitershausen [:philikon] 2012-08-07 12:59:28 PDT
(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.
Comment 53 Albert [:albert] 2012-08-08 00:26:22 PDT
Created attachment 649982 [details] [diff] [review]
Network Stats V5
Comment 54 Albert [:albert] 2012-08-08 00:34:04 PDT
(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.
Comment 55 Albert [:albert] 2012-08-17 05:51:06 PDT
Created attachment 652736 [details] [diff] [review]
Netd integration based on V9 of bug 735547

Updated to last version of netd at bug 735547
Comment 56 Albert [:albert] 2012-08-17 05:56:14 PDT
Created attachment 652738 [details] [diff] [review]
NetworkStats IDL v6

Updated to remove the use of long arrays following the comment 46 on bug 746073
Comment 57 Albert [:albert] 2012-08-17 05:57:44 PDT
Created attachment 652739 [details] [diff] [review]
NetworkStats Implementation v6
Comment 58 Philipp von Weitershausen [:philikon] 2012-08-17 11:53:40 PDT
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/
Comment 59 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-08-17 15:25:20 PDT
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.
Comment 60 Albert [:albert] 2012-08-18 00:53:16 PDT
(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.
Comment 61 Albert [:albert] 2012-08-20 04:36:43 PDT
Created attachment 653332 [details] [diff] [review]
Netd integration based on V9 of bug 735547
Comment 62 Albert [:albert] 2012-08-20 04:40:29 PDT
Created attachment 653333 [details] [diff] [review]
NetworkStats IDL v7

Changes based on comment 59
Comment 63 Albert [:albert] 2012-08-20 04:51:15 PDT
Created attachment 653337 [details] [diff] [review]
NetworkStats Implementation v7

Minor changes to adapt implementation to new version of the idl
Comment 64 Jason Duell [:jduell] (needinfo me) 2012-08-21 18:27:15 PDT
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 65 Jason Duell [:jduell] (needinfo me) 2012-08-21 18:27:31 PDT
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.
Comment 66 Albert [:albert] 2012-08-22 05:56:28 PDT
(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.
Comment 67 Albert [:albert] 2012-08-22 08:15:51 PDT
Created attachment 654213 [details] [diff] [review]
NetworkStats IDL v8

Changes based on comment 64
Comment 68 Albert [:albert] 2012-08-22 09:22:47 PDT
Created attachment 654236 [details] [diff] [review]
Netd integration based on V9 of bug 735547

Changes based on comment 65
Comment 69 Albert [:albert] 2012-08-22 11:55:59 PDT
Created attachment 654301 [details] [diff] [review]
NetworkStats Implementation v8

Changes according to IDL v8
Comment 70 Dietrich Ayala (:dietrich) 2012-08-27 08:54:52 PDT
Hey Jason, it's been 5 days since these were posted - do you have an ETA for when you can review?
Comment 71 Johnny Stenback (:jst, jst@mozilla.com) 2012-08-27 12:06:26 PDT
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 72 Jason Duell [:jduell] (needinfo me) 2012-08-27 23:26:17 PDT
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"
Comment 73 Jason Duell [:jduell] (needinfo me) 2012-08-28 00:12:09 PDT
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.
Comment 74 Johnny Stenback (:jst, jst@mozilla.com) 2012-08-28 07:37:04 PDT
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.
Comment 75 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-08-29 10:31:48 PDT
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.
Comment 76 Jason Duell [:jduell] (needinfo me) 2012-08-29 10:44:26 PDT
> 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...
Comment 77 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-08-29 14:24:21 PDT
Crap! Sorry about that. Entirely my bad. Totally ok to ask for sr with that clarification.
Comment 78 Philipp von Weitershausen [:philikon] 2012-08-30 13:28:19 PDT
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.
  */
Comment 79 Philipp von Weitershausen [:philikon] 2012-08-30 14:20:37 PDT
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;
  };
Comment 80 Albert [:albert] 2012-09-03 05:21:28 PDT
Created attachment 657833 [details] [diff] [review]
NetworkStats Implementation v9

Changes based on comment 79
Comment 81 Albert [:albert] 2012-09-05 04:03:52 PDT
Created attachment 658440 [details] [diff] [review]
NetworkStats IDL v9

Modified IDL to include sampleRate and storageDateLimit to nsIDOMMozNetworkStatsManager.

These parameters are included because the people of costcontrol webapp requested them.
Comment 82 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-09-05 17:54:29 PDT
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.
Comment 83 Albert [:albert] 2012-09-06 08:05:04 PDT
Created attachment 658886 [details] [diff] [review]
NetworkStats Implementation v10

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).
Comment 84 Albert [:albert] 2012-09-06 08:07:13 PDT
Created attachment 658887 [details] [diff] [review]
NetworkStats IDL v9

Applied comments of Comment 82
Comment 85 Albert [:albert] 2012-09-06 08:10:49 PDT
Created attachment 658888 [details] [diff] [review]
NetworkStats IDL v10

Applied comments of Comment 82

Last comment 84 has wrong patch, corrected.
Sorry about that..
Comment 86 Philipp von Weitershausen [:philikon] 2012-09-07 17:05:19 PDT
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;
  }
  ...
Comment 87 Albert [:albert] 2012-09-10 07:20:51 PDT
Created attachment 659703 [details] [diff] [review]
Netd integration based on V9 of bug 735547
Comment 88 Albert [:albert] 2012-09-10 07:22:48 PDT
Created attachment 659704 [details] [diff] [review]
Netd integration

Changed uuid of NetworkManager idl
Comment 89 Albert [:albert] 2012-09-10 07:24:53 PDT
Created attachment 659705 [details] [diff] [review]
NetworkStats Implementation v11

Changes from comment 86
Comment 90 Albert [:albert] 2012-09-10 07:27:35 PDT
(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.
Comment 91 Philipp von Weitershausen [:philikon] 2012-09-10 11:01:13 PDT
(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?
Comment 92 Albert [:albert] 2012-09-10 11:27:05 PDT
(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 93 Philipp von Weitershausen [:philikon] 2012-09-10 11:30:04 PDT
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?
Comment 94 Albert [:albert] 2012-09-13 10:10:42 PDT
Created attachment 660881 [details] [diff] [review]
NetworkStats IDL v11

Changed interface name 'nsINetworkStatsData' by 'nsIDOMMozNetworkStatsData'.
Comment 95 Albert [:albert] 2012-09-13 10:12:45 PDT
Created attachment 660882 [details] [diff] [review]
NetworkStats Implementation v12

Changes from comment 93.

Still working on implementing mochitest for idl and unit tests for service
Comment 96 Albert [:albert] 2012-09-18 08:45:42 PDT
Created attachment 662182 [details] [diff] [review]
Tests V1

Added tests
Comment 97 Albert [:albert] 2012-09-18 09:44:44 PDT
Created attachment 662200 [details] [diff] [review]
NetworkStats Implementation v12

Changes from Comment 93 and changes from bugs detected when implementing tests.
Comment 98 Philipp von Weitershausen [:philikon] 2012-09-19 16:35:52 PDT
(In reply to Albert from comment #97)
> and changes from bugs detected when implementing tests.

That's why we require tests. :)
Comment 99 Philipp von Weitershausen [:philikon] 2012-09-19 16:55:51 PDT
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.
Comment 100 Albert [:albert] 2012-09-20 08:59:47 PDT
Created attachment 663036 [details] [diff] [review]
NetworkStats Implementation v14

Changes from comment 99 and rebase
Comment 101 Albert [:albert] 2012-09-20 09:01:46 PDT
Created attachment 663037 [details] [diff] [review]
Tests V2

Added xpcshell tests
Comment 102 Philipp von Weitershausen [:philikon] 2012-09-25 09:58:55 PDT
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.
Comment 103 Albert [:albert] 2012-09-26 06:59:50 PDT
Created attachment 664937 [details] [diff] [review]
Tests V3

Changes from comment 102.
Comment 104 Philipp von Weitershausen [:philikon] 2012-09-27 22:59:53 PDT
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.
Comment 105 Albert [:albert] 2012-09-28 03:56:49 PDT
Created attachment 665849 [details] [diff] [review]
NetworkStats IDL v12

Rebased
Comment 106 Fernando Jiménez Moreno [:ferjm] 2012-09-28 04:38:18 PDT
Try build: https://tbpl.mozilla.org/?tree=Try&rev=ad1c3f319b74
Comment 107 Mozilla RelEng Bot 2012-09-28 05:00:42 PDT
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
Comment 108 Mozilla RelEng Bot 2012-09-28 09:15:31 PDT
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
Comment 109 Albert [:albert] 2012-10-01 14:44:15 PDT
Created attachment 666719 [details] [diff] [review]
Tests V4

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
Comment 110 Fernando Jiménez Moreno [:ferjm] 2012-10-02 03:28:50 PDT
New try build: https://tbpl.mozilla.org/?tree=Try&rev=cd9e56a21ec3
Comment 111 Marshall Culpepper [:marshall_law] 2012-10-02 08:59:11 PDT
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?
Comment 112 Vicamo Yang [:vicamo][:vyang] 2012-10-02 13:26:38 PDT
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
Comment 113 Albert [:albert] 2012-10-03 10:48:45 PDT
Created attachment 667544 [details] [diff] [review]
NetworkStats IDL v13

Rebased
Comment 114 Albert [:albert] 2012-10-03 10:51:02 PDT
Created attachment 667545 [details] [diff] [review]
NetworkStats Implementation v15

rebased
Comment 115 Albert [:albert] 2012-10-03 10:52:47 PDT
Created attachment 667548 [details] [diff] [review]
Netd integration V7

rebased
Comment 116 Albert [:albert] 2012-10-03 10:55:09 PDT
Created attachment 667550 [details] [diff] [review]
Tests V5

Changes from comment 112 and rebased
Comment 117 Marshall Culpepper [:marshall_law] 2012-10-03 11:06:01 PDT
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
Comment 118 Albert [:albert] 2012-10-03 15:05:33 PDT
Created attachment 667675 [details] [diff] [review]
Part 1: Netd integration (v7)
Comment 119 Albert [:albert] 2012-10-03 15:07:04 PDT
Created attachment 667678 [details] [diff] [review]
Part 2: NetworkStats IDL (v13)
Comment 120 Albert [:albert] 2012-10-03 15:08:37 PDT
Created attachment 667681 [details] [diff] [review]
Part 3: NetworkStats Implementation (v15)
Comment 121 Albert [:albert] 2012-10-03 15:09:50 PDT
Created attachment 667684 [details] [diff] [review]
Part 4: NetworkStats tests (v5)
Comment 122 Marshall Culpepper [:marshall_law] 2012-10-04 08:00:11 PDT
(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?
Comment 123 Albert [:albert] 2012-10-04 08:24:55 PDT
Created attachment 667997 [details] [diff] [review]
NetworkStats Implementation (v16)

Rebase
Comment 124 Marshall Culpepper [:marshall_law] 2012-10-04 09:13:10 PDT
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.
Comment 125 Albert [:albert] 2012-10-04 09:49:18 PDT
Created attachment 668033 [details] [diff] [review]
Part 1: Netd integration (v7)

Changed author
Comment 126 Albert [:albert] 2012-10-04 09:52:09 PDT
Created attachment 668034 [details] [diff] [review]
Part 2: NetworkStats IDL (v13)

Changed author
Comment 127 Albert [:albert] 2012-10-04 09:53:35 PDT
Created attachment 668035 [details] [diff] [review]
Part 3: NetworkStats Implementation (v16)
Comment 128 Albert [:albert] 2012-10-04 09:54:44 PDT
Created attachment 668037 [details] [diff] [review]
Part 4: NetworkStats tests (v6)

rebase
Comment 130 Marshall Culpepper [:marshall_law] 2012-10-04 13:17:54 PDT
Required a follow-up because of a missing comma in test_interfaces.html:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61dbc8b4ca76
Comment 132 Marshall Culpepper [:marshall_law] 2012-10-18 10:02:18 PDT
Comment on attachment 668037 [details] [diff] [review]
Part 4: NetworkStats tests (v6)

Clearing these checkin flags, this has landed.
Comment 133 Paul Theriault [:pauljt] 2013-01-29 15:46:55 PST
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.

Note You need to log in before you can comment on or make changes to this bug.