Closed Bug 784575 Opened 7 years ago Closed 6 years ago

Provide service to get aggregate network traffic per app

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
blocking-basecamp -

People

(Reporter: jduell.mcbugs, Assigned: johnshih.bugs)

References

Details

Attachments

(9 files, 69 obsolete files)

38.09 KB, application/pdf
Details
5.89 KB, patch
Details | Diff | Splinter Review
36.69 KB, application/pdf
Details
1.68 KB, patch
johnshih.bugs
: review+
Details | Diff | Splinter Review
16.01 KB, patch
airpingu
: review+
albert
: review+
Details | Diff | Splinter Review
3.86 KB, patch
johnshih.bugs
: review+
Details | Diff | Splinter Review
33.83 KB, patch
johnshih.bugs
: review+
Details | Diff | Splinter Review
10.40 KB, patch
johnshih.bugs
: review+
Details | Diff | Splinter Review
6.13 KB, patch
johnshih.bugs
: review+
Details | Diff | Splinter Review
Biesi and I decided to create a single service that will provide the simple aggregate network traffic per appID, rather than splitting out duplicate (and more complex) copies of nsIHttpActivityDistributor.

So far I just have IDLs.

Let me know if this service needs to be accessible from child processes--for now I'm assuming just the parent.
attachment 652065 [details] [diff] [review] from bug 746073 is a start here for the HTTP implementation.
blocking-basecamp: --- → ?
(In reply to Jason Duell (:jduell) from comment #0)
> Biesi and I decided to create a single service that will provide the simple
> aggregate network traffic per appID, rather than splitting out duplicate
> (and more complex) copies of nsIHttpActivityDistributor.
> 
> So far I just have IDLs.
> 
> Let me know if this service needs to be accessible from child processes--for
> now I'm assuming just the parent.

Just having a IRC discussion with Jason. We're hoping to design a new service NetworkTrafficService in Necko, which will aggregate the traffic from all all protocols (HTTP, FTP, websockets). Some questions about the HTTP part:

1. So for each HTTP transaction IO (in nsHTTPTransaction), we will aggregate the counts in the NetworkTrafficService. Right?

2. For any other applications like network metering (bug 746073), they can get the dynamic traffic counts they want through this NetworkTrafficService?

3. May I ask what's the current IDL you have?
Move this patch from bug 746073 to here and include :mcmanus's suggested changes. Although this patch was originally designed based on the use of nsHttpActivityDistributor, some code segments could be reused for the new service.
Chris Lee, if we make bug 746073 a blocker, please also make this one a blocker.
Whiteboard: [blocked-on-input Chris Lee]
Some quick draft about the possible IDL as below. Hi, :jduell and :mcmanus, could you please have some comments on the design? Just some personal thoughts, I believe it wouldn't be that simple. ;)

dictionary NetworkTrafficOptions
{
  /**
   * The ID of an app which contributes to the traffic.
   */
  uint32_t appID;

  /**
   * "mobile", "wifi" or null
   */
  DOMString connectionType;

  /**
   * "http", "ftp", "websocket" or null
   */
  DOMString protocalType;

  /**
   * "read", "write" or null
   */
  DOMString IODirection;
};

interface nsIByteCountChangedCb : nsISupports 
{
  void onByteCountChanged();
};

interface nsINetworkTrafficService : nsISupports
{
  /**
   * |aOptions|: a NetworkTrafficOptions type.
   */
  uint64_t getByteCount(in jsval aOptions);

  /**
   * |aOptions|: a NetworkTrafficOptions type.
   * |aByteCount|: the byte count to be aggregated respecting to the |options|.
   */
  void addByteCount(in jsval aOptions, in uint64_t aByteCount);

  /**
   * |aOptions|: a NetworkTrafficOptions type.
   * |aByteCountChangedCb|: will be called to notify the subscribers,
   * when the byte count respecting to the |options| is changed.
   */
  void addByteCountChangedCb(in jsval aOptions, 
                             in nsIByteCountChangedCb aByteCountChangedCb);

  /**
   * |aOptions|: a NetworkTrafficOptions type.
   */
  void clearByteCount(in jsval aOptions);
};
Hi :jduell,

Ping on this one. May I follow up with you for the IDL you are going to propose? :) Thanks!
bug 746073 is not a blocker today so minus'ing.
blocking-basecamp: ? → -
Whiteboard: [blocked-on-input Chris Lee]
Hi :jduell :mcmanus,
it's seems like we need to start at working on this bug again
Could you provide any advice/thought about this bug you currently think?
Are we just going to follow the previous work and working on other protocol or something?

Thanks
Flags: needinfo?(jduell.mcbugs)
As discussed in email thread we're going to hook at the HTTP/FTP/Websockets (and also the TCP socket JS API) here, as it makes attributing traffic to apps easy.   We will want some sort of fudge factor (Honza suggests multiplying by 1.07) to account for SSL overhead when https or secure websockets are used.  (We may try to gauge SSL overhead more accurately in followup work, but not in this bug).

I suggest we split out HTTP, FTP, Websockets, and JS TCP metering into separate bugs (or at least patches) as reviewers may differ.  Honza/mcmanus are prob best for HTTP review, maybe FTP too (no one know the code well, so anyone? :), JS TCP would be josh matthews (:jdm).
Flags: needinfo?(jduell.mcbugs)
Attached file Network Metering Architecture v1 (obsolete) —
Here is a rough architecture for stage 1 of network metering per app.
First we're going to count the traffic at different protocal layer (HTTP/FTP/WebSockets/TCP API/UDP API)
Each of the result (in a format based on the IDL) will save to the database (probably reuse the NetworkStatsDB) through a service (We need to add this one, temporarily called NetworkTrafficService)
Then the result can be gotten by NetworkManager.js (which is the implementation of mozNetworkStats API) through NetworkStatsService.jsm

kindly ask for your advice,
Thanks!
Flags: needinfo?(jduell.mcbugs)
Based on the architecture, we need to split this bug into:
Part 0: Proposed IDL
Part 1: NetworkTrafficService
Part 2: Count in HTTP
Part 3: Count in FTP
Part 4: Count in WebSockets
Part 5: Count in TCP API
Part 6: Count in UDP API
Part 7: Modify NetworkManager/NetworkStatsService/NetworkStatsDB

I suggest to open bugs at least for Part 2 - Part 7
Assignee: nobody → jshih
Depends on: 855946
Depends on: 855948
Depends on: 855949
Depends on: 855951
Attached file Network Metering Architecture v1 (obsolete) —
Refine: Add the boundary between Content and Chrome Process
Attachment #730595 - Attachment is obsolete: true
The architecture looks good!  Everything up to the NetworkStatsService will exist for desktop too, not just B2G, right?  We may want to use this info in desktop at some point.

A couple notes on what data to keep:

1) Let's have the NetworkTrafficService say whether traffic was over TLS (ie. https, wss, I think we may support secure JS TCP socket too).  This may be useful for various reasons: if nothing else so we can adjust the "total traffic" number we generate if we decide 1.07 isn't the right multiplier, etc.

2) dougt suggests that in the future we may want to provide a feature that allows users to see which hostnames use most of their traffic.  So let's also see if it's easy to also have NetworkTrafficService say which domain the traffic was sent to/from. I think we'll want both the full hostname ("mail.google.com") and the effective top level domain (eTLD: i.e. "facebook.com" not "host123.facebook.com")--you can get that from the URI or host:  see

   http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#3126  (or line 3172 for host)

(to avoid calculating base domain for each packet I'd store it as mBaseDomain in channel).

I like doug's idea here, but I'm not sure if storing all this hostname info will bloat the database too much, at least for B2G phone.  Thoughts?  If we just store the eTLD, it will be less rows in the database. 

Even if we don't use the hostname info at all in B2G I can see it being useful in desktop.  Ex: if my browser with 100 tabs open is consuming lots of bandwidth I could see which hosts are consuming that data, which helps me figure out which tabs are to blame. (This is why I think we want both "mail.google.com" as well as google.com, BTW).
Flags: needinfo?(jduell.mcbugs)
(In reply to Jason Duell (:jduell) from comment #13)
> The architecture looks good!  Everything up to the NetworkStatsService will
> exist for desktop too, not just B2G, right?  We may want to use this info in
> desktop at some point.
> 

> A couple notes on what data to keep:
> 
> 1) Let's have the NetworkTrafficService say whether traffic was over TLS
> (ie. https, wss, I think we may support secure JS TCP socket too).  This may
> be useful for various reasons: if nothing else so we can adjust the "total
> traffic" number we generate if we decide 1.07 isn't the right multiplier,
> etc.
> 
> 2) dougt suggests that in the future we may want to provide a feature that
> allows users to see which hostnames use most of their traffic.  So let's
> also see if it's easy to also have NetworkTrafficService say which domain
> the traffic was sent to/from. I think we'll want both the full hostname
> ("mail.google.com") and the effective top level domain (eTLD: i.e.
> "facebook.com" not "host123.facebook.com")--you can get that from the URI or
> host:  see
> 
>   
> http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.
> cpp#3126  (or line 3172 for host)
> 
> (to avoid calculating base domain for each packet I'd store it as
> mBaseDomain in channel).
> 
> I like doug's idea here, but I'm not sure if storing all this hostname info
> will bloat the database too much, at least for B2G phone.  Thoughts?  If we
> just store the eTLD, it will be less rows in the database. 


> Even if we don't use the hostname info at all in B2G I can see it being
> useful in desktop.  Ex: if my browser with 100 tabs open is consuming lots
> of bandwidth I could see which hosts are consuming that data, which helps me
> figure out which tabs are to blame. (This is why I think we want both
> "mail.google.com" as well as google.com, BTW).

As we want this bug can be both applied to desktop and b2g, I have some questions here:
1. The previous solution in [1], we identify an HTTP transaction by getting the appid from the channel. What does the appid means in desktop and b2g ? It seems an important information for the per "app" traffic meter. I wonder if the appid can represent an app in b2g and an tab in desktop.

2. We now are going to count the traffic per HTTP transaction (for example), will there be only one host in a transaction? 

3. We also need IDL here. (now we have appid, hostname, incoming traffic, outgoing traffic, connection type, protocol type, timestamp...)

Thanks.
What is the NetworkManager?  Is it something already existing or invented in this bug?

For desktop or mobile (no apps) it would be interesting to know how many bytes consumes a particular website when I browse around it.  When e.g. www.example.com refers google fonts, 3th party hosted jquery and a pile of CDN hosted images, I want to know how many bytes cost me in the whole visiting pages on www.example.com.  It has to include all subresources, not just data under *.example.com, IMO.

Thus, we could group by the initial URI only (loadgroup's default channel).
(In reply to Honza Bambas (:mayhemer) from comment #15)
> What is the NetworkManager?  Is it something already existing or invented in
> this bug?

I think it is a typo. We should extend NetworkStatsManager to support per-app network statistic. 

NetworkManager is implemented in dom/system/gonk/NetworkManager.js including following functions, 
1. Listening to link status of network interfaces and configure the default routing rule and dns server. 
2. An interface to send/receive commands to netd to configure nat rules, routing, get network interface's stat....

But it is for B2G only right now.
It would be better if we can also know that these network traffics are transmit/receive via which network interface, and the users can have information to know about how many traffics are transmit/receive via 3G interface, which app contributes most of traffics.
 
Any ideas about how to obtain these information ? Android can obtain these information from /proc interface. These information are collected in IP layer uisng ip filter module.  

/proc/net/xt_qtaguid/stats
/proc/net/xt_qtaguid/iface_stat/interface name/rx_bytes    
/proc/net/xt_qtaguid/iface_stat/interface name/rx_packets  
/proc/net/xt_qtaguid/iface_stat/interface name/tx_bytes
/proc/net/xt_qtaguid/iface_stat/interface name/tx_packets
Fix the typo, NetworkManager is actually NetworkStatsManager
which is the implementation of navigator.mozNetworkStats
Attachment #731062 - Attachment is obsolete: true
Now seems we have one more question:
Because NetworkStats API (NetworkStatsManager) [1] which we suppose to extent to support per app network statistic doesn't support desktop, are we going to create a new DOM API? (Still we need to clarify how get per traffic in b2g as well as per hostname traffic in desktop)
(In reply to Vincent Chang[:vchang] from comment #17)
> It would be better if we can also know that these network traffics are
> transmit/receive via which network interface, and the users can have
> information to know about how many traffics are transmit/receive via 3G
> interface, which app contributes most of traffics.
>  
> Any ideas about how to obtain these information ? Android can obtain these
> information from /proc interface. These information are collected in IP
> layer uisng ip filter module.  
> 
> /proc/net/xt_qtaguid/stats
> /proc/net/xt_qtaguid/iface_stat/interface name/rx_bytes    
> /proc/net/xt_qtaguid/iface_stat/interface name/rx_packets  
> /proc/net/xt_qtaguid/iface_stat/interface name/tx_bytes
> /proc/net/xt_qtaguid/iface_stat/interface name/tx_packets

Android creates an UID (user ID) per app, so the app is always executed by the same user and stats are collected according to the uid. Looking for something similar in B2G I have seen we have '/proc/{PID}/net/netstat' files.

The problem is that each time an app is launched the PID changes and the uid is mapped to the PID, so we can't track the stats using this approach. Also, the netstats file does not provide the stats per PID, it gives global stats.
> It would be better if we can also know that these network traffics 
> are transmit/receive via which network interface

Very good point.  I assume that when you look at app data usage on Android it's only showing data for non-wifi use.  Does anyone know if that's correct?  

Ideas for getting traffic by interface: 

1) Ideally we could know from the channel/connection which interface is being used (we can get the local IP address of a connection from nsISocketTransport.getSelfAddr. Do different interfaces on B2G map to different local IP subnets?).  

2) Otherwise maybe a hack like setting a global variable  (gWifiInUse) and metering into different buckets based on that.  I assume existing TCP connection for 2G, etc, connections don't get dropped when the user turns on wifi (right?  If I'm silly enough to start a phone update over 2G and turn wifi on partway through I assume we keep downloading via 2G).  In that case we could check gWifiInUse at connect time and mark each connection/channel as wifi or not so we get accurate measurements.

3) Sounds like we might be able to use the *current* pid and see which interface /proc file is increasing as we download traffic.  Sounds really gross and error prone.


> it would be interesting to know how many bytes consumes a particular website 
> when I browse around it... we could group by the initial URI only
> (loadgroup's default channel)

Good point.  I increasingly suspect that per-site traffic metering is probably best done as a followup bug.  I'm willing to hear other opinions.  Perhaps in this bug we could start by having NetworkTrafficService identify the loadgroup's default channel's base domain (and maybe able the domain of the loaded object).  But if this starts getting complicated let's keep the focus on B2G requirements.
This is the prototype of NetworkTrafficService
The NetworkTrafficService provides simple interface of transfering the networkstates data to NetworkStatsDB
Each protocol can get the service by do_GetService()

The pending patch will be the modification of NetworkStatsDB, NetworkStatsService and NetworkStatsManager
Attachment #654105 - Attachment is obsolete: true
Fix Nit
Attachment #744539 - Attachment is obsolete: true
wip patch.
Modification of NetworkStatsManager.jg, NetworkStatsService.jsm, NetworkStatsDB.jan
patch update
Attachment #744548 - Attachment is obsolete: true
patch update.
I've done the modification in NetworStatsManager.js, NetworkStatsService.jsm, NetworkStatsDB.jsm
Attachment #747884 - Attachment is obsolete: true
Attachment #750331 - Attachment is obsolete: true
Hi John, I'm glad to give your some quick feedback when you're ready with you patches (we still need Necko peers' and Web API peers' reviews in the long run).
Depends on: 858003
Hi Gene, 
the next patch will be aggregate NetworkStatsService with NetworkTrafficService
I'm working on it.
Hi John, if you're wanting to convert your NetworkStatsService into a service, which can be launched at start-up, you need to add some configurations in the .manifest. Link [1] is a very good example.

[1] dom/mobilemessage/src/ril/MobileMessageDatabaseService.manifest
In order to integrate NetworkStatsService and NetworkTrafficService into one service, the first step is modify NetworkStatsService.jsm to NetworkStatsService.js
Is really necessary to change NetworkStatsService.jsm to NetworkStatsService.js?

Most services under dom folder are jsm files (alarms, contacts, push, etc). Now the NetworkStatsService is being launched at start-up..
This patch integrate the NetworkTrafficService into NetworStatsService
NetworkStats API (NetworkStatsManager & idl) and NetworkStatsDB modifcation
Attachment #750951 - Attachment is obsolete: true
Attachment #750328 - Attachment is obsolete: true
Hi Albert,
since we found that the role of NetworkTrafficService is actually very similar to NetworkStatsService (interact with NetworkStatsDB), so we then decide to make these two Services into only one.
The only problem is that the new service needs to provide an interface which can allow protocol layer to get the service and pass the traffic data through it.

That's why we have to change NetworkStatsService .jsm to .js (.jsm can not be accessed in c++ code)

About your concern, the .js service can still be launched at start-up according to what Gene referred in https://bugzilla.mozilla.org/show_bug.cgi?id=784575#c31

Maybe there is other concern about this change, I think we can have more discussion

Thanks for your response!
Because NetworkTrafficService have similar role to NetworkStatsService, we decide to integrate two services into only one service.
(In reply to John Shih from comment #37)
> Hi Albert,
> since we found that the role of NetworkTrafficService is actually very
> similar to NetworkStatsService (interact with NetworkStatsDB), so we then
> decide to make these two Services into only one.
> The only problem is that the new service needs to provide an interface which
> can allow protocol layer to get the service and pass the traffic data
> through it.
> 
> That's why we have to change NetworkStatsService .jsm to .js (.jsm can not
> be accessed in c++ code)
> 
> About your concern, the .js service can still be launched at start-up
> according to what Gene referred in
> https://bugzilla.mozilla.org/show_bug.cgi?id=784575#c31
> 
> Maybe there is other concern about this change, I think we can have more
> discussion
> 
> Thanks for your response!

Ok, I didn't know that .jsm can't be accessed from c++ code. As I see, more than an extension problem, the thing is that you need to acces the service with the contract defined in the manifest.

It's fine, thanks for the info!
add a quene to maintain app stats data in NetworkStatsService with aiming at reducing the frequency of writing data to DB
Attachment #753173 - Attachment is obsolete: true
fix nit.
Attachment #753629 - Attachment is obsolete: true
Attachment #753630 - Attachment is obsolete: true
We set five scenarios to write data in quene to the DB.
1. timer fire
2. query data (call getAppStats())
3. date of incoming data is different from the date save in quence (lastDate)
4. system shut down
5. a single traffic amount (rxBytes/txBytes) keeped in quence exceeds a upper bound (MAX_TRAFFIC)
Attachment #753631 - Attachment is obsolete: true
Attachment #753174 - Attachment is obsolete: true
Attachment #753130 - Attachment is obsolete: true
Attachment #753637 - Attachment is obsolete: true
Attachment #753712 - Attachment is obsolete: true
Attachment #753715 - Attachment is obsolete: true
Attachment #753716 - Attachment is obsolete: true
Modify IDL: Use app URL instead of app id to get app's traffic data
Attachment #753696 - Attachment is obsolete: true
After receiving app URL from gaia side, NetworkStatsService will obtain app id from app URL through AppsService. 
app id is used to retrieve app's traffic from indexed DB.
Attachment #753717 - Attachment is obsolete: true
Blocks: 871452
Attachment #755823 - Attachment is obsolete: true
Attachment #762569 - Flags: review?(gene.lian)
Attachment #753723 - Flags: review?(gene.lian)
Attachment #755821 - Flags: review?(gene.lian)
No longer blocks: 871452
Sorry for the delay. I'll start with the reviews on the next Monday. Also, we definitely need Albert's second review when I'm done the review.
(In reply to Gene Lian [:gene] from comment #53)
> Sorry for the delay. I'll start with the reviews on the next Monday. Also,
> we definitely need Albert's second review when I'm done the review.

Sure, I'll have a look after your review.
Fix bugs in NetworkStatsDB.jsm
Attachment #755821 - Attachment is obsolete: true
Attachment #755821 - Flags: review?(gene.lian)
Attachment #768245 - Flags: review?(gene.lian)
Fix bugs.
Attachment #768245 - Attachment is obsolete: true
Attachment #768245 - Flags: review?(gene.lian)
Attachment #768823 - Flags: review?(gene.lian)
Attachment #768823 - Attachment is obsolete: true
Attachment #768823 - Flags: review?(gene.lian)
Attachment #768850 - Flags: review?(gene.lian)
Comment on attachment 753723 [details] [diff] [review]
Bug 784575 - Part 0: Change NetworkStatsService.jsm to NetworkStatsService.js

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

Nice work! r=gene after fixing a tiny nit.

Hi Albert, could you please take the second review? Thanks!

::: dom/network/interfaces/nsINetworkStatsService.idl
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> +* License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +* You can obtain one at http://mozilla.org/MPL/2.0/. */
> + 

Please remove the trailing white spaces.

@@ +2,5 @@
> +* License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +* You can obtain one at http://mozilla.org/MPL/2.0/. */
> + 
> +#include "nsISupports.idl"
> + 

Please remove the trailing white spaces.

::: dom/network/src/NetworkStatsManager.js
@@ +19,5 @@
>  let appInfo = Cc["@mozilla.org/xre/app-info;1"];
>  let isParentProcess = !appInfo || appInfo.getService(Ci.nsIXULRuntime)
>                          .processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT;
>  if (isParentProcess) {
> + XPCOMUtils.defineLazyServiceGetter(this, "networkstatsService",

Please indent the line with 2 spaces.
Attachment #753723 - Flags: review?(gene.lian)
Attachment #753723 - Flags: review?(acperez)
Attachment #753723 - Flags: review+
Comment on attachment 753723 [details] [diff] [review]
Bug 784575 - Part 0: Change NetworkStatsService.jsm to NetworkStatsService.js

Nice!
Attachment #753723 - Flags: review?(acperez) → review+
Comment on attachment 768850 [details] [diff] [review]
Bug 784575 - Part 2: NetworkStats API & DB Modification.

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

Some reasons for giving review-:

1. You have to use .upgradeSchema(...) to update the DB schema.
2. I don't think you have to create new functions for app specific.

Btw, please move the IDL and the NetworkStatsManager.js into separate patch. DB is more related to the Service instead of Manager/IDL. Note that it's also more convenient for the super reviewer to review the IDL.

::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
@@ +13,5 @@
>     * 'mobile', 'wifi' or null.
>     * If null, stats for both mobile and wifi are returned.
>     */
>    DOMString connectionType;
> +  DOMString appURL;

s/appURL/manifestURL

|appURL| is a bit implicit to me. It sounds a URL where you can download the app. Also, please add some comments just like |connectionType|.

@@ +24,5 @@
>  {
>    /**
> +   * Query network statistics.
> +   *
> +   * If options.appURL is not provided, return netd statistics.

s/return netd statistics/return statistics regardless of the app/

Netd is a concept of implementation. The caller doesn't need to do know that.

::: dom/network/src/NetworkStatsDB.jsm
@@ +58,5 @@
>           */
>  
> +        objectStore = db.createObjectStore(STORE_NAME, { keyPath: ["appId", "connectionType", "timestamp"] });
> +        objectStore.createIndex("appId", "appId", { unique: false });
> +        objectStore.createIndex("isInBrowser", "isInBrowser", { unique: false });

We have a big issue here. Since the existing DB might already have a bunch of records, we cannot create new indexes and modify key patch like this. You have to take use of .upgradeSchema() to update the database schema. Please refer to MobileMessageDatabaseService.js for some references.

Updating indexes is easy but updating key path is difficult... You have to call .createObjectStore(NEW_STORE_NAME, { keyPath: [...] }) to create a new object store and copy the records from the old object store to the new one. Finally, call .deleteObjectStore(STORE_NAME) to delete the deprecated one.

@@ +80,5 @@
>      timestamp = Math.floor(timestamp / SAMPLE_RATE) * SAMPLE_RATE;
>      return timestamp;
>    },
>  
>    saveStats: function saveStats(stats, aResultCb) {

Let's s/saveStats/saveNetdStats/ since you're going to have another .saveAppStats(...).

@@ +98,5 @@
>          debug("Filtered time: " + new Date(timestamp));
>          debug("New stats: " + JSON.stringify(stats));
>        }
>  
> +      let request = store.index("appId").openCursor(stats.appId, "prev");

This change is weird. Why not opening cursor by connection type and filtering out the app ID instead? Note that you break the reverse searching for connection type here.

@@ +106,5 @@
> +          if (stats.connectionType != cursor.value.connectionType) {
> +            cursor.continue();
> +            return;
> +          }
> +        }

Please do:

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

  if (stats.connectionType != cursor.value.connectionType) {
    cursor.continue();
    return;
  }

@@ +195,5 @@
>  
>        // If diff < 0, clock or timezone changed back. Place data in the last sample.
>  
> +      if (newSample.rxTotalBytes >= lastSample.rxTotalBytes &&
> +          newSample.txTotalBytes >= lastSample.txTotalBytes) {

I don't why you add this condition...

@@ +208,5 @@
> +      }
> +    }
> +  },
> +
> +  saveAppStats: function saveAppStats(stats, aResultCb) {

It's fine to create another .saveAppStats(...) since you're going to search records by the index of App ID.

@@ +250,5 @@
> +        // Remove stats previous to now - VALUE_MAX_LENGTH
> +        this._removeOldStats(txn, store, stats.appId, stats.connectionType, stats.timestamp);
> +
> +        // Process stats before save
> +        this._processAppSamplesDiff(txn, store, cursor, stats);

But can we reuse the original ._processSamplesDiff()? I don't see much difference.

@@ +255,5 @@
> +      }.bind(this);
> +    }.bind(this), aResultCb);
> +  },
> +
> +  _processAppSamplesDiff: function _processAppSamplesDiff(txn, store, lastSampleCursor, newSample) {

Is that possible to reuse the original ._processSamplesDiff() instead of creating a new one for app specific? Too many copy-and-paste is not allowed in general.

@@ +373,5 @@
>      }
>  
>      this.dbNewTxn("readonly", function(txn, store) {
> +      let lowFilter = [0, aOptions.connectionType, start];
> +      let upFilter = [0, aOptions.connectionType, end];

Let's try to merge the .findAppStats() below with this function.

@@ +452,5 @@
> +      }.bind(this);
> +    }.bind(this), aResultCb);
> +  },
> +
> +  findAppStats: function findAppStats(aResultCb, aOptions) {

I don't think you have to create a new function for app specific. Please try to extend the original .find(...). I believe you can defer the branches by checking if the |aOptions.appId| is present or not.

@@ +495,5 @@
> +      }.bind(this);
> +    }.bind(this), aResultCb);
> +  },
> +
> +  findAppAllStats: function findAppAllStats(aResultCb, aOptions) {

The same. Please try to extend the original .findAll(...). You can defer the branches by checking if |aOptions.appId| is present or not.
Attachment #768850 - Flags: review?(gene.lian) → review-
Comment on attachment 762569 [details] [diff] [review]
Bug 784575 - Part 1: Integrate NetworkTrafficService into NetworkStatsService.

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

::: dom/network/interfaces/nsINetworkStatsService.idl
@@ +7,1 @@
>  [scriptable, uuid(18725604-e9ac-488a-8aa0-2471e7f6c0a4)]

It would be nice if you could update the uuid whenever you have something change for the interface, but you don't need to do that for this case because you've done that in the first patch. Just a reminder. :)

@@ +7,4 @@
>  [scriptable, uuid(18725604-e9ac-488a-8aa0-2471e7f6c0a4)]
>  interface nsINetworkStatsService : nsISupports
>  {
> +    void sendAppStats(in unsigned long aAppId,

Please use 2-space indentation. Also, I'd prefer:

  s/sendAppStats/saveAppStats

since you're wanting to save some statistics. Also, please add some comments to describe how to use this function in the format of:

  /*
   * Summary for the purpose of this API.
   * @param aAppId          Blah blah.
   * @param aIsInBrowser    Blah blah.
   * @param aConnectionType Blah blah.
   * ...
   */
  void saveAppStats(...);

::: dom/network/src/NetworkStatsService.js
@@ +21,5 @@
>  const TOPIC_INTERFACE_REGISTERED = "network-interface-registered";
>  const NETWORK_TYPE_WIFI = Ci.nsINetworkInterface.NETWORK_TYPE_WIFI;
>  const NETWORK_TYPE_MOBILE = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE;
>  
> +// Define maximum traffic amount can be save in appstatsQuene.

Please rewrite this as below:

// The maximum traffic amount that can be saved in the |appStatsQueue|.

@@ +22,5 @@
>  const NETWORK_TYPE_WIFI = Ci.nsINetworkInterface.NETWORK_TYPE_WIFI;
>  const NETWORK_TYPE_MOBILE = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE;
>  
> +// Define maximum traffic amount can be save in appstatsQuene.
> +const MAX_TRAFFIC = 1000 * 500;

s/MAX_TRAFFIC/MAX_TRAFFIC_AMOUNT_IN_QUEUE/

@@ +78,5 @@
>  
>    this.updateQueue = [];
>    this.isQueueRunning = false;
> +
> +  // App stats are first stored in the quence

In general, we the comment should be in the format of:

//{one_space}{upper_case_letter}{...}{period}.

So, let's rewrite it as:

// App stats are firstly stored in the queue.

Note that s/quence/queue/

@@ +79,5 @@
>    this.updateQueue = [];
>    this.isQueueRunning = false;
> +
> +  // App stats are first stored in the quence
> +  this.appstatsQuene = [];

s/appstatsQuene/appStatsQueue/

@@ +80,5 @@
>    this.isQueueRunning = false;
> +
> +  // App stats are first stored in the quence
> +  this.appstatsQuene = [];
> +  this.lastDate;

s/lastDate/appStatsQueueDate

because this queue always contains one kind of Date instead of *last* Date.

@@ +97,5 @@
>      let msg = aMessage.json;
>  
>      switch (aMessage.name) {
>        case "NetworkStats:Get":
>          this.getStats(mm, msg);

Please s/getStats/getNetdStats/ since you've already had another .getAppStats(...).

@@ +147,5 @@
>          Services.obs.removeObserver(this, "profile-after-change");
>          this.timer.cancel();
>          this.timer = null;
>  
> +        // Update netd stats and app stats before shutdown

Since you're here, add a period for this sentence.

@@ +156,5 @@
>    },
>  
>    /*
>     * nsITimerCallback
> +   * Timer triggers the update of all stats (netd and appstats)

Add a period.

@@ +380,5 @@
>        }
>      });
>    },
>  
> +  convertDate: function convertDate(aDate) {

You don't need this function. Please see below.

@@ +404,5 @@
> +                  date:            new Date(aTimeStamp/1000),
> +                  rxBytes:         aRxBytes,
> +                  txBytes:         aTxBytes};
> +
> +    // appstatsQuene is empty, keeps the first data.

s/appstatsQuene/|appstatsQuene|/ for variable names in the comment.

@@ +414,5 @@
> +
> +    // appstatsQuene only keeps the stats data with the same date
> +    // If found incoming data is different from lastDate,
> +    // both appstatsQuene and lastDate will update.
> +    let diff = (this.convertDate(stats.date) - this.convertDate(this.lastDate)) / this._db.sampleRate;

s/this.convertDate/this._db.normalizeDate(...)/

@@ +425,5 @@
> +
> +    // Try to find matched row in quene (by appId and connectionType)
> +    // If found, accumulate the traffic amount.
> +    for (let i in this.appstatsQuene) {
> +      if (this.appstatsQuene[i].appId == stats.appId) {

Please use:

  if (this.appstatsQuene[i].appId != stats.appId) {
    continue;
  }

to simplify the codes.

@@ +426,5 @@
> +    // Try to find matched row in quene (by appId and connectionType)
> +    // If found, accumulate the traffic amount.
> +    for (let i in this.appstatsQuene) {
> +      if (this.appstatsQuene[i].appId == stats.appId) {
> +        if (this.appstatsQuene[i].connectionType == stats.connectionType) {

The same. 

  if (this.appstatsQuene[i].connectionType != stats.connectionType) {
    continue;
  }

@@ +431,5 @@
> +          this.appstatsQuene[i].rxBytes += stats.rxBytes;
> +          this.appstatsQuene[i].txBytes += stats.txBytes;
> +          // If rxBytes or txBytes stored in quene is higher than MAX_TRAFFIC
> +          // the corresponding column will update.
> +          if (this.appstatsQuene[i].rxBytes > MAX_TRAFFIC || this.appstatsQuene[i].txBytes > MAX_TRAFFIC) {

Too long. Please fold it.

@@ +432,5 @@
> +          this.appstatsQuene[i].txBytes += stats.txBytes;
> +          // If rxBytes or txBytes stored in quene is higher than MAX_TRAFFIC
> +          // the corresponding column will update.
> +          if (this.appstatsQuene[i].rxBytes > MAX_TRAFFIC || this.appstatsQuene[i].txBytes > MAX_TRAFFIC) {
> +            this._db.saveAppStats(this.appstatsQuene.splice(i,1), function (aResult) {

I'd write by the following indentation:

this._db.saveAppStats(this.appstatsQuene.splice(i, 1),
  function (aResult) {
    ...
  },
  function (aErrorMsg) {
    ...
  }
);

Also, add a space between i and 1.

@@ +451,5 @@
> +    }
> +
> +    // No matched row found, push the whole incoming stats into quene.
> +    this.appstatsQuene.push(stats);
> +    return;

No need to explicitly call return in JS.

@@ +454,5 @@
> +    this.appstatsQuene.push(stats);
> +    return;
> +  },
> +
> +  updateAppStats: function updateAppStats() {

Can you move this logic into .updateAllStats(...) instead of creating a new one?

@@ +490,5 @@
> +   */
> +  getAppStats: function getAppStats(mm, msg) {
> +    this.updateAllStats(function onStatsUpdated(aResult, aMessage) {
> +
> +      // update appstatsQuence before get AppstatData

s/update/Update and add a period.

@@ +491,5 @@
> +  getAppStats: function getAppStats(mm, msg) {
> +    this.updateAllStats(function onStatsUpdated(aResult, aMessage) {
> +
> +      // update appstatsQuence before get AppstatData
> +      this.updateAppStats();

If you move .updateAppStats() into updateAllStats() then you don't need this.

@@ +493,5 @@
> +
> +      // update appstatsQuence before get AppstatData
> +      this.updateAppStats();
> +
> +      // get appid from appURL

Ditto.

@@ +501,5 @@
> +                      start:           msg.data.start,
> +                      end:             msg.data.end};
> +      if (DEBUG) {
> +        debug("get appId: " + options.appId + " from appURL: " + msg.data.appURL);
> +        debug("getstats for appId: " + options.appId + " connectionType:" + options.connectionType + " start:" + options.start + " end:" + options.end);

This debug message is too long. Please fold that.

@@ +505,5 @@
> +        debug("getstats for appId: " + options.appId + " connectionType:" + options.connectionType + " start:" + options.start + " end:" + options.end);
> +      }
> +
> +      if (!options.connectionType || options.connectionType.length == 0) {
> +        this._db.findAppAllStats(function onStatsFound(error, result) {

Please see another patch that I'm hoping to extend the original .findAll(...) instead of creating a new one.

@@ +514,5 @@
> +      }
> +
> +      for (let i in this._connectionTypes) {
> +        if (this._connectionTypes[i] == options.connectionType) {
> +          this._db.findAppStats(function onStatsFound(error, result) {

Please extend and use the original .find(...).
Attachment #762569 - Flags: review?(gene.lian) → review-
revise
Attachment #753723 - Attachment is obsolete: true
Attachment #771214 - Flags: review+
patch update.
Attachment #762569 - Attachment is obsolete: true
Attachment #771216 - Flags: review?(gene.lian)
As suggested, IDL & DB modification are split into two patches.
This one is only for IDL modification. (Note that, I removed the modification in NetworkStatsManager.js to make code more simple)

Another question, who can help non superreview on this patch?
Attachment #768850 - Attachment is obsolete: true
Attachment #771217 - Flags: review?(gene.lian)
Attachment #771216 - Attachment is obsolete: true
Attachment #771216 - Flags: review?(gene.lian)
Attachment #771219 - Flags: review?(gene.lian)
the wip patch for NetowrkStatsDB.jsm modification.
Attachment #771217 - Flags: review?(gene.lian) → review?(jonas)
Comment on attachment 771217 [details] [diff] [review]
Bug 784575 - Part 2: NetworkStatsManager IDL Modification.

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

Need Jonas' superreview.

::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
@@ +13,5 @@
>     * 'mobile', 'wifi' or null.
>     * If null, stats for both mobile and wifi are returned.
> +   *
> +   * Manifest URL used to retrieve network stats per app.
> +   * If null, system stats (regardless of the app) are return.

s/return/returned/
Attachment #771217 - Flags: superreview?(jonas)
Attachment #771217 - Flags: review?(jonas)
Attachment #771217 - Flags: review+
Comment on attachment 771219 [details] [diff] [review]
Bug 784575 - Part 1: Integrate NetworkTrafficService into NetworkStatsService.

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

Overall, you wrongly spell the queue as quene. Anyway, we don't use it anymore. Please see below for why.

::: dom/network/interfaces/nsINetworkStatsService.idl
@@ +7,5 @@
>  [scriptable, uuid(18725604-e9ac-488a-8aa0-2471e7f6c0a4)]
>  interface nsINetworkStatsService : nsISupports
>  {
> +  /*
> +   * An interface used to send per-app traffic data.

s/send/record/

@@ +15,5 @@
> +   * @param aTimeStamp       time stamp
> +   * @param aRxBytes         received data amount
> +   * @param aTxBytes         transmitted data amount
> +   */
> +  void sendAppStats(in unsigned long aAppId,

s/sendAppStats/saveAppStats/

::: dom/network/src/NetworkStatsService.js
@@ +22,5 @@
>  const TOPIC_INTERFACE_UNREGISTERED = "network-interface-unregistered";
>  const NET_TYPE_WIFI = Ci.nsINetworkInterface.NETWORK_TYPE_WIFI;
>  const NET_TYPE_MOBILE = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE;
>  
> +// The maximum traffic amount can be saved in the |appStatsQuene|.

s/appStatsQueue/cachedAppStats/

@@ +23,5 @@
>  const NET_TYPE_WIFI = Ci.nsINetworkInterface.NETWORK_TYPE_WIFI;
>  const NET_TYPE_MOBILE = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE;
>  
> +// The maximum traffic amount can be saved in the |appStatsQuene|.
> +const MAX_TRAFFIC = 1000 * 1000 * 500;

s/MAX_TRAFFIC/MAX_CACHED_TRAFFIC/
s/1000 * 1000 * 500/500 * 1000 * 1000

and add "// 500 MB" at the end.

@@ +85,5 @@
>    this.isQueueRunning = false;
> +
> +  // App stats are firstly stored in the quene.
> +  this.appStatsQuene = [];
> +  this.appStatsQueneDate;

The term "queue" doesn't sound appropriate to me. Queue means you want to put something by first-in-first-out. Let's do:

s/appStatsQuene/cachedAppStats/
s/appStatsQueneDate/cachedAppStatsDate/

@@ +188,5 @@
> +      }
> +      let options = { appId:           aAppId,
> +                      connectionType:  msg.data.connectionType,
> +                      start:           msg.data.start,
> +                      end:             msg.data.end};

Let's do:

let data = msg.data;

let options = { appId:          0,
		connectionType: data.connectionType,
		start:          data.start,
		end:            data.end };

let manifestURL = data.manifestURL;
if (manifestURL && appsService) {
  let appId = appsService.getAppLocalIdByManifestURL(manifestURL);
  if (DEBUG) {
    debug("get appId: " + appId + " from manifestURL: " + manifestURL);
  }

  if (appId) {
    options.appId = appId;
  }
}

@@ +193,2 @@
>        if (DEBUG) {
> +        debug("get appId: " + options.appId + " from manifestURL: " + msg.data.manifestURL);

You don't need the above debug message here since we've done that.

@@ +193,3 @@
>        if (DEBUG) {
> +        debug("get appId: " + options.appId + " from manifestURL: " + msg.data.manifestURL);
> +        debug("getstats for appId: " + options.appId + " connectionType:" + 

Trailing white space.

@@ +193,4 @@
>        if (DEBUG) {
> +        debug("get appId: " + options.appId + " from manifestURL: " + msg.data.manifestURL);
> +        debug("getstats for appId: " + options.appId + " connectionType:" + 
> +              options.connectionType + " start:" + options.start + " end:" + options.end);

Please add white spaces after " connectionType: ", " start: " and " end: ". Also, let's rewrite it to be:

debug("getStats for options: " + JSON.stringify(options));

@@ +228,5 @@
>    },
>  
>    updateAllStats: function updateAllStats(callback) {
> +    // Update |appStatsQuene|.
> +    this.updateAppStats();

s/appStatsQueue/cachedAppStats/
s/updateAppStats/updateCachedAppStats/

@@ +404,5 @@
>    },
>  
> +  /*
> +   * Funcation called in the protocol layer (HTTP, FTP, WebSocket ...etc)
> +   * to collect the per app stats.

s/Funcation/Function/
s/per app/per-app/

@@ +406,5 @@
> +  /*
> +   * Funcation called in the protocol layer (HTTP, FTP, WebSocket ...etc)
> +   * to collect the per app stats.
> +   */
> +  sendAppStats: function saveAppNetworkStats(aAppId, aIsInBrowser, aConnectionType, aTimeStamp, aRxBytes, aTxBytes) {

s/sendAppStats/saveAppStats/
s/saveAppNetworkStats/saveAppStats/

@@ +408,5 @@
> +   * to collect the per app stats.
> +   */
> +  sendAppStats: function saveAppNetworkStats(aAppId, aIsInBrowser, aConnectionType, aTimeStamp, aRxBytes, aTxBytes) {
> +    if (DEBUG) {
> +       debug("[NetworkStatsService] sendAppStats: " + aAppId + " " + aIsInBrowser + " " +

s/sendAppStats/saveAppStats/

Also, you don't need [NetworkStatsService]. The debug() already had that.

@@ +422,5 @@
> +                  isInBrowser:     aIsInBrowser,
> +                  connectionType:  this._connectionTypes[aConnectionType].name,
> +                  date:            new Date(aTimeStamp/1000),
> +                  rxBytes:         aRxBytes,
> +                  txBytes:         aTxBytes};

Add one space before "};"

@@ +431,5 @@
> +      this.appStatsQuene.push(stats);
> +      return;
> +    }
> +
> +    // |appStatsQuene| only keeps the stats data with the same date

s/date/date./

@@ +432,5 @@
> +      return;
> +    }
> +
> +    // |appStatsQuene| only keeps the stats data with the same date
> +    // If found incoming date is different from |appStatsQueneDate|,

s/found/the/

@@ +433,5 @@
> +    }
> +
> +    // |appStatsQuene| only keeps the stats data with the same date
> +    // If found incoming date is different from |appStatsQueneDate|,
> +    // both |appStatsQuene| and |appStatsQueneDate| will update.

s/will update/will get updated/

@@ +434,5 @@
> +
> +    // |appStatsQuene| only keeps the stats data with the same date
> +    // If found incoming date is different from |appStatsQueneDate|,
> +    // both |appStatsQuene| and |appStatsQueneDate| will update.
> +    let diff = (this._db.normalizeDate(stats.date) - this._db.normalizeDate(this.appStatsQueneDate)) / this._db.sampleRate;

Fold this line a bit:

let diff = (this._db.normalizeDate(stats.date) - this._db.normalizeDate(this.cachedAppStatsDate)) /
           this._db.sampleRate;

@@ +443,5 @@
> +      return;
> +    }
> +
> +    // Try to find matched row in quene (by appId and connectionType)
> +    // If found, accumulate the traffic amount.

Please rewrite the comment as below:

// Try to find the matched row in the cached data by appId and connectionType.
// If found, accumulate the traffic amount.

@@ +444,5 @@
> +    }
> +
> +    // Try to find matched row in quene (by appId and connectionType)
> +    // If found, accumulate the traffic amount.
> +    for (let i in this.appStatsQuene) {

Please use:

for (let i = 0; i < cachedAppStats.length; i++) {
  let appStat = cachedAppStats[i];
  ...
}

@@ +451,5 @@
> +      }
> +
> +      if (this.appStatsQuene[i].connectionType != stats.connectionType) {
> +        continue;
> +      }

Let's combine the above 2 conditions:

if (appStats.appId != stats.appId ||
    appStats.connectionType != stats.connectionType) {
  continue;
}

@@ +454,5 @@
> +        continue;
> +      }
> +
> +      this.appStatsQuene[i].rxBytes += stats.rxBytes;
> +      this.appStatsQuene[i].txBytes += stats.txBytes;

s/appStatsQuene[i]/appStats/ for these and below.

@@ +456,5 @@
> +
> +      this.appStatsQuene[i].rxBytes += stats.rxBytes;
> +      this.appStatsQuene[i].txBytes += stats.txBytes;
> +      // If rxBytes or txBytes stored in quene is higher than MAX_TRAFFIC
> +      // the corresponding column will update.

// If rxBytes or txBytes stored in the cached data is higher than MAX_CACHED_TRAFFIC
// the corresponding stats will get updated.

@@ +457,5 @@
> +      this.appStatsQuene[i].rxBytes += stats.rxBytes;
> +      this.appStatsQuene[i].txBytes += stats.txBytes;
> +      // If rxBytes or txBytes stored in quene is higher than MAX_TRAFFIC
> +      // the corresponding column will update.
> +      if (this.appStatsQuene[i].rxBytes > MAX_TRAFFIC || this.appStatsQuene[i].txBytes > MAX_TRAFFIC) {

if (appStats.rxBytes > MAX_CACHED_TRAFFIC || appStats.txBytes > MAX_CACHED_TRAFFIC) {

@@ +459,5 @@
> +      // If rxBytes or txBytes stored in quene is higher than MAX_TRAFFIC
> +      // the corresponding column will update.
> +      if (this.appStatsQuene[i].rxBytes > MAX_TRAFFIC || this.appStatsQuene[i].txBytes > MAX_TRAFFIC) {
> +        this._db.saveStats(this.appStatsQuene.splice(i, 1),
> +          function (aResult) {

Should be:

function (aError, aResult) {

@@ +461,5 @@
> +      if (this.appStatsQuene[i].rxBytes > MAX_TRAFFIC || this.appStatsQuene[i].txBytes > MAX_TRAFFIC) {
> +        this._db.saveStats(this.appStatsQuene.splice(i, 1),
> +          function (aResult) {
> +            if (DEBUG) {
> +              debug("[NetworkStatsService] Application stats inserted in indexedDB");

You don't need [NetworkStatsService]. The debug(...) already had that.

@@ +464,5 @@
> +            if (DEBUG) {
> +              debug("[NetworkStatsService] Application stats inserted in indexedDB");
> +            }
> +          },
> +          function (aErrorMsg) {

_db.saveStats doesn't eat the third argument!

@@ +474,5 @@
> +      }
> +      return;
> +    }
> +
> +    // No matched row found, push the whole incoming stats into quene.

// No record found, push the whole incoming stats into the cached data.

@@ +478,5 @@
> +    // No matched row found, push the whole incoming stats into quene.
> +    this.appStatsQuene.push(stats);
> +  },
> +
> +  updateAppStats: function updateAppStats() {

s/updateAppStats/updateCachedAppStats

@@ +480,5 @@
> +  },
> +
> +  updateAppStats: function updateAppStats() {
> +    if (DEBUG) {
> +       debug("[NetworkStatsService] updateAppStats: " + this.appStatsQueneDate);

Don't need [NetworkStatsService].

@@ +484,5 @@
> +       debug("[NetworkStatsService] updateAppStats: " + this.appStatsQueneDate);
> +    }
> +
> +    if (this.appStatsQuene.length == 0) {
> +      // appstatsQuence is empty, no need to update.

s/appstatsQuence/cachedAppStats/

@@ +488,5 @@
> +      // appstatsQuence is empty, no need to update.
> +      return;
> +    }
> +
> +    for (let i in this.appStatsQuene) {

Please use

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

@@ +490,5 @@
> +    }
> +
> +    for (let i in this.appStatsQuene) {
> +      this._db.saveStats(this.appStatsQuene.shift(),
> +        function (aResult) {

Should be:

function (aError, aResult) {

@@ +495,5 @@
> +          if (DEBUG) {
> +            debug("[NetworkStatsService] Application stats inserted in indexedDB");
> +          }
> +        },
> +        function (aErrorMsg) {

this._db.saveStats don't eat the third arg.

@@ +497,5 @@
> +          }
> +        },
> +        function (aErrorMsg) {
> +          if (DEBUG) {
> +            debug("[NetworkStatsService] Error saving application states" + aErrorMsg);

Don't need "[NetworkStatsService]".
Attachment #771219 - Flags: review?(gene.lian)
patch update
Attachment #771220 - Attachment is obsolete: true
Attachment #773875 - Flags: review?(gene.lian)
Comment on attachment 773875 [details] [diff] [review]
Bug 784575 - Part 3: NetworkStatsDB.jsm Modification.

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

Overall, this is good but I'm hoping to have a quick discussion with you to clarify me before giving review+. Thanks!

::: dom/network/src/NetworkStatsDB.jsm
@@ +19,1 @@
>  const STORE_NAME = "net_stats";

And add a comment like:

// Deprecated. Use "net_stats_v2" instead.

@@ +19,2 @@
>  const STORE_NAME = "net_stats";
> +const STORE_NAME_v2 = "net_stats_v2";

s/STORE_NAME_v2/STORE_NAME_V2/

@@ +93,5 @@
> +      } else if (currVersion == 1) {
> +        // In order to support per-app traffic data storage, the original
> +        // objectStore needs to be replaced by a new objectStore with new
> +        // key path ("appId") and new indexes ("appId", "isInBrowser").
> +        let objectStore_new;

s/objectStore_new/newObjectStore and below.

@@ +107,5 @@
> +        if (DEBUG) {
> +          debug("Created new object stores and indexes");
> +        }
> +
> +        // Copy the data from original objectStore to the new objectStore.

s/original/the original/

@@ +117,5 @@
> +            db.deleteObjectStore(STORE_NAME);
> +            return;
> +          }
> +
> +          let stats_old = {appId:          0,

s/stats_old/newStats

since it's not really "old".

@@ +124,5 @@
> +                           timestamp:      cursor.value.timestamp,
> +                           rxBytes:        cursor.value.rxBytes,
> +                           txBytes:        cursor.value.txBytes,
> +                           rxTotalBytes:   cursor.value.rxTotalBytes,
> +                           txTotalBytes:   cursor.value.txTotalBytes};

Also please refactor the common part for |cursor.value|:

let oldStats = cursor.value;

@@ +208,5 @@
>      }
>  
>      let rxDiff = newSample.rxTotalBytes - lastSample.rxTotalBytes;
>      let txDiff = newSample.txTotalBytes - lastSample.txTotalBytes;
> +    if (rxDiff < 0 || txDiff < 0 || newSample.appId != 0) {

I don't understand this change. Could you please come to me to have a quick discussion? Thanks!

@@ +219,5 @@
>      if (diff == 1) {
>        // New element.
> +      if (newSample.appId != 0) {
> +        newSample.rxTotalBytes = newSample.rxBytes + lastSample.rxTotalBytes;
> +        newSample.txTotalBytes = newSample.txBytes + lastSample.txTotalBytes;

Please add a comment:

// To explain why do you need to do this for app specific.
if (newSample.appId != 0) {

@@ +264,5 @@
> +      lastSample.txTotalBytes += newSample.txBytes;
> +      if (newSample.appId == 0) {
> +        lastSample.rxTotalBytes = newSample.rxTotalBytes;
> +        lastSample.txTotalBytes = newSample.txTotalBytes;
> +      }

Please do:

// Add some comments here to explain why we need to do this.
if (newSample.appId == 0) {
  lastSample.rxTotalBytes = newSample.rxTotalBytes;
  lastSample.txTotalBytes = newSample.txTotalBytes;
} else {
  lastSample.rxTotalBytes += newSample.rxBytes;
  lastSample.txTotalBytes += newSample.txBytes;
}

@@ +378,5 @@
>        }
>  
>        let request = store.index("timestamp").openCursor(range).onsuccess = function(event) {
>          var cursor = event.target.result;
>          if (cursor) {

Please use early return.

if (cursor.value.appId != aOptions.appId) {
  cursor.continue();
  return;
}
Attachment #773875 - Flags: review?(gene.lian)
patch update.
Attachment #771219 - Attachment is obsolete: true
Attachment #776132 - Attachment is obsolete: true
Attachment #776133 - Attachment is obsolete: true
Attachment #776135 - Flags: review?(gene.lian)
patch update.
Attachment #773875 - Attachment is obsolete: true
Attachment #776271 - Attachment is obsolete: true
Attachment #776287 - Attachment is obsolete: true
Attachment #776310 - Flags: review?(gene.lian)
Attachment #776135 - Attachment is obsolete: true
Attachment #776135 - Flags: review?(gene.lian)
Attachment #776900 - Flags: review?(gene.lian)
Comment on attachment 776900 [details] [diff] [review]
Bug 784575 - Part 1: Integrate NetworkTrafficService into NetworkStatsService.

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

::: dom/network/src/NetworkStatsService.js
@@ +417,5 @@
> +   * to record the per-app stats.
> +   */
> +  saveAppStats: function saveAppStats(aAppId, aIsInBrowser, aConnectionType, aTimeStamp, aRxBytes, aTxBytes) {
> +    if (DEBUG) {
> +       debug("saveAppStats: " + aAppId + " " + aIsInBrowser + " " +

nit: 2-space alignment.

@@ +445,5 @@
> +    // If the incoming date is different from |cachedAppStatsDate|,
> +    // both |cachedAppStats| and |cachedAppStatsDate| will get updated.
> +    let diff = (this._db.normalizeDate(stats.date) -
> +      this._db.normalizeDate(this.cachedAppStatsDate)) /
> +      this._db.sampleRate;

nit:

let diff = (this._db.normalizeDate(stats.date) -
            this._db.normalizeDate(this.cachedAppStatsDate)) /
           this._db.sampleRate;

@@ +446,5 @@
> +    // both |cachedAppStats| and |cachedAppStatsDate| will get updated.
> +    let diff = (this._db.normalizeDate(stats.date) -
> +      this._db.normalizeDate(this.cachedAppStatsDate)) /
> +      this._db.sampleRate;
> +    if (diff != 0) {

Please change the order to be:

let diff = ...;
if (diff != 0) {
  this.updateCachedAppStats();
}

// |cachedAppStats| is empty, keeps the first data.
if (this.cachedAppStats.length == 0) {
  this.cachedAppStatsDate = stats.date;
  this.cachedAppStats.push(stats);
  return;
}

@@ +467,5 @@
> +      appStats.rxBytes += stats.rxBytes;
> +      appStats.txBytes += stats.txBytes;
> +      this.cachedAppStats[i] = appStats;
> +
> +      // If rxBytes or txBytes stored in quene exceeds MAX_CACHED_TRAFFIC

nit: s/quene/the cached/

@@ +470,5 @@
> +
> +      // If rxBytes or txBytes stored in quene exceeds MAX_CACHED_TRAFFIC
> +      // the corresponding column will get updated.
> +      if (appStats.rxBytes > MAX_CACHED_TRAFFIC || appStats.txBytes >
> +          MAX_CACHED_TRAFFIC) {

nit: 

if (appStats.rxBytes > MAX_CACHED_TRAFFIC ||
    appStats.txBytes > MAX_CACHED_TRAFFIC) {

@@ +482,5 @@
> +      }
> +      return;
> +    }
> +
> +    // No record found, push the whole incoming stats into quene.

nit: s/quene/the cached data/

@@ +488,5 @@
> +  },
> +
> +  updateCachedAppStats: function updateCachedAppStats() {
> +    if (DEBUG) {
> +       debug("updateCachedAppStats: " + this.cachedAppStatsDate);

nit: 2-space for alignment.

@@ +497,5 @@
> +      return;
> +    }
> +
> +    for (let i = 0; i < this.cachedAppStats.length; i++) {
> +      this._db.saveStats(this.cachedAppStats.shift(),

Wait... this looks incorrect. I don't think the loop can correctly run when the cachedAppStats has changed.

Please use:

while (this.cachedAppStats.length)
Attachment #776900 - Flags: review?(gene.lian) → review-
Comment on attachment 776310 [details] [diff] [review]
Bug 784575 - Part 3: NetworkStatsDB.jsm Modification.

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

Great! Review+! Could you please come up with the new patch to let Albert review? Thanks!

::: dom/network/src/NetworkStatsDB.jsm
@@ +151,5 @@
> +              timestamp:      timestamp,
> +              rxBytes:        stats.rxBytes,
> +              txBytes:        stats.txBytes,
> +              rxTotalBytes:   stats.rxBytes,
> +              txTotalBytes:   stats.txBytes };

Could you just do:

rxBytes:      (stats.appId == 0) ? 0 : stats.rxBytes,
txBytes:      (stats.appId == 0) ? 0 : stats.txBytes,
rxTotalBytes: (stats.appId == 0) ? stats.rxBytes : 0,
txTotalBytes: (stats.appId == 0) ? stats.txBytes : 0

@@ +211,5 @@
> +    // If the incoming data is obtained from netd (|newSample.appId| is 0),
> +    // the new |txBytes|/|rxBytes| is assigend by the differnce between the new
> +    // |txTotalBytes|/|rxTotalBytes| and the last |txTotalBytes|/|rxTotalBytes|.
> +    // Else, the incoming data is per-app data (|newSample.appId| is not 0),
> +    // the |txBytes|/|rxBytes| is directly the new |txBytes|/|rxBytes|.

// the |txBytes|/|rxBytes| is directly assigned.

@@ +311,4 @@
>      // Callback function to remove old items when new ones are added.
>      let filterDate = date - (SAMPLE_RATE * VALUES_MAX_LENGTH - 1);
> +    let lowFilter = [appId, connType, 0];
> +    let upFilter = [appId, connType, filterDate];

nit:

s/lowFilter/lowerFilter/
s/upFilter/upperFilter/

These names are more correct in my dictionary.

@@ +343,5 @@
>      }
>  
>      this.dbNewTxn("readonly", function(txn, store) {
> +      let lowFilter = [aOptions.appId, aOptions.connectionType, start];
> +      let upFilter = [aOptions.appId, aOptions.connectionType, end];

nit:

s/lowFilter/lowerFilter/
s/upFilter/upperFilter/
Attachment #776310 - Flags: review?(gene.lian) → review+
Attachment #776310 - Attachment is obsolete: true
Attachment #776964 - Flags: review?(acperez)
patch update.
Attachment #776900 - Attachment is obsolete: true
Attachment #776966 - Flags: review?(gene.lian)
Attachment #776966 - Flags: review?(gene.lian)
Attachment #776966 - Flags: review?(acperez)
Attachment #776966 - Flags: review+
John, you can push your patches to try server while they're still under review. I'm wondering you need to fix test cases since you've changed NetworkStatsService to an XPCOM service.

Also, that would be great if you could provide test cases for you patches.
https://tbpl.mozilla.org/?tree=Try&rev=6c4e3cc25a33

The current result in try server.
since there is some modification in Makefile.in & moz.build
update the patch.
Attachment #771214 - Attachment is obsolete: true
Attachment #781603 - Flags: review+
Comment on attachment 776966 [details] [diff] [review]
Bug 784575 - Part 1: Integrate NetworkTrafficService into NetworkStatsService.

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

Patch is ok, but there are two points that should be fixed.

::: dom/network/src/NetworkStatsService.js
@@ +436,5 @@
> +
> +    // |cachedAppStats| is empty, keeps the first data.
> +    if (this.cachedAppStats.length == 0) {
> +      this.cachedAppStatsDate = stats.date;
> +      this.cachedAppStats.push(stats);

Save cacheAppStatsDate normalized, thus you avoid problems comparing dates if timezone changes in the same sample period and also you will not have to normalize each time that saveAppStats is called.
 
this.cachedAppStatsDate = this._db.normalizeDAte(stats.date);

@@ +455,5 @@
> +    }
> +
> +    // Try to find the matched row in the cached data by appId and connectionType.
> +    // If found, accumulate the traffic amount.
> +    for (let i = 0; i < this.cachedAppStats.length; i++) {

I don't know how often this function gets called, but maybe can be optimized changing the cachedAppStats array for an object in order to remove the for loop and check if there is cached data in the following way:
if (this.cachedAppStats[stats.appId + stats.connectionType]) {
  ...
}

Is just a proposal, up to you..

@@ +486,5 @@
> +    // No record found, push the whole incoming stats into the cached.
> +    this.cachedAppStats.push(stats);
> +  },
> +
> +  updateCachedAppStats: function updateCachedAppStats() {

This function should be called before system shutdown, if not, cached stats will be lost. You can call it in observe function for the xpcom-shutdown event.
Attachment #776966 - Flags: review?(acperez) → review-
Comment on attachment 776964 [details] [diff] [review]
Bug 784575 - Part 3: NetworkStatsDB.jsm Modification.

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

::: dom/network/src/NetworkStatsDB.jsm
@@ +342,5 @@
>        debug("End time: " + new Date(end));
>      }
>  
>      this.dbNewTxn("readonly", function(txn, store) {
> +      let lowerFilter = [aOptions.appId, aOptions.connectionType, start];

I don't see where does aOptions.appId come from. You added 'DOMString manifestURL' to NetworkStatsOptions in nsIDOMNetworkStatsManager.idl, so aOptions.appId will be undefined. I think you need to convert aOptions.manifestURL to appId, am I wrong?

@@ +398,5 @@
>  
>        let request = store.index("timestamp").openCursor(range).onsuccess = function(event) {
>          var cursor = event.target.result;
>          if (cursor) {
> +          if (cursor.value.appId != aOptions.appId) {

Why not modify lowerFilter and upperFilter to filter by timestamp and appId?
Attachment #776964 - Flags: review?(acperez) → review-
(In reply to Albert [:albert] from comment #86)
> Comment on attachment 776964 [details] [diff] [review]
> Bug 784575 - Part 3: NetworkStatsDB.jsm Modification.
> 
> Review of attachment 776964 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/src/NetworkStatsDB.jsm
> @@ +342,5 @@
> >        debug("End time: " + new Date(end));
> >      }
> >  
> >      this.dbNewTxn("readonly", function(txn, store) {
> > +      let lowerFilter = [aOptions.appId, aOptions.connectionType, start];
> 
> I don't see where does aOptions.appId come from. You added 'DOMString
> manifestURL' to NetworkStatsOptions in nsIDOMNetworkStatsManager.idl, so
> aOptions.appId will be undefined. I think you need to convert
> aOptions.manifestURL to appId, am I wrong?
> 

Hi,
manifestURL is converted into appId by AppService in NetworkStatsService.js
('let appId = appsService.getAppLocalIdByManifestURL(manifestURL);' in getStats function)
therefore DB won't receive manifestURL information.


> @@ +398,5 @@
> >  
> >        let request = store.index("timestamp").openCursor(range).onsuccess = function(event) {
> >          var cursor = event.target.result;
> >          if (cursor) {
> > +          if (cursor.value.appId != aOptions.appId) {
> 
> Why not modify lowerFilter and upperFilter to filter by timestamp and appId?

ok, I'll refine this.
Attached patch bug_784575_part3.patch (obsolete) — Splinter Review
patch update.
Attachment #776964 - Attachment is obsolete: true
Attachment #783622 - Attachment is obsolete: true
(In reply to Albert [:albert] from comment #85)
> Comment on attachment 776966 [details] [diff] [review]
> Bug 784575 - Part 1: Integrate NetworkTrafficService into
> NetworkStatsService.
> 
> Review of attachment 776966 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch is ok, but there are two points that should be fixed.
> 
> ::: dom/network/src/NetworkStatsService.js
> @@ +436,5 @@
> > +
> > +    // |cachedAppStats| is empty, keeps the first data.
> > +    if (this.cachedAppStats.length == 0) {
> > +      this.cachedAppStatsDate = stats.date;
> > +      this.cachedAppStats.push(stats);
> 
> Save cacheAppStatsDate normalized, thus you avoid problems comparing dates
> if timezone changes in the same sample period and also you will not have to
> normalize each time that saveAppStats is called.
>  
> this.cachedAppStatsDate = this._db.normalizeDAte(stats.date);
> 

ok, I'll refine this.

> @@ +455,5 @@
> > +    }
> > +
> > +    // Try to find the matched row in the cached data by appId and connectionType.
> > +    // If found, accumulate the traffic amount.
> > +    for (let i = 0; i < this.cachedAppStats.length; i++) {
> 
> I don't know how often this function gets called, but maybe can be optimized
> changing the cachedAppStats array for an object in order to remove the for
> loop and check if there is cached data in the following way:
> if (this.cachedAppStats[stats.appId + stats.connectionType]) {
>   ...
> }
> 
> Is just a proposal, up to you..

seems like a good way but the problem is how to make sure the index is unique
for example,
    appId = 12, connectionType = 0
    appId = 11, connectionType = 1
then there is a conflict, 
what do you think about this?

> 
> @@ +486,5 @@
> > +    // No record found, push the whole incoming stats into the cached.
> > +    this.cachedAppStats.push(stats);
> > +  },
> > +
> > +  updateCachedAppStats: function updateCachedAppStats() {
> 
> This function should be called before system shutdown, if not, cached stats
> will be lost. You can call it in observe function for the xpcom-shutdown
> event.

I've added the 'this.updateCachedAppStats()' in the updateAllStats function,
so the cached stats will write into db when updateAllStats() is called.
patch update.
Attachment #776966 - Attachment is obsolete: true
Attachment #781603 - Attachment is obsolete: true
Attachment #783656 - Flags: review+
I change back to the original patch because I found that
I use     

let diff = (this._db.normalizeDate(stats.date) -
                this._db.normalizeDate(this.cachedAppStatsDate)) /
               this._db.sampleRate;

to compare the date change, therefore the timezone change problem
can be avoided.
Attachment #783655 - Attachment is obsolete: true
Attachment #783656 - Attachment is obsolete: true
Attachment #783693 - Flags: review+
Attachment #783624 - Flags: review?(acperez)
Attachment #783692 - Flags: review?(acperez)
Comment on attachment 783624 [details] [diff] [review]
Bug 784575 - Part 3: NetworkStatsDB.jsm Modification.

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

Nice patch
Attachment #783624 - Flags: review?(acperez) → review+
(In reply to John Shih from comment #93)
> Created attachment 783692 [details] [diff] [review]
> Bug 784575 - Part 1: Integrate NetworkTrafficService into
> NetworkStatsService.
> 
> I change back to the original patch because I found that
> I use     
> 
> let diff = (this._db.normalizeDate(stats.date) -
>                 this._db.normalizeDate(this.cachedAppStatsDate)) /
>                this._db.sampleRate;
> 
> to compare the date change, therefore the timezone change problem
> can be avoided.

I don't think so. For example, a user in Paris (UTC+1) pushes stats at 17:00. Then he travels to London (UTC+0) and a new stats record occurs at 17:00.

Actually you will calculate diff with UTC+0 for both dates, so:
diff = 17:00 - 17:00. (wrong)

Otherwise, if you save the cached date normalized, Paris date will be stored as 16:00, so:
diff = 17:00 - 16:00
Which is correct.

Moreover, you need to do something with 'cachedAppStats' because if a timezone change happens after stats record pushed to cacheAppStats and before you call updateCachedAppStats, at the moment to insert the record in the database, NetworkStatsDB will normalize with the wrong timezone offset. I think you need to push stats to cachedAppStats with normalized timezone and then 'unnormalize' in updateCachedAppStats to avoid that NetworkStatsDB normalize it again.

To make it easier you can add a field cachedDate to your cachedStats, then you won't need to 'unnormalize'.

So, in saveAppStats:

let stats = { appId:           aAppId,
              isInBrowser:     aIsInBrowser,
              connectionType:  this._connectionTypes[aConnectionType].name,
              date:            new Date(aTimeStamp/1000),
              rxBytes:         aRxBytes,
              txBytes:         aTxBytes,
              cachedDate:      this._db.normalizeDAte(new Date(aTimeStamp/1000)) };

// |cachedAppStats| is empty, keeps the first data.
if (this.cachedAppStats.length == 0) {
  this.cachedAppStatsDate = stats.cachedDate;
  this.cachedAppStats.push(stats);
  return;
}

// |cachedAppStats| only keeps the stats data with the same date.
// If the incoming date is different from |cachedAppStatsDate|,
// both |cachedAppStats| and |cachedAppStatsDate| will get updated.
let diff = (stats.cachedDate - this.cachedAppStatsDate) / this._db.sampleRate;


Another option is to modify the normalizeDate function in NetworkStatsDB changing:
let timestamp = aDate.getTime() - (new Date()).getTimezoneOffset() * 60 * 1000;
for:
let timestamp = aDate.getTime() - aDate.getTimezoneOffset() * 60 * 1000;

In this case, timezone is taken from your date object and your code will work fine without any changes.
(In reply to John Shih from comment #90)
> (In reply to Albert [:albert] from comment #85)
> > Comment on attachment 776966 [details] [diff] [review]
> > Bug 784575 - Part 1: Integrate NetworkTrafficService into
> > NetworkStatsService.
> > 
> > Review of attachment 776966 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Patch is ok, but there are two points that should be fixed.
> > 
> > @@ +455,5 @@
> > > +    }
> > > +
> > > +    // Try to find the matched row in the cached data by appId and connectionType.
> > > +    // If found, accumulate the traffic amount.
> > > +    for (let i = 0; i < this.cachedAppStats.length; i++) {
> > 
> > I don't know how often this function gets called, but maybe can be optimized
> > changing the cachedAppStats array for an object in order to remove the for
> > loop and check if there is cached data in the following way:
> > if (this.cachedAppStats[stats.appId + stats.connectionType]) {
> >   ...
> > }
> > 
> > Is just a proposal, up to you..
> 
> seems like a good way but the problem is how to make sure the index is unique
> for example,
>     appId = 12, connectionType = 0
>     appId = 11, connectionType = 1
> then there is a conflict, 
> what do you think about this?
> 

To make sure the index is unique you can combine appId and connectionType:
let index = appId + '' + connectionType

But once again, this approach is just a proposal.
Ping Jonas for the IDL review.
(In reply to Albert [:albert] from comment #96)
> (In reply to John Shih from comment #93)
> > Created attachment 783692 [details] [diff] [review]
> > Bug 784575 - Part 1: Integrate NetworkTrafficService into
> > NetworkStatsService.
> > 
> > I change back to the original patch because I found that
> > I use     
> > 
> > let diff = (this._db.normalizeDate(stats.date) -
> >                 this._db.normalizeDate(this.cachedAppStatsDate)) /
> >                this._db.sampleRate;
> > 
> > to compare the date change, therefore the timezone change problem
> > can be avoided.
> 
> I don't think so. For example, a user in Paris (UTC+1) pushes stats at
> 17:00. Then he travels to London (UTC+0) and a new stats record occurs at
> 17:00.
> 
> Actually you will calculate diff with UTC+0 for both dates, so:
> diff = 17:00 - 17:00. (wrong)
> 
> Otherwise, if you save the cached date normalized, Paris date will be stored
> as 16:00, so:
> diff = 17:00 - 16:00
> Which is correct.


doesn't 17:00 in Paris turn into 16:00 after normalizing?
e.g,
normalized(17:00 in London) - normalized(17:00 in Paris)
= 17:00 - 16:00

actually the way to compute diff is the same as the way in NetworkStatsDB.
Initially, I was planning that the timezone issue can be handled in the DB
that's why I keep the original date in cached.

> 
> Moreover, you need to do something with 'cachedAppStats' because if a
> timezone change happens after stats record pushed to cacheAppStats and
> before you call updateCachedAppStats, at the moment to insert the record in
> the database, NetworkStatsDB will normalize with the wrong timezone offset.
> I think you need to push stats to cachedAppStats with normalized timezone
> and then 'unnormalize' in updateCachedAppStats to avoid that NetworkStatsDB
> normalize it again.
> 
> To make it easier you can add a field cachedDate to your cachedStats, then
> you won't need to 'unnormalize'.
> 
> So, in saveAppStats:
> 
> let stats = { appId:           aAppId,
>               isInBrowser:     aIsInBrowser,
>               connectionType:  this._connectionTypes[aConnectionType].name,
>               date:            new Date(aTimeStamp/1000),
>               rxBytes:         aRxBytes,
>               txBytes:         aTxBytes,
>               cachedDate:      this._db.normalizeDAte(new
> Date(aTimeStamp/1000)) };
> 
> // |cachedAppStats| is empty, keeps the first data.
> if (this.cachedAppStats.length == 0) {
>   this.cachedAppStatsDate = stats.cachedDate;
>   this.cachedAppStats.push(stats);
>   return;
> }
> 
> // |cachedAppStats| only keeps the stats data with the same date.
> // If the incoming date is different from |cachedAppStatsDate|,
> // both |cachedAppStats| and |cachedAppStatsDate| will get updated.
> let diff = (stats.cachedDate - this.cachedAppStatsDate) /
> this._db.sampleRate;
> 
> 
> Another option is to modify the normalizeDate function in NetworkStatsDB
> changing:
> let timestamp = aDate.getTime() - (new Date()).getTimezoneOffset() * 60 *
> 1000;
> for:
> let timestamp = aDate.getTime() - aDate.getTimezoneOffset() * 60 * 1000;
> 
> In this case, timezone is taken from your date object and your code will
> work fine without any changes.
(In reply to Albert [:albert] from comment #97)
> (In reply to John Shih from comment #90)
> > (In reply to Albert [:albert] from comment #85)
> > > Comment on attachment 776966 [details] [diff] [review]
> > > Bug 784575 - Part 1: Integrate NetworkTrafficService into
> > > NetworkStatsService.
> > > 
> > > Review of attachment 776966 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Patch is ok, but there are two points that should be fixed.
> > > 
> > > @@ +455,5 @@
> > > > +    }
> > > > +
> > > > +    // Try to find the matched row in the cached data by appId and connectionType.
> > > > +    // If found, accumulate the traffic amount.
> > > > +    for (let i = 0; i < this.cachedAppStats.length; i++) {
> > > 
> > > I don't know how often this function gets called, but maybe can be optimized
> > > changing the cachedAppStats array for an object in order to remove the for
> > > loop and check if there is cached data in the following way:
> > > if (this.cachedAppStats[stats.appId + stats.connectionType]) {
> > >   ...
> > > }
> > > 
> > > Is just a proposal, up to you..
> > 
> > seems like a good way but the problem is how to make sure the index is unique
> > for example,
> >     appId = 12, connectionType = 0
> >     appId = 11, connectionType = 1
> > then there is a conflict, 
> > what do you think about this?
> > 
> 
> To make sure the index is unique you can combine appId and connectionType:
> let index = appId + '' + connectionType
> 
> But once again, this approach is just a proposal.

Thanks for the advice!
I'll try to refine that.
Attachment #771217 - Flags: superreview?(jonas) → superreview+
(In reply to John Shih from comment #99)
> (In reply to Albert [:albert] from comment #96)
> > (In reply to John Shih from comment #93)
> > > Created attachment 783692 [details] [diff] [review]
> > > Bug 784575 - Part 1: Integrate NetworkTrafficService into
> > > NetworkStatsService.
> > > 
> > > I change back to the original patch because I found that
> > > I use     
> > > 
> > > let diff = (this._db.normalizeDate(stats.date) -
> > >                 this._db.normalizeDate(this.cachedAppStatsDate)) /
> > >                this._db.sampleRate;
> > > 
> > > to compare the date change, therefore the timezone change problem
> > > can be avoided.
> > 
> > I don't think so. For example, a user in Paris (UTC+1) pushes stats at
> > 17:00. Then he travels to London (UTC+0) and a new stats record occurs at
> > 17:00.
> > 
> > Actually you will calculate diff with UTC+0 for both dates, so:
> > diff = 17:00 - 17:00. (wrong)
> > 
> > Otherwise, if you save the cached date normalized, Paris date will be stored
> > as 16:00, so:
> > diff = 17:00 - 16:00
> > Which is correct.
> 
> 
> doesn't 17:00 in Paris turn into 16:00 after normalizing?
> e.g,
> normalized(17:00 in London) - normalized(17:00 in Paris)
> = 17:00 - 16:00
> 
> actually the way to compute diff is the same as the way in NetworkStatsDB.
> Initially, I was planning that the timezone issue can be handled in the DB
> that's why I keep the original date in cached.

As I pointed before this is true if you modify the normalizeDate function, actually it takes the timezone offset from new Date() instead of the aDate parameter. So if you have a date with a paris timezone but mobile is in london timezone, new Date will create a "london date" and you will normalize the paris date with the london offset.

> > Another option is to modify the normalizeDate function in NetworkStatsDB
> > changing:
> > let timestamp = aDate.getTime() - (new Date()).getTimezoneOffset() * 60 *
> > 1000;
> > for:
> > let timestamp = aDate.getTime() - aDate.getTimezoneOffset() * 60 * 1000;
> > 
> > In this case, timezone is taken from your date object and your code will
> > work fine without any changes.
(In reply to Albert [:albert] from comment #101)
> (In reply to John Shih from comment #99)
> > (In reply to Albert [:albert] from comment #96)
> > > (In reply to John Shih from comment #93)
> > > > Created attachment 783692 [details] [diff] [review]
> > > > Bug 784575 - Part 1: Integrate NetworkTrafficService into
> > > > NetworkStatsService.
> > > > 
> > > > I change back to the original patch because I found that
> > > > I use     
> > > > 
> > > > let diff = (this._db.normalizeDate(stats.date) -
> > > >                 this._db.normalizeDate(this.cachedAppStatsDate)) /
> > > >                this._db.sampleRate;
> > > > 
> > > > to compare the date change, therefore the timezone change problem
> > > > can be avoided.
> > > 
> > > I don't think so. For example, a user in Paris (UTC+1) pushes stats at
> > > 17:00. Then he travels to London (UTC+0) and a new stats record occurs at
> > > 17:00.
> > > 
> > > Actually you will calculate diff with UTC+0 for both dates, so:
> > > diff = 17:00 - 17:00. (wrong)
> > > 
> > > Otherwise, if you save the cached date normalized, Paris date will be stored
> > > as 16:00, so:
> > > diff = 17:00 - 16:00
> > > Which is correct.
> > 
> > 
> > doesn't 17:00 in Paris turn into 16:00 after normalizing?
> > e.g,
> > normalized(17:00 in London) - normalized(17:00 in Paris)
> > = 17:00 - 16:00
> > 
> > actually the way to compute diff is the same as the way in NetworkStatsDB.
> > Initially, I was planning that the timezone issue can be handled in the DB
> > that's why I keep the original date in cached.
> 
> As I pointed before this is true if you modify the normalizeDate function,
> actually it takes the timezone offset from new Date() instead of the aDate
> parameter. So if you have a date with a paris timezone but mobile is in
> london timezone, new Date will create a "london date" and you will normalize
> the paris date with the london offset.

Thanks for clarifying!
I'll fix it.

> 
> > > Another option is to modify the normalizeDate function in NetworkStatsDB
> > > changing:
> > > let timestamp = aDate.getTime() - (new Date()).getTimezoneOffset() * 60 *
> > > 1000;
> > > for:
> > > let timestamp = aDate.getTime() - aDate.getTimezoneOffset() * 60 * 1000;
> > > 
> > > In this case, timezone is taken from your date object and your code will
> > > work fine without any changes.
Modify normalizeDate function.
the time zone offset is obtained from aDate instead of new Date().
Attachment #783624 - Attachment is obsolete: true
Attachment #784855 - Flags: review?(acperez)
Patch update.
include:
|cachedAppStats| use Object to substitute array.
some logic update in saveAppStats function.
Attachment #783692 - Attachment is obsolete: true
Attachment #783692 - Flags: review?(acperez)
Hi Gene, 
the refined patch needs your review first.
Attachment #784857 - Attachment is obsolete: true
Attachment #784858 - Flags: review?(gene.lian)
Comment on attachment 784855 [details] [diff] [review]
Bug 784575 - Part 3: NetworkStatsDB.jsm Modification.

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

Nice
Attachment #784855 - Flags: review?(acperez) → review+
Comment on attachment 784858 [details] [diff] [review]
Bug 784575 - Part 1: Integrate NetworkTrafficService into NetworkStatsService.

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

::: dom/network/src/NetworkStatsService.js
@@ +434,5 @@
> +                  rxBytes:         aRxBytes,
> +                  txBytes:         aTxBytes };
> +
> +    // Generate an unique key from |appId| and |connectionType|.
> +    // The key is use to retrieve data in |cachedAppStats|.

// Generate an unique key from |appId| and |connectionType|,
// which is used to retrieve data in |cachedAppStats|.

@@ +480,5 @@
> +    }
> +
> +    // If doesn't exceed MAX_CACHED_TRAFFIC,
> +    // save updated data back to the cached.
> +    this.cachedAppStats[key] = appStats;

You don't need this line. The object in the cachedAppStats[key] should already get updated when you call:

appStats.rxBytes += stats.rxBytes;
appStats.txBytes += stats.txBytes;

@@ +502,5 @@
> +          }
> +        }
> +      );
> +      delete this.cachedAppStats[i];
> +    }

If sounds weird to me the for loop is working after removing some elements. Please do:

for (let i in this.cachedAppStats) {
  ...
}

this.cachedAppStats = Object.create(null);
Attachment #784858 - Flags: review?(gene.lian)
Blocks: 855951
No longer depends on: 855951
patch update.
Attachment #784858 - Attachment is obsolete: true
Attachment #786674 - Flags: review?(gene.lian)
Attachment #786674 - Attachment is obsolete: true
Attachment #786674 - Flags: review?(gene.lian)
Attachment #786696 - Flags: review?(gene.lian)
Comment on attachment 786696 [details] [diff] [review]
Bug 784575 - Part 1: Integrate NetworkTrafficService into NetworkStatsService.

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

Looks nice! We need Albert's second review.

::: dom/network/src/NetworkStatsService.js
@@ +475,5 @@
> +          }
> +        }
> +      );
> +      delete this.cachedAppStats[key];
> +      return;

nit: you don't need this return.

@@ +499,5 @@
> +        }
> +      );
> +    }
> +
> +    // Initialize the |cachedAppStats| after updating.

nit: s/Initialize/Clean up/
Attachment #786696 - Flags: review?(gene.lian)
Attachment #786696 - Flags: review?(acperez)
Attachment #786696 - Flags: review+
patch revise.
Attachment #786696 - Attachment is obsolete: true
Attachment #786696 - Flags: review?(acperez)
Attachment #786767 - Flags: review?(acperez)
Comment on attachment 786767 [details] [diff] [review]
Bug 784575 - Part 1: Integrate NetworkTrafficService into NetworkStatsService.

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

Good patch!
Attachment #786767 - Flags: review?(acperez) → review+
Hi Albert,
as I writing test cases, I found that in function findAll()

 let lowerFilter = [aOptions.appId, start];
 let upperFilter = [aOptions.appId, end];
 let range = this.dbGlobal.IDBKeyRange.bound(lowerFilter, upperFilter, false, false);

doesn't work, therefore I move back to the original way.
(filter by timestamp and then check the appId with dtor)

The way of filter by two of three key-path seems have some difficulties.

Also, there is some solutions
For example, use 

 let lowerFilter = [aOptions.appId, connectionType_Min, start];
 let upperFilter = [aOptions.appId, connectionType_Max, end];

However, such way needs the connectionType to be integer and the upper bound of connectionType be known (which I don't think is a flexible way).

What do you think?
Attachment #784855 - Attachment is obsolete: true
Attachment #787417 - Flags: review?(acperez)
Blocks: 854206
No longer blocks: 854206
Depends on: 854206
Comment on attachment 787417 [details] [diff] [review]
Bug 784575 - Part 3: NetworkStatsDB.jsm Modification.

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

Maybe, in order to be able to filter by appId and timestamp you need to create a new index in the database, something like:
newObjectStore.createIndex("searchIndex", ["appId", "timestamp"], { unique: false });
I did something similar in other bug, but I don't remember well.

Anyway it means to add redundant data to the database, which is not good. So, for me is Ok your solution.
Attachment #787417 - Flags: review?(acperez) → review+
Thanks!

By the way, I notice that the test cases, including

test_networkstats_basics.html
test_networkstats_db.js
test_networkstats_service.js

were not executed (or I didn't find) in the try-server (e.g, https://tbpl.mozilla.org/?tree=Try&rev=0e108bffd094)

can you check it?
Flags: needinfo?(acperez)
Are you sure? I found bug 849642. Which seems to be a problem running tests.
Flags: needinfo?(acperez)
Blocks: 854206
No longer depends on: 854206
log: https://tbpl.mozilla.org/php/getParsedLog.php?id=26511955&tree=Mozilla-Central&full=1

only 

test_networkstats_disabled.html
test_networkstats_enabled_no_perm.html
test_networkstats_enabled_perm.html

being executed

this is also the only log that I can find the result with networkstats as keyword
Hi gene & albert, 

there are several changes in order to support the more complete testing on this bug.
I'll list the changes in the following.

a. Move back NetworkStatsService .jsm to .js and create NetworkStatsServiceProxy.js
While I'm working on writing test cases, I found that the change of .jsm to .js will prevent the NetworkStatsService from being tested by xpcshell. (only .jsm file can be imported in a xpcshell testing and test detail of each functions. In other hand, when using .js, only the interface that is defined in the .idl can be tested)

After discussing with Gene, we decide to split the new NetworkStatsService into two parts:
  1. NetworkStatsService.jsm
  I remain the .jsm type so that won't impact the original test case (test_networkstat_service.js), and also put the logic of maintain |cachedAppStats| into this file.

  2. NetworkStatsServiceProxy.js
  I create a new file as to expose an interface so that protocol layer can save the collected per-app network stats into database through it.
  Besides, I also add a optional callback in the interface so that the asynchronous behaviour can be tested. 

b. Return manifestURL
In order to check the result of get per-app stats, I return the manifestURL back to the NetworkStatsManager after calling getStats function.

c. Add test_networkstats_service_proxy.js
I add the test especially for the exposed interface to test the main logic of it.
Attachment #783693 - Attachment is obsolete: true
Attachment #796539 - Flags: review?(gene.lian)
Attachment #796539 - Flags: review?(acperez)
Attachment #786767 - Attachment is obsolete: true
Attachment #796542 - Flags: review?(gene.lian)
Attachment #796542 - Flags: review?(acperez)
Attachment #771217 - Attachment is obsolete: true
Attachment #796543 - Flags: review+
Attachment #787417 - Attachment is obsolete: true
Attachment #796545 - Flags: review?(gene.lian)
Attachment #796545 - Flags: review?(acperez)
Attachment #796548 - Flags: review?(gene.lian)
Attachment #796548 - Flags: review?(acperez)
Attached patch Bug 784575 - Part 6: Testing. (obsolete) — Splinter Review
Attachment #796551 - Flags: review?(gene.lian)
Attachment #796551 - Flags: review?(acperez)
Attached patch Bug 784575 - Part 6: Testing. (obsolete) — Splinter Review
Attachment #796551 - Attachment is obsolete: true
Attachment #796551 - Flags: review?(gene.lian)
Attachment #796551 - Flags: review?(acperez)
Attachment #796553 - Flags: review?(gene.lian)
Attachment #796553 - Flags: review?(acperez)
Attachment #796542 - Attachment is obsolete: true
Attachment #796542 - Flags: review?(gene.lian)
Attachment #796542 - Flags: review?(acperez)
Attachment #797098 - Flags: review?(gene.lian)
Attachment #797098 - Flags: review?(acperez)
Comment on attachment 796539 [details] [diff] [review]
Bug 784575 - Part 1: NetworkStatsService Modification.

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

::: dom/network/src/NetworkStatsService.jsm
@@ +196,5 @@
> +        }
> +
> +        if (!appId) {
> +          mm.sendAsyncMessage("NetworkStats:Get:Return",
> +          { id: msg.id, error: "Invalid manifestURL", result: null });

nit: 2-space indention from mm.

@@ +419,5 @@
> +    if (DEBUG) {
> +      debug("updateCachedAppStats: " + this.cachedAppStatsDate);
> +    }
> +
> +    let keys = Object.keys(this.cachedAppStats);

s/keys/stats/ and below.

@@ +436,5 @@
> +        // Clean up the |cachedAppStats| after updating.
> +        if (index == keys.length - 1) {
> +          this.cachedAppStats = Object.create(null);
> +
> +          if (callback) {

if (!callback) {
  return;
}

if (error) {
  callback(false, error);
  return;
}

callback(true, "ok");
return;
Attachment #796539 - Flags: review?(gene.lian) → review+
Comment on attachment 797098 [details] [diff] [review]
Bug 784575 - Part 2: Create NetworkStatsServiceProxy.

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

You cannot use TABs in the new files. Please fix them.

After some thoughts, I think we should put all the cachedAppStats related calculation into NetworkStatsService.jsm. NetworkStatsServiceProxy shouldn't do anything other than just passing parameters (that's what the "proxy" mean). JS codes can still have chance to include NetworkStatsService.jsm to save data instead of using the proxy. We don't want half codes in the proxy and half codes in the service to deal with cachedAppStats, which will cause inconsistent behaviour.
Attachment #797098 - Flags: review?(gene.lian)
Attachment #797098 - Flags: review?(acperez)
Attachment #797098 - Flags: review-
Comment on attachment 796539 [details] [diff] [review]
Bug 784575 - Part 1: NetworkStatsService Modification.

As mentioned above, please move all the cachedAppStats logic into the .jsm.
Attachment #796539 - Flags: review?(acperez)
Attachment #796539 - Flags: review-
Attachment #796539 - Flags: review+
Comment on attachment 796545 [details] [diff] [review]
Bug 784575 - Part 4: NetworkStatsDB Modification.

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

Did you do anything new in the DB codes? If not, you can directly carry on my and Albert's review+.
Attachment #796545 - Flags: review?(gene.lian) → review+
Comment on attachment 796548 [details] [diff] [review]
Bug 784575 - Part 5: NetworkStatsManager Modification.

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

::: dom/network/interfaces/nsIDOMNetworkStats.idl
@@ +17,5 @@
>  {
>    /**
> +   * Manifest URL of an application which is used
> +   * to obtain the per-app stats of the specific app.
> +   * If null, system stats are returned.

* Manifest URL of an application for specifying the per-app
* stats of the specific app. If null, system stats are returned.
Attachment #796548 - Flags: review?(gene.lian) → review+
Comment on attachment 796545 [details] [diff] [review]
Bug 784575 - Part 4: NetworkStatsDB Modification.

>+        txn.result.manifestURL = aOptions.manifestURL;

OK. New review is for manifestURL. It's fine.
Comment on attachment 796553 [details] [diff] [review]
Bug 784575 - Part 6: Testing.

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

Tests look fine. Please make sure you don't use TABs anywhere. We still need Albert's eyes on this.

::: dom/network/tests/unit_stats/test_networkstats_db.js
@@ +232,5 @@
>  add_test(function test_processSamplesDiffSameSample() {
>    var sampleRate = netStatsDb.sampleRate;
>    var date = filterTimestamp(new Date());
> +  var lastStat = {appId:               0,
> +	                connectionType: "wifi", timestamp:    date,

Never use TABs!

@@ +237,5 @@
>                    rxBytes:             0, txBytes:      0,
>                    rxTotalBytes:     1234, txTotalBytes: 1234};
>  
> +  var newStat = {appId:                 0,
> +	               connectionType:   "wifi", timestamp:    date,

Ditto.

@@ +258,5 @@
>  add_test(function test_processSamplesDiffNextSample() {
>    var sampleRate = netStatsDb.sampleRate;
>    var date = filterTimestamp(new Date());
> +  var lastStat = {appId:               0,
> +	                connectionType: "wifi", timestamp:    date,

Ditto.

@@ +263,5 @@
>                    rxBytes:             0, txBytes:      0,
>                    rxTotalBytes:     1234, txTotalBytes: 1234};
>  
> +  var newStat = {appId:                 0,
> +	               connectionType:   "wifi", timestamp:    date + sampleRate,

Ditto.

@@ +285,5 @@
>    var samples = 5;
>    var sampleRate = netStatsDb.sampleRate;
>    var date = filterTimestamp(new Date());
> +  var lastStat = {appId:               0,
> +	                connectionType: "wifi", timestamp:    date,

Ditto.

@@ +290,5 @@
>                    rxBytes:             0, txBytes:      0,
>                    rxTotalBytes:     1234, txTotalBytes: 1234};
>  
> +  var newStat = {appId:                0,
> +	               connectionType:  "wifi", timestamp:    date + (sampleRate * samples),

Ditto.

@@ +309,5 @@
>  });
>  
>  add_test(function test_saveStats() {
> +  var stats = {appId:          0,
> +	             connectionType: "wifi",

Ditto.

@@ +384,5 @@
>    start = new Date(start - sampleRate);
>    var stats = [];
> +  for (var i = 0; i < samples; i++) {
> +    stats.push({appId:               0,
> +	              connectionType: "wifi", timestamp:    saveDate + (sampleRate * i),

Ditto.

@@ +389,5 @@
>                  rxBytes:             0, txBytes:      10,
>                  rxTotalBytes:        0, txTotalBytes: 0});
>  
> +    stats.push({appId:                 0,
> +	              connectionType: "mobile", timestamp:    saveDate + (sampleRate * i),

Ditto.

@@ +500,5 @@
> +      function callback(error, result) {
> +        do_check_eq(error, null);
> +        if (index != keys.length - 1) {
> +          index += 1;
> +          netStatsDb.saveStats(cached[keys[index]], callback);

Use early return.

::: dom/network/tests/unit_stats/test_networkstats_service_proxy.js
@@ +62,5 @@
> +                            today.getTime(), 10, 20);
> +
> +      var saveAppStatsCb = {
> +        notify: function notify(success, message) {
> +            do_check_eq(success, true);

2-space indention.

@@ +69,5 @@
> +            var key = 2 + 'mobile';
> +            do_check_eq(Object.keys(cachedAppStats).length, 1);
> +            do_check_eq(cachedAppStats[key].appId, 2);
> +            do_check_eq(cachedAppStats[key].connectionType, 'mobile');
> +            do_check_eq(new Date(cachedAppStats[key].date).getTime()/1000,

" / 1000"

@@ +70,5 @@
> +            do_check_eq(Object.keys(cachedAppStats).length, 1);
> +            do_check_eq(cachedAppStats[key].appId, 2);
> +            do_check_eq(cachedAppStats[key].connectionType, 'mobile');
> +            do_check_eq(new Date(cachedAppStats[key].date).getTime()/1000,
> +                        Math.floor(tomorrow.getTime()/1000));

Ditto.
Attachment #796553 - Flags: review?(gene.lian) → review+
(In reply to John Shih from comment #118)
> Created attachment 796539 [details] [diff] [review]
> Bug 784575 - Part 1: NetworkStatsService Modification.
> 
> b. Return manifestURL
> In order to check the result of get per-app stats, I return the manifestURL
> back to the NetworkStatsManager after calling getStats function.
>

I don't understand why do we need to expose / return the manifestURL in the NetworkStatsManager. If you mean to return the manifestURL of the requested app, I think it should be added to the nsIDOMMozNetworkStats, the object containing the result. I miss something?
(In reply to Albert [:albert] from comment #133)
> (In reply to John Shih from comment #118)
> > Created attachment 796539 [details] [diff] [review]
> > Bug 784575 - Part 1: NetworkStatsService Modification.
> > 
> > b. Return manifestURL
> > In order to check the result of get per-app stats, I return the manifestURL
> > back to the NetworkStatsManager after calling getStats function.
> >
> 
> I don't understand why do we need to expose / return the manifestURL in the
> NetworkStatsManager. If you mean to return the manifestURL of the requested
> app, I think it should be added to the nsIDOMMozNetworkStats, the object
> containing the result. I miss something?

Forget it, my mistake...
Comment on attachment 796548 [details] [diff] [review]
Bug 784575 - Part 5: NetworkStatsManager Modification.

Nice
Attachment #796548 - Flags: review?(acperez) → review+
(In reply to Gene Lian [:gene] (taking PTOs until 9/5) from comment #131)
> Comment on attachment 796545 [details] [diff] [review]
> Bug 784575 - Part 4: NetworkStatsDB Modification.
> 
> >+        txn.result.manifestURL = aOptions.manifestURL;
> 
> OK. New review is for manifestURL. It's fine.

Also, isInBrowser attribute removed and minor changes.
Comment on attachment 796545 [details] [diff] [review]
Bug 784575 - Part 4: NetworkStatsDB Modification.

Looks good
Attachment #796545 - Flags: review?(acperez) → review+
Comment on attachment 796553 [details] [diff] [review]
Bug 784575 - Part 6: Testing.

Need to fix nits pointed by Gene.

Nice!
Attachment #796553 - Flags: review?(acperez) → review+
No longer depends on: 855948, 855949, 858003, 855946
Blocks: 855948
Blocks: 855949
revised
Attachment #796548 - Attachment is obsolete: true
Attachment #802900 - Flags: review+
Attached patch Bug 784575 - Part 6: Testing. (obsolete) — Splinter Review
revised
Attachment #796553 - Attachment is obsolete: true
Attachment #802907 - Flags: review+
move the code related to |CachedAppStats| back to networkservice.
Attachment #796539 - Attachment is obsolete: true
Attachment #802909 - Flags: review?(gene.lian)
Attachment #802909 - Flags: review?(acperez)
Now NetworkStatsServiceProxy only passed the parameters to NetworkStatsService.
Attachment #797098 - Attachment is obsolete: true
Attachment #802912 - Flags: review?(gene.lian)
Attachment #802912 - Flags: review?(acperez)
fix the TABs
Attachment #802912 - Attachment is obsolete: true
Attachment #802912 - Flags: review?(gene.lian)
Attachment #802912 - Flags: review?(acperez)
Attachment #803444 - Flags: review?(gene.lian)
Attachment #803444 - Flags: review?(acperez)
Comment on attachment 802909 [details] [diff] [review]
Bug 784575 - Part 1: NetworkStatsService Modification.

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

::: dom/network/src/NetworkStatsService.jsm
@@ +415,5 @@
>      });
>    },
>  
> +  /*
> +   * Function responsible of handle receiving per-app stats.

Function responsible for receiving per-app stats.

@@ +443,5 @@
> +    // If the incoming date is different from |cachedAppStatsDate|,
> +    // both |cachedAppStats| and |cachedAppStatsDate| will get updated.
> +    let diff = (this._db.normalizeDate(stats.date) -
> +                this._db.normalizeDate(this.cachedAppStatsDate)) /
> +                this._db.sampleRate;

Align the last line to the first '('.

@@ +459,5 @@
> +          return;
> +        }
> +
> +        aCallback(true, "ok");
> +        return;

Don't need this return.
Attachment #802909 - Flags: review?(gene.lian) → review+
Comment on attachment 803444 [details] [diff] [review]
Bug 784575 - Part 2: Create NetworkStatsServiceProxy.

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

::: dom/network/src/NetworkStatsServiceProxy.js
@@ +19,5 @@
> +const nsINetworkStatsServiceProxy = Ci.nsINetworkStatsServiceProxy;
> +
> +function NetworkStatsServiceProxy() {
> +  if (DEBUG) {
> +	debug("Proxy started");

TAB...

@@ +28,5 @@
> +  /*
> +   * Function called in the protocol layer (HTTP, FTP, WebSocket ...etc)
> +   * to pass the per-app stats to NetworkStatsService.
> +   */
> +  saveAppStats: function saveAppStats(aAppId, aConnectionType, aTimeStamp, aRxBytes, aTxBytes, aCallback) {

Wrap this line from |aRxBytes| and align it to the first arg.

@@ +32,5 @@
> +  saveAppStats: function saveAppStats(aAppId, aConnectionType, aTimeStamp, aRxBytes, aTxBytes, aCallback) {
> +    if (DEBUG) {
> +	    debug("saveAppStats: " + aAppId + " " + aConnectionType + " " +
> +	          aTimeStamp + " " + aRxBytes + " " + aTxBytes);
> +	  }

You still use TABs at lots of places... Please do remove all of them!

@@ +35,5 @@
> +	          aTimeStamp + " " + aRxBytes + " " + aTxBytes);
> +	  }
> +
> +    if (!aCallback) {
> +      NetworkStatsService.saveAppStats(aAppId, aConnectionType, aTimeStamp, aRxBytes, aTxBytes);

Wrap this line from |aRxBytes| and align it to the first arg.

@@ +39,5 @@
> +      NetworkStatsService.saveAppStats(aAppId, aConnectionType, aTimeStamp, aRxBytes, aTxBytes);
> +      return;
> +    }
> +
> +    NetworkStatsService.saveAppStats(aAppId, aConnectionType, aTimeStamp, aRxBytes, aTxBytes,

Wrap this line from |aRxBytes| and align it to the first arg
Attachment #803444 - Flags: review?(gene.lian) → review+
patch revised.
Attachment #802909 - Attachment is obsolete: true
Attachment #802909 - Flags: review?(acperez)
Attachment #803597 - Flags: review?(acperez)
patch revised.
Attachment #803444 - Attachment is obsolete: true
Attachment #803444 - Flags: review?(acperez)
Attachment #803598 - Flags: review?(acperez)
Comment on attachment 803597 [details] [diff] [review]
Bug 784575 - Part 1: NetworkStatsService Modification.

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

Nice!

::: dom/network/src/NetworkStatsService.jsm
@@ +398,3 @@
>                    date:           date,
>                    rxBytes:        txBytes,
> +                  txBytes:        rxBytes };

Is not your fault but the assignation of rxBytes and txBytes is worng, please fix it:

rxBytes: rxBytes,
txBytes: txBytes };
Attachment #803597 - Flags: review?(acperez) → review+
Comment on attachment 803598 [details] [diff] [review]
Bug 784575 - Part 2: Create NetworkStatsServiceProxy.

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

Good job!

You still need to remove some TABs :)

::: dom/network/src/NetworkStatsServiceProxy.js
@@ +38,5 @@
> +
> +    if (!aCallback) {
> +      NetworkStatsService.saveAppStats(aAppId, aConnectionType, aTimeStamp,
> +                                       aRxBytes, aTxBytes);
> +      return;

No need to check if callback exists because you are checking it again in NetworkStatsService saveAppStats implementation. Just call saveAppStats and nothing else is needed.

saveAppStats: function saveAppStats(aAppId, aConnectionType, aTimeStamp,
                                    aRxBytes, aTxBytes, aCallback) {
    if (DEBUG) {
      debug("saveAppStats: " + aAppId + " " + aConnectionType + " " +
            aTimeStamp + " " + aRxBytes + " " + aTxBytes);
    }

    NetworkStatsService.saveAppStats(aAppId, aConnectionType, aTimeStamp,
                                     aRxBytes, aTxBytes);
},

Following code is duplicated in the NetworkStatsService.saveAppStats implementation.
Attachment #803598 - Flags: review?(acperez) → review+
(In reply to Albert [:albert] from comment #149)
> Comment on attachment 803598 [details] [diff] [review]
> Bug 784575 - Part 2: Create NetworkStatsServiceProxy.
> 
> Review of attachment 803598 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good job!
> 
> You still need to remove some TABs :)
> 
> ::: dom/network/src/NetworkStatsServiceProxy.js
> @@ +38,5 @@
> > +
> > +    if (!aCallback) {
> > +      NetworkStatsService.saveAppStats(aAppId, aConnectionType, aTimeStamp,
> > +                                       aRxBytes, aTxBytes);
> > +      return;
> 
> No need to check if callback exists because you are checking it again in
> NetworkStatsService saveAppStats implementation. Just call saveAppStats and
> nothing else is needed.
> 
> saveAppStats: function saveAppStats(aAppId, aConnectionType, aTimeStamp,
>                                     aRxBytes, aTxBytes, aCallback) {
>     if (DEBUG) {
>       debug("saveAppStats: " + aAppId + " " + aConnectionType + " " +
>             aTimeStamp + " " + aRxBytes + " " + aTxBytes);
>     }
> 
>     NetworkStatsService.saveAppStats(aAppId, aConnectionType, aTimeStamp,
>                                      aRxBytes, aTxBytes);
> },
> 
> Following code is duplicated in the NetworkStatsService.saveAppStats
> implementation.

I forgot the callback:

saveAppStats: function saveAppStats(aAppId, aConnectionType, aTimeStamp,
                                    aRxBytes, aTxBytes, aCallback) {
    if (DEBUG) {
      debug("saveAppStats: " + aAppId + " " + aConnectionType + " " +
            aTimeStamp + " " + aRxBytes + " " + aTxBytes);
    }

    NetworkStatsService.saveAppStats(aAppId, aConnectionType, aTimeStamp,
                                     aRxBytes, aTxBytes, aCallback);
},
Attachment #802907 - Attachment is obsolete: true
Attachment #804200 - Flags: review+
patch update
Attachment #803597 - Attachment is obsolete: true
Attachment #804201 - Flags: review+
patch update
Attachment #803598 - Attachment is obsolete: true
Attachment #804202 - Flags: review+
Keywords: checkin-needed
Blocks: 746074
Duplicate of this bug: 838056
Blocks: 917667
Blocks: 922926
Blocks: 858003
Blocks: 925670
You need to log in before you can comment on or make changes to this bug.