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

RESOLVED FIXED in Firefox 38

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mai, Assigned: ethan, NeedInfo)

Tracking

({dev-doc-needed})

unspecified
2.2 S6 (20feb)
x86_64
Windows 7
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.1 affected, b2g-v2.2 fixed, b2g-master fixed)

Details

User Story

As a user, I want data associated with browsing (via the Browser app) separated from data associated with the System app so that it is easier to make choices about connectivity settings and my device usage.

Acceptance Criteria:
1. All data used to browse to websites (not including apps with a manifest) is accumulated under the "Browser" app in the Usage app.
2. The "System" app's data usage is not increased during browsing activity (from AC#1 above).

Attachments

(6 attachments, 29 obsolete attachments)

19.78 KB, patch
Details | Diff | Splinter Review
1.64 KB, application/javascript
Details
1.98 KB, patch
Details | Diff | Splinter Review
18.90 KB, patch
Details | Diff | Splinter Review
32.14 KB, patch
Details | Diff | Splinter Review
41.49 KB, patch
Details | Diff | Splinter Review
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
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
Flags: needinfo?(kcaldwell)
Flags: needinfo?(bfrancis)
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".
Flags: needinfo?(bfrancis)
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.
Flags: needinfo?(kcaldwell)
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?
Flags: needinfo?(johnshih.bugs)
Note that "browser" usage as described here will also apply to the use of EverythingMe "apps" and homescreen bookmarks.
(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?
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 :)
Flags: needinfo?(ehsan.akhgari)
(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.
Flags: needinfo?(ehsan.akhgari)
Yes, I think for this case we surely need to open a bug for that!
I think :vyang can help with this.
Flags: needinfo?(johnshih.bugs) → needinfo?(vyang)
> 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 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.
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.
Flags: needinfo?(vyang)
Flags: needinfo?(tlee)
Flags: needinfo?(alive)
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.
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.
Flags: needinfo?(alive)
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?
Flags: needinfo?(alive)
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.
Flags: needinfo?(alive)
(In reply to Peter Dolanjski [:pdol] from comment #16)
> Katie will file a separate bug for this.

See bug 1078654.
Whiteboard: [NaBfT]
Summary: [ResourceStats API] Data traffic accounting per app does not works correctly → [ResourceStats API] Separation of Browsing data from System data
User Story: (updated)
User Story: (updated)
feature-b2g: --- → 2.2?
I'll take this bug.
Assignee: nobody → ettseng
Blocks: 1089983
blocking-b2g: --- → backlog
Blocks: 1094897
No longer blocks: 1089983
feature-b2g: 2.2? → 2.2+
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.
Summary: [ResourceStats API] Separation of Browsing data from System data → [NetworkStats API] Separation of Browsing data from System data
Posted patch bug-1070944-wip.patch (obsolete) — Splinter Review
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.
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"?
Flags: needinfo?(alive)
Flags: needinfo?(alberto.crespellperez)
(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.
Flags: needinfo?(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?   
> 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.
Flags: needinfo?(alberto.crespellperez)
Duplicate of this bug: 1100891
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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(dharris)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(dharris) → needinfo?
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).
Flags: needinfo?
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?
Flags: needinfo?(salva)
A WIP patch.
Add one parameter |elementType| to nsINetworkStatsServiceProxy.idl.
Attachment #8524325 - Attachment is obsolete: true
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`?
Flags: needinfo?(salva)
(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!
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.
What kind of concern does Albert mention? Can you elaborate?
(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.
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.
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.
(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.
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?
Posted patch networkstats_webidl.patch (obsolete) — Splinter Review
A proposal of NetworkStats WebIDL change.
(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?
Posted patch networkstats_webidl_v2.patch (obsolete) — Splinter Review
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;
[Tracking Requested - why for this release]:
feature-b2g: 2.2+ → ---
tracking-b2g: --- → ?
Posted patch networkstats_webidl_v2.patch (obsolete) — Splinter Review
Fix syntax error in WebIDL proposal.
Attachment #8534188 - Attachment is obsolete: true
Posted patch networkstats_webidl_v3.patch (obsolete) — Splinter Review
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.
Hi Hsin-Yi,

This is the proposed change to MozNetworkStats WebIDL.
Could you provide some feedback?
Attachment #8534144 - Attachment is obsolete: true
Attachment #8534791 - Attachment is obsolete: true
Attachment #8535416 - Attachment is obsolete: true
Attachment #8537052 - Flags: feedback?(htsai)
Attachment #8537052 - Attachment description: bug-1070944-part1.patch → Bug 1070944 Part 1: MozNetworkStats WebIDL change
Posted file 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 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.
Attachment #8537052 - Flags: feedback?(htsai) → feedback+
(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 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"?
(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?
Part 1: MozNetworkStats WebIDL change.
Attachment #8537052 - Attachment is obsolete: true
Added a new parameter category to nsINetworkStatsServiceProxy::saveAppStats().
Support the new 'category' key in NetworkStatsDB.
Trim whitespaces in the previous patch.
Attachment #8540641 - Attachment is obsolete: true
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
};
Flags: needinfo?(ehsan.akhgari)
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.  :-)
Flags: needinfo?(ehsan)
(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.
(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?
Flags: needinfo?(ehsan)
Duplicate of this bug: 1114592
(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!
Flags: needinfo?(ehsan)
Depends on: 1115502
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.
Keywords: dev-doc-needed
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).
(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! :)
(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.
Depends on: 1116715
I filed bug 1118028 on adding a rename() function to idb.
(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. :)
This is essential function for cost control and should be landed to 2.2.
feature-b2g: --- → 2.2?
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.
blocking-b2g: backlog → 2.2+
feature-b2g: 2.2? → ---
tracking-b2g: ? → ---
Hi Ehsan,

This is the WebIDL patch according to our discussion in webapi mailing list.
Could you help to review it?
Attachment #8540633 - Attachment is obsolete: true
Attachment #8540823 - Attachment is obsolete: true
Attachment #8549458 - Flags: review?(ehsan)
Attachment #8549458 - Flags: review?(ehsan) → review+
Whiteboard: [NaBfT]
Rebase the patch.
Attachment #8540638 - Attachment is obsolete: true
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?
Attachment #8551092 - Flags: review?(alberto.crespellperez)
Attachment #8551092 - Attachment description: bug-1070944-part2.patch → Part 2: Implementation of saving 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.
Attachment #8540644 - Attachment is obsolete: true
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
Attachment #8551092 - Flags: review?(alberto.crespellperez) → review+
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.
Attachment #8551092 - Attachment is obsolete: true
Refresh comment "r=ehsan".
Attachment #8549458 - Attachment is obsolete: true
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?
Flags: needinfo?(salva)
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?
Attachment #8551198 - Attachment is obsolete: true
Attachment #8553608 - Flags: feedback?(alberto.crespellperez)
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
Attachment #8553608 - Flags: feedback?(alberto.crespellperez) → feedback-
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.
Attachment #8552369 - Attachment is obsolete: true
Fixed typo.
Attachment #8553670 - Attachment is obsolete: true
WIP patch.
Attachment #8553608 - Attachment is obsolete: true
(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.
Flags: needinfo?(salva)
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.
Attachment #8553698 - Attachment is obsolete: true
Attachment #8554403 - Flags: feedback?(alberto.crespellperez)
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.
Attachment #8554403 - Attachment is obsolete: true
Attachment #8554403 - Flags: feedback?(alberto.crespellperez)
Attachment #8554487 - Flags: review?(alberto.crespellperez)
Summary: [NetworkStats API] Separation of Browsing data from System data → [NetworkStats API] Separation of Browsing data from System data (Gecko)
Rebase the patch because bug 1100184 flattened netwerk/base directory.
Attachment #8552346 - Attachment is obsolete: true
Per offline discussion with Albert, move the conversion of isInBrowser between
true/false and 1/0 to importData() and exportData().
Attachment #8554487 - Attachment is obsolete: true
Attachment #8554487 - Flags: review?(alberto.crespellperez)
Attachment #8555129 - Flags: review?(alberto.crespellperez)
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
Attachment #8555129 - Flags: review?(alberto.crespellperez) → review+
(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?
Status: NEW → ASSIGNED
Flags: needinfo?(alberto.crespellperez)
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.
Flags: needinfo?(timdream)
(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.
Flags: needinfo?(timdream)
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".
Attachment #8555129 - Attachment is obsolete: true
Remove debugging message.
Attachment #8555738 - Attachment is obsolete: true
(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.
Fixed one bug in previous patch:
|isInBrowser| should be added into the key of cached statistics in 
NetworkStatsService.jsm.
Attachment #8555743 - Attachment is obsolete: true
Posted patch Part 4: Update test cases (obsolete) — Splinter Review
This patch updates XPCShell unit tests.
Attachment #8556420 - Flags: review?(alberto.crespellperez)
(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.
Flags: needinfo?(alberto.crespellperez)
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
Attachment #8556420 - Flags: review?(alberto.crespellperez) → review-
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.
Attachment #8556419 - Attachment is obsolete: true
Attachment #8558420 - Flags: review?(alberto.crespellperez)
Add test cases for isInBrowser = 1 and browsingTrafficOnly option.
Attachment #8556420 - Attachment is obsolete: true
Attachment #8558423 - Flags: review?(alberto.crespellperez)
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?
Attachment #8558420 - Flags: review?(alberto.crespellperez) → review+
Attachment #8558423 - Flags: review?(alberto.crespellperez) → review+
(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. :)
1. Refresh comment "r=albert".
2. Add some code comments and rename existedData as foundData.
Attachment #8558420 - Attachment is obsolete: true
Attachment #8561152 - Attachment description: bug-1070944-part3.patch → Part 3: Implementation of NetworkStatsDB and querying statistics
Refresh comment "r=albert".
Attachment #8558423 - Attachment is obsolete: true
Refresh comment "r=albert".
Attachment #8561153 - Attachment is obsolete: true
Please request b2g37 approval on this when you get a chance.
Flags: needinfo?(ettseng)
(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. :)
Flags: needinfo?(ettseng)
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
Attachment #8553671 - Flags: approval-mozilla-b2g37?
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 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
Attachment #8555128 - Flags: approval-mozilla-b2g37?
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
Attachment #8561152 - Flags: approval-mozilla-b2g37?
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
Attachment #8561155 - Flags: approval-mozilla-b2g37?
Attachment #8553671 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8555128 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8561152 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8561155 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.