We cannot yet port MozNetworkStatsManager as it breaks the Market Place feature detection.

Comment on attachment 8395230 [details] [diff] [review] Part 1: Port MozNetworkStatsData to WebIDL; r=bzbarsky Gotta make this pass tests on try...

Green b2g test run: https://tbpl.mozilla.org/?tree=Try&rev=f436abb60c41

Comment on attachment 8395453 [details] [diff] [review] Part 2: Port MozNetworkStatsInterface to WebIDL; r=bzbarsky Shouldn't we have an instanceof check on the argument in addAlarm, like we do in getSamples? Similar in getAllAlarms and clearStats. r=me with those added or their lack explained.

Comment on attachment 8395454 [details] [diff] [review] Part 3: Port MozNetworkStatsAlarm to WebIDL; r=bzbarsky r=me

Comment on attachment 8395455 [details] [diff] [review] Part 4: Port MozNetworkStats to WebIDL; r=bzbarsky This changes the semantics a bit: it'll create a new Date object every time .start or .end is gotten, instead of returning the internal Date object stored in the object. Yes, the WebIDL "Date" type sucks. :( You may want to keep start/end as "object" instead of Date, esp. since they're already content-side Date objects. r=me with that fixed.

Comment on attachment 8395452 [details] [diff] [review] Part 1: Port MozNetworkStatsData to WebIDL; r=bzbarsky Per IRC discussion, I'd like to see a patch merged to tip here.

(In reply to Boris Zbarsky [:bz] from comment #11) > Comment on attachment 8395453 [details] [diff] [review] > Part 2: Port MozNetworkStatsInterface to WebIDL; r=bzbarsky > > Shouldn't we have an instanceof check on the argument in addAlarm, like we > do in getSamples? > > Similar in getAllAlarms and clearStats. Because I suck!

Comment on attachment 8400246 [details] [diff] [review] Part 1: Port MozNetworkStatsData to WebIDL; r=bzbarsky (rebased)

New try run: https://tbpl.mozilla.org/?tree=Try&rev=98d4fa501306

Comment on attachment 8400238 [details] [diff] [review] Part 2: Port MozNetworkStatsInterface to WebIDL; r=bzbarsky I messed up getAllAlarms() because aNetwork is optional there.

Comment on attachment 8400246 [details] [diff] [review] Part 1: Port MozNetworkStatsData to WebIDL; r=bzbarsky The "date" member should probably be of type "object" with a comment about how IDL Date doesn't do what you'd think... r=me

This test: <http://mxr.mozilla.org/mozilla-central/source/dom/network/tests/test_networkstats_alarms.html?force=1#41> suggests that I should not check the network argument to getAllAlarms at all. What do you think, Boris? Is that fine with you? (It's a very weird API I've got to say...)

> What do you think, Boris? I think you need to talk to whoever created this API. It's possible that they mean the argument to getAllAlarms() (and perhaps some of the others) to be a dictionary, not an interface, but simply couldn't express that in xpidl. Or it's possible that the test is just wrong.

And in particular, note that scriptable xpidl interfaces just accept any object so can act a lot more like dictionaries than interfaces....

OK, so we cannot do this check on addAlarm either if we want to maintain the current behavior. :( <https://tbpl.mozilla.org/?tree=Try&rev=eed559f67b5a> <https://tbpl.mozilla.org/php/getParsedLog.php?id=37142274&tree=Try#error0> <http://mxr.mozilla.org/mozilla-central/source/dom/network/tests/test_networkstats_alarms.html?force=1#78> It really looks like the network interface is supposed to be a dictionary, which basically means I should rewrite part if not all of my patches here.

Borting, do you know who would be the right person to answer comment 26? Thanks!

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #29) > Borting, do you know who would be the right person to answer comment 26? > Thanks! Hi Ehsan, I think we just take nsIDOMMozNetworkStatsInterface as a dictionary, instead of an interface. We don't do anything special in NetworkStatsService after receiving the network argument (i.g., it is just taken as a js obeject without going trough any interface query). I bet that's why the test failed.

OK, thanks John. So, MozNetworkStatsInterface is used as a readonly attribute in other parts of this API (for example, MozNetworkStats.network). Should I convert them to be |object| or something?

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #31) > OK, thanks John. > > So, MozNetworkStatsInterface is used as a readonly attribute in other parts > of this API (for example, MozNetworkStats.network). Yes. In the previous API version, we only use a single string to specify 'wifi' or 'mobile'. Then, due the requirement of supporting mult-sim, we change the argument to the combination of connection type and icc id. > Should I convert them to be |object| or something? I think it's fine!

(In reply to John Shih from comment #32) > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment > #31) > > OK, thanks John. > > > > So, MozNetworkStatsInterface is used as a readonly attribute in other parts > > of this API (for example, MozNetworkStats.network). > > Yes. In the previous API version, we only use a single string to specify > 'wifi' or 'mobile'. Then, due the requirement of supporting mult-sim, we > change the argument to the combination of connection type and icc id. > > > Should I convert them to be |object| or something? > > I think it's fine! I mean, just make it compatible with js object ;)

You could leave the interface for return values if you want, btw. That's just an API design decision... (e.g. in terms of whether we want all the return values to share a proto that's not just Object.prototype).

(In reply to Boris Zbarsky [:bz] from comment #26) > > What do you think, Boris? > > I think you need to talk to whoever created this API. It's possible that > they mean the argument to getAllAlarms() (and perhaps some of the others) to > be a dictionary, not an interface, but simply couldn't express that in > xpidl. Or it's possible that the test is just wrong. Hi guys, MozNetworkStatsInterface was designed to be an interface not a dictionary. I think there is a mistake in the implementation of the tests. In costcontrol it is used as an interface.

OK. Note that the costcontrol tests in gaia also treat it as a dictionary, because they're not passing in actual MozNetworkStatsInterface objects... But as far as I can tell, the actual costcontrol app is in fact passing in interface instances.

(In reply to Boris Zbarsky [:bz] from comment #36) > OK. Note that the costcontrol tests in gaia also treat it as a dictionary, > because they're not passing in actual MozNetworkStatsInterface objects... > But as far as I can tell, the actual costcontrol app is in fact passing in > interface instances. Then they had the same mistake regarding the tests, because I just ask them to ensure they are using the interface and they say yes. I'm gonna to advice them.

(In reply to Boris Zbarsky [:bz] from comment #36) > OK. Note that the costcontrol tests in gaia also treat it as a dictionary, > because they're not passing in actual MozNetworkStatsInterface objects... > But as far as I can tell, the actual costcontrol app is in fact passing in > interface instances. If you define it as a interface I'm going to open a bug to modify costcontrol tests because we are going to have problems when your patches land. Are you agree?

If we start enforcing it being an interface then we need to fix the tests, yes.

OK, so I read up to comment 33, spent some time to rewrite parts 2-4 based on that, and came here to attach my patches just to see that we want this to be an interface again. I'm going to give up, attach my patches, and let someone else do what they will with them. Here's the latest try run with MozNetworkStatsInterface being a dictionary: https://tbpl.mozilla.org/?tree=Try&rev=a338dbf49c86

(Note to the next person, the three recent patches should probably be reviewed again if you want to use them.)

I would appreciate whoever owns this API stepping up and fixing it... Albert, do you know who that is?

Albert, if you don't mind I can work on this, which I think it was almost done by Ehsan.

I think you are already working on it. It is fine :) Let me know if you need any help

(In reply to Albert [:albert] from comment #51) > I think you are already working on it. It is fine :) > > Let me know if you need any help The remaining problem is in test_networkstats_alarms.html. As previous discussion, we now should use real MozNetworkStatsInterface instead of js object for testing getAllAlarms(). But I'm not sure if we can create that instance directly...

(In reply to John Shih from comment #52) > (In reply to Albert [:albert] from comment #51) > > I think you are already working on it. It is fine :) > > > > Let me know if you need any help > > The remaining problem is in test_networkstats_alarms.html. > As previous discussion, we now should use real MozNetworkStatsInterface > instead of js object for testing getAllAlarms(). But I'm not sure if we can > create that instance directly... Same problem in costcontrol tests, we can't create an instance, we need to expose the MozNetworkStatsInterface, like mozContact for example.

Comment on attachment 8403141 [details] [diff] [review] Bug 986836 - Part 5: Expose MozNetworkInterface for testing. r=bzbarsky I'm not sure what this patch is trying to achieve but we almost definitely don't want to have both an interface and a dictionary to specify the same thing.

> but we almost definitely don't want to have both an interface and a dictionary to specify > the same thing We may in fact want just that if we want to have a canonical interface thing we return but want to accept anything that quacks a certain way.

Patch update. The test passed on my local test.

Try result: https://tbpl.mozilla.org/?tree=Try&rev=4737cf4791ad

(In reply to comment #56) > > but we almost definitely don't want to have both an interface and a dictionary to specify > > the same thing > > We may in fact want just that if we want to have a canonical interface thing we > return but want to accept anything that quacks a certain way. That's a bit mind-bending... but sure.

Comment on attachment 8402566 [details] [diff] [review] Bug 986837 - Part 1: Port MozNetworkStatsData to WebIDL. r=bzbarsky Sorry this took so long.... r=me

Comment on attachment 8402567 [details] [diff] [review] Bug 986837 - Part 2: Port MozNetworkStatsInterface to WebIDL. r=bzbarsky Need to rev the IIDs of nsIDOMMozNetworkStats and nsIDOMMozNetworkStatsAlarm. >+ nsIDOMDOMRequest getSamples(in jsval network, I believe that needs to be nsISupports, not jsval, for the instanceof test it does to mean anything. >+ nsIDOMDOMRequest addAlarm(in jsval network, Why doesn't this need an instanceof test? If it means to accept arbitrary objects that quack a certain way, please document that. >+ nsIDOMDOMRequest getAllAlarms([optional] in jsval network); Similar. >+ nsIDOMDOMRequest clearStats(in jsval network); And here. >+ // This should be used when passing network objects to the parent process. The comment should explain why. Is it because we end up structured cloning? Does that ignore toJSON methods, so adding "jsonifier" on the interface would not help? >+ // MozNetworkStatsInterface launch an exception "throw", not "launch". r=me with those dealt with.

Comment on attachment 8402569 [details] [diff] [review] Bug 986837 - Part 3: Port MozNetworkStatsAlarm to WebIDL. r=bzbarsky r=me

Comment on attachment 8402571 [details] [diff] [review] Bug 986837 - Part 4: Port MozNetworkStats to WebIDL. r=bzbarsky >+ readonly attribute Date start; >+ readonly attribute Date end; See comment 13. r=me with that fixed.

Comment on attachment 8403857 [details] [diff] [review] Bug 986836 - Part 5: Expose MozNetworkInterface for testing. r=bzbarsky I would think the constructor argument here should be a dictionary, not "any". I also assume that in practice HasNetworkStatsSupport returns false for web pages and the like, right? r=me with the constructor argument fixed if that assumption is correct.

patch revised. r=bz

Boris, could you take one more look to make sure the modification works for you? Thanks!

patch revised. r=bz

patch revised. r=bz

Hi Boris, According to Comment 64, I think change 'any' to 'dictionary' doesn't work (or we may need a redundant dictionary as I did in previous patch [1]). So I just change 'any' to 'object' here, what do you think? By the way, I'm little confused with the assumption you mentioned (i.e., I also assume that in practice HasNetworkStatsSupport returns false for web pages and the like, right?). If it returns false for web pages, how could we use it in mochitest? Can you also clarify on this? Really appreciate! [1] https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=986837&attachment=8403141

Comment on attachment 8414963 [details] [diff] [review] Bug 986837 - Part 2: Port MozNetworkStatsInterface to WebIDL. r=bzbarsky What exact modifications did you make? Please post an interdiff against the last version I reviewed.

> I think change 'any' to 'dictionary' doesn't work Why not? > or we may need a redundant dictionary Redundant in what sense? > So I just change 'any' to 'object' here, what do you think? I think that doesn't help anything. Have you audited the code to make sure it's OK with this.type and this.id being arbitrary page-controlled objects? > If it returns false for web pages, how could we use it in mochitest? Mochitest presumably sets the permissions. What I was assuming is that normal web pages never have that permission.

diff file

(In reply to Boris Zbarsky [:bz] from comment #71) > I think that doesn't help anything. Have you audited the code to make sure > it's OK with this.type and this.id being arbitrary page-controlled objects? > Hmm, I know your concern and just realize that you supported my previous patch (Comment 56). I'll change back to it. > > If it returns false for web pages, how could we use it in mochitest? > > Mochitest presumably sets the permissions. What I was assuming is that > normal web pages never have that permission. Got it, you are right.

Comment on attachment 8415036 [details] bug_986837_part2_diff.txt Thanks. You still need to rev the IIDs. Please do make sure to do that before landing!

Comment on attachment 8414963 [details] [diff] [review] Bug 986837 - Part 2: Port MozNetworkStatsInterface to WebIDL. r=bzbarsky r=me if you rev the IIDs.

Comment on attachment 8415054 [details] [diff] [review] Bug 986837 - Part 5: Expose MozNetworkInterface for testing. r=bzbarsky r=me

rebase. r=bz

rev IIDs. r=bz

rebase. r=bz

rebase. r=bz

rebase. r=bz

test result: https://tbpl.mozilla.org/?tree=Try&rev=61facc8b71a6

https://hg.mozilla.org/integration/b2g-inbound/rev/52bbcb23a7cc https://hg.mozilla.org/integration/b2g-inbound/rev/41a097ac9d6f https://hg.mozilla.org/integration/b2g-inbound/rev/2ececf8578f4 https://hg.mozilla.org/integration/b2g-inbound/rev/28d11c3268d3

Part 5 had the wrong bug number originally. https://hg.mozilla.org/integration/b2g-inbound/rev/fdd739afef78

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #85) > Part 5 had the wrong bug number originally. > https://hg.mozilla.org/integration/b2g-inbound/rev/fdd739afef78 Thanks for correcting that!

Thanks John for finishing this!

https://hg.mozilla.org/mozilla-central/rev/52bbcb23a7cc https://hg.mozilla.org/mozilla-central/rev/41a097ac9d6f https://hg.mozilla.org/mozilla-central/rev/2ececf8578f4 https://hg.mozilla.org/mozilla-central/rev/28d11c3268d3 https://hg.mozilla.org/mozilla-central/rev/fdd739afef78

These changes made https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIDOMMozNetworkStats mostly lies as far as I can tell... We need to update the docs.