951976 - API for Resource Statistics

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Borting Chen [:borting]]

As we discussed in [1], we decided to create a new resource stats API to merge the NetworkStats API and PowerStats API. This bug is created for discussing the design and implementation of this new API.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=854200#c76

Comment 1

[Borting Chen [:borting]]

Attachment: (WIP) Part 0: webidl for ResourceStatsManager

This is the draft for ResourceStats API. (We are still working on this.)

-- We categorize resource statistics into two types: 'network' & 'power', which count network usage and power consumption, respectively. When calling the ResourceStats API, users should specify which type of statistics they are curious about.

-- getStats() & clearStats() are generalized to support the query & delete for network statistics and power statistics. The |resourceOptions| argument specifies the detail of the operation, such as retrieves the network usage of a specific App or the power consumption of LCD.

-- addAlarm(), getAlarm(), and removeAlarm() are designed for resource control. These functions are inherited from existing networkStats API directly. [1]

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=858005

Comment 2

[Borting Chen [:borting]]

Attachment: (WIP) Part 0: webidl for ResourceStatsManager

Comment 3

[Borting Chen [:borting]]

Attachment: ResourceStats-part0-webidl.patch

According to the discussion in [1], we got a conclusion for the webidl of ResourceStats API.

Note:
1. In ResourceStatsManager interface,
   |resourceTypes|, |sampleRate| and |maxStorageAge| should be static.
   But, since webidl doesn't support static for JS implementation (see Bug #863952),
   I temporarily remove the static declaration.
2. In ResourceStats interface,
   |stats| should be defined as a union of sequence<NetworkStatsData> and sequence<PowerStatsData>
   but this causes webidl.py output an error which says it cannot distinguish sequence<NetworkStatsData> from sequence<PowerStatsData>.
   Hence, I temporarily declare it as sequence<any>.


[1] https://groups.google.com/forum/#!topic/mozilla.dev.webapi/tWkgbD1v_Gg

Comment 4

[Borting Chen [:borting]]

Attachment: ResourceStats-part0-webidl.patch

Update Note:
1. rename the |stats| attribute of ResourceStats interface back to |data|

Comment 5

[Borting Chen [:borting]]

Comment on attachment 8364917 [details] [diff] [review]
ResourceStats-part0-webidl.patch

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

Hi Ehsan:

Thanks for your comments and suggestions for the webidl design.
Could you help review the patch for webidl?

Comment 6

[Borting Chen [:borting]]

Attachment: ResourceStats-part0-webidl.patch

Hi Ehsan:

I have made some updates. Please review this patch. Thanks.

Update Note:
1. Change the type of |threshold| to unsigned long long

Comment 7

[(no longer active)]

Comment on attachment 8364978 [details] [diff] [review]
ResourceStats-part0-webidl.patch

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

Thanks for your perseverance on this so far, Borting!

I have some higher level comments which you can see below, so I'm minusing the patch for those comments right now.  Once you address/explain these points, I'll take a closer look at the next iteration.

::: dom/webidl/ResourceStats.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +[JSImplementation="@mozilla.org/networkStatsData;1", Pref="dom.resourceStats.enabled"]

Do we want to make this API available to workers at some point?  If yes, it seems to me like using Javascript as an implementation language is a mistake.

Also, is there any reason why we would want to expose this API to non-certified apps?

@@ +47,5 @@
> +   * Statistics, one element per day
> +   */
> +  [Cached, Pure]
> +  //readonly attribute (sequence<NetworkStatsData> or sequence<PowerStatsData>) data;
> +  readonly attribute sequence<any>  data;  // array of NetworkStatsData or PowerStatsData

Please see my comments about this posted to the list.  r- because of this.

::: dom/webidl/ResourceStatsManager.webidl
@@ +168,5 @@
> +  /**
> +   * Return supporting resource statistics
> +   */
> +  [Cached, Pure]
> +  //static readonly attribute sequence<DOMString> resourceTypes;

Why aren't these static?  I thought we agreed on that.

@@ +169,5 @@
> +   * Return supporting resource statistics
> +   */
> +  [Cached, Pure]
> +  //static readonly attribute sequence<DOMString> resourceTypes;
> +  readonly attribute sequence<DOMString> resourceTypes;

The semantics of this attribute is not completely clear to me.  Can you please clarify what this is supposed to return exactly, and whether it would be subject to the same considerations as ResourceStats.data?

Comment 8

[Borting Chen [:borting]]

Hi Eshan:

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #7)
> ::: dom/webidl/ResourceStats.webidl
> @@ +3,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/.
> > + */
> > +
> > +[JSImplementation="@mozilla.org/networkStatsData;1", Pref="dom.resourceStats.enabled"]
> 
> Do we want to make this API available to workers at some point?  If yes, it
> seems to me like using Javascript as an implementation language is a mistake.

I think we do not need a worker thread.
For network stats part, it is already implemented in JS.
For power stats part, we plan to use a lightweight algorithm to provide a rough power consumption estimation.
So, I think use JS as the implementation language could be okay.

> Also, is there any reason why we would want to expose this API to
> non-certified apps?

No, only certified apps can access this API.
Did I miss anything? Or, should I include the change of PermissionTable in this patch?

> @@ +47,5 @@
> > +   * Statistics, one element per day
> > +   */
> > +  [Cached, Pure]
> > +  //readonly attribute (sequence<NetworkStatsData> or sequence<PowerStatsData>) data;
> > +  readonly attribute sequence<any>  data;  // array of NetworkStatsData or PowerStatsData
> 
> Please see my comments about this posted to the list.  r- because of this.

Okay, this attribute will be changed to 

    sequence<(NetworkStatsData or PowerStatsData)> getStats();

Thanks for your help.

> ::: dom/webidl/ResourceStatsManager.webidl
> @@ +168,5 @@
> > +  /**
> > +   * Return supporting resource statistics
> > +   */
> > +  [Cached, Pure]
> > +  //static readonly attribute sequence<DOMString> resourceTypes;
> 
> Why aren't these static?  I thought we agreed on that.

Yes, as we discussed, |resourceTypes|, |sampleRate| and |maxStorageAge| should be static.
But, because static attributes is not supported by JS-implemented WebIDL at this time (please see Bug #863952),
I temporarily remove the static declaration and I will add them back when that bug is resolved.

Sorry, I should mention this change in the comment.

> @@ +169,5 @@
> > +   * Return supporting resource statistics
> > +   */
> > +  [Cached, Pure]
> > +  //static readonly attribute sequence<DOMString> resourceTypes;
> > +  readonly attribute sequence<DOMString> resourceTypes;
> 
> The semantics of this attribute is not completely clear to me.  Can you
> please clarify what this is supposed to return exactly, and whether it would
> be subject to the same considerations as ResourceStats.data?

This attribute will return the resource types that we supported (i.e. "Network" and "Power").
And the ResourceStats.data contains the query results of getStats(), it is an arrary of "NetworkStatsData" or "PowerStatsData."

Comment 9

[Borting Chen [:borting]]

Attachment: ResourceStats-part0-webidl.patch

Hi Ehsan:

Please help review this updated patch. Thanks a lot.

Update Note:
1. change |data| attribute to getStats() method in ResourceStats interface.
2. Add comment in ResourceStatsManager interface that mentions we temoraily mark out the static declaration due to Bug #863952

Comment 10

[(no longer active)]

(In reply to Borting Chen [:borting] from comment #8)
> Hi Eshan:
> 
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #7)
> > ::: dom/webidl/ResourceStats.webidl
> > @@ +3,5 @@
> > > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > > + * You can obtain one at http://mozilla.org/MPL/2.0/.
> > > + */
> > > +
> > > +[JSImplementation="@mozilla.org/networkStatsData;1", Pref="dom.resourceStats.enabled"]
> > 
> > Do we want to make this API available to workers at some point?  If yes, it
> > seems to me like using Javascript as an implementation language is a mistake.
> 
> I think we do not need a worker thread.
> For network stats part, it is already implemented in JS.
> For power stats part, we plan to use a lightweight algorithm to provide a
> rough power consumption estimation.
> So, I think use JS as the implementation language could be okay.

OK, fair enough.

> > Also, is there any reason why we would want to expose this API to
> > non-certified apps?
> 
> No, only certified apps can access this API.
> Did I miss anything? Or, should I include the change of PermissionTable in
> this patch?

I think it would be nice if we use the annotations that Boris is adding in bug 958667.

> > ::: dom/webidl/ResourceStatsManager.webidl
> > @@ +168,5 @@
> > > +  /**
> > > +   * Return supporting resource statistics
> > > +   */
> > > +  [Cached, Pure]
> > > +  //static readonly attribute sequence<DOMString> resourceTypes;
> > 
> > Why aren't these static?  I thought we agreed on that.
> 
> Yes, as we discussed, |resourceTypes|, |sampleRate| and |maxStorageAge|
> should be static.
> But, because static attributes is not supported by JS-implemented WebIDL at
> this time (please see Bug #863952),
> I temporarily remove the static declaration and I will add them back when
> that bug is resolved.
> 
> Sorry, I should mention this change in the comment.

OK, I guess it's fine to start working on this without bug 863952 for now, but we should definitely block the implementation on that bug.

> > @@ +169,5 @@
> > > +   * Return supporting resource statistics
> > > +   */
> > > +  [Cached, Pure]
> > > +  //static readonly attribute sequence<DOMString> resourceTypes;
> > > +  readonly attribute sequence<DOMString> resourceTypes;
> > 
> > The semantics of this attribute is not completely clear to me.  Can you
> > please clarify what this is supposed to return exactly, and whether it would
> > be subject to the same considerations as ResourceStats.data?
> 
> This attribute will return the resource types that we supported (i.e.
> "Network" and "Power").

So it's going to be a static array that will never change at runtime, right?

Comment 11

[(no longer active)]

Comment on attachment 8365767 [details] [diff] [review]
ResourceStats-part0-webidl.patch

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

::: dom/webidl/ResourceStats.webidl
@@ +8,5 @@
> +interface NetworkStatsData
> +{
> +  readonly attribute unsigned long long   receivedBytes;
> +  readonly attribute unsigned long long   sentBytes;
> +  readonly attribute DOMTimeStamp         timestamp;      // date of the record

s/date/timestamp/

@@ +15,5 @@
> +[JSImplementation="@mozilla.org/powerStatsData;1", Pref="dom.resourceStats.enabled"]
> +interface PowerStatsData
> +{
> +  readonly attribute unsigned long long   consumedPower;  // unit: mW
> +  readonly attribute DOMTimeStamp         timestamp;      // date of the record

s/date/timestamp/

@@ +35,5 @@
> +
> +  /**
> +   * Type of system service
> +   */
> +  readonly attribute SystemService? serviceType;

Nit: please document what null means here.

@@ +40,5 @@
> +
> +  /**
> +   * Manifest URL of the application.
> +   */
> +  readonly attribute DOMString?     manifestURL;

Nit: please document what null means here.

@@ +43,5 @@
> +   */
> +  readonly attribute DOMString?     manifestURL;
> +
> +  /**
> +   * Statistics, one element per day

Hmm, I wonder whether we should try to make getStats accept an interval argument instead of assuming per day here.  That way you could build an application which for example tells you what apps are using the most system resources at specific times during the day, etc.

@@ +51,5 @@
> +  /**
> +   * Date range
> +   */
> +  readonly attribute DOMTimeStamp   start;  // start date
> +  readonly attribute DOMTimeStamp   end;    // end date

s/date/timestamp/ in all of the above.

::: dom/webidl/ResourceStatsManager.webidl
@@ +72,5 @@
> +
> +[JSImplementation="@mozilla.org/resourceStatsAlarm;1", Pref="dom.resourceStats.enabled"]
> +interface ResourceStatsAlarm
> +{
> +  readonly attribute unsigned long          alarmId;

Why is this useful?

@@ +74,5 @@
> +interface ResourceStatsAlarm
> +{
> +  readonly attribute unsigned long          alarmId;
> +  readonly attribute ResourceType           type;
> +  readonly attribute any                    statsOptions; // ResourceStatsOptions

Why is this any?

@@ +93,5 @@
> +   * If |end| is null or undefined. retrieve the stats until the current time.
> +   *
> +   * If success, the |result| field keeps statistics.
> +   * For networkStatsManager, the |result| is an array of NetworkStats object.
> +   * For powerStatsManager, the |result| is an array of PowerStats object.

What is the |result| field that you mention here?

@@ +102,5 @@
> +
> +  /**
> +   * Clear resource statistics stored in database.
> +   *
> +   * |statsOptions| specifies the detial of statistics want to delete.

Nit: detail, and s/want //.

@@ +150,5 @@
> +   * Remove the specified alarm.
> +   *
> +   * |alarmId| specifies the alarm to be removed.
> +   */
> +  Promise removeAlarm(unsigned long alarmId);

Can't we just accept the alarm object here directly?

Comment 12

[Borting Chen [:borting]]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailacopolypse) from comment #10)

> > > Also, is there any reason why we would want to expose this API to
> > > non-certified apps?
> > 
> > No, only certified apps can access this API.
> > Did I miss anything? Or, should I include the change of PermissionTable in
> > this patch?
> 
> I think it would be nice if we use the annotations that Boris is adding in
> bug 958667.

Okay, I would add availableIn="CertifiedApps" annotation to the ResourceStatsManager interface.
Should I mark this annotation on other interfaces?
 
> > > @@ +169,5 @@
> > > > +   * Return supporting resource statistics
> > > > +   */
> > > > +  [Cached, Pure]
> > > > +  //static readonly attribute sequence<DOMString> resourceTypes;
> > > > +  readonly attribute sequence<DOMString> resourceTypes;
> > > 
> > > The semantics of this attribute is not completely clear to me.  Can you
> > > please clarify what this is supposed to return exactly, and whether it would
> > > be subject to the same considerations as ResourceStats.data?
> > 
> > This attribute will return the resource types that we supported (i.e.
> > "Network" and "Power").
> 
> So it's going to be a static array that will never change at runtime, right?

Yes, it is.

Comment 13

[(no longer active)]

Happy new year!  :-)

(In reply to Borting Chen [:borting] from comment #12)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailacopolypse) from comment #10)
> 
> > > > Also, is there any reason why we would want to expose this API to
> > > > non-certified apps?
> > > 
> > > No, only certified apps can access this API.
> > > Did I miss anything? Or, should I include the change of PermissionTable in
> > > this patch?
> > 
> > I think it would be nice if we use the annotations that Boris is adding in
> > bug 958667.
> 
> Okay, I would add availableIn="CertifiedApps" annotation to the
> ResourceStatsManager interface.
> Should I mark this annotation on other interfaces?

Yes please.

> > > > @@ +169,5 @@
> > > > > +   * Return supporting resource statistics
> > > > > +   */
> > > > > +  [Cached, Pure]
> > > > > +  //static readonly attribute sequence<DOMString> resourceTypes;
> > > > > +  readonly attribute sequence<DOMString> resourceTypes;
> > > > 
> > > > The semantics of this attribute is not completely clear to me.  Can you
> > > > please clarify what this is supposed to return exactly, and whether it would
> > > > be subject to the same considerations as ResourceStats.data?
> > > 
> > > This attribute will return the resource types that we supported (i.e.
> > > "Network" and "Power").
> > 
> > So it's going to be a static array that will never change at runtime, right?
> 
> Yes, it is.

OK, that's fine.

Comment 14

[Borting Chen [:borting]]

Hi Ehsan:

Happy Chinese New Year! :)

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailacopolypse) from comment #11)
> @@ +43,5 @@
> > +   */
> > +  readonly attribute DOMString?     manifestURL;
> > +
> > +  /**
> > +   * Statistics, one element per day
> 
> Hmm, I wonder whether we should try to make getStats accept an interval
> argument instead of assuming per day here.  That way you could build an
> application which for example tells you what apps are using the most system
> resources at specific times during the day, etc.

Yeh, that sounds interesting. Could you give some details about your idea?

I think there are some reasons that current networkStatsManager.getStats() returns a per-day record.
Maybe we can ask some comments from the gaia guys?

> ::: dom/webidl/ResourceStatsManager.webidl
> @@ +72,5 @@
> > +
> > +[JSImplementation="@mozilla.org/resourceStatsAlarm;1", Pref="dom.resourceStats.enabled"]
> > +interface ResourceStatsAlarm
> > +{
> > +  readonly attribute unsigned long          alarmId;
> 
> Why is this useful?

The |alarmId| is used for removeAlarm() method.
I define this attribute in the dictionary because I want to consist the return of getAlarms() with
that of mozAlarms.getAll() [1].
 
> @@ +74,5 @@
> > +interface ResourceStatsAlarm
> > +{
> > +  readonly attribute unsigned long          alarmId;
> > +  readonly attribute ResourceType           type;
> > +  readonly attribute any                    statsOptions; // ResourceStatsOptions
> 
> Why is this any?

I specified statsOptions because an attribute can not be of a dictionary type. [2]

I think one possible solution is to define ResourceStatsOptions as an interface, just like they have done
in Bug 876238. But I am afraid that doing so would disturb the flexibiliy we want when specigying parameters
for methods, such as getStats(), because of the sequence of parameters of ResourceStatsOptions's constructor.

For example, 
If we define ResourceStatsOptions as a dictionary, specifying parameters for getting an app's total pwoer 
consumption could be simple:

  var statsOptions = {manifestURL: "app://..."};
  var result = powerMgr.getStats(statsOptions, ...);

But, if we define ResourceStatsOptions as an interface as following:

  [Constructor(optional DOMString component, optional SystemService type, optional DOMString manifestURL), ...]

Specifying parameter could become a little complicated like this:

  var statsOptions = new ResourceStatsOptions(null, null, "app://...");
  var result = powerMgr.getStats(statsOptions, ...);

Any suggestion about this?

> @@ +93,5 @@
> > +   * If |end| is null or undefined. retrieve the stats until the current time.
> > +   *
> > +   * If success, the |result| field keeps statistics.
> > +   * For networkStatsManager, the |result| is an array of NetworkStats object.
> > +   * For powerStatsManager, the |result| is an array of PowerStats object.
> 
> What is the |result| field that you mention here?

Oops, my fault. The |result| field is what we used for DOMRequest.
The corrected comment should be something like:

  "If sucess, the fulfillment value is an array of ResourceStats object"

Is that okay?
 
> @@ +150,5 @@
> > +   * Remove the specified alarm.
> > +   *
> > +   * |alarmId| specifies the alarm to be removed.
> > +   */
> > +  Promise removeAlarm(unsigned long alarmId);
> 
> Can't we just accept the alarm object here directly?

It's okay for me, but I think it could be better if we can align these alarm setting methods with the 
MozAlarmsManager API [3]. What do you think?

[1] https://developer.mozilla.org/en-US/docs/Web/API/MozAlarmsManager.getAll#mozAlarm
[2] http://heycam.github.io/webidl/#idl-attributes
[3] https://developer.mozilla.org/en-US/docs/Web/API/MozAlarmsManager

Comment 15

[Borting Chen [:borting]]

Attachment: ResourceStats-part0-webidl.patch

Update Note:
1. fix nit and comment
2. Add new interface annotation: availableIn="CertifiedApps"
   (blocked by Bug 958667)

Comment 16

[Borting Chen [:borting]]

Attachment: ResourceStats-part0-webidl.patch

Update Note:
1. Add AvailableIn="CertifiedApps" annotation to all interfaces due to Bug 958667 is resolved

Comment 17

[(no longer active)]

(In reply to Borting Chen [:borting] from comment #14)
> > Hmm, I wonder whether we should try to make getStats accept an interval
> > argument instead of assuming per day here.  That way you could build an
> > application which for example tells you what apps are using the most system
> > resources at specific times during the day, etc.
> 
> Yeh, that sounds interesting. Could you give some details about your idea?
> 
> I think there are some reasons that current networkStatsManager.getStats()
> returns a per-day record.
> Maybe we can ask some comments from the gaia guys?

I'm not sure if it's actually needed.  Do you know who on the gaia side knows about the usage sites of this API?

> > ::: dom/webidl/ResourceStatsManager.webidl
> > @@ +72,5 @@
> > > +
> > > +[JSImplementation="@mozilla.org/resourceStatsAlarm;1", Pref="dom.resourceStats.enabled"]
> > > +interface ResourceStatsAlarm
> > > +{
> > > +  readonly attribute unsigned long          alarmId;
> > 
> > Why is this useful?
> 
> The |alarmId| is used for removeAlarm() method.
> I define this attribute in the dictionary because I want to consist the
> return of getAlarms() with
> that of mozAlarms.getAll() [1].

Hmm, I notice that the ScheduledTask interface in the sysapps spec for alarms uses a string identifier.  This is a mess.  :(  Marcos, what is our best practices these days for this kind of thing?  Do you advise the usage of dummy integer/string IDs or passing the actual object to the removeAlarm() method?

> > @@ +74,5 @@
> > > +interface ResourceStatsAlarm
> > > +{
> > > +  readonly attribute unsigned long          alarmId;
> > > +  readonly attribute ResourceType           type;
> > > +  readonly attribute any                    statsOptions; // ResourceStatsOptions
> > 
> > Why is this any?
> 
> I specified statsOptions because an attribute can not be of a dictionary
> type. [2]

Sure, but why not just expose component, serviceType and manifestURL directly on ResourceStatsAlarm?  We don't need a dictionary for that, right?

> > @@ +93,5 @@
> > > +   * If |end| is null or undefined. retrieve the stats until the current time.
> > > +   *
> > > +   * If success, the |result| field keeps statistics.
> > > +   * For networkStatsManager, the |result| is an array of NetworkStats object.
> > > +   * For powerStatsManager, the |result| is an array of PowerStats object.
> > 
> > What is the |result| field that you mention here?
> 
> Oops, my fault. The |result| field is what we used for DOMRequest.
> The corrected comment should be something like:
> 
>   "If sucess, the fulfillment value is an array of ResourceStats object"
> 
> Is that okay?

Yes!

> > @@ +150,5 @@
> > > +   * Remove the specified alarm.
> > > +   *
> > > +   * |alarmId| specifies the alarm to be removed.
> > > +   */
> > > +  Promise removeAlarm(unsigned long alarmId);
> > 
> > Can't we just accept the alarm object here directly?
> 
> It's okay for me, but I think it could be better if we can align these alarm
> setting methods with the 
> MozAlarmsManager API [3]. What do you think?

Let's see what Marcos thinks.

Comment 18

[Marcos Caceres [:marcosc]]

> hmm, I notice that the ScheduledTask interface in the sysapps spec for alarms uses a string identifier.  This is a mess.  :(  Marcos, what is our best practices these days for this kind of thing?  Do you advise the usage of dummy integer/string IDs or passing the actual object to the removeAlarm() method?

I would send the actual object. If you have the object, then you might as well use it.

However, it seems that MozAlarmsManager already sets a precedence to use an ID. Let's not make things even more messy by introducing a new way. 

On the W3C side, we will eventually work it out... but those APIs won't be compatible with any of the FxOS APIs anyway.

Comment 19

[(no longer active)]

(In reply to comment #18)
> > hmm, I notice that the ScheduledTask interface in the sysapps spec for alarms uses a string identifier.  This is a mess.  :(  Marcos, what is our best practices these days for this kind of thing?  Do you advise the usage of dummy integer/string IDs or passing the actual object to the removeAlarm() method?
> 
> I would send the actual object. If you have the object, then you might as well
> use it.

Yeah, that was my thinking as well.

> However, it seems that MozAlarmsManager already sets a precedence to use an ID.
> Let's not make things even more messy by introducing a new way. 

OK, fair enough.

> On the W3C side, we will eventually work it out... but those APIs won't be
> compatible with any of the FxOS APIs anyway.

Yep, understood.  Thanks!

Comment 20

[Borting Chen [:borting]]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailacopolypse) from comment #17)
> (In reply to Borting Chen [:borting] from comment #14)
> > > Hmm, I wonder whether we should try to make getStats accept an interval
> > > argument instead of assuming per day here.  That way you could build an
> > > application which for example tells you what apps are using the most system
> > > resources at specific times during the day, etc.
> > 
> > Yeh, that sounds interesting. Could you give some details about your idea?
> > 
> > I think there are some reasons that current networkStatsManager.getStats()
> > returns a per-day record.
> > Maybe we can ask some comments from the gaia guys?
> 
> I'm not sure if it's actually needed.  Do you know who on the gaia side
> knows about the usage sites of this API?

Maybe we can ask Salvador.

Hi Salvador, we had several questions about the usage of NetworkStats API.
1. Is there any reason that we return stats in a per-day manner?
2. we also wonder whether we can add something to the getStats() method of ResourceStats (not the one of ResourceStatsManager) to enrich its support to Gaia Apps. Could you give some suggestion about this?


> > > @@ +74,5 @@
> > > > +interface ResourceStatsAlarm
> > > > +{
> > > > +  readonly attribute unsigned long          alarmId;
> > > > +  readonly attribute ResourceType           type;
> > > > +  readonly attribute any                    statsOptions; // ResourceStatsOptions
> > > 
> > > Why is this any?
> > 
> > I specified statsOptions because an attribute can not be of a dictionary
> > type. [2]
> 
> Sure, but why not just expose component, serviceType and manifestURL
> directly on ResourceStatsAlarm?  We don't need a dictionary for that, right?

Yep, that is a good idea. Thanks.

Comment 21

[Borting Chen [:borting]]

Attachment: ResourceStats-part0-webidl.patch

Update Note:
1. Replace statsOptions of ResourceStatsAlarm interface with component, serverType and manifestURL.

Comment 22

[Borting Chen [:borting]]

Attachment: ResourceStats-part0-webidl.patch

Update Note:
1. Replace statsOptions of ResourceStatsAlarm interface with component, serverType and manifestURL.

Comment 23

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

(In reply to Borting Chen [:borting] from comment #20)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailacopolypse) from comment #17)
> > (In reply to Borting Chen [:borting] from comment #14)
> > > > Hmm, I wonder whether we should try to make getStats accept an interval
> > > > argument instead of assuming per day here.  That way you could build an
> > > > application which for example tells you what apps are using the most system
> > > > resources at specific times during the day, etc.
> > > 
> > > Yeh, that sounds interesting. Could you give some details about your idea?
> > > 
> > > I think there are some reasons that current networkStatsManager.getStats()
> > > returns a per-day record.
> > > Maybe we can ask some comments from the gaia guys?
> > 
> > I'm not sure if it's actually needed.  Do you know who on the gaia side
> > knows about the usage sites of this API?
> 
> Maybe we can ask Salvador.
> 
> Hi Salvador, we had several questions about the usage of NetworkStats API.
> 1. Is there any reason that we return stats in a per-day manner?

Apart of the increase in size in the back-end DB, I think there is no technical impediment for not supporting more accurate intervals, let's ask Albert who is in charge of the back-end.

> 2. we also wonder whether we can add something to the getStats() method of
> ResourceStats (not the one of ResourceStatsManager) to enrich its support to
> Gaia Apps. Could you give some suggestion about this?


Excure me, I dont understand you here? What do you mean with `enrich its support to Gaia Apps` and what do you mean with `something`? Thanks!
> 
> 
> > > > @@ +74,5 @@
> > > > > +interface ResourceStatsAlarm
> > > > > +{
> > > > > +  readonly attribute unsigned long          alarmId;
> > > > > +  readonly attribute ResourceType           type;
> > > > > +  readonly attribute any                    statsOptions; // ResourceStatsOptions
> > > > 
> > > > Why is this any?
> > > 
> > > I specified statsOptions because an attribute can not be of a dictionary
> > > type. [2]
> > 
> > Sure, but why not just expose component, serviceType and manifestURL
> > directly on ResourceStatsAlarm?  We don't need a dictionary for that, right?
> 
> Yep, that is a good idea. Thanks.

I think it is better if the returned Alarm matches the format to request one. But it is just a suggestion.

Comment 24

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

Albert, do you know if there is some technical impediment preventing us from monitoring data usage at more rate than one day rate? Thank you!

Comment 25

[Borting Chen [:borting]]

> > 2. we also wonder whether we can add something to the getStats() method of
> > ResourceStats (not the one of ResourceStatsManager) to enrich its support to
> > Gaia Apps. Could you give some suggestion about this?
> 
> 
> Excure me, I dont understand you here? What do you mean with `enrich its
> support to Gaia Apps` and what do you mean with `something`? Thanks!

Oops, I should explain this in more detail.
We thought about adding a new parameter for getStats() method of ResourceStats in order to provide more stats retrieving options.
For example,
  var powerMger = new ResourceStatsManager("power");
  var resStats = powerMgr.getStats()
  resStats.getStats("max");   // returns the largest consumption.
  resStats.getStats("sort");  // returns a sorted array according to the consumption

Is this could help gaia implementation?
Or, returning an array containing all stats is enough.

Comment 26

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

Hi guys!

I was looking at the entire thread and I think I understand your idea. You are replacing `data` attribute by `getStats()` function in order to allow further back-end digest as that you're suggesting such like max, sort...

Well I like the idea very much but here are my thoughts. I would like to listen for Albert's opinion as well.

As Gaia developer I asked once for my data so I don't want more asynchronous calls than needed. With the complete list of the stats is enough but sometimes, if the data is so big, it could be great if some data-related operations could happen in the back-end. Even more, it would be great if I could ask for a cursor to pass through great amounts of data so, my proposal (just a draft):

Instead of `getStats()` method, you can have a `stats` attribute which is an interface with the following methods:

  .getAll() -> Returns an array of stats.
  .getCursor() -> A promose with a cursor to pass through the collection-
  .sortedBy() -> A promise with a cursor to pass through the sorted collection.
  .max() / min() -> To get max / min values.

...and so on. Notice this would depend on each kind of stats object so some operations could not be supported and other could require parameters.

We could support chainability here: stats.sortedBy('rxBytes').where(function (s) { return s.rxBytes > 1000; }).then(...); Or maybe this is only dreaming ;)

This is a complex change and we could start by simply implement the `getAll()` and `getCursor()` methods which should be universal and agnostic. Indeed, may we should discuss in the mailing list and simply return to `data` until we agree? I think front-end post-processing is enough by now.

Oh, and finally, I think this API should be privileged: it is ok we should not allow or discourage stealing usage stats from the device but If some developer want to perform big data processing and he is able to do it better than us I don't see the reason to prevent the user for trusting this app.

WDYT?

Comment 27

[(no longer active)]

(In reply to comment #26)
> Hi guys!
> 
> I was looking at the entire thread and I think I understand your idea. You are
> replacing `data` attribute by `getStats()` function in order to allow further
> back-end digest as that you're suggesting such like max, sort...
> 
> Well I like the idea very much but here are my thoughts. I would like to listen
> for Albert's opinion as well.

Yes, absolutely.

> As Gaia developer I asked once for my data so I don't want more asynchronous
> calls than needed. With the complete list of the stats is enough but sometimes,
> if the data is so big, it could be great if some data-related operations could
> happen in the back-end. Even more, it would be great if I could ask for a
> cursor to pass through great amounts of data so, my proposal (just a draft):
> 
> Instead of `getStats()` method, you can have a `stats` attribute which is an
> interface with the following methods:
> 
>   .getAll() -> Returns an array of stats.
>   .getCursor() -> A promose with a cursor to pass through the collection-
>   .sortedBy() -> A promise with a cursor to pass through the sorted collection.
>   .max() / min() -> To get max / min values.
> 
> ...and so on. Notice this would depend on each kind of stats object so some
> operations could not be supported and other could require parameters.
> 
> We could support chainability here: stats.sortedBy('rxBytes').where(function
> (s) { return s.rxBytes > 1000; }).then(...); Or maybe this is only dreaming ;)

I think we're dangerously close to reinventing SQL at this point.  :-)

I think we should try to think in terms of the specific use cases of this API, and try to design the data access interface around those use cases, not the generic use case of obtaining arbitrary information out of the database.

If, however, you think that most apps will want to use this data in ways that we cannot easily predicts (for example, "sorted data where the consumption is above 1000 bytes", etc.) then perhaps the right thing to do is to avoid exposing the data here and instead provide it through a DataStore which individual apps can access and index in whatever way interests them.

> This is a complex change and we could start by simply implement the `getAll()`
> and `getCursor()` methods which should be universal and agnostic. Indeed, may
> we should discuss in the mailing list and simply return to `data` until we
> agree? I think front-end post-processing is enough by now.

That has basically been my assumption so far, but please see above.  It's important for us to agree what we want to design here.

> Oh, and finally, I think this API should be privileged: it is ok we should not
> allow or discourage stealing usage stats from the device but If some developer
> want to perform big data processing and he is able to do it better than us I
> don't see the reason to prevent the user for trusting this app.

I think we would like this to be a certified only API for now.  This is privacy sensitive data, so exposing it to anything other than certified applications will have privacy implications which should probably be addressed separately.

Comment 28

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

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailacopolypse) from comment #27)
> I think we're dangerously close to reinventing SQL at this point.  :-)

Yes, you're right and therefore, I would push for returning to the `data` alternative as an attribute (not a method) being a sequence of samples. 

> I think we should try to think in terms of the specific use cases of this
> API, and try to design the data access interface around those use cases, not
> the generic use case of obtaining arbitrary information out of the database.

Agree, given my experience I see no need to provide aggregation or sorting in back-end right now. Anyway, it could be great to discuss about this option for future versions.

> If, however, you think that most apps will want to use this data in ways
> that we cannot easily predicts (for example, "sorted data where the
> consumption is above 1000 bytes", etc.) then perhaps the right thing to do
> is to avoid exposing the data here and instead provide it through a
> DataStore which individual apps can access and index in whatever way
> interests them.

Yes, you're right but I don't see a need here. Most If we don't expose this as a Privileged API.

> > This is a complex change and we could start by simply implement the `getAll()`
> > and `getCursor()` methods which should be universal and agnostic. Indeed, may
> > we should discuss in the mailing list and simply return to `data` until we
> > agree? I think front-end post-processing is enough by now.
> 
> That has basically been my assumption so far, but please see above.  It's
> important for us to agree what we want to design here.

Basically, I think we agree here.
 
> > Oh, and finally, I think this API should be privileged: it is ok we should not
> > allow or discourage stealing usage stats from the device but If some developer
> > want to perform big data processing and he is able to do it better than us I
> > don't see the reason to prevent the user for trusting this app.
> 
> I think we would like this to be a certified only API for now.  This is
> privacy sensitive data, so exposing it to anything other than certified
> applications will have privacy implications which should probably be
> addressed separately.

IMHO, we are exposing much more dangerous APIs right now such as:

 * Contacts, that allows to send correlated sensitive data.
 * Device storage, which virtually allow to read all data in your SD card.
 * Keyboard, which allow to implement a keylogger.
 * Mobile Network, which allow to read wich network you are and this is sensitive data as well.
 * systemXHR, well, guess it.
 * tcpSOCKET, even more harmfull than systemXHR. 

So, respecting data retrieval, I think there are more dangerous alternatives out there than a read-only historical of anonymous data as the app trying to correlate this info with a specific id should first ask the user for joining a service or something like that.

WDYT?

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't know that users have asked for 3rd party apps that display this type of usage statistics. So I'd rather keep the API certified for now so that we can change around the API more without having to worry about backwards compatibility.

There's really very little useful that a 3rd party app could do with this API. You can't even get the names and icons of the applications that you have data for.

Once we release the API to 3rd parties we'll have much less ability to fix problems in the API and especially do the big changes that we've been debating in this bug.

Comment 30

[Albert [:albert]]

(In reply to Salvador de la Puente González [:salva] from comment #24)
> Albert, do you know if there is some technical impediment preventing us from
> monitoring data usage at more rate than one day rate? Thank you!

As Salvador said, the main reason to store daily samples is the size of the database. At the beginning, Jonas was concerned about it and after making some estimations based on the samplerate and the 'time to live' of the samples we decided to set daily samplerate and store samples for 6 months. However, both parameters in the current implementation can be modified so it won't be a problem if you decide to change it.

Comment 31

[Borting Chen [:borting]]

(In reply to Albert [:albert] from comment #30)
> (In reply to Salvador de la Puente González [:salva] from comment #24)
> > Albert, do you know if there is some technical impediment preventing us from
> > monitoring data usage at more rate than one day rate? Thank you!
> 
> As Salvador said, the main reason to store daily samples is the size of the
> database. At the beginning, Jonas was concerned about it and after making
> some estimations based on the samplerate and the 'time to live' of the
> samples we decided to set daily samplerate and store samples for 6 months.
> However, both parameters in the current implementation can be modified so it
> won't be a problem if you decide to change it.

Understood, thanks.

Comment 32

[(no longer active)]

(In reply to Salvador de la Puente González [:salva] from comment #28)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailacopolypse) from comment #27)
> > I think we're dangerously close to reinventing SQL at this point.  :-)
> 
> Yes, you're right and therefore, I would push for returning to the `data`
> alternative as an attribute (not a method) being a sequence of samples. 

Please see the last part of <https://groups.google.com/d/msg/mozilla.dev.webapi/tWkgbD1v_Gg/SKbRP_fC88wJ> on the background on why we chose to use a method here.

> > I think we should try to think in terms of the specific use cases of this
> > API, and try to design the data access interface around those use cases, not
> > the generic use case of obtaining arbitrary information out of the database.
> 
> Agree, given my experience I see no need to provide aggregation or sorting
> in back-end right now. Anyway, it could be great to discuss about this
> option for future versions.

Agreed!

> > If, however, you think that most apps will want to use this data in ways
> > that we cannot easily predicts (for example, "sorted data where the
> > consumption is above 1000 bytes", etc.) then perhaps the right thing to do
> > is to avoid exposing the data here and instead provide it through a
> > DataStore which individual apps can access and index in whatever way
> > interests them.
> 
> Yes, you're right but I don't see a need here. Most If we don't expose this
> as a Privileged API.

Yes, we're planning to expose this only to certified apps for now.

> > > This is a complex change and we could start by simply implement the `getAll()`
> > > and `getCursor()` methods which should be universal and agnostic. Indeed, may
> > > we should discuss in the mailing list and simply return to `data` until we
> > > agree? I think front-end post-processing is enough by now.
> > 
> > That has basically been my assumption so far, but please see above.  It's
> > important for us to agree what we want to design here.
> 
> Basically, I think we agree here.
>  
> > > Oh, and finally, I think this API should be privileged: it is ok we should not
> > > allow or discourage stealing usage stats from the device but If some developer
> > > want to perform big data processing and he is able to do it better than us I
> > > don't see the reason to prevent the user for trusting this app.
> > 
> > I think we would like this to be a certified only API for now.  This is
> > privacy sensitive data, so exposing it to anything other than certified
> > applications will have privacy implications which should probably be
> > addressed separately.
> 
> IMHO, we are exposing much more dangerous APIs right now such as:
> 
>  * Contacts, that allows to send correlated sensitive data.
>  * Device storage, which virtually allow to read all data in your SD card.
>  * Keyboard, which allow to implement a keylogger.
>  * Mobile Network, which allow to read wich network you are and this is
> sensitive data as well.
>  * systemXHR, well, guess it.
>  * tcpSOCKET, even more harmfull than systemXHR. 
> 
> So, respecting data retrieval, I think there are more dangerous alternatives
> out there than a read-only historical of anonymous data as the app trying to
> correlate this info with a specific id should first ask the user for joining
> a service or something like that.

I don't think that once we expose one potentially dangerous API, we should use it as a precedence to expose more.  Also, see Jonas' comment.  I would prefer to focus this to certified apps only for now, and work on improving the API once we get feedback from gaia based on the usage of the API, and wait for third-party app developers to ask for this API before considering exposing it to privileged apps.

Comment 33

[(no longer active)]

Comment on attachment 8372008 [details] [diff] [review]
ResourceStats-part0-webidl.patch

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

Thanks a lot for your perseverance here!  I think we ended up with a good API here.  Much appreciated!

::: dom/webidl/ResourceStats.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +[JSImplementation="@mozilla.org/networkStatsData;1", Pref="dom.resourceStats.enabled", AvailableIn="CertifiedApps"]

Nit: we usually name prefs without camelCasing (although there are counter examples.)  Maybe call this dom.resource_stats.enabled?

@@ +28,5 @@
> +  readonly attribute ResourceType   type;
> +
> +  /**
> +   * The component that statistics belongs to.
> +   * If null, sum of all components' statistics are returned.

Perhaps clarify this by saying that this will be null if the ResourceStatsOptions.component argument passed to getStats() is null?

@@ +43,5 @@
> +   * If |manifestURL| is not null, then getStats() returns statistics of
> +   * that application.
> +   *
> +   * If |serviceType| is not null, then getStats returns statistics of
> +   * that system service.

Same for these two attributes.

@@ +55,5 @@
> +
> +  /**
> +   * Statistics, one element per day
> +   */
> +  sequence<(NetworkStatsData or PowerStatsData)> getStats();

Not sure why I did not notice this before, but now we have ended up with two getStats methods, which do different things.  I think that's confusing, so let's rename one of them.  My suggestion is to rename this one to getData().  I'm open to other suggestions if you think that's not a good name.

::: dom/webidl/ResourceStatsManager.webidl
@@ +78,5 @@
> +  readonly attribute DOMString              component;
> +  readonly attribute SystemService          serviceType;
> +  readonly attribute DOMString              manifestURL;
> +  readonly attribute unsigned long long     threshold;
> +  readonly attribute any                    data;

Nit: please add some docs for these members.

@@ +168,5 @@
> +
> +  /**
> +   * Return supporting resource statistics, i.e. ["Network", "Power"]
> +   *
> +   * This should be specified as static attribute after Bug 863952 is resolved.

FWIW I think we should fix bug 863952 before landing the implementation of this API and make these attributes static when landing this feature.  It's semantically incorrect to leave them as non-static.

Comment 34

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

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #32) 
> I don't think that once we expose one potentially dangerous API, we should
> use it as a precedence to expose more.  Also, see Jonas' comment.  I would
> prefer to focus this to certified apps only for now, and work on improving
> the API once we get feedback from gaia based on the usage of the API, and
> wait for third-party app developers to ask for this API before considering
> exposing it to privileged apps.

Indeed, I think we expose them because they are useful. So I don't see a security concern here. I agree with Jonas about if there is no demand, then, there is no need but I want to point you to a thread in the WebAPI mailing list called "Making Power Management and Wifi APIs (at least partically) available to privileged APIs".

May it is not referring to these APIs but it could be a first step.

Anyway, despite I think it could have great potential to expose this API, I should look for real apps. And, of course, exposing an API makes its evolution more complicated but I remind you this is experimental technology and susceptible to change.

Comment 35

[(no longer active)]

(In reply to comment #34)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailapocalypse) from comment #32) 
> > I don't think that once we expose one potentially dangerous API, we should
> > use it as a precedence to expose more.  Also, see Jonas' comment.  I would
> > prefer to focus this to certified apps only for now, and work on improving
> > the API once we get feedback from gaia based on the usage of the API, and
> > wait for third-party app developers to ask for this API before considering
> > exposing it to privileged apps.
> 
> Indeed, I think we expose them because they are useful. So I don't see a
> security concern here. I agree with Jonas about if there is no demand, then,
> there is no need but I want to point you to a thread in the WebAPI mailing list
> called "Making Power Management and Wifi APIs (at least partically) available
> to privileged APIs".
> 
> May it is not referring to these APIs but it could be a first step.

I think that post is talking about the wake lock API.

> Anyway, despite I think it could have great potential to expose this API, I
> should look for real apps. And, of course, exposing an API makes its evolution
> more complicated but I remind you this is experimental technology and
> susceptible to change.

Agreed!

Comment 36

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

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #32)
> (In reply to Salvador de la Puente González [:salva] from comment #28)
> > (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> > emailacopolypse) from comment #27)
> > > I think we're dangerously close to reinventing SQL at this point.  :-)
> > 
> > Yes, you're right and therefore, I would push for returning to the `data`
> > alternative as an attribute (not a method) being a sequence of samples. 
> 
> Please see the last part of
> <https://groups.google.com/d/msg/mozilla.dev.webapi/tWkgbD1v_Gg/
> SKbRP_fC88wJ> on the background on why we chose to use a method here.

Sorry, but it is not clear to me why this is not an attribute. I don't understand why the developer won't expect the statistics when asking for statistics. I mean, this use is strange to me:

var manager = new ResourceStatsManager('power');
manager.getStats().then(function(result) {
  var sampleCollection = result.getStats();
});

As you note, you are asking for stats twice. This seems quite unnatural to me. IMHO, this feels more friendly:

var manager = new ResourceStatsManager('power');
manager.getStats().then(function(result) {
  var sampleCollection = result.samples;
});

Notice ResourceStats is a return type, not the main interface which you use to get the historical data.

WDYT?

Comment 37

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

Borting, here, about this IDL:

+  /**
+   * Query resource statistics.
+   *
+   * |statsOptions| specifies the detail of statistics of interest.
+   *
+   * |start| and |end| specifies the time range of interest, both included.
+   * If |start| is null or undefined, retrieve the stats since measurements.
+   * If |end| is null or undefined. retrieve the stats until the current time.
+   *
+   * If success, the fulfillment value is an array of ResourceStats object.
+   */
+  Promise getStats(optional ResourceStatsOptions statsOptions,
+                   optional DOMTimeStamp? start = null,
+                   optional DOMTimeStamp? end = null);

I don't understand the line you're saying "the fulfillment value is an **array** of ResourceStats (...)". Indeed, the result is only one ResourceStats object. Inside you will find the current getStats() method with the historical samples. Is not it? Could you clarify me under which circumstances the fulfillment value could be an array, please?

Comment 38

[Borting Chen [:borting]]

(In reply to Salvador de la Puente González [:salva] from comment #37)
> Borting, here, about this IDL:
> 
> +  /**
> +   * Query resource statistics.
> +   *
> +   * |statsOptions| specifies the detail of statistics of interest.
> +   *
> +   * |start| and |end| specifies the time range of interest, both included.
> +   * If |start| is null or undefined, retrieve the stats since measurements.
> +   * If |end| is null or undefined. retrieve the stats until the current
> time.
> +   *
> +   * If success, the fulfillment value is an array of ResourceStats object.
> +   */
> +  Promise getStats(optional ResourceStatsOptions statsOptions,
> +                   optional DOMTimeStamp? start = null,
> +                   optional DOMTimeStamp? end = null);
> 
> I don't understand the line you're saying "the fulfillment value is an
> **array** of ResourceStats (...)". Indeed, the result is only one
> ResourceStats object. Inside you will find the current getStats() method
> with the historical samples. Is not it? Could you clarify me under which
> circumstances the fulfillment value could be an array, please?

In the original design of PowerStats API, we return an array of stats of applications when specifying the manifestURL to null. This is because we thought that sometimes user may want to see a list showing the power consumption of each application. But, you are right, according to our current design, getStats() method would only return "one" ResourceStats object no matter what option is specified. Thank you for pointing out my mistake. :-)

Comment 39

[Borting Chen [:borting]]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #33)
> Comment on attachment 8372008 [details] [diff] [review]
> ResourceStats-part0-webidl.patch
> 
> Review of attachment 8372008 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks a lot for your perseverance here!  I think we ended up with a good
> API here.  Much appreciated!

Thank you for all your assistance, too.

> ::: dom/webidl/ResourceStats.webidl
> @@ +3,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/.
> > + */
> > +
> > +[JSImplementation="@mozilla.org/networkStatsData;1", Pref="dom.resourceStats.enabled", AvailableIn="CertifiedApps"]
> 
> Nit: we usually name prefs without camelCasing (although there are counter
> examples.)  Maybe call this dom.resource_stats.enabled?

Okay.

> @@ +28,5 @@
> > +  readonly attribute ResourceType   type;
> > +
> > +  /**
> > +   * The component that statistics belongs to.
> > +   * If null, sum of all components' statistics are returned.
> 
> Perhaps clarify this by saying that this will be null if the
> ResourceStatsOptions.component argument passed to getStats() is null?
> 
> @@ +43,5 @@
> > +   * If |manifestURL| is not null, then getStats() returns statistics of
> > +   * that application.
> > +   *
> > +   * If |serviceType| is not null, then getStats returns statistics of
> > +   * that system service.
> 
> Same for these two attributes.

Understood, thanks.

> @@ +55,5 @@
> > +
> > +  /**
> > +   * Statistics, one element per day
> > +   */
> > +  sequence<(NetworkStatsData or PowerStatsData)> getStats();
> 
> Not sure why I did not notice this before, but now we have ended up with two
> getStats methods, which do different things.  I think that's confusing, so
> let's rename one of them.  My suggestion is to rename this one to getData().
> I'm open to other suggestions if you think that's not a good name.

I like this one. Thanks.

> @@ +168,5 @@
> > +
> > +  /**
> > +   * Return supporting resource statistics, i.e. ["Network", "Power"]
> > +   *
> > +   * This should be specified as static attribute after Bug 863952 is resolved.
> 
> FWIW I think we should fix bug 863952 before landing the implementation of
> this API and make these attributes static when landing this feature.  It's
> semantically incorrect to leave them as non-static.

Agreed. But I have no idea about that bug right now :-(

Comment 40

[Borting Chen [:borting]]

Attachment: ResourceStats-part0-webidl.patch

Update Note:
1. Fixed nits
2. Replaced ResouceStats.getStats() with ResouceStats.getData() to avoid confusing
3. Added comment for ResouceStatsAlarm
4. Corrected the comment of ResouceStatsManager.getStats(), which returns one ResouceStats object instead of an array of that.

Comment 41

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

Please, Ehsan, could you take a look at comment #36. Don't you agree this syntax is a little bit weird?

Comment 42

[(no longer active)]

Sorry that I overlooked this comment.

(In reply to Salvador de la Puente González [:salva] from comment #36)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailapocalypse) from comment #32)
> > (In reply to Salvador de la Puente González [:salva] from comment #28)
> > > (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> > > emailacopolypse) from comment #27)
> > > > I think we're dangerously close to reinventing SQL at this point.  :-)
> > > 
> > > Yes, you're right and therefore, I would push for returning to the `data`
> > > alternative as an attribute (not a method) being a sequence of samples. 
> > 
> > Please see the last part of
> > <https://groups.google.com/d/msg/mozilla.dev.webapi/tWkgbD1v_Gg/
> > SKbRP_fC88wJ> on the background on why we chose to use a method here.
> 
> Sorry, but it is not clear to me why this is not an attribute. I don't
> understand why the developer won't expect the statistics when asking for
> statistics. I mean, this use is strange to me:
> 
> var manager = new ResourceStatsManager('power');
> manager.getStats().then(function(result) {
>   var sampleCollection = result.getStats();
> });
> 
> As you note, you are asking for stats twice. This seems quite unnatural to
> me. IMHO, this feels more friendly:
> 
> var manager = new ResourceStatsManager('power');
> manager.getStats().then(function(result) {
>   var sampleCollection = result.samples;
> });
> 
> Notice ResourceStats is a return type, not the main interface which you use
> to get the historical data.

As it was stated in the dev-webapi post I linked to, it's bad practice to expose arrays that can change their values depending on things outside of the DOM calls that the script makes as an array, because people expect that attributes retain the same value unless you make a DOM call to change them.  For example, if this were an attribute, you could write code like:

  if (result.samples[0] == foo) {
    report(result.samples[0]);
  }

Which would be semantically different than:

  var firstSample = result.samples[0];
  if (firstSample == foo) {
    report(firstSample);
  }

The reason is that the samples attribute could return a different array the second time that it's called (during the call to the report function.)  With a function call, however, people don't assume that the return value will stay the same, therefore the chances of this kind of error are minimized.

Also note that we renamed ResourceStats.getStats() to getData to make the naming less confusing.

Comment 43

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

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #42)
> Sorry that I overlooked this comment.
> As it was stated in the dev-webapi post I linked to, it's bad practice to
> expose arrays that can change their values depending on things outside of
> the DOM calls that the script makes as an array, because people expect that
> attributes retain the same value unless you make a DOM call to change them. 

Understood. But notice you are doing this twice. When you create a new ResourceStatsManager, you're not accessing an attribute with the historical data. You're actually calling a .getStats() method which return a complex type wrapping the historical data. Why to delay again the access to samples by introducing a synchronous .getData() method to this return type.

> Also note that we renamed ResourceStats.getStats() to getData to make the
> naming less confusing.

Yep, ok, it's more clear to me. Anyway, I would fully understand your point of view if we provide some like .getData(i) where i is the i-th element among the samples. From a pragmatic point of view, providing .getData() which, indeed, returns an array is just a delaying step.

Anyway, thanks for the clarification. I'm only pushing because I'm one of the consumers of this API (if not the only one right now xP)

Comment 44

[(no longer active)]

(In reply to comment #43)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailapocalypse) from comment #42)
> > Sorry that I overlooked this comment.
> > As it was stated in the dev-webapi post I linked to, it's bad practice to
> > expose arrays that can change their values depending on things outside of
> > the DOM calls that the script makes as an array, because people expect that
> > attributes retain the same value unless you make a DOM call to change them. 
> 
> Understood. But notice you are doing this twice. When you create a new
> ResourceStatsManager, you're not accessing an attribute with the historical
> data. You're actually calling a .getStats() method which return a complex type
> wrapping the historical data. Why to delay again the access to samples by
> introducing a synchronous .getData() method to this return type.

Not sure why you think there will be a delay.  getData should return immediately.

> > Also note that we renamed ResourceStats.getStats() to getData to make the
> > naming less confusing.
> 
> Yep, ok, it's more clear to me. Anyway, I would fully understand your point of
> view if we provide some like .getData(i) where i is the i-th element among the
> samples. From a pragmatic point of view, providing .getData() which, indeed,
> returns an array is just a delaying step.

Hmm, I hope that the i in my example did not confuse you.  The issue was that the two accesses to the |samples| property (i.e., the |result.samples| getter in the example code) return different array objects.  The rest, including the |[0]| I added to demonstrate how this can manifest itself in real code.

> Anyway, thanks for the clarification. I'm only pushing because I'm one of the
> consumers of this API (if not the only one right now xP)

I hope that the problem we're trying to solve by using a method is clear to you now.  If not, please let me know where I should elaborate.  Thanks!

Comment 45

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

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #44)
> 
> Not sure why you think there will be a delay.  getData should return
> immediately.

Yes but, notice, it is like saying:

- Developer: Hey DOM, can you give me the statistics of Power usage?
- DOM: Yep, give me a moment... here you have, now ask Result.
- Developer: Ok, now, Result, can you give me the statistics I already asked DOM for?
- Results: sure, here you have.
 
> Hmm, I hope that the i in my example did not confuse you.  The issue was
> that the two accesses to the |samples| property (i.e., the |result.samples|
> getter in the example code) return different array objects.  The rest,
> including the |[0]| I added to demonstrate how this can manifest itself in
> real code.
> 
> > Anyway, thanks for the clarification. I'm only pushing because I'm one of the
> > consumers of this API (if not the only one right now xP)
> 
> I hope that the problem we're trying to solve by using a method is clear to
> you now.  If not, please let me know where I should elaborate.  Thanks!

Maybe I can not explain myself with clarity or maybe I don't understand the problem. I will be on IRC, we could talk there to avoid introducing noise.

Thank you anyway.

Comment 46

[(no longer active)]

(In reply to comment #45)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailapocalypse) from comment #44)
> > 
> > Not sure why you think there will be a delay.  getData should return
> > immediately.
> 
> Yes but, notice, it is like saying:
> 
> - Developer: Hey DOM, can you give me the statistics of Power usage?
> - DOM: Yep, give me a moment... here you have, now ask Result.
> - Developer: Ok, now, Result, can you give me the statistics I already asked
> DOM for?
> - Results: sure, here you have.

So by delay here do you mean an actual time delay?  Or just another step to get to the desired information?  If the latter, how is a method different with an attribute?  You need to access the attribute or call the method to get to the desired information at any rate, right?

> > Hmm, I hope that the i in my example did not confuse you.  The issue was
> > that the two accesses to the |samples| property (i.e., the |result.samples|
> > getter in the example code) return different array objects.  The rest,
> > including the |[0]| I added to demonstrate how this can manifest itself in
> > real code.
> > 
> > > Anyway, thanks for the clarification. I'm only pushing because I'm one of the
> > > consumers of this API (if not the only one right now xP)
> > 
> > I hope that the problem we're trying to solve by using a method is clear to
> > you now.  If not, please let me know where I should elaborate.  Thanks!
> 
> Maybe I can not explain myself with clarity or maybe I don't understand the
> problem. I will be on IRC, we could talk there to avoid introducing noise.

Today is a holiday in Canada so technically I'm not working, please ping me on IRC tomorrow if it's still not clear.  Thanks!

Comment 47

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

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #46)
> So by delay here do you mean an actual time delay?  Or just another step to
> get to the desired information?  If the latter, how is a method different
> with an attribute?  You need to access the attribute or call the method to
> get to the desired information at any rate, right?

It's a mix. I meant "another step" because I feel .getData() is lower than .data . Just a personal feeling. I suppose I would cache the result being either an array or an attribute. You're right here.

> Today is a holiday in Canada so technically I'm not working, please ping me
> on IRC tomorrow if it's still not clear.  Thanks!

Ok, no problem.

Comment 48

[(no longer active)]

(In reply to comment #47)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailapocalypse) from comment #46)
> > So by delay here do you mean an actual time delay?  Or just another step to
> > get to the desired information?  If the latter, how is a method different
> > with an attribute?  You need to access the attribute or call the method to
> > get to the desired information at any rate, right?
> 
> It's a mix. I meant "another step" because I feel .getData() is lower than
> .data . Just a personal feeling. I suppose I would cache the result being
> either an array or an attribute. You're right here.

Yeah, we can definitely do the smart thing in the implementation.  I'm glad that your concern here seems to be addressed.  :-)

Comment 49

[Borting Chen [:borting]]

Attachment: (WIP) ResourceStats-part1-impl.patch

Comment 50

[Borting Chen [:borting]]

Attachment: ResourceStats-part1-impl_v1.patch

Hi Ehsan:

Could you help review this patch? Thanks.

--
Update Note:

1. Instead of providing a large patch, this patch only implements the generic DB query methods. That means this patch doesn't include the logic of collecting network stats and power stats, nor the logic for network usage control.

2. The collection of power stats would be implemented in the Bug 854200 (as well as its following bugs).

3. For network stats collection and usage control, since the existing NetworkStats API has already been used by a Gaia App, a new bug (maybe several) would be filed to handle the integration of NetworkStats to ResourceStats and the modification of the Gaia App.

4. Bug 947779 would handle user privacy as it proposed (clearing the associated stats when an app is uninstalled)

5. Three methods in this patch (resourceTypes, sampleRate, and maxStorageAge of ResourceStatsManager) are still not implemented as static functions because we are stilled blocked by Bug 863952.

6. In ResourceStatsDS.jsm, two additional stats saving functions, saveNetworkStats() and savePowerStats(), are implemented for DB testing.

Comment 51

[(no longer active)]

Comment on attachment 8388335 [details] [diff] [review]
ResourceStats-part1-impl_v1.patch

Sorry, I'm not the right reviewer for this patch.  You want someone who is more familiar with indexed DB, the existing implementation of the network stats API and is comfortable reviewing a huge chunk of js code.  :-)

Comment 52

[Borting Chen [:borting]]

Attachment: ResourceStats-part1-impl_v1.patch

Hi Gene, could you help review this patch? Thanks.

Some notes of this patch:

1. This patch only contains the implementation of the generic DB query methods. That means this patch doesn't include the logic of collecting network stats and power stats, nor the logic for network usage control.

2. The collection of power stats would be implemented in the Bug 854200 (as well as its following bugs).

3. For network stats collection and usage control, since the existing NetworkStats API has already been used by a Gaia App, a new bug (maybe several) would be filed to handle the integration of NetworkStats to ResourceStats, and the modification of the Gaia App.

4. Bug 947779 would handle user privacy as it proposed (clearing the associated stats when an app is uninstalled).

5. Three methods in this patch (resourceTypes, sampleRate, and maxStorageAge of ResourceStatsManager) are still non-static functions because we are still blocked by Bug 863952.

6. In ResourceStatsDS.jsm, two additional stats saving functions, saveNetworkStats() and savePowerStats(), are included for DB testing.

Comment 53

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8390965 [details] [diff] [review]
ResourceStats-part1-impl_v1.patch

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

Just a very quick review through the coding style first. Hope to have a chat with you regarding the flow before looking into the codes in details. Could you please come by when you're available? Thanks!

The following comments are just some random pick-up. Please apply them to other else as possible as you can. Btw, I like your way of coding and commenting, which looks very comfortable and clear. Keep the good work. :)

::: dom/apps/src/PermissionsTable.jsm
@@ +185,5 @@
>                               app: DENY_ACTION,
>                               privileged: DENY_ACTION,
>                               certified: ALLOW_ACTION
>                             },
> +                           "resourcestats-manage": {

I assume adding this permission has already confirmed by Ehsan.

::: dom/resourcestats/ResourceStatsDB.jsm
@@ +7,5 @@
> +this.EXPORTED_SYMBOLS = ['ResourceStatsDB'];
> +
> +let debug;
> +const DEBUG = true;
> +if(DEBUG) {

I don't think you need this if you already used 

if (DEBUG) {
  debug("...");
}

in the implementation.

@@ +15,5 @@
> +} else {
> +  debug = function (s) {};
> +}
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;

Nits about coding style. Please use { ... } instead of {...} in the codes.

@@ +28,5 @@
> +
> +const statsStore = { power: POWER_STORE, network: NETWORK_STORE };
> +
> +// Constant defining the sampling rate
> +const SAMPLE_RATE = 24 * 60 * 60 * 1000;    // 1 day

Nits: put "//..." one space after ";" if you don't want to align it with other comments in other lines.

@@ +62,5 @@
> +
> +    let db = aDb;
> +    let objectStore;
> +
> +    // create PowerStore

Nits: comments should follow:

// {upper_case}...{one_period}

please do it over the codes.

@@ +87,5 @@
> +                            { unique: false });
> +  },
> +
> +  // Limit start & end in the range of [currentTime - MAX_AGE, currentTime]
> +  _limitTimeRange: function _limitTimeRange(aStart, aEnd) {

Nits: please don't use:

name: function name() anymore.

Just use:

name: function() instead.

@@ +89,5 @@
> +
> +  // Limit start & end in the range of [currentTime - MAX_AGE, currentTime]
> +  _limitTimeRange: function _limitTimeRange(aStart, aEnd) {
> +    let currentTime = (new Date()).getTime();
> + 

Nits: extra spaces.

@@ +103,5 @@
> +
> +    return { start: start, end: end };
> +  },
> +
> +  // Convert to UTC according to timezone and

s/timezone/the timezone/
s/filter/ the filter/

Just grammar. Please add "the" for specific vars.

@@ +112,5 @@
> +    return time;
> +  },
> +
> +  // fill empty slots in statsData array so that the timestamp of the first
> +  // element is equal to aStart and the timestamp of the last is equal to aEnd

s/last/last one/

@@ +183,5 @@
> +          aStore.put(stats);
> +          debug("saved network stats: " + JSON.stringify(stats));
> +        }
> +      });
> +    }.bind(this), aResultCb);

I don't think you need "bind(this)" here because you don't use "this" in the callback.

@@ +228,5 @@
> +          aStore.put(stats);
> +          debug("saved power stats: " + JSON.stringify(stats));
> +        }
> +      });
> +    }.bind(this), aResultCb);

Ditto.

@@ +234,5 @@
> +
> +  // get stats from a store
> +  getStats: function getStats(aResultCb, aType, aAppId, aManifestURL, aServiceType, aComponent, aStart, aEnd) {
> +    debug(aType + "Mgr.getStats()");
> +    let self = this;

If you already used "self" then you don't need to bind(this) below. Please choose one style.

::: dom/resourcestats/ResourceStatsManager.js
@@ +5,5 @@
> +"use strict";
> +
> +let debug;
> +const DEBUG = true;
> +if (DEBUG) {

Ditto.

@@ +22,5 @@
> +
> +// Ensure ResourceStatsService and ResourceStatsDB are loaded in the parent
> +// process to receive messages from the child processes.
> +let appInfo = Cc["@mozilla.org/xre/app-info;1"];
> +let isParentProcess = !appInfo || appInfo.getService(Ci.nsIXULRuntime).processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT;

This line is too long. Please wrap it within 80 chars.

@@ +177,5 @@
> +  _checkTimeRange: function _checkTimeRange(aStart, aEnd) {
> +    debug("_checkTimeRange(): " + aStart + " to " + aEnd);
> +
> +    let start = aStart ? aStart : 0;
> +    let end = aEnd ? aEnd : new Date().getTime();

(new Date())

@@ +185,5 @@
> +        start > end) {
> +      throw Cr.NS_ERROR_INVALID_ARG;
> +    }
> +
> +    return {start: start, end: end};

Ditto.

@@ +376,5 @@
> +    }
> +
> +    switch (aMessage.name) {
> +      case "ResourceStats:GetStats:Resolved":
> +        debug("data.value = " + JSON.stringify(data.value));

Please use:

if (DEBUG) {
  debug("...");
}

@@ +449,5 @@
> +
> +    // Check permission
> +    let principal = aWindow.document.nodePrincipal;
> +    let perm = Services.perms.testExactPermissionFromPrincipal(principal,
> +                     "resourcestats-manage");

Align this param with "principal" above.

::: dom/resourcestats/ResourceStatsService.jsm
@@ +38,5 @@
> +this.ResourceStatsService = {
> +
> +  init: function() {
> +    debug("Service started");
> +    let self = this;

You don't need self here.

@@ +266,5 @@
> +    }
> +
> +    // execute DB operation
> +    let onAlarmGot = this._createDbCallBack(aMm, aData.resolverId,
> +                                         "ResourceStats:GetAlarms");

Nits: alignment.

Comment 54

[Gene Lian (I already quit Mozilla)]

Just had a face-to-face discussion with Borting and went through all the details. He'll come back with a better version based on our conclusions. :)

Comment 55

[Borting Chen [:borting]]

Attachment: ResourceStats-part1-impl_v2.patch

Hi Gene, Could you help review this patch? Thanks.

Update note:
1. fix coding style nits.
2. remove classInfo as Bug 981845 suggested
3. remove __exposedProps__
4. remove Ci.nsIsupport from generateQI()
5. remove unecessary bind(this)
6. when calling getStats(), the entries of resulting statsData array will be filled in Content process rather than Chrome process to reduce the amount of data that IPC transfer
7. when calling getStats(), manifestURL is validated in resourceStatsDB.jsm instead of resourceStatsService.jsm 
8. move all callback function to the end of parameter list in all functions.
9. add assertContainApp() to check the IPC source when Chrome process receives message
10. maxAge -> maxStorageAge

Comment 56

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8395498 [details] [diff] [review]
ResourceStats-part1-impl_v2.patch

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

The codes look pretty nice. Nice work!

::: dom/resourcestats/ResourceStatsDB.jsm
@@ +6,5 @@
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ['ResourceStatsDB'];
> +
> +const DEBUG = true;

Just a reminder. Please flip this before landing.

@@ +24,5 @@
> +const POWER_STORE = "power_stats_store";
> +const NETWORK_STORE = "network_stats_store";
> +const ALARM_STORE = "alarm_store";
> +
> +const statsStore = { power: POWER_STORE, network: NETWORK_STORE };

s/statsStore/statsStoreNames/

@@ +30,5 @@
> +// Constant defining the sampling rate.
> +const SAMPLE_RATE = 24 * 60 * 60 * 1000; // 1 day.
> +
> +// Constant defining MAX age of stored stats.
> +const MAX_STORAGE_AGE = 180 * SAMPLE_RATE; // 180 days.

s/MAX/the MAX/

@@ +81,5 @@
> +      autoIncrement: true
> +    });
> +    objectStore.createIndex("type", "type", { unique: false });
> +    // Index for resource control target.
> +    objectStore.createIndex("control_target",

Let's use "controlTarget" too meet the coding style over these codes.

@@ +87,5 @@
> +                            { unique: false });
> +  },
> +
> +  // Convert to UTC according to current timezone and filter timestamp to get
> +  // SAMPLE_RATE precission.

s/current/the current/
s/filter/the filter/

@@ +141,5 @@
> +          if (DEBUG) {
> +            debug("Save network stats: " + JSON.stringify(stats));
> +          }
> +
> +          // Accumulate to tatal stats.

s/Accumulate/Accumulated/

@@ +290,5 @@
> +            return;
> +          }
> +          aTxn.result.statsData = statsData;
> +        };
> +      } else { // aType == "network"

Just use:

else if (aType == "network")

@@ +513,5 @@
> +          cursor.continue();
> +          return;
> +        }
> +
> +        // Remove "" from result, which indicates sum of all components' stats.

s/result/the result/

::: dom/resourcestats/ResourceStatsManager.js
@@ +19,5 @@
> +let appInfo = Cc["@mozilla.org/xre/app-info;1"];
> +let isParentProcess = !appInfo || appInfo.getService(Ci.nsIXULRuntime)
> +                        .processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT;
> +if (isParentProcess) {
> +  Cu.import("resource://gre/modules/ResourceStatsService.jsm");

Why do you want to do this?

@@ +174,5 @@
> +      debug("_checkTimeRange(): " + aStart + " to " + aEnd);
> +    }
> +
> +    let start = aStart ? aStart : 0;
> +    let end = aEnd ? aEnd : (new Date()).getTime();

Please use "Date.now()" over the codes.

@@ +250,5 @@
> +
> +    // Parse alarm options.
> +		if (DEBUG) {
> +			debug("aAlarmOptions: " + JSON.stringify(aAlarmOptions));
> +		}

Please never use TABs in codes.

@@ +489,5 @@
> +      return;
> +    }
> +
> +    // Get the manifestURL of the current app.
> +    this._manifestURL = principal.origin + "/manifest.webapp";

Please use:

this._manifestURL = appsService.getManifestURLByLocalId(principal.appId);

to get the manifestURL.

Comment 57

[Borting Chen [:borting]]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #56)
> Comment on attachment 8395498 [details] [diff] [review]
> ResourceStats-part1-impl_v2.patch
> 
> Review of attachment 8395498 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/resourcestats/ResourceStatsManager.js
> @@ +19,5 @@
> > +let appInfo = Cc["@mozilla.org/xre/app-info;1"];
> > +let isParentProcess = !appInfo || appInfo.getService(Ci.nsIXULRuntime)
> > +                        .processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT;
> > +if (isParentProcess) {
> > +  Cu.import("resource://gre/modules/ResourceStatsService.jsm");
> 
> Why do you want to do this?

Oh, I copied them from NetworkStats API, but I found it is not necessary after testing. (Because we import ResourceStatsService.jsm in b2g/chrome/content/shell.js, right?) Thanks for your kind reminder.

Comment 58

[Borting Chen [:borting]]

Attachment: ResourceStats-part1-impl_v3.patch

Hi Gene, here is the update. Please help me review it, thanks!

Update note:
1. fix nits.
2. remove importing ResourceStatsService.jsm in ResourceStatsManager.js
3. turn off debug message

Comment 59

[Borting Chen [:borting]]

Attachment: (WIP) ResourceStats-part2-testCases.patch

Comment 60

[Borting Chen [:borting]]

Attachment: ResourceStats-part2-testCases_v1.patch

Comment 61

[Borting Chen [:borting]]

Attachment: ResourceStats-part1-impl_v4.patch

Comment 62

[Borting Chen [:borting]]

Attachment: ResourceStats-part0-webidl_v3.patch

Comment 63

[Borting Chen [:borting]]

Hi Ehsan:

I wanna to propse two updates to the WebIDL.
(The difference of the updates is here: https://bugzilla.mozilla.org/attachment.cgi?oldid=8376026&action=interdiff&newid=8417247&headers=1)

1. In the webidl attribute, I replace [Pref="dom.resource_stats.enabled] with

  [HeaderFile="mozilla/dom/ResourceStatsControl.h", 
   Func="mozilla::dom::ResourceStatsControl::HasResourceStatsSupport"] 

  The HasResourceStatsSupport method would check both pref and permission because I need to make sure that user cannot create a ResourceStatsManager object without permission.
  
2.  For ResourceStatsAlarm interface, I change the types of |component|, |serviceType| and |manifestURL| to nullable types. This is because:
  a. |component| would be null when the return alarm monitors total resource usage,
  b. |serviceType| would be null when the return alarm monitors the resource usage of an app,
  c. |manifestURL| would be null when the return alarm monitors the resource usage of an system service.
  
I also got a problem when running the mochitest. It seems that the [AvailableIn=] attribute would cause mochitest app fail to access the ResourceStatsManager. I guess the reason is that mochitest app does not a real certified app so the access is blocked. Do you know who can help on this problem? Thanks.

Comment 64

[(no longer active)]

(In reply to Borting Chen [:borting] from comment #63)
> Hi Ehsan:
> 
> I wanna to propse two updates to the WebIDL.
> (The difference of the updates is here:
> https://bugzilla.mozilla.org/attachment.
> cgi?oldid=8376026&action=interdiff&newid=8417247&headers=1)
> 
> 1. In the webidl attribute, I replace [Pref="dom.resource_stats.enabled] with
> 
>   [HeaderFile="mozilla/dom/ResourceStatsControl.h", 
>    Func="mozilla::dom::ResourceStatsControl::HasResourceStatsSupport"] 
> 
>   The HasResourceStatsSupport method would check both pref and permission
> because I need to make sure that user cannot create a ResourceStatsManager
> object without permission.

Sure.  Hopefully we'll fix bug 961116 soon and we'll be able to denote the permission in WebIDL too, but a Func would do fine for now.

> 2.  For ResourceStatsAlarm interface, I change the types of |component|,
> |serviceType| and |manifestURL| to nullable types. This is because:
>   a. |component| would be null when the return alarm monitors total resource
> usage,
>   b. |serviceType| would be null when the return alarm monitors the resource
> usage of an app,
>   c. |manifestURL| would be null when the return alarm monitors the resource
> usage of an system service.

Sounds good.

> I also got a problem when running the mochitest. It seems that the
> [AvailableIn=] attribute would cause mochitest app fail to access the
> ResourceStatsManager. I guess the reason is that mochitest app does not a
> real certified app so the access is blocked. Do you know who can help on
> this problem? Thanks.

Yes, you can add a [Pref] for a testing only pref.  See for example how the dom.testing.datastore_enabled_for_hosted_apps is used for this same purpose.  You can add a dom.testing.resourcestats_enabled_for_hosted_apps pref like that.

Comment 65

[Borting Chen [:borting]]

Attachment: ResourceStats-part0-webidl_v4.patch

Comment 66

[Borting Chen [:borting]]

Attachment: ResourceStats-part1-impl_v4.patch

Comment 67

[Borting Chen [:borting]]

Attachment: ResourceStats-part2-testCases_v2.patch

Comment 68

[Borting Chen [:borting]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #64)
> (In reply to Borting Chen [:borting] from comment #63)
> > I also got a problem when running the mochitest. It seems that the
> > [AvailableIn=] attribute would cause mochitest app fail to access the
> > ResourceStatsManager. I guess the reason is that mochitest app does not a
> > real certified app so the access is blocked. Do you know who can help on
> > this problem? Thanks.
> 
> Yes, you can add a [Pref] for a testing only pref.  See for example how the
> dom.testing.datastore_enabled_for_hosted_apps is used for this same purpose.
> You can add a dom.testing.resourcestats_enabled_for_hosted_apps pref like
> that.

Thank for the suggestion. I solved this problem by setting "dom.ignore_webidl_scope_checks" preference in the test cases. [1]

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=967475

Comment 69

[Borting Chen [:borting]]

Comment on attachment 8420897 [details] [diff] [review]
ResourceStats-part0-webidl_v4.patch

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

Hi Ehsan:

  This patch update the webidl based on previous discussion. Besides, adding [EnforceRange] to clamp the input range of unsigned input variables.
  Could you help review this patch? Thanks.

Update Notes:
1. Replace [Pref="dom.resource_stats.enabled] with a [Func=...] and a [HeaderFile=...] in order to check preference and permission at the same time.
2. Change the type of ResourceStatsAlarm's attributes (|component|, |serviceType| and |manifestURL|), so that they can return null type.
3. For the methods of ResourceStatsAlarm and ResourceStatsManager, add [EnforceRange] to unsigned type input parameter (including DOMTimeStamp) to clamp the input range.

Comment 70

[(no longer active)]

Comment on attachment 8420897 [details] [diff] [review]
ResourceStats-part0-webidl_v4.patch

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

Please note that you will need to get review from a DOM peer for the WebIDL bits as well, according to <https://groups.google.com/forum/#!topic/mozilla.dev.b2g/Ee1Hfp6QeHE>.

Thanks!

Comment 71

[Borting Chen [:borting]]

Attachment: ResourceStats-part2-testCases_v2.patch

Comment 72

[Borting Chen [:borting]]

Attachment: ResourceStats-part2-testCases_v2.patch

Comment 73

[Borting Chen [:borting]]

Attachment: ResourceStats-part2-testCases_v2.patch

Comment 74

[Borting Chen [:borting]]

Try result: https://tbpl.mozilla.org/?tree=Try&rev=44aea05135b7

Comment 75

[Borting Chen [:borting]]

Comment on attachment 8420898 [details] [diff] [review]
ResourceStats-part1-impl_v4.patch

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

Hi gene:

I've done several updates for the implementation patch, could you help review the update?

Comapring to the previous patch (Attachment #8395575 [details] [diff]), the following lists the updates and their reasons:

1. In ver 3, returning the objects of ResourceStatsAlarm, ResourceStats, NetworkStatsData and PowerStatsData would get fail because these objects are not exposed to content side. In ver 4, this problem is resolved by invoking the _create method in these interface.

2. According to Bug 986940, replace Cu.createArrayIn(aWindow) with new aWindow.Array()

3. Add ResourceStatsControl.h and ResourceStatsControl.cpp which implement the HasResourceStatsSupport function for pref checking and permission checking

4. According to 3, remove all permission checking in the implementation of ResourceStatsManager.

5. In ResourceStatsAlarm, |compment|, |serviceType| and |ManifestURL| should be null when not specified. So, replacing them as following:

  this.component = aAlarm.component; --> this.component = aAlarm.component || null;
  this.serviceType = aAlarm.serviceType; --> this.serviceType = aAlarm.serviceType || null;
  this.manifestURL = aAlarm.manifestURL; --> this.manifestURL = aAlarm.manifestURL || null;
  
6. Because [EnforceRange] is declared in the webidl, checking whether timestamp and threshold are greater than zero is not necessary anymore. So, the input checkings are deleted.

Comment 76

[Borting Chen [:borting]]

Comment on attachment 8429141 [details] [diff] [review]
ResourceStats-part2-testCases_v2.patch

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

Hi gene:

This patch contains the test cases (Mochitest and xpcshell test) for ResourceStats API. Could you help me review it? Thanks.

Comment 77

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8420897 [details] [diff] [review]
ResourceStats-part0-webidl_v4.patch

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

Sweet!

Comment 78

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8420898 [details] [diff] [review]
ResourceStats-part1-impl_v4.patch

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

Nice work but I'd like to examine just one more time after you fix these nits. ;)

::: dom/resourcestats/ResourceStatsControl.cpp
@@ +6,5 @@
> +#include "nsIPermissionManager.h"
> +#include "nsJSUtils.h"
> +#include "nsPIDOMWindow.h"
> +#include "nsServiceManagerUtils.h"
> +#include "mozilla/Preferences.h"

nit: please include these headers alphabetically.

@@ +12,5 @@
> +using namespace mozilla::dom;
> +
> +/* static */ bool
> +ResourceStatsControl::HasResourceStatsSupport(JSContext* /* unused */,
> +			                                        JSObject* aGlobal)

nit: please don't use TABs.

@@ +23,5 @@
> +  // Get window.
> +  nsCOMPtr<nsPIDOMWindow> win =
> +    do_QueryInterface((nsISupports*)nsJSUtils::GetStaticScriptGlobal(aGlobal));
> +  MOZ_ASSERT(!win || win->IsInnerWindow());
> +  win = win.forget();

I cannot figure out why you need this line. Would you educate me?

::: dom/resourcestats/ResourceStatsControl.h
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_apps_ResourceStatsConotrl_h
> +#define mozilla_dom_apps_ResourceStatsConotrl_h

Please follow how you export the header file to name this: "mozilla_dom_ResourceStatsConotrl_h".

@@ +4,5 @@
> +
> +#ifndef mozilla_dom_apps_ResourceStatsConotrl_h
> +#define mozilla_dom_apps_ResourceStatsConotrl_h
> +
> +#include "nsIScriptContext.h"

Don't need this line. Forward declarations are enough.

@@ +23,5 @@
> +
> +} // namespace dom
> +} // namespace mozilla
> +
> +#endif // mozilla_dom_apps_ResourceStatsConotrl_h

Ditto.

::: dom/resourcestats/ResourceStatsDB.jsm
@@ +20,5 @@
> +                                   "nsIAppsService");
> +
> +const DB_NAME = "resource_stats";
> +const DB_VERSION = 1;
> +const POWER_STORE = "power_stats_store";

Can you name this POWER_STATS_STORE?

@@ +21,5 @@
> +
> +const DB_NAME = "resource_stats";
> +const DB_VERSION = 1;
> +const POWER_STORE = "power_stats_store";
> +const NETWORK_STORE = "network_stats_store";

Can you name this NETWORK_STATS_STORE?

@@ +44,5 @@
> +
> +ResourceStatsDB.prototype = {
> +  __proto__: IndexedDBHelper.prototype,
> +
> +  dbNewTxn: function(store_name, txn_type, callback, txnCb) {

Would you:

s/store_name/aStoreName
s/txn_type/aTxnType
s/callback/aCallback
s/txnCb/aTxnCb

to have a consistent coding style within this file.

Btw, can this be s/dbNewTxn/_dbNewTxn? since it's a local function?

@@ +45,5 @@
> +ResourceStatsDB.prototype = {
> +  __proto__: IndexedDBHelper.prototype,
> +
> +  dbNewTxn: function(store_name, txn_type, callback, txnCb) {
> +    function successCb(result) {

s/result/aResult/

@@ +48,5 @@
> +  dbNewTxn: function(store_name, txn_type, callback, txnCb) {
> +    function successCb(result) {
> +      txnCb(null, result);
> +    }
> +    function errorCb(error) {

s/error/aError/

@@ +59,5 @@
> +    if (DEBUG) {
> +      debug("Upgrade DB from ver." + aOldVersion + " to ver." + aNewVersion);
> +    }
> +
> +    let db = aDb;

nit: don't need this line. Just use aDb.

@@ +62,5 @@
> +
> +    let db = aDb;
> +    let objectStore;
> +
> +    // Create PowerStore.

s/PowerStore/PowerStatsStore

@@ +68,5 @@
> +      keyPath: ["appId", "serviceType", "component", "timestamp"]
> +    });
> +    objectStore.createIndex("component", "component", { unique: false });
> +
> +    // Create NetworkStore.

s/NetworkStore/NetworkStatsStore/

::: dom/resourcestats/ResourceStatsManager.js
@@ +66,5 @@
> +  let createStatsDataObject = null;
> +  let self = this;
> +  switch (this.type) {
> +    case "power":
> +      createStatsDataObject = function(stats) {

s/stats/aStats/

@@ +67,5 @@
> +  let self = this;
> +  switch (this.type) {
> +    case "power":
> +      createStatsDataObject = function(stats) {
> +        let chromeObj = new PowerStatsData(stats);

Ditto.

@@ +72,5 @@
> +        return self._window.PowerStatsData._create(self._window, chromeObj);
> +      };
> +      break;
> +    case "network":
> +      createStatsDataObject = function(stats) {

s/stats/aStats/

@@ +73,5 @@
> +      };
> +      break;
> +    case "network":
> +      createStatsDataObject = function(stats) {
> +        let chromeObj = new NetworkStatsData(stats);

Ditto.

@@ +145,5 @@
> +
> +ResourceStatsManager.prototype = {
> +  __proto__: DOMRequestIpcHelper.prototype,
> +
> +  _sendPromise: function(callback) {

s/_sendPromise/_getPromise makes more sense.

@@ +302,5 @@
> +    });
> +  },
> +
> +  get resourceTypes() {
> +    let supportTypes = new this._window.Array();

s/supportTypes/supportedTypes. I prefer just |types|.

::: dom/resourcestats/ResourceStatsService.jsm
@@ +135,5 @@
> +    }
> +  },
> +
> +  // Closure generates callback function for DB request.
> +  _createDbCallBack: function(aMm, aId, aMessage) {

s/_createDbCallBack/_createDbCallback and it's caller.

@@ +141,5 @@
> +    let rejectMsg = aMessage + ":Rejected";
> +
> +    return function(error, result) {
> +      if (error) {
> +        aMm.sendAsyncMessage(rejectMsg, { resolverId: aId, reason: error });

return?

@@ +275,5 @@
> +      };
> +    }
> +
> +    // Execute DB operation.
> +    let onAlarmGot = this._createDbCallBack(aMm, aData.resolverId,

s/onAlarmGot/onAlarmsGot/

@@ +277,5 @@
> +
> +    // Execute DB operation.
> +    let onAlarmGot = this._createDbCallBack(aMm, aData.resolverId,
> +                                            "ResourceStats:GetAlarms");
> +    this._db.getAlarms(aData.type, options, onAlarmGot);

Ditto.

Comment 79

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8429141 [details] [diff] [review]
ResourceStats-part2-testCases_v2.patch

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

Well done! r=gene

::: dom/resourcestats/tests/mochitest/mochitest.ini
@@ +1,2 @@
> +[test_basic.html]
> +skip-if = toolkit != "gonk"

Just wonder if we could have a better way to refactorize |skip-if = toolkit != "gonk"|.

::: dom/resourcestats/tests/mochitest/test_network_alarm.html
@@ +74,5 @@
> +    // Test addAlarm.
> +    var threshold = Math.floor(Math.random() * 1000);
> +    var promise = networkStatsMgr.addAlarm(threshold,
> +                                         { 'component': wifiComponent },
> +                                         { 'startTime': Date.now() });

Align the arguments.

@@ +93,5 @@
> +    // Test addAlarm with negative threshold.
> +    var threshold = -1;
> +    var promise = networkStatsMgr.addAlarm(threshold,
> +                                         { 'component': wifiComponent },
> +                                         { 'startTime': Date.now() });

Align the arguments.

@@ +122,5 @@
> +    // Test addAlarm with negative startTime.
> +    var threshold = Math.floor(Math.random() * 1000);
> +    var promise = networkStatsMgr.addAlarm(threshold,
> +                                         { 'component': wifiComponent },
> +                                         { 'startTime': -1 });

Align the arguments.

@@ +140,5 @@
> +    var threshold = Math.floor(Math.random() * 1000);
> +    var promise = networkStatsMgr.addAlarm(threshold,
> +                                         { 'component': wifiComponent,
> +                                           'manifestURL': invalidManifestURL },
> +                                         { 'startTime': Date.now() });

Align the arguments.

@@ +160,5 @@
> +    // Execution steps:
> +    // 1. Add a new alarm.
> +    var promise = networkStatsMgr.addAlarm(threshold,
> +                                         { 'component': wifiComponent },
> +                                         { 'startTime': Date.now() });

Align the arguments.

@@ +204,5 @@
> +    var threshold = Math.floor(Math.random() * 1000);
> +    var promise = networkStatsMgr.addAlarm(threshold,
> +                                         { 'component': wifiComponent,
> +                                           'manifestURL': invalidManifestURL },
> +                                         { 'startTime': Date.now() });

Align the arguments.

@@ +223,5 @@
> +    // Execution steps:
> +    // 1. Add a new alarm.
> +    var promise = networkStatsMgr.addAlarm(threshold,
> +                                         { 'component': wifiComponent },
> +                                         { 'startTime': Date.now() });

Align the arguments.

::: dom/resourcestats/tests/mochitest/test_network_stats.html
@@ +35,5 @@
> +   "resourceTypes should be a ResourceStatsManager attribute.");
> +  ok(Array.isArray(networkStatsMgr.resourceTypes),
> +   "networkStatsMgr.resourceTypes is an array.");
> +  ok(networkStatsMgr.resourceTypes.indexOf("network") > -1,
> +   "\"network\" is an element of networkStatsMgr.resourceTypes.");

Overall you can just use |'| instead of |\"| since you already used |'| a lot at other places.

::: dom/resourcestats/tests/xpcshell/test_resourcestats_db.js
@@ +129,5 @@
> +  var record = { appId: aAppId,
> +                 serviceType: aServiceType,
> +                 componentStats: componentStats };
> +
> +  return {record: record, result: result};

Spaces, you know. ;)

@@ +170,5 @@
> +  var record = { appId: aAppId,
> +                 serviceType: aServiceType,
> +                 componentStats: componentStats };
> +
> +  return {record: record, result: result};

Ditto.

@@ +174,5 @@
> +  return {record: record, result: result};
> +}
> +
> +// Compare stats saved in network store with expected results.
> +function checkNetworkStore(aExpectedResult, aDumpResult, aTimestamp) {

s/NetworkStore/NetworkStatsStore and other similar codes.

@@ +211,5 @@
> +  run_next_test();
> +}
> +
> +// Compare stats saved in power store with expected results.
> +function checkPowerStore(aExpectedResult, aDumpResult, aTimestamp) {

s/PowerStore/PowerStatsStore and other similar codes.

@@ +247,5 @@
> +  run_next_test();
> +}
> +
> +// Prepare network store for testing.
> +function prepareNetworkStore(recordArray, timestamp, callback) {

Ditto.

@@ +256,5 @@
> +  });
> +}
> +
> +// Prepare power store for testing.
> +function preparePowerStore(recordArray, timestamp, callback) {

Ditto.

Comment 80

[Borting Chen [:borting]]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #78)
> Comment on attachment 8420898 [details] [diff] [review]
> ResourceStats-part1-impl_v4.patch
> 
> Review of attachment 8420898 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +12,5 @@
> > +using namespace mozilla::dom;
> > +
> > +/* static */ bool
> > +ResourceStatsControl::HasResourceStatsSupport(JSContext* /* unused */,
> > +			                                        JSObject* aGlobal)
> 
> nit: please don't use TABs.
> 
> @@ +23,5 @@
> > +  // Get window.
> > +  nsCOMPtr<nsPIDOMWindow> win =
> > +    do_QueryInterface((nsISupports*)nsJSUtils::GetStaticScriptGlobal(aGlobal));
> > +  MOZ_ASSERT(!win || win->IsInnerWindow());
> > +  win = win.forget();
> 
> I cannot figure out why you need this line. Would you educate me?

I referred how GetWindowFromGlobal() works in Navigator.cpp [1].
GetWindowFromGlobal() returns app's content window so that we can check permission owned by an app.
This function also calls win.forget() brfore return; but honestly speaking, I don't why it doing so ...

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#2367

Comment 81

[Borting Chen [:borting]]

Attachment: ResourceStats-part1-impl_v5.patch

Nit fixed.

Comment 82

[Gene Lian (I already quit Mozilla)]

(In reply to Borting Chen [:borting] from comment #80)
> (In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #78)
> > Comment on attachment 8420898 [details] [diff] [review]
> > ResourceStats-part1-impl_v4.patch
> > 
> > Review of attachment 8420898 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > @@ +12,5 @@
> > > +using namespace mozilla::dom;
> > > +
> > > +/* static */ bool
> > > +ResourceStatsControl::HasResourceStatsSupport(JSContext* /* unused */,
> > > +			                                        JSObject* aGlobal)
> > 
> > nit: please don't use TABs.
> > 
> > @@ +23,5 @@
> > > +  // Get window.
> > > +  nsCOMPtr<nsPIDOMWindow> win =
> > > +    do_QueryInterface((nsISupports*)nsJSUtils::GetStaticScriptGlobal(aGlobal));
> > > +  MOZ_ASSERT(!win || win->IsInnerWindow());
> > > +  win = win.forget();
> > 
> > I cannot figure out why you need this line. Would you educate me?
> 
> I referred how GetWindowFromGlobal() works in Navigator.cpp [1].
> GetWindowFromGlobal() returns app's content window so that we can check
> permission owned by an app.
> This function also calls win.forget() brfore return; but honestly speaking,
> I don't why it doing so ...
> 
> [1] http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#2367

Note that it needs to return already_AddRefed<nsPIDOMWindow>, which explicitly means the variable catching this return will not add refCount for it. Therefore, we have to manually call .forget() to keep its refCount, which won't be released when the internal variable goes out of scope.

Comment 83

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8436674 [details] [diff] [review]
ResourceStats-part1-impl_v5.patch

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

Looks nice to me! r=gene

::: dom/resourcestats/ResourceStatsControl.cpp
@@ +23,5 @@
> +  // Get window.
> +  nsCOMPtr<nsPIDOMWindow> win =
> +    do_QueryInterface((nsISupports*)nsJSUtils::GetStaticScriptGlobal(aGlobal));
> +  MOZ_ASSERT(!win || win->IsInnerWindow());
> +  win = win.forget();

Yeap, please do remove this before landing.

Comment 84

[Borting Chen [:borting]]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #82)
> (In reply to Borting Chen [:borting] from comment #80)
> > (In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #78)
> > > Comment on attachment 8420898 [details] [diff] [review]
> > > ResourceStats-part1-impl_v4.patch
> > > 
> > > Review of attachment 8420898 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > @@ +12,5 @@
> > > > +using namespace mozilla::dom;
> > > > +
> > > > +/* static */ bool
> > > > +ResourceStatsControl::HasResourceStatsSupport(JSContext* /* unused */,
> > > > +			                                        JSObject* aGlobal)
> > > 
> > > nit: please don't use TABs.
> > > 
> > > @@ +23,5 @@
> > > > +  // Get window.
> > > > +  nsCOMPtr<nsPIDOMWindow> win =
> > > > +    do_QueryInterface((nsISupports*)nsJSUtils::GetStaticScriptGlobal(aGlobal));
> > > > +  MOZ_ASSERT(!win || win->IsInnerWindow());
> > > > +  win = win.forget();
> > > 
> > > I cannot figure out why you need this line. Would you educate me?
> > 
> > I referred how GetWindowFromGlobal() works in Navigator.cpp [1].
> > GetWindowFromGlobal() returns app's content window so that we can check
> > permission owned by an app.
> > This function also calls win.forget() brfore return; but honestly speaking,
> > I don't why it doing so ...
> > 
> > [1] http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#2367
> 
> Note that it needs to return already_AddRefed<nsPIDOMWindow>, which
> explicitly means the variable catching this return will not add refCount for
> it. Therefore, we have to manually call .forget() to keep its refCount,
> which won't be released when the internal variable goes out of scope.

Understood. Thanks!

Comment 85

[Borting Chen [:borting]]

Attachment: ResourceStats-part0-webidl_v5.patch

Code rebase. Previous patch is reviewed by Ehsan and Jonas.

Comment 86

[Borting Chen [:borting]]

Attachment: Bug-854200-part0-ContentParentNotification.patch (Wrong Submission)

Code rebase. Previous patch is reviewed by Ehsan and Jonas.

Comment 87

[Borting Chen [:borting]]

Attachment: ResourceStats-part1-impl_v6.patch,r=gene

Comment 88

[Borting Chen [:borting]]

Attachment: ResourceStats-part2-testCases_v3.patch,r=gene

Comment 89

[Borting Chen [:borting]]

Attachment: ResourceStats-part0-webidl_v5.patch,r=ehsan,jonas

(Submitted wrong patch ...)
Code rebase. Previous patch is reviewed by Ehsan and Jonas.

Comment 90

[Borting Chen [:borting]]

Try result: https://tbpl.mozilla.org/?tree=Try&rev=7be2d4ac697f

Comment 91

[Borting Chen [:borting]]

Attachment: ResourceStats-part2-testCases_v4.patch,r=gene

Update due to change of function name in ResourceStatsDB.jsm

Comment 92

[Borting Chen [:borting]]

Attachment: ResourceStats-part1-impl_v6.patch,r=gene

Comment 93

[Borting Chen [:borting]]

Attachment: ResourceStats-part2-testCases_v4.patch,r=gene

Comment 94

[Borting Chen [:borting]]

Try result: https://tbpl.mozilla.org/?tree=Try&rev=4e9b931f36be

Comment 95

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/610b1e0c7562
https://hg.mozilla.org/integration/b2g-inbound/rev/b2afa18bea00
https://hg.mozilla.org/integration/b2g-inbound/rev/5309d91dc590

Comment 96

[Ryan VanderMeulen [:RyanVM]]

Backed out for introducing new rooting hazards.
https://hg.mozilla.org/integration/b2g-inbound/rev/98deb29a6c1f

Function '_ZN7mozilla3dom20ResourceStatsControl23HasResourceStatsSupportEP9JSContextP8JSObject|uint8 mozilla::dom::ResourceStatsControl::HasResourceStatsSupport(JSContext*, JSObject*)' has unrooted 'aGlobal' of type 'JSObject*' live across GC call '_ZN7mozilla11Preferences7GetBoolEPKcb|uint8 mozilla::Preferences::GetBool(int8*, uint8)' at dom/resourcestats/ResourceStatsControl.cpp:19
    dom/resourcestats/ResourceStatsControl.cpp:19: Call(1,2, __temp_1 := GetBool("dom.resource_stats.enabled",0))
    dom/resourcestats/ResourceStatsControl.cpp:19: Assume(2,4, !__temp_1*, false)
    dom/resourcestats/ResourceStatsControl.cpp:25: Call(4,5, __temp_4 := GetStaticScriptGlobal(aGlobal*))
GC Function: _ZN7mozilla11Preferences7GetBoolEPKcb|uint8 mozilla::Preferences::GetBool(int8*, uint8)
    uint32 mozilla::Preferences::GetBool(int8*, uint8*)
    PREF_GetBoolPref
    PrefHashEntry* pref_HashTableLookup(void*)
    PLDHashEntryHdr* PL_DHashTableOperate(PLDHashTable*, void*, uint32)
    FieldCall: PLDHashTableOps.initEntry

Comment 97

[Borting Chen [:borting]]

Attachment: ResourceStats-part0-webidl_v6.patch

Hi Ehsan, because Bug 961116 was resolved, I removed those "Func=" in WebIDL files and uses "CheckPermissions" and "Pref" instead. Could you help review this update? Thanks.

Comment 98

[Borting Chen [:borting]]

Attachment: ResourceStats-part1-impl_v7.patch

Since "Pref" and "CheckPermissions" are used in the WebIDL definition, the "HasResourceStatsSupport" is no more required. Compared to previous patch (v6), this patch removes ResourceStatsControl.cpp and ResourceStatsControl.h files which implement the HasResourceStatsSupport function.

Comment 99

[Borting Chen [:borting]]

(In reply to Borting Chen [:borting] from comment #98)
> Created attachment 8439288 [details] [diff] [review]
> ResourceStats-part1-impl_v7.patch
> 
> Since "Pref" and "CheckPermissions" are used in the WebIDL definition, the
> "HasResourceStatsSupport" is no more required. Compared to previous patch
> (v6), this patch removes ResourceStatsControl.cpp and ResourceStatsControl.h
> files which implement the HasResourceStatsSupport function.

And, hoping that this patch could solve the problem in comment 96.

Comment 100

[(no longer active)]

Comment on attachment 8439287 [details] [diff] [review]
ResourceStats-part0-webidl_v6.patch

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

Looks great!  Can you also add [FeatureDetectible] to these interfaces if you land this after bug 1009645, or add a comment there if you land it before then?  Either way I won't need to review it again!

Thanks.

Comment 101

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8439287 [details] [diff] [review]
ResourceStats-part0-webidl_v6.patch

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

sr=me. Though it's great if you just attach an interdiff for small changes like this.

Comment 102

[Borting Chen [:borting]]

(In reply to Jonas Sicking (:sicking) from comment #101)
> Comment on attachment 8439287 [details] [diff] [review]
> ResourceStats-part0-webidl_v6.patch
> 
> Review of attachment 8439287 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> sr=me. Though it's great if you just attach an interdiff for small changes
> like this.

Okay, thanks for the help!

Comment 103

[Borting Chen [:borting]]

Attachment: ResourceStats-part1-impl_v7.patch,r=gene

Comment 104

[Borting Chen [:borting]]

Attachment: ResourceStats-part0-webidl_v7.patch,r=ehsan,sr=jonas

Comment 105

[Borting Chen [:borting]]

Waiting for try: https://tbpl.mozilla.org/?tree=Try&rev=64127396aa78

Comment 106

[Borting Chen [:borting]]

Try result: https://tbpl.mozilla.org/?tree=Try&rev=64127396aa78
The errors are not related to this bug.

Comment 107

[[:fabrice] Fabrice Desré]

Removing checkin-needed as the M10 orange may be real.

Comment 108

[Borting Chen [:borting]]

(In reply to Fabrice Desré [:fabrice] from comment #107)
> Removing checkin-needed as the M10 orange may be real.

Do I need to run another try?

Comment 109

[[:fabrice] Fabrice Desré]

(In reply to Borting Chen [:borting] from comment #108)
> (In reply to Fabrice Desré [:fabrice] from comment #107)
> > Removing checkin-needed as the M10 orange may be real.
> 
> Do I need to run another try?

No, I retriggered the failing test another couple of times. Let's see what happens!

Comment 110

[Borting Chen [:borting]]

(In reply to Fabrice Desré [:fabrice] from comment #109)
> (In reply to Borting Chen [:borting] from comment #108)
> > (In reply to Fabrice Desré [:fabrice] from comment #107)
> > > Removing checkin-needed as the M10 orange may be real.
> > 
> > Do I need to run another try?
> 
> No, I retriggered the failing test another couple of times. Let's see what
> happens!

It seems that one test was passed and the other test failed for the same reason again.

Comment 111

[Borting Chen [:borting]]

Some more try results: https://tbpl.mozilla.org/?tree=Try&rev=d1fb77588642
It seems that test timed out occur irregularly.

Comment 112

[[:fabrice] Fabrice Desré]

Well, 4 failures in 6 runs will probably get you backed out by the sheriffs. It's not failing that often currently.

Comment 113

[Borting Chen [:borting]]

I took some try based on the patch of Bug 1025994.

Test with the patch of Bug 1025994:
  https://tbpl.mozilla.org/?tree=Try&rev=1ed62e10bf22

Plus my patch:
  https://tbpl.mozilla.org/?tree=Try&rev=d78d254ab523

It shows that the patch of Bug 1025994 can fix the timeout warning problem, and my patch can pass M10 based on that patch. So, wait Bug 1025994 land to mc.

Comment 114

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/f303b286cce8
https://hg.mozilla.org/integration/b2g-inbound/rev/1a22dd8f9d91
https://hg.mozilla.org/integration/b2g-inbound/rev/03d0a5fd13cf

Comment 115

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f303b286cce8
https://hg.mozilla.org/mozilla-central/rev/1a22dd8f9d91
https://hg.mozilla.org/mozilla-central/rev/03d0a5fd13cf

Comment 116

[Gene Lian (I already quit Mozilla)]

Congratulations! Borting! :)