Closed Bug 986837 Opened 6 years ago Closed 6 years ago

Port most of the network stats API to WebIDL

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: ehsan, Assigned: johnshih.bugs)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(6 files, 27 obsolete files)

9.62 KB, text/plain
Details
7.93 KB, patch
johnshih.bugs
: review+
Details | Diff | Splinter Review
23.68 KB, patch
johnshih.bugs
: review+
Details | Diff | Splinter Review
5.71 KB, patch
johnshih.bugs
: review+
Details | Diff | Splinter Review
7.33 KB, patch
johnshih.bugs
: review+
Details | Diff | Splinter Review
7.31 KB, patch
johnshih.bugs
: review+
Details | Diff | Splinter Review
We cannot yet port MozNetworkStatsManager as it breaks the Market Place feature detection.
Attachment #8395230 - Flags: review?(bzbarsky)
Attachment #8395231 - Flags: review?(bzbarsky)
Attachment #8395232 - Flags: review?(bzbarsky)
Attachment #8395233 - Flags: review?(bzbarsky)
Comment on attachment 8395230 [details] [diff] [review]
Part 1: Port MozNetworkStatsData to WebIDL; r=bzbarsky

Gotta make this pass tests on try...
Attachment #8395230 - Attachment is obsolete: true
Attachment #8395230 - Flags: review?(bzbarsky)
Attachment #8395231 - Attachment is obsolete: true
Attachment #8395231 - Flags: review?(bzbarsky)
Attachment #8395232 - Attachment is obsolete: true
Attachment #8395232 - Flags: review?(bzbarsky)
Attachment #8395233 - Attachment is obsolete: true
Attachment #8395233 - Flags: review?(bzbarsky)
Attachment #8395452 - Flags: review?(bzbarsky)
Attachment #8395453 - Flags: review?(bzbarsky)
Attachment #8395454 - Flags: review?(bzbarsky)
Attachment #8395455 - Flags: review?(bzbarsky)
Blocks: SH-APIs
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.
Attachment #8395453 - Flags: review?(bzbarsky) → review+
Comment on attachment 8395454 [details] [diff] [review]
Part 3: Port MozNetworkStatsAlarm to WebIDL; r=bzbarsky

r=me
Attachment #8395454 - Flags: review?(bzbarsky) → review+
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.
Attachment #8395455 - Flags: review?(bzbarsky) → review+
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.
Attachment #8395452 - Flags: review?(bzbarsky)
(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!
Attachment #8395453 - Attachment is obsolete: true
Attachment #8395455 - Attachment is obsolete: true
Attachment #8395452 - Attachment is obsolete: true
Comment on attachment 8400246 [details] [diff] [review]
Part 1: Port MozNetworkStatsData to WebIDL; r=bzbarsky

(rebased)
Attachment #8400246 - Flags: review?(bzbarsky)
Comment on attachment 8400238 [details] [diff] [review]
Part 2: Port MozNetworkStatsInterface to WebIDL; r=bzbarsky

I messed up getAllAlarms() because aNetwork is optional there.
Attachment #8400238 - Attachment is obsolete: true
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
Attachment #8400246 - Flags: review?(bzbarsky) → review+
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...)
Flags: needinfo?(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.
Flags: needinfo?(jonas)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(acperez)
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!
Flags: needinfo?(bochen)
(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.
Flags: needinfo?(acperez)
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
Assignee: ehsan → nobody
(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?
Flags: needinfo?(acperez)
Albert, if you don't mind I can work on this, which I think it was almost done by Ehsan.
Assignee: nobody → jshih
Attachment #8400246 - Attachment is obsolete: true
Attachment #8402566 - Flags: review?(bzbarsky)
Attachment #8402002 - Attachment is obsolete: true
Attachment #8402567 - Flags: review?(bzbarsky)
Attachment #8395454 - Attachment is obsolete: true
Attachment #8400369 - Attachment is obsolete: true
Attachment #8402003 - Attachment is obsolete: true
Attachment #8402569 - Flags: review?(bzbarsky)
Attachment #8400245 - Attachment is obsolete: true
Attachment #8402004 - Attachment is obsolete: true
Attachment #8402571 - Flags: review?(bzbarsky)
I think you are already working on it. It is fine :)

Let me know if you need any help
Flags: needinfo?(acperez)
(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.
Blocks: 993311
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.
Attachment #8403141 - Attachment is obsolete: true
Attachment #8403857 - Flags: review?(bzbarsky)
(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
Attachment #8402566 - Flags: review?(bzbarsky) → review+
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.
Attachment #8402567 - Flags: review?(bzbarsky) → review+
Comment on attachment 8402569 [details] [diff] [review]
Bug 986837 - Part 3: Port MozNetworkStatsAlarm to WebIDL. r=bzbarsky

r=me
Attachment #8402569 - Flags: review?(bzbarsky) → review+
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.
Attachment #8402571 - Flags: review?(bzbarsky) → review+
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.
Attachment #8403857 - Flags: review?(bzbarsky) → review+
Flags: needinfo?(bochen)
patch revised. r=bz
Attachment #8402566 - Attachment is obsolete: true
Attachment #8414962 - Flags: review+
Boris, could you take one more look to make sure the modification works for you? Thanks!
Attachment #8402567 - Attachment is obsolete: true
Attachment #8414963 - Flags: review?(bzbarsky)
patch revised. r=bz
Attachment #8402569 - Attachment is obsolete: true
Attachment #8414964 - Flags: review+
patch revised. r=bz
Attachment #8402571 - Attachment is obsolete: true
Attachment #8414965 - Flags: review+
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
Attachment #8403857 - Attachment is obsolete: true
Attachment #8414968 - Flags: review?(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.
Flags: needinfo?(jshih)
> 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
Flags: needinfo?(jshih)
(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.
Attachment #8414968 - Attachment is obsolete: true
Attachment #8414968 - Flags: review?(bzbarsky)
Attachment #8415054 - Flags: review?(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 on attachment 8414963 [details] [diff] [review]
Bug 986837 - Part 2: Port MozNetworkStatsInterface to WebIDL. r=bzbarsky

r=me if you rev the IIDs.
Attachment #8414963 - Flags: review?(bzbarsky) → review+
Comment on attachment 8415054 [details] [diff] [review]
Bug 986837 - Part 5: Expose MozNetworkInterface for testing. r=bzbarsky

r=me
Attachment #8415054 - Flags: review?(bzbarsky) → review+
rebase. r=bz
Attachment #8414962 - Attachment is obsolete: true
Attachment #8417315 - Flags: review+
rev IIDs. r=bz
Attachment #8414963 - Attachment is obsolete: true
Attachment #8417316 - Flags: review+
rebase. r=bz
Attachment #8414964 - Attachment is obsolete: true
Attachment #8417317 - Flags: review+
rebase. r=bz
Attachment #8414965 - Attachment is obsolete: true
Attachment #8417319 - Flags: review+
rebase. r=bz
Attachment #8415054 - Attachment is obsolete: true
Attachment #8417321 - Flags: review+
Keywords: checkin-needed
(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!
Depends on: 1020305
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.
Keywords: dev-doc-needed
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.