1070944 - [NetworkStats API] Separation of Browsing data from System data (Gecko)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Cost Control, defect)

Description

[Marina Rodríguez [:mai]]

When I'm browsing, the data traffic generated is collected on the system app instead on the Browser app.

Current behaviour:
  The data traffic generated by the browser App is accounted by the System App.

Expected behaviour:
  The data traffic generated by the browser app is accounted by the Browser app.



Video url showing the incorrect behaviour of the Costcontrol app.
  http://youtu.be/tZDblZIPezY

Regards

Comment 1

[Marina Rodríguez [:mai]]

Hi Ben and Katie,
do you mind taking a look to the current behaviour of the Usage app?
I'm not sure if it's correct showing all applications when the traffic generated by some of them is going to be accounted by system app. 
Maybe it's necessary create a blacklist to hide some applications. WDYT?

Regards

Comment 2

[Ben Francis [:benfrancis]]

Which version of Firefox OS are you referring to? In version 2.1 onwards, there is no separate browser app which is probably why data usage inside browser windows appears under "system".

Comment 3

[katieC]

Hi Marina,

I've noticed similar things with usage tracker... for improved UX
1. we should definitely not be showing any "apps" that have used 0.00 B
2. we need to define what we mean by "app" for the usage tracker, as much of the data activity is actually "browser" activity. This will be part of 2.2 effort.

However, as a general guideline for now - if the user has launched an app (as your activity/video shows) the data used from that activity should tie back to the specific app data usage (reflect the users choice), and not "system" (which the user has no clear understanding of). If data was used, this should not be a blacklisted/hidden activity.

Comment 4

[Jason Duell]

Yes,the browser's appID is now 0 (system app). 

However, all traffic that originates as part of web pages (i.e. the pages browsed, but not the load of the browser's own UI, etc) gets loaded with "isInBrowserElement=true", so at a platform level we can still distinguish what traffic is caused by browsing vs other system activities.  

But--we didn't anticipate this use case when we designed nsINetworkStatsServiceProxy, so we're not currently differentiating between inBrowser or not traffic. 

I think the right fix here is to add an inBrowser field here

   http://mxr.mozilla.org/mozilla-central/source/dom/network/interfaces/nsINetworkStatsServiceProxy.idl#32

and also in the query function (and obviously in webIDL too), and then the CostControl app can query for {appID=0, inBrowser=true} to get the browser usage.

John Shih wrote the code in question so he may have an opinion here too.  John, we should probably open a new bug if you agree and make this depend on it?

Comment 5

[Ben Francis [:benfrancis]]

Note that "browser" usage as described here will also apply to the use of EverythingMe "apps" and homescreen bookmarks.

Comment 6

[(no longer active)]

(In reply to Jason Duell (:jduell) from comment #4)
> and also in the query function (and obviously in webIDL too), and then the
> CostControl app can query for {appID=0, inBrowser=true} to get the browser
> usage.

Can we map the browser usage to its manifest format so that the Resource Stats API consumer doesn't need to be aware of this distinction?

Comment 7

[Jason Duell]

Ehsan: what do you mean exactly?  Right now the database just stores appID--so we'd store by manifest additonally/instead?  

We can store whatever indexes we like here--it's our database and thus just a SMOP :)

Comment 8

[(no longer active)]

(In reply to Jason Duell (:jduell) from comment #7)
> Ehsan: what do you mean exactly?  Right now the database just stores
> appID--so we'd store by manifest additonally/instead?  

No, I'm suggesting to store the traffic for isInBrowserElement iframes with a synthetic app id for the browser app.  (As I understand, the actual app id is 0, so that is not suitable).  My goal is to make the existing ResourceStats API to work without requiring the WebIDL change you suggested in comment 4.

Comment 9

[John Shih]

Yes, I think for this case we surely need to open a bug for that!
I think :vyang can help with this.

Comment 10

[Jason Duell]

> I'm suggesting to store the traffic for isInBrowserElement iframes with a synthetic app id for the browser app.

Sure, we could do it that way.  Are we fine with Everything.me and bookmarks getting thrown into the mix (comment 5)?  If not we would need to find some way to differentiate that traffic.

Comment 11

[John Shih]

(In reply to Jason Duell (:jduell) from comment #10)
> > I'm suggesting to store the traffic for isInBrowserElement iframes with a synthetic app id for the browser app.
> 
> Sure, we could do it that way.  Are we fine with Everything.me and bookmarks
> getting thrown into the mix (comment 5)?  If not we would need to find some
> way to differentiate that traffic.

In my understanding, the host apps opened outside browser app (such as homescreen bookmark) will have their own app id.

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

There is no more a browser app. If we still want to keep track of browsing traffic as an independent category, then that manifestURL field has to be renamed to reflect this. Note that Resources API is still not used in Gaia last time I checked, so we don't really have to be stick with the naming right now. If there's something missed, face it, not fake it. Because for the API you may simply use a special string to represent a fake browser app or similar concept, but there is just no such thing in the underlying mechanism.

CC Alive & Thinker `cause we had a discuss the day before. Maybe they'll have other opinions.

Comment 13

[Jason Duell]

I've been talking to Katie Caldwell in UI and it sounds like our ultimate goal here should be that we can split out traffic per-bookmark and per-Everything.me app.  So we need some way to differentiate that in the network traffic stats service.  I'm not sure of the best way to do that, but hopefully there's a good way.

Short-term we could probably live with just filtering out {appid=0, inBrowser=true} and reporting it as some magic appID (as ehsan suggested) and reporting that traffic as belonging to "Web" instead of "System."

If we can't get either of these happening soon enough the UI solution here will be to rename "System" something like "Web and Downloads".  But I'm thinking we can at least get separate System/Web buckets without much work.

Comment 14

[(inactive after 6/18) Alive Kuo [:alive]]

I will recommend that we group netstat for each webpage and count it in UI side if we want to show the overall webpage states.

System app has its own network access and it doesn't seem right to combine website netstat with system app.

The root issue of this problem is the netstat implementation assumes the website is living in a browser app but now there is not browser app anymore, and all the websites are embedded in system app. We should not assume this in gecko level. We should provide a query interface to
* mozbrowser iframe with mozapps(manifestURL) as identity.
* mozbrowser iframe without mozapps(manifestURL) and use origin or something else as identify.

Comment 15

[Peter Dolanjski [:pdol]]

I think we need a quick fix for now in 2.1 if we're to keep the per-app breakdown.  Renaming System to "Web and Downloads" seems like a good approach.  Does the System app use a significant amount of data in ways other than browsing sessions/apps and downloads?

Comment 16

[Peter Dolanjski [:pdol]]

Never mind, Katie filled me in.  
Okay, in the short term we should rename System if we can to better describe what it is to the user.  Katie will file a separate bug for this.

Comment 17

[Peter Dolanjski [:pdol]]

(In reply to Peter Dolanjski [:pdol] from comment #16)
> Katie will file a separate bug for this.

See bug 1078654.

Comment 18

[Ethan Tseng [:ethan]]

I'll take this bug.

Comment 19

[Ethan Tseng [:ethan]]

ResourceStats (the integration plan of NetworkStats and PowerStats) is still incomplete and is pending
for a while. So far no Gaia apps use ResourceStats yet.
I plan to fix this bug in NetworkStats, which is utilized by Usage (costcontrol) app.
So change the title to reflect the module being fixed.

Comment 20

[Ethan Tseng [:ethan]]

Attachment: bug-1070944-wip.patch

This is just a patch for proof-of-concept.
Basically it applies the solution suggested by Jason in comment 4.
The conversion logic is in NetworkStatsService.jsm.
If the |inBrowser| flag is true, it stores the record using the AppId of 
search app.

Comment 21

[Ethan Tseng [:ethan]]

I have a confusion here needed to be clarified:
What is the definition of browser app?

As several comments above stated, there is no browser app anymore since b2g v2.1.
However, from the perspective of users, he/she can still see a browser app in homescreen and in
usage (costcontrol) app.
Currently, the traffic occurring in browser app (from user's view) is actually generated by the
system app with |InBrowserElement| = true.
And the network usage of the |Browser| app shown in the usage app actually is the amount of
the search app.
(That's why my WIP patch hardcoded that way).

I want to clarify the definition of browser app in two perspectives:
1) From the perspective of saving statistics:
   What kind of traffic should be treated as belonging to browser?
   Is it possible that traffic is generated by apps other than system app but |InBrowserElement| = true?   
2) From the perspective of getting statistics:
   In terms of costcontrol, does "search app" exactly mean "browser app"?

Comment 22

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Ethan Tseng [:ethan] from comment #21)
> I have a confusion here needed to be clarified:
> What is the definition of browser app?
> 
> As several comments above stated, there is no browser app anymore since b2g
> v2.1.
> However, from the perspective of users, he/she can still see a browser app
> in homescreen and in
> usage (costcontrol) app.
> Currently, the traffic occurring in browser app (from user's view) is
> actually generated by the
> system app with |InBrowserElement| = true.
> And the network usage of the |Browser| app shown in the usage app actually
> is the amount of
> the search app.
> (That's why my WIP patch hardcoded that way).
> 
> I want to clarify the definition of browser app in two perspectives:
> 1) From the perspective of saving statistics:
>    What kind of traffic should be treated as belonging to browser?
>    Is it possible that traffic is generated by apps other than system app
> but |InBrowserElement| = true?   

I don't know what is InBrowserElement.

> 2) From the perspective of getting statistics:
>    In terms of costcontrol, does "search app" exactly mean "browser app"?

No. Search app is just the landing page (portal). Please treat search app to be a standalone app.

Comment 23

[Albert [:albert]]

(In reply to Ethan Tseng [:ethan] from comment #21)
> I have a confusion here needed to be clarified:
> What is the definition of browser app?
> 
> As several comments above stated, there is no browser app anymore since b2g
> v2.1.
> However, from the perspective of users, he/she can still see a browser app
> in homescreen and in
> usage (costcontrol) app.
> Currently, the traffic occurring in browser app (from user's view) is
> actually generated by the
> system app with |InBrowserElement| = true.
> And the network usage of the |Browser| app shown in the usage app actually
> is the amount of
> the search app.
> (That's why my WIP patch hardcoded that way).
> 
> I want to clarify the definition of browser app in two perspectives:
> 1) From the perspective of saving statistics:
>    What kind of traffic should be treated as belonging to browser?
>    Is it possible that traffic is generated by apps other than system app
> but |InBrowserElement| = true?   
> 2) From the perspective of getting statistics:
>    In terms of costcontrol, does "search app" exactly mean "browser app"?

Sorry Ethan, I can't help you with that questions.

Comment 25

[Ethan Tseng [:ethan]]

Acquired some information from Tim Chien.

Tim told me:
"System app hosts all apps in it’s children <iframe>s since it’s the first document Gecko loads 
 when the phone starts.
 An app gets loaded inside <iframe mozbrowser remote mozapp=XXX> where mozapp points to the url 
 of the manifest.
 A content webpage gets loaded in <iframe mozbrowser remote> without the mozapp attribute
 (and it’s permission boundary etc)."

Reference:
https://developer.mozilla.org/en-US/docs/Web/API/Using_the_Browser_API

Comment 26

[Ethan Tseng [:ethan]]

1. Clarification of the definition of browser app
   As Ben said in comment 2, there is no separate browser app since v2.1.
   - There is not a "browser" app in Gaia's app manifest list.
   - The browser app users see on UI actually is "search" app.
   - "No separate browser app" means any app could utilize the |mozbrowser| attribute to embed a
     browserwindow inside the app.

2. Why is browsing traffic accounted to system app?
   When a user opens the browser/search app, the system app creates a browser iframe as the
   browser container.
   This browsing iframe is contained in the system app's iframe and it doesn't have the |mozapp|
   attribute. Thus, this iframe element belongs to system app.

3. Use cases
   A simplified DOM focusing on iframe elements.

   |---------- <iframe mozbrowser mozapp=system> (1)
       |
       |---------- <iframe mozbrowser> (2)
       |
       |---------- <iframe mozbrowser mozapp=search> (3)
       |
       |---------- <iframe mozbrowser mozapp=calendar> (4)
           |
           |--------- <iframe mozbrowser> (5)
       |
       |---------- <iframe mozbrowser mozapp=XXX>


   (1) Traffic generated by system app itself
   (2) Traffic generated by system app's browsing element
   (3) Traffic generated by search app itself
   (4) Traffic generated by calendar app itself
   (5) Traffic generated by calendar app's browsing element

   Our current implementation accounts (2) for system app and arises confusion to users.

4. isInBrowserElement
   "nsILoadContext::isInBrowserElement is true iff the load is occurring inside a browser element."
   AFAIK, this means it is true only when an iframe has the |mozbrowser| attribute but doesn't 
   have |mozapp|. Such as use case (2) and (5).

Comment 27

[Ethan Tseng [:ethan]]

As Jason suggested in comment 4 and comment 13, we should differentiate traffic of the system app itself
and the browsing activity happened in its iframe element.

A more general idea is, to separate traffic generated from an "app iframe" and a "browser iframe".
An app iframe contains the |mozapp| attribute which specified the app's manifestURL.
A browser iframe doesn't contain the |mozapp| attribute and lives in another app (could be system app).

This solution means we need to add one more key, such as "IsBrowser", to the indexedDB of NetworkStats.
And usage app could decide how to aggregate records according to some specific rules.
For example,
System app   : records of app iframe and browser iframe should be separated.
Any other app: records of app iframe and browser iframe should be aggregated.

Hi Salva,
What do you think?

Comment 28

[Ethan Tseng [:ethan]]

Attachment: bug-1070944-wip.patch

A WIP patch.
Add one parameter |elementType| to nsINetworkStatsServiceProxy.idl.

Comment 29

[Salvador de la Puente González [:salva]]

Hi Ethan. I was on vacation last Friday but one of the tasks I agree to be in charge during the sprint planning was to clarify with you some all these points related with Browser application. But you were faster than me ;)

As suggested in comment 27, I think the best option here is to extend statistics entries to differentiate between normal usage and usage produced inside "browser iframes" for all the applications. Then we could move the problem to be completely in Gaia. There are already some filtering operations happening before displaying the breakdown of applications so it should be not difficult to fold "in-browser" usage from System with the "search application" traffic into a "synthetic" application called Browser or something like that. This way, UX guys have total control about what is being displayed.

So what do you think about adding btxBytes and brxBytes where b stands for `browsing`?

Comment 30

[Ethan Tseng [:ethan]]

(In reply to Salvador de la Puente González [:salva] from comment #29)
> As suggested in comment 27, I think the best option here is to extend
> statistics entries to differentiate between normal usage and usage produced
> inside "browser iframes" for all the applications. Then we could move the
> problem to be completely in Gaia.
Yes! Totally agree.

> There are already some filtering
> operations happening before displaying the breakdown of applications so it
> should be not difficult to fold "in-browser" usage from System with the
> "search application" traffic into a "synthetic" application called Browser
> or something like that. This way, UX guys have total control about what is
> being displayed.
Sounds great.

> So what do you think about adding btxBytes and brxBytes where b stands for
> `browsing`?
My initial idea is to add a new index such as elementType (enum) inBrowser (boolean)
to the objectStore as a new key. In this way, the kayPath would become:
["appId", "elementType", "serviceType", "network", "timestamp"]
It means every per-app record will be split into two unique records in the updated indexedDB.

Your suggestion extends existing records without changing keys. It sounds easier and more 
reasonable. We just breakdown the usage of applications.
Okay. I'll move forward to this way. Will let you know if I have any concern. :)

Thanks!

Comment 31

[Salvador de la Puente González [:salva]]

Hi, yesterday Albert told me he has some concerns about this idea. I'm going to ping the WebAPI list in order to gather more feedback but I think this is a good step for the short coming.

Comment 32

[Ethan Tseng [:ethan]]

What kind of concern does Albert mention? Can you elaborate?

Comment 33

[Albert [:albert]]

(In reply to Ethan Tseng [:ethan] from comment #32)
> What kind of concern does Albert mention? Can you elaborate?

I thought you want to add some kind of fake manifest and set brxBytes, btxBytes.

I talked with Salva and I see I was wrong, so no concerns with your proposal.

Comment 34

[Ethan Tseng [:ethan]]

Thanks, Albert. :)

According to Salva's suggestion, we can choose to add two attributes to MozNetworkStatsData.
It would become like this.
  interface MozNetworkStatsData {
    readonly attribute unsigned long   rxBytes;   // Received bytes from app itself.
    readonly attribute unsigned long   txBytes;   // Sent bytes from app itself.
    readonly attribute unsigned long   brxBytes;  // Received bytes from browsing.
    readonly attribute unsigned long   btxBytes;  // Sent bytes from browsing.
    readonly attribute Date            date;      // Date.
  };
Personally I feel "brx/btx" is too esoteric to understand, but anyway we can refine naming later. :)

Another approach is to modify MozNetworkStats instead of MozNetworkStatsData.
Aesthetically it looks better to me (and we don't need to add "brx/btxBytes").
  interface MozNetworkStats {
    readonly attribute DOMString    appManifestURL;
    ......
    [Cached, Pure]
    readonly attribute sequence<MozNetworkStatsData> appData;
    readonly attribute sequence<MozNetworkStatsData> browsingData;
  };

What do you think?
Please let me know if you have any idea.

Comment 35

[Ethan Tseng [:ethan]]

Hi Salva and Albert,

I had some discussion with my colleague and reconsidered this issue.
Then I realized adding attributes to the data part of MozNetworkStats is not a good idea.
Either of the approaches in comment 34 makes the data part not general anymore and subsequently
lacks extensibility.
(What if we have a third type of traffic per application someday?)

Being inspired by Alive's comment (comment 14), I think we can redefine MozNetworkStats.appManifestURL
so it can represent different identities.

One way is to extend this attribute to contain additional information other than app's manifestURL.
Another way is to turn it into a more general "identity" attribute and add a new attribute to indicate
how to explain the identity.

For example:
enum MozNetworkStatsIdType {"app", "browsing"};

interface MozNetworkStats {
  readonly attribute MozNetworkStatsIdType  idType;
  readonly attribute DOMString              id;  // appManifestURL for app; origin for browsing
  // the rest remains the same
};
But I'm not sure if Gaia can aggregate browsing data per application in this way.

Just my two cents. Maybe we can figure out some better solution.

Comment 36

[Albert [:albert]]

(In reply to Ethan Tseng [:ethan] from comment #35)
> Hi Salva and Albert,
> 
> I had some discussion with my colleague and reconsidered this issue.
> Then I realized adding attributes to the data part of MozNetworkStats is not
> a good idea.
> Either of the approaches in comment 34 makes the data part not general
> anymore and subsequently
> lacks extensibility.
> (What if we have a third type of traffic per application someday?)
> 
> Being inspired by Alive's comment (comment 14), I think we can redefine
> MozNetworkStats.appManifestURL
> so it can represent different identities.
> 
> One way is to extend this attribute to contain additional information other
> than app's manifestURL.
> Another way is to turn it into a more general "identity" attribute and add a
> new attribute to indicate
> how to explain the identity.
> 
> For example:
> enum MozNetworkStatsIdType {"app", "browsing"};
> 
> interface MozNetworkStats {
>   readonly attribute MozNetworkStatsIdType  idType;
>   readonly attribute DOMString              id;  // appManifestURL for app;
> origin for browsing
>   // the rest remains the same
> };
> But I'm not sure if Gaia can aggregate browsing data per application in this
> way.
> 
> Just my two cents. Maybe we can figure out some better solution.

I like that approach, similar to Mike's proposal in the webapi list. The only problem I see is that I think that usage app needs to make two requests because they always need both types of traffic. Well is not a problem.. Maybe you can allow to request multiple types once.

Salva can give more details on this.

Comment 37

[Salvador de la Puente González [:salva]]

As said in the WebAPI lists, I like Mike's idea but including the breakdown by app inside the data sample. This approach of specifying different kind of sources (idType) does not convince me. From a Gaia developer's perspective there is no other thing than applications and a system (although we know system is another app) and it makes sense for him to have manifests as the sources of the consumptions.

I would like to ask for the consumption in the categories ['default', 'browsing'] for an specific app in a time interval.

You have an example in the WebAPI list.

What do you think?

Comment 38

[Ethan Tseng [:ethan]]

Attachment: networkstats_webidl.patch

A proposal of NetworkStats WebIDL change.

Comment 39

[Ethan Tseng [:ethan]]

(In reply to Salvador de la Puente González [:salva] from comment #37)
> As said in the WebAPI lists, I like Mike's idea but including the breakdown
> by app inside the data sample. This approach of specifying different kind of
> sources (idType) does not convince me. From a Gaia developer's perspective
> there is no other thing than applications and a system (although we know
> system is another app) and it makes sense for him to have manifests as the
> sources of the consumptions.
> 
> I would like to ask for the consumption in the categories ['default',
> 'browsing'] for an specific app in a time interval.
> 
> You have an example in the WebAPI list.
> What do you think?

Hi Salva and Albert,

Sorry for my late response.
The proposal above is based on the suggestion of Mike de Boer from WebAPI mailing list.
Here we introduce MozNetworkStatsCategory as one level of indirection to the real data.

Then, the result of getSamples() would be like this:
  app: "system"
  serviceType: null
  network: { type: MOBILE, id: iccid-xxx }
  start: 2014/12/1
  end: 2014/12/30
  categories: [
    { name: total,
      data: [
        {date: 12/1, rxBytes: xxx, txBytes: yyy},
        {date: 12/2, rxBytes: xxx, txBytes: yyy},
        ...
      ]
    },
    { name: app,
      data: [
        {date: 12/1, rxBytes: xxx, txBytes: yyy},
        {date: 12/2, rxBytes: xxx, txBytes: yyy},
        ...
      ]
    },
    ...
  ]

As we discussed in Portland work week, one of your concern is that multiple queries for each
app might result in too many JS instances and could impair the performance of usage app.
By this approach, we can return all the records for one app by one query.
And if necessary, you can still specify a certain category using the options parameter of 
getSamples().

Do you have other concern with this approach?

Comment 40

[Ethan Tseng [:ethan]]

Attachment: networkstats_webidl_v2.patch

This is another approach.
This approach also introduces one level of indirection, but it turns this level
into a function instead of an attribute.
So Gaia code doesn't have to locate a certain category by iterating the array in
for loops.

For example, it could retrieve the usage of some app like this;
var rxBytes = result.data[i].fromCategory("total").rxBytes;

Comment 41

[Jacob Shen]

[Tracking Requested - why for this release]:

Comment 42

[Ethan Tseng [:ethan]]

Attachment: networkstats_webidl_v2.patch

Fix syntax error in WebIDL proposal.

Comment 43

[Ethan Tseng [:ethan]]

Attachment: networkstats_webidl_v3.patch

As discussed with Salva offline, it's better to be backward-compatible.
This is another idea to introduce the category attribute as breakdown of
applications.
It simply adds category as a new key to NetworkStatsGetOptions and
MozNetworkStats.

Comment 44

[Ethan Tseng [:ethan]]

Attachment: Bug 1070944 Part 1: MozNetworkStats WebIDL change

Hi Hsin-Yi,

This is the proposed change to MozNetworkStats WebIDL.
Could you provide some feedback?

Comment 45

[Ethan Tseng [:ethan]]

Attachment: usageapp_example.js

Salva provided some code segments to demonstrate the use of newly added
"category" attribute in NetworkStatsGetOptions and MozNetworkStats.
This example also demonstrates our idea to convert both the "options" argument
and the result of nsIDOMMozNetworkStatsManager::getSamples() from scalar type
to array.

Comment 46

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8537052 [details] [diff] [review]
Bug 1070944 Part 1: MozNetworkStats WebIDL change

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

Thanks for the patch, and thanks for inviting me to the discussion, Ethan :)

The API changes look really good to me. The newly introduced enum NetworkStatsCategory provides high extensibility for future state classes. The nullable category offers a friendly way to retrieving data all at a time that looks chic.

Comment 47

[Ethan Tseng [:ethan]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #46)
> Thanks for the patch, and thanks for inviting me to the discussion, Ethan :)
Cheers! Thanks Hsin-Yi! :D

Comment 48

[(no longer active)]

Comment on attachment 8537052 [details] [diff] [review]
Bug 1070944 Part 1: MozNetworkStats WebIDL change

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

This looks good!

::: dom/webidl/MozNetworkStats.webidl
@@ +24,5 @@
> +  /**
> +   * Category is used to retrieve breakdown statistics of an application.
> +   * If category is not specified, the total traffic is returned.
> +   */
> +  NetworkStatsCategory? category;

Shouldn't this default to "app"?

Comment 49

[Ethan Tseng [:ethan]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #48)
> Comment on attachment 8537052 [details] [diff] [review]
> This looks good!
Thanks Ehsan! :)

> ::: dom/webidl/MozNetworkStats.webidl
> @@ +24,5 @@
> > +  /**
> > +   * Category is used to retrieve breakdown statistics of an application.
> > +   * If category is not specified, the total traffic is returned.
> > +   */
> > +  NetworkStatsCategory? category;
> Shouldn't this default to "app"?
As I explained in comment 26 and comment 27, app and browsing category corresponds to the statistics
accounted in app iframe and browser iframe.
We assume that for most applications, users just want to know the total traffic of both categories.
Thus making the default as null could be more convenient to the API user (the usage app).
What do you think?

Comment 50

[Ethan Tseng [:ethan]]

Attachment: Part 1: MozNetworkStats WebIDL change

Part 1: MozNetworkStats WebIDL change.

Comment 51

[Ethan Tseng [:ethan]]

Attachment: Part 2: Implementation of saving statistics

Added a new parameter category to nsINetworkStatsServiceProxy::saveAppStats().

Comment 52

[Ethan Tseng [:ethan]]

Attachment: Part 3: Implementation of NetworkStatsDB

Support the new 'category' key in NetworkStatsDB.

Comment 53

[Ethan Tseng [:ethan]]

Attachment: Part 3: Implementation of NetworkStatsDB

Trim whitespaces in the previous patch.

Comment 54

[Ethan Tseng [:ethan]]

Hi Ehsan,

I need your help to review the WebIDL change (attachment 8540633 [details] [diff] [review]).
But before the review, I would like to discuss two issues with you.

The first is about your suggestion of the default value of category. Please see my comment 49.
The second is about the naming.
As discussed in web-api mailing list, the name "category" is a more generic idea.
It seems we should choose some other name here. Do you have any suggestion?


p.s. The most naive name comes to my mind is "iframeType". But it looks weird in this WebIDL.
enum NetworkStatsIframeType {
  "app";     // mozapp iframe
  "browser"; // mozbrowser iframe
};

Comment 55

[(no longer active)]

I just commented on the dev-webapi thread.  I would like to understand how you're planning to differentiate categories other than browser/app first, since without having a solid plan on how to do that in the future, the current design seem to try to be too extensible.  :-)

Comment 56

[(no longer active)]

(In reply to Ethan Tseng [:ethan] from comment #49)
> > ::: dom/webidl/MozNetworkStats.webidl
> > @@ +24,5 @@
> > > +  /**
> > > +   * Category is used to retrieve breakdown statistics of an application.
> > > +   * If category is not specified, the total traffic is returned.
> > > +   */
> > > +  NetworkStatsCategory? category;
> > Shouldn't this default to "app"?
> As I explained in comment 26 and comment 27, app and browsing category
> corresponds to the statistics
> accounted in app iframe and browser iframe.
> We assume that for most applications, users just want to know the total
> traffic of both categories.
> Thus making the default as null could be more convenient to the API user
> (the usage app).
> What do you think?

Makes sense.

Comment 57

[Ethan Tseng [:ethan]]

Attachment: Part 1: MozNetworkStats WebIDL change (v2)

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #55)
> I just commented on the dev-webapi thread.  I would like to understand how
> you're planning to differentiate categories other than browser/app first,
> since without having a solid plan on how to do that in the future, the
> current design seem to try to be too extensible.  :-)

I have to admit that we do not have plans to differentiate categories other than browsing/app.
The idea of using enum values is simply to leave the possibility for future extension, in case someday
we have to classify a third type of statistics other than browsing and app.

If you think using enum is not necessary or too extensible, we can roll back to previous proposals,
such as the ones in comment 34.
And I would like to support Salva's initial advice to add btxBytes/brxBytes to MozNetworkStatsData
because of its simplicity.
What do you think about this change?

Comment 59

[(no longer active)]

(In reply to Ethan Tseng [:ethan] from comment #57)
> Created attachment 8540823 [details] [diff] [review]
> Part 1: MozNetworkStats WebIDL change (v2)
> 
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #55)
> > I just commented on the dev-webapi thread.  I would like to understand how
> > you're planning to differentiate categories other than browser/app first,
> > since without having a solid plan on how to do that in the future, the
> > current design seem to try to be too extensible.  :-)
> 
> I have to admit that we do not have plans to differentiate categories other
> than browsing/app.
> The idea of using enum values is simply to leave the possibility for future
> extension, in case someday
> we have to classify a third type of statistics other than browsing and app.
> 
> If you think using enum is not necessary or too extensible, we can roll back
> to previous proposals,
> such as the ones in comment 34.
> And I would like to support Salva's initial advice to add btxBytes/brxBytes
> to MozNetworkStatsData
> because of its simplicity.
> What do you think about this change?

That sounds good to me!

Comment 60

[Eric Shepherd [:sheppy]]

If/when this lands, there are API changes that need to be documented. This will involve updating or creating these pages:

https://developer.mozilla.org/en-US/docs/Web/API/Network_Stats_API
https://developer.mozilla.org/en-US/docs/Web/API/MozNetworkStatsManager

In addition, subpages will be needed under https://developer.mozilla.org/en-US/docs/Web/API/MozNetworkStatsManager/<API term names> for each new method or property added to the MozNetworkStatsManager interface.

Then the appropriate Firefox X for developers page will need an update.

Comment 61

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8540644 [details] [diff] [review]
Part 3: Implementation of NetworkStatsDB

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

The existing upgrade code uses a dangerously broken pattern that your new upgrade step copies... It does async work (e.g. |openCursor()|) within a for-loop. The for-loop doesn't wait for each async operation to complete before looping, so you have a nasty problem. Consider this simplified example code:

  // Pseudo-code, most details omitted...
  function upgradeSchema(aTransaction, aDb, aOldVersion, aNewVersion) {
    for (let currVersion = aOldVersion; currVersion < aNewVersion; currVersion++) {
      if (currVersion == 0) {
        aDb.createObjectStore("foo");
      } else if (currVersion == 1) {
        aTransaction.objectStore("foo").openCursor().onsuccess = function(event) {
          let cursor = event.target.result;
          if (cursor) {
            alert(cursor.key);
            cursor.continue();
          }
        };
      } else if (currVersion == 2) {
        aDB.deleteObjectStore("foo");
      }
    }
  }

This code doesn't actually loop over all the values in the objectStore like you probably think it should in the 1->2 upgrade. Instead the sequence of executed calls is:

  1. createObjectStore("foo"); // upgrade 0->1
  2. objectStore("foo").openCursor(); // upgrade 1->2
  3. deleteObjectStore("foo"); // upgrade 2->3
  4. cursor.continue(); // EXCEPTION!

The |openCursor()| success callback is asynchronous and will happen after the objectStore has been deleted, so an exception will be thrown when calling |cursor.continue()|.

This needs to be fixed, and please make sure that your new upgrade step doesn't use the same pattern!

The same broken pattern was fixed in the contacts database in bug 892497, and for the SMS database in bug 887701.

::: dom/network/NetworkStatsDB.jsm
@@ +323,5 @@
> +        // applications. See bug 1070944 for detail.
> +
> +        // Copy records from the current object store to a temporary store.
> +        let tmpObjectStore;
> +        tmpObjectStore = db.createObjectStore(TEMP_STATS_STORE_NAME,

Rather than creating a temporary store you should probably just do what the 5->6 upgrade did (make a new store, copy once, delete the old one, update the code to use the new store name).

Comment 62

[Ethan Tseng [:ethan]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #61)
> This needs to be fixed, and please make sure that your new upgrade step
> doesn't use the same pattern!
> 
> The same broken pattern was fixed in the contacts database in bug 892497,
> and for the SMS database in bug 887701.

Ben, thanks for your information!
Then I'll file a new bug to fix the upgrade pattern of NetworkStatsDB since it should be
independent of this bug.


> ::: dom/network/NetworkStatsDB.jsm
> @@ +323,5 @@
> > +        // Copy records from the current object store to a temporary store.
> > +        let tmpObjectStore;
> > +        tmpObjectStore = db.createObjectStore(TEMP_STATS_STORE_NAME,
> 
> Rather than creating a temporary store you should probably just do what the
> 5->6 upgrade did (make a new store, copy once, delete the old one, update
> the code to use the new store name).

Okay.
As we discussed offline, I think it's better to keep the object store name and avoid copying
twice, but that requires an IDBObjectStore.rename() function which we don't have yet.
Thus I will apply your advice for now.
Thank! :)

Comment 63

[Ethan Tseng [:ethan]]

(In reply to Ethan Tseng [:ethan] from comment #62)
> Then I'll file a new bug to fix the upgrade pattern of NetworkStatsDB since
> it should be independent of this bug.
Bug 1116715 was filed for this.

Comment 64

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I filed bug 1118028 on adding a rename() function to idb.

Comment 65

[Ethan Tseng [:ethan]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #64)
> I filed bug 1118028 on adding a rename() function to idb.
Thanks! It will be great to have this function. :)

Comment 66

[Vincent Chang[:vchang][changyihsin]]

This is essential function for cost control and should be landed to 2.2.

Comment 67

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: this is must-have for not miscalculating data usage between browser and system.
We had this defect since 2.0, and should get it fixed in 2.2 at least.

Comment 68

[Ethan Tseng [:ethan]]

Attachment: Part 1: MozNetworkStats WebIDL change

Hi Ehsan,

This is the WebIDL patch according to our discussion in webapi mailing list.
Could you help to review it?

Comment 69

[Ethan Tseng [:ethan]]

Attachment: Part 2: Implementation of saving statistics.

Rebase the patch.

Comment 70

[Ethan Tseng [:ethan]]

Comment on attachment 8551092 [details] [diff] [review]
Part 2: Implementation of saving statistics.

Hi Albert,

This patch stores |IsInBrowser| information from different network channels to
NetworkStatsService.
The change of database will be in part 3 patch.

Could you help to review it?

Comment 71

[Ethan Tseng [:ethan]]

Attachment: Part 3: Implementation of NetworkStatsDB and quering statistics

This patch adds implementation for |sourceIsBrowser|:
1. Add keyPath to network statistics object store in NetworkStatsDB.
2. Add code to query records by this option.

Comment 72

[Albert [:albert]]

Comment on attachment 8551092 [details] [diff] [review]
Part 2: Implementation of saving statistics.

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

Looks good

::: dom/network/NetworkStatsServiceProxy.js
@@ +55,5 @@
>    },
>  
>    /*
>     * Function called in the points of different system services
>     * to pass the per-serive stats to NetworkStatsService.

As you are editing this file fix typo please
s/per-serive/per-service

Comment 73

[Ethan Tseng [:ethan]]

Attachment: Part 2: Implementation of saving statistics

Albert, thanks for your review.

This patch:
1. Correct a typo.
2. Refresh comment "r=albert".

With this patch, isInBrowser information is passed from different network
channels to NetworkStatsService.

I will prepare another patch (part 3) to store this information to database
and augment getSamples function to support querying by this flag.

Comment 74

[Ethan Tseng [:ethan]]

Attachment: Part 1: MozNetworkStats WebIDL change

Refresh comment "r=ehsan".

Comment 75

[Ethan Tseng [:ethan]]

Hi Salva,

The part 1 patch (attachment 8552369 [details] [diff] [review]) adds the boolean attribute |sourceIsBrowser| to MozNetworkStats
and NetworkStatsGetOptions.
My idea is, if |sourceIsBrowser| is not specified in GetOptions, NetworkStatsManager returns both
records (in browser and not in browser) at once.
That implies nsIDOMNetworkStatsManager.idl getSamples() would return an array of MozNetworkStats
instead of singular one.

For example:
options = { appManifestURL: "system" };  // isInBrowser not specified
var req = statistics.getSamples(networkId, start, end, options);

It might return:
[ { appManifestURL: "system", sourceIsBrowser: false, ... },
  { appManifestURL: "system", sourceIsBrowser: true, ... }, ]

What do you think?

Comment 76

[Ethan Tseng [:ethan]]

Attachment: Part 3: Implementation of NetworkStatsDB and quering statistics

This is just a WIP patch for feedback.

Albert,

This patch does not change the return result of getSamples() yet.
As we discussed offline, we have to confirm this with Gaia developers.

However, I would like to have your feedback first on other parts.
1. The key value of isInBrowser is number 0 and 1.
   This patch upgrades database schema version from 8 to 9 (adding isInBrowser as keyPath).
   I found that the key of IndexedDB cannot use boolean value.
   So I converted true/false to 1/0 while storing record to database, and vice versa.
2. The naming of object stores.
   As Ben suggested in comment 61, I copy records from the previous object store to the
   new one as the way 5->6 upgrade does. Then I found the naming of object stores become
   dirty and hard to read (see my patch).

Do you have any idea on these two points?

Comment 77

[Albert [:albert]]

Comment on attachment 8553608 [details] [diff] [review]
Part 3: Implementation of NetworkStatsDB and quering statistics

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

See comments

::: dom/network/NetworkStatsDB.jsm
@@ +795,5 @@
>  
>    _getCurrentStats: function _getCurrentStats(aNetwork, aResultCb) {
>      this.dbNewTxn(STATS_STORE_NAME, "readonly", function(txn, store) {
>        let request = null;
> +      // FIXME: Should we pass 0 for isInBrowser?

0 because the _getCurrentStats is used by getAlarmQuota in NetworkStatsService, which always deal with total traffic.

@@ +796,5 @@
>    _getCurrentStats: function _getCurrentStats(aNetwork, aResultCb) {
>      this.dbNewTxn(STATS_STORE_NAME, "readonly", function(txn, store) {
>        let request = null;
> +      // FIXME: Should we pass 0 for isInBrowser?
> +      let upperFilter = [0, 0, "", aNetwork, Date.now()];

You miss to do the same in following function _getCurrentStatsFromDate

@@ +876,5 @@
>        debug("End time: " + new Date(end));
>      }
>  
> +    // FIXME: aIsInBrowser could be undefined. In that case, we have to return
> +    // two records of isInBrowser is true and false.

As talked offline undefined is false.

@@ +882,3 @@
>      this.dbNewTxn(STATS_STORE_NAME, "readonly", function(aTxn, aStore) {
>        let network = [aNetwork.id, aNetwork.type];
> +      let lowerFilter = [aAppId, 0, aServiceType, network, start]; // FIXME

Why 0? should be aIsInBroser?

@@ +905,5 @@
>          // now - VALUES_MAX_LENGTH, fill with empty samples.
>          this.fillResultSamples(start + offset, end + offset, data);
>  
>          aTxn.result.appManifestURL = aAppManifestURL;
> +        aTxn.result.isInBrowser = 0; // FIXME

Why 0? should be aIsInBroser?

::: dom/network/NetworkStatsManager.js
@@ +71,5 @@
>    if (DEBUG) {
>      debug("NetworkStats Constructor");
>    }
>    this.appManifestURL = aStats.appManifestURL || null;
> +  this.isInBrowser = aStats.isInBrowser || null;

this.isInBrowser = aStats.isInBrowser || 0;

@@ +138,5 @@
>        throw Components.results.NS_ERROR_INVALID_ARG;
>      }
>  
>      let appManifestURL = null;
> +    let isInBrowser = null;

0

@@ +146,5 @@
>          throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
>        }
>        appManifestURL = aOptions.appManifestURL;
>        serviceType = aOptions.serviceType;
> +      isInBrowser = aOptions.sourceIsBrowser || null;

0

::: dom/network/NetworkStatsService.jsm
@@ +414,5 @@
>          return;
>        }
>      }
>  
> +    let isInBrowser = msg.isInBrowser || "";

0 as default value

Comment 78

[Ethan Tseng [:ethan]]

Attachment: Part 1: MozNetworkStats WebIDL change

Rename |sourceIsBrowser| as |browsingTrafficOnly|.

As discussed with Salva and Albert, in order to keep compatibility, if this
option is not set in queries, we return the aggregated statistics of app and
browsing traffic to Gaia.

Comment 79

[Ethan Tseng [:ethan]]

Attachment: Part 1: MozNetworkStats WebIDL change

Fixed typo.

Comment 80

[Ethan Tseng [:ethan]]

Attachment: Part 3: Implementation of NetworkStatsDB and quering statistics

WIP patch.

Comment 81

[Salvador de la Puente González [:salva]]

(In reply to Ethan Tseng [:ethan] from comment #75)
> Hi Salva,
> 
> The part 1 patch (attachment 8552369 [details] [diff] [review]) adds the
> boolean attribute |sourceIsBrowser| to MozNetworkStats
> and NetworkStatsGetOptions.
> My idea is, if |sourceIsBrowser| is not specified in GetOptions,
> NetworkStatsManager returns both
> records (in browser and not in browser) at once.
> That implies nsIDOMNetworkStatsManager.idl getSamples() would return an
> array of MozNetworkStats
> instead of singular one.
> 
> For example:
> options = { appManifestURL: "system" };  // isInBrowser not specified
> var req = statistics.getSamples(networkId, start, end, options);
> 
> It might return:
> [ { appManifestURL: "system", sourceIsBrowser: false, ... },
>   { appManifestURL: "system", sourceIsBrowser: true, ... }, ]
> 
> What do you think?

Per the offline discussion, we agree on keep backward compatbibilty of the current API. So if `isInBrowser` is not specified, null or false, the result of `getSamples()` is the same as now, this is the total traffic for an application. If `isInBrowser` is set, the result is that originated from a `mozbrowser` iframe.

Comment 82

[Ethan Tseng [:ethan]]

Attachment: Part 3: Implementation of NetworkStatsDB and quering statistics

Hi Albert,

I am still doing some testing work to verify the patch.
If you have time, could you please provide some feedback?

1. I turned the deprecated store names into an array so we don't have to create
   many variable names for the same purpose.
2. browsingTrafficOnly option is added as we proposed.

Comment 83

[Ethan Tseng [:ethan]]

Attachment: Part 3: Implementation of NetworkStatsDB and quering statistics

Albert,

I've done the basic manual tests, and I think the patch is ready for review.
In the meanwhile, I will try to add automation test cases for it.

Comment 84

[Ethan Tseng [:ethan]]

Attachment: Part 2: Implementation of saving statistics

Rebase the patch because bug 1100184 flattened netwerk/base directory.

Comment 85

[Ethan Tseng [:ethan]]

Attachment: Part 3: Implementation of NetworkStatsDB and quering statistics

Per offline discussion with Albert, move the conversion of isInBrowser between
true/false and 1/0 to importData() and exportData().

Comment 86

[Albert [:albert]]

Comment on attachment 8555129 [details] [diff] [review]
Part 3: Implementation of NetworkStatsDB and quering statistics

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

LGTM

Thanks Ethan!

::: dom/network/NetworkStatsDB.jsm
@@ +806,5 @@
>  
>    _getCurrentStats: function _getCurrentStats(aNetwork, aResultCb) {
>      this.dbNewTxn(STATS_STORE_NAME, "readonly", function(txn, store) {
>        let request = null;
> +      let lowerFilter = [0, 0, "", aNetwork, Date.now()];

lower filter is not being used, remove it

@@ +831,5 @@
>      aDate = new Date(aDate);
>      this.dbNewTxn(STATS_STORE_NAME, "readonly", function(txn, store) {
>        let request = null;
>        let start = this.normalizeDate(aDate);
> +      let lowerFilter = [0, 0, "", aNetwork, start];

remove it

Comment 87

[Ethan Tseng [:ethan]]

(In reply to Albert [:albert] from comment #86)
> Part 3: Implementation of NetworkStatsDB and quering statistics
> Review of attachment 8555129 [details] [diff] [review]:
> -----------------------------------------------------------------
> >    _getCurrentStats: function _getCurrentStats(aNetwork, aResultCb) {
> >      this.dbNewTxn(STATS_STORE_NAME, "readonly", function(txn, store) {
> >        let request = null;
> > +      let lowerFilter = [0, 0, "", aNetwork, Date.now()];
> 
> lower filter is not being used, remove it

Thanks! I didn't notice it uses only upperBound here.
I will update the patch.

In the meanwhile, I am thinking of one issue regarding to database upgrade.
My patch augments records by adding the new attribute |isInBrowser| with the default value 0 (false),
which should be reasonable for most applications.
I am considering to assign value 1 for the system app because most traffic we accounted for the
system app is from its browser iframe.

What do you think?

Comment 88

[Ethan Tseng [:ethan]]

Hi Tim,

Except for the browser iframe created by the system app, do you know when would the system app itself
consume network traffic?
I just want to know the most common or possible use cases because of the concern in comment 87.

Comment 89

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Ethan Tseng [:ethan] from comment #88)
> Hi Tim,
> 
> Except for the browser iframe created by the system app, do you know when
> would the system app itself
> consume network traffic?
> I just want to know the most common or possible use cases because of the
> concern in comment 87.

The System app can download something with xhr in it's own frame right? But I don't know explicitly which feature do that and I don't know everything System app do. If you definitely need an answer for this I would recommend you send an e-mail to dev-gaia.

The one instance I know of is the notification icon. We simply use an <img> to load whatever URL the app pass to us and we do not cache it in the application. Such request will definitely hit Necko.

Comment 90

[Ethan Tseng [:ethan]]

Attachment: Part 3: Implementation of NetworkStatsDB and quering statistics

This patch:
1. Assign |inInBrowser| as 1 for the system app in database upgrade.
2. Address issues in comment 86.
3. Refresh comment "r=albert".

Comment 91

[Ethan Tseng [:ethan]]

Attachment: Part 3: Implementation of NetworkStatsDB and quering statistics

Remove debugging message.

Comment 92

[Ethan Tseng [:ethan]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #89)
> The System app can download something with xhr in it's own frame right? But
> I don't know explicitly which feature do that and I don't know everything
> System app do. If you definitely need an answer for this I would recommend
> you send an e-mail to dev-gaia.
> 
> The one instance I know of is the notification icon. We simply use an <img>
> to load whatever URL the app pass to us and we do not cache it in the
> application. Such request will definitely hit Necko.

Tim, thanks for your information. :)
So far I just want to ensure the majority of traffic consumed by the system app is from the
system browser.

Comment 93

[Ethan Tseng [:ethan]]

Attachment: Part 3: Implementation of NetworkStatsDB and quering statistics

Fixed one bug in previous patch:
|isInBrowser| should be added into the key of cached statistics in 
NetworkStatsService.jsm.

Comment 94

[Ethan Tseng [:ethan]]

Attachment: Part 4: Update test cases

This patch updates XPCShell unit tests.

Comment 95

[Albert [:albert]]

(In reply to Ethan Tseng [:ethan] from comment #87)
> (In reply to Albert [:albert] from comment #86)
> > Part 3: Implementation of NetworkStatsDB and quering statistics
> > Review of attachment 8555129 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > >    _getCurrentStats: function _getCurrentStats(aNetwork, aResultCb) {
> > >      this.dbNewTxn(STATS_STORE_NAME, "readonly", function(txn, store) {
> > >        let request = null;
> > > +      let lowerFilter = [0, 0, "", aNetwork, Date.now()];
> > 
> > lower filter is not being used, remove it
> 
> Thanks! I didn't notice it uses only upperBound here.
> I will update the patch.
> 
> In the meanwhile, I am thinking of one issue regarding to database upgrade.
> My patch augments records by adding the new attribute |isInBrowser| with the
> default value 0 (false),
> which should be reasonable for most applications.
> I am considering to assign value 1 for the system app because most traffic
> we accounted for the
> system app is from its browser iframe.
> 
> What do you think?

As it is not critical I think we can assume that system traffic isInBrowser for the upgrade process of the database, just leave a comment in your code to clarify it.

Comment 96

[Albert [:albert]]

Comment on attachment 8556420 [details] [diff] [review]
Part 4: Update test cases

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

Patch looks good but I miss some test of new property 'isInBrowser'. You always set it to false, you can add a test that save some samples with 'isInBrowser' set to false and some to true and then check if find returns the correct result.

::: dom/network/tests/unit_stats/test_networkstats_db.js
@@ +170,5 @@
>  
>      netStatsDb.logAllRecords(function(error, result) {
>        do_check_eq(error, null);
>        do_check_eq(result.length, 1);
>        do_check_eq(result[0].appId, stats.appId);

Add check of isInBrowser

@@ +214,5 @@
>  
>        netStatsDb.logAllRecords(function(error, result) {
>          do_check_eq(error, null);
>  
>          do_check_eq(result.length, samples);

ditto

@@ +324,5 @@
>                    rxTotalBytes:     2234, txTotalBytes:  2234 };
>  
>    processSamplesDiff(networks, lastStat, newStat, function(result) {
>      do_check_eq(result.length, 1);
>      do_check_eq(result[0].appId, newStat.appId);

ditto

@@ +361,5 @@
>                    rxTotalBytes:        0, txTotalBytes:  0 };
>  
>    processSamplesDiff(networks, lastStat, newStat, function(result) {
>      do_check_eq(result.length, 2);
>      do_check_eq(result[1].appId, newStat.appId);

ditto

@@ +397,5 @@
>                    rxTotalBytes:        0, txTotalBytes:  0 };
>  
>    processSamplesDiff(networks, lastStat, newStat, function(result) {
>      do_check_eq(result.length, samples + 1);
>      do_check_eq(result[0].appId, newStat.appId);

ditto

@@ +432,5 @@
>        netStatsDb.logAllRecords(function(error, result) {
>          do_check_eq(error, null);
>          do_check_eq(result.length, 1);
>          do_check_eq(result[0].appId, stats.appId);
>          do_check_eq(result[0].serviceType, stats.serviceType);

ditto

@@ +469,5 @@
>        netStatsDb.logAllRecords(function(error, result) {
>          do_check_eq(error, null);
>          do_check_eq(result.length, 1);
>          do_check_eq(result[0].appId, stats.appId);
>          do_check_eq(result[0].serviceType, stats.serviceType);

ditto

@@ +506,5 @@
>        netStatsDb.logAllRecords(function(error, result) {
>          do_check_eq(error, null);
>          do_check_eq(result.length, 1);
>          do_check_eq(result[0].appId, stats.appId);
>          do_check_eq(result[0].serviceType, stats.serviceType);

ditto

@@ +573,2 @@
>        do_check_eq(result.network.type, networks[0].type);
>        do_check_eq(result.start.getTime(), start.getTime());

You should check 'browsingTrafficOnly', right?

@@ +619,2 @@
>        do_check_eq(result.network.type, networks[0].type);
>        do_check_eq(result.start.getTime(), start.getTime());

ditto

@@ +665,2 @@
>        do_check_eq(result.network.type, networks[0].type);
>        do_check_eq(result.start.getTime(), start.getTime());

ditto

@@ +745,5 @@
>  
>          if (index == keys.length - 1) {
>            netStatsDb.logAllRecords(function(error, result) {
>              do_check_eq(error, null);
>              do_check_eq(result.length, 6);

ditto

::: dom/network/tests/unit_stats/test_networkstats_service_proxy.js
@@ +58,4 @@
>  
>        do_check_eq(Object.keys(cachedStats).length, 2);
>        do_check_eq(cachedStats[key1].appId, 1);
>        do_check_eq(cachedStats[key1].serviceType.length, 0);

You forget to check isInBrowser property

Comment 97

[Ethan Tseng [:ethan]]

Albert,

Sorry I have to request you to review this part again.

I fixed two issues that were discussed with you offline.
I just modified two places:
1. NetworkStatsSerivce.jsm writeCache()
   isInBrowser has to be one of the keys of the cache.
2. NetworkStatsDB.jsm find()
   We cannot filter correct records by changing both isInBrowser and timestamp at the same time
   in lowerFilter and upperFilter. So I split one query into two queries.

Comment 98

[Ethan Tseng [:ethan]]

Attachment: Part 3: Implementation of NetworkStatsDB and quering statistics

Comment 99

[Ethan Tseng [:ethan]]

Attachment: Part 4: Update and add test cases

Add test cases for isInBrowser = 1 and browsingTrafficOnly option.

Comment 100

[Albert [:albert]]

Comment on attachment 8558420 [details] [diff] [review]
Part 3: Implementation of NetworkStatsDB and quering statistics

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

LGTM

Thanks!

::: dom/network/NetworkStatsDB.jsm
@@ +898,5 @@
>        debug("Start time: " + new Date(start));
>        debug("End time: " + new Date(end));
>      }
>  
> +    // Find samples of browsing traffic (isInBrowser = 1) first because they are

Could you add a comment explaining why do we need to do two requests?

Comment 101

[Ethan Tseng [:ethan]]

(In reply to Albert [:albert] from comment #100)
> Comment on attachment 8558420 [details] [diff] [review]
> Part 3: Implementation of NetworkStatsDB and quering statistics
> Review of attachment 8558420 [details] [diff] [review]:
> -----------------------------------------------------------------
> LGTM
> Thanks!
> 
> ::: dom/network/NetworkStatsDB.jsm
> @@ +898,5 @@
> >        debug("Start time: " + new Date(start));
> >        debug("End time: " + new Date(end));
> >      }
> >  
> > +    // Find samples of browsing traffic (isInBrowser = 1) first because they are
> 
> Could you add a comment explaining why do we need to do two requests?


Albert,

Thanks for your review!
I will add comments for explanation and rename existedData as foundData. :)

Comment 102

[Ethan Tseng [:ethan]]

Attachment: Part 3: Implementation of NetworkStatsDB and querying statistics

1. Refresh comment "r=albert".
2. Add some code comments and rename existedData as foundData.

Comment 103

[Ethan Tseng [:ethan]]

Attachment: Part 4: Update and add test cases

Refresh comment "r=albert".

Comment 104

[Ethan Tseng [:ethan]]

Attachment: Part 4: Update and add test cases

Refresh comment "r=albert".

Comment 105

[Ethan Tseng [:ethan]]

The result of Treeherder:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d3c00ea4dda

Comment 106

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/901aaee07b80
https://hg.mozilla.org/integration/b2g-inbound/rev/07f55ff86e15
https://hg.mozilla.org/integration/b2g-inbound/rev/a67a9f37cb21
https://hg.mozilla.org/integration/b2g-inbound/rev/2ca4450d6a33

Comment 107

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/901aaee07b80
https://hg.mozilla.org/mozilla-central/rev/07f55ff86e15
https://hg.mozilla.org/mozilla-central/rev/a67a9f37cb21
https://hg.mozilla.org/mozilla-central/rev/2ca4450d6a33

Comment 108

[Ryan VanderMeulen [:RyanVM]]

Please request b2g37 approval on this when you get a chance.

Comment 109

[Ethan Tseng [:ethan]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #108)
> Please request b2g37 approval on this when you get a chance.
Thanks Ryan.
I will request for approval. :)

Comment 110

[Ethan Tseng [:ethan]]

Comment on attachment 8553671 [details] [diff] [review]
Part 1: MozNetworkStats WebIDL change

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: Incorrect usage reported by cost control app
Testing completed: On m-c
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: MozNetworkStats.webidl

Comment 111

[Ethan Tseng [:ethan]]

Comment on attachment 8555128 [details] [diff] [review]
Part 2: Implementation of saving statistics

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: Incorrect usage reported by cost control app
Testing completed: On m-c
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: nsINetworkStatsServiceProxy

Comment 112

[Ethan Tseng [:ethan]]

Comment on attachment 8555128 [details] [diff] [review]
Part 2: Implementation of saving statistics

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: Incorrect usage reported by cost control app
Testing completed: On m-c
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: nsINetworkStatsServiceProxy

Comment 113

[Ethan Tseng [:ethan]]

Comment on attachment 8561152 [details] [diff] [review]
Part 3: Implementation of NetworkStatsDB and querying statistics

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: Incorrect usage reported by cost control app
Testing completed: On m-c
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None

Comment 114

[Ethan Tseng [:ethan]]

Comment on attachment 8561155 [details] [diff] [review]
Part 4: Update and add test cases

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: Incorrect usage reported by cost control app
Testing completed: On m-c
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None

Comment 115

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a642d239acff
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/248abdff56f1
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/06d2f606100d
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/bfb33050d127