986837 - Port most of the network stats API to WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[(no longer active)]

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

Comment 1

[(no longer active)]

Attachment: Part 1: Port MozNetworkStatsData to WebIDL; r=bzbarsky

Comment 2

[(no longer active)]

Attachment: Part 2: Port MozNetworkStatsInterface to WebIDL; r=bzbarsky

Comment 3

[(no longer active)]

Attachment: Part 3: Port MozNetworkStatsAlarm to WebIDL; r=bzbarsky

Comment 4

[(no longer active)]

Attachment: Part 4: Port MozNetworkStats to WebIDL; r=bzbarsky

Comment 5

[(no longer active)]

Comment on attachment 8395230 [details] [diff] [review]
Part 1: Port MozNetworkStatsData to WebIDL; r=bzbarsky

Gotta make this pass tests on try...

Comment 6

[(no longer active)]

Attachment: Part 1: Port MozNetworkStatsData to WebIDL; r=bzbarsky

Comment 7

[(no longer active)]

Attachment: Part 2: Port MozNetworkStatsInterface to WebIDL; r=bzbarsky

Comment 8

[(no longer active)]

Attachment: Part 3: Port MozNetworkStatsAlarm to WebIDL; r=bzbarsky

Comment 9

[(no longer active)]

Attachment: Part 4: Port MozNetworkStats to WebIDL; r=bzbarsky

Comment 10

[(no longer active)]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

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 12

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 13

[Boris Zbarsky [:bzbarsky]]

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 14

[Boris Zbarsky [:bzbarsky]]

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.

Comment 15

[(no longer active)]

(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 16

[(no longer active)]

Attachment: Part 2: Port MozNetworkStatsInterface to WebIDL; r=bzbarsky

Comment 17

[(no longer active)]

Attachment: Part 4: Port MozNetworkStats to WebIDL; r=bzbarsky

Comment 18

[(no longer active)]

Attachment: Part 1: Port MozNetworkStatsData to WebIDL; r=bzbarsky

Comment 19

[(no longer active)]

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

(rebased)

Comment 20

[(no longer active)]

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

Comment 21

[(no longer active)]

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 22

[(no longer active)]

Attachment: Part 3: Port MozNetworkStatsAlarm to WebIDL; r=bzbarsky

Comment 23

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=e1c488778cc3

Comment 24

[Boris Zbarsky [:bzbarsky]]

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

Comment 25

[(no longer active)]

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...)

Comment 26

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 27

[Boris Zbarsky [:bzbarsky]]

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

Comment 28

[(no longer active)]

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.

Comment 29

[(no longer active)]

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

Comment 30

[John Shih]

(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.

Comment 31

[(no longer active)]

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?

Comment 32

[John Shih]

(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!

Comment 33

[John Shih]

(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 ;)

Comment 34

[Boris Zbarsky [:bzbarsky]]

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).

Comment 35

[Albert [:albert]]

(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.

Comment 36

[Boris Zbarsky [:bzbarsky]]

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.

Comment 37

[Albert [:albert]]

(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.

Comment 38

[Albert [:albert]]

(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?

Comment 39

[Boris Zbarsky [:bzbarsky]]

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

Comment 40

[(no longer active)]

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

Comment 41

[(no longer active)]

Attachment: Part 2: Port MozNetworkStatsInterface to WebIDL; r=bzbarsky

Comment 42

[(no longer active)]

Attachment: Part 3: Port MozNetworkStatsAlarm to WebIDL; r=bzbarsky

Comment 43

[(no longer active)]

Attachment: Part 4: Port MozNetworkStats to WebIDL; r=bzbarsky

Comment 44

[(no longer active)]

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

Comment 45

[Boris Zbarsky [:bzbarsky]]

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

Comment 46

[John Shih]

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

Comment 47

[John Shih]

Attachment: Bug 986837 - Part 1: Port MozNetworkStatsData to WebIDL. r=bzbarsky

Comment 48

[John Shih]

Attachment: Bug 986837 - Part 2: Port MozNetworkStatsInterface to WebIDL. r=bzbarsky

Comment 49

[John Shih]

Attachment: Bug 986837 - Part 3: Port MozNetworkStatsAlarm to WebIDL. r=bzbarsky

Comment 50

[John Shih]

Attachment: Bug 986837 - Part 4: Port MozNetworkStats to WebIDL. r=bzbarsky

Comment 51

[Albert [:albert]]

I think you are already working on it. It is fine :)

Let me know if you need any help

Comment 52

[John Shih]

(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...

Comment 53

[Albert [:albert]]

(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 54

[John Shih]

Attachment: Bug 986836 - Part 5: Expose MozNetworkInterface for testing. r=bzbarsky

Comment 55

[(no longer active)]

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.

Comment 56

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 57

[John Shih]

Attachment: Bug 986836 - Part 5: Expose MozNetworkInterface for testing. r=bzbarsky

Patch update. The test passed on my local test.

Comment 58

[John Shih]

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

Comment 59

[(no longer active)]

(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 60

[Boris Zbarsky [:bzbarsky]]

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 61

[Boris Zbarsky [:bzbarsky]]

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 62

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 63

[Boris Zbarsky [:bzbarsky]]

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 64

[Boris Zbarsky [:bzbarsky]]

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.

Comment 65

[John Shih]

Attachment: Bug 986837 - Part 1: Port MozNetworkStatsData to WebIDL. r=bzbarsky

patch revised. r=bz

Comment 66

[John Shih]

Attachment: Bug 986837 - Part 2: Port MozNetworkStatsInterface to WebIDL. r=bzbarsky

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

Comment 67

[John Shih]

Attachment: Bug 986837 - Part 3: Port MozNetworkStatsAlarm to WebIDL. r=bzbarsky

patch revised. r=bz

Comment 68

[John Shih]

Attachment: Bug 986837 - Part 4: Port MozNetworkStats to WebIDL. r=bzbarsky

patch revised. r=bz

Comment 69

[John Shih]

Attachment: Bug 986836 - Part 5: Expose MozNetworkInterface for testing. r=bzbarsky

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 70

[Boris Zbarsky [:bzbarsky]]

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.

Comment 71

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 72

[John Shih]

Attachment: bug_986837_part2_diff.txt

diff file

Comment 73

[John Shih]

(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 74

[John Shih]

Attachment: Bug 986837 - Part 5: Expose MozNetworkInterface for testing. r=bzbarsky

Comment 75

[Boris Zbarsky [:bzbarsky]]

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 76

[Boris Zbarsky [:bzbarsky]]

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 77

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 78

[John Shih]

Attachment: Bug 986837 - Part 1: Port MozNetworkStatsData to WebIDL. r=bzbarsky

rebase. r=bz

Comment 79

[John Shih]

Attachment: Bug 986837 - Part 2: Port MozNetworkStatsInterface to WebIDL. r=bzbarsky

rev IIDs. r=bz

Comment 80

[John Shih]

Attachment: Bug 986837 - Part 3: Port MozNetworkStatsAlarm to WebIDL. r=bzbarsky

rebase. r=bz

Comment 81

[John Shih]

Attachment: Bug 986837 - Part 4: Port MozNetworkStats to WebIDL. r=bzbarsky

rebase. r=bz

Comment 82

[John Shih]

Attachment: bug_986837_part5.patch

rebase. r=bz

Comment 83

[John Shih]

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

Comment 84

[Ryan VanderMeulen [:RyanVM]]

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

Comment 85

[Ryan VanderMeulen [:RyanVM]]

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

Comment 86

[John Shih]

(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!

Comment 87

[(no longer active)]

Thanks John for finishing this!

Comment 88

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 89

[Boris Zbarsky [:bzbarsky]]

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.