993311 - Convert Network Stats API to WebIDL

Status: RESOLVED FIXED

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

Description

[John Shih]

This bug is a follow-up of Bug 986837, which is to convent the remaining part of NetworkStats API to WebIDL.

Comment 1

[John Shih]

Attachment: Bug 993311 - Convert Network Stats API to WebIDL. r=bzbarsky

Comment 2

[John Shih]

Attachment: Bug 993311 - Convert Network Stats API to WebIDL. r=bzbarsky

Comment 3

[John Shih]

Attachment: Bug 993311 - Convert Network Stats API to WebIDL. r=bzbarsky

Comment 4

[(no longer active)]

Please don't land this unless bug 983502 has been fixed, this will break the MarketPlace feature detection without that bug.

Comment 5

[John Shih]

Attachment: Bug 993311 - Convert Network Stats API to WebIDL. r=bzbarsky

Comment 6

[John Shih]

Try result: https://tbpl.mozilla.org/?tree=Try&rev=4797a798c00b

Comment 7

[John Shih]

Attachment: Bug 993311 - Convert Network Stats API to WebIDL. r=bzbarsky

fix typo

Comment 8

[John Shih]

Attachment: Bug 993311 - Convert Network Stats API to WebIDL. r=bzbarsky

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8405216 [details] [diff] [review]
Bug 993311 - Convert Network Stats API to WebIDL. r=bzbarsky

>+++ b/dom/network/src/NetworkStatsManager.js
>+    if (aAlarmId == 0 || typeof aAlarmId == 'undefined') {
>       aAlarmId = -1;

If you want "not passed" to be treated as 1, just make -1 the default value in the IDL, please:

  DOMRequest removeAlarms(optional long alarmId = -1);

>+++ b/dom/network/tests/test_networkstats_alarms.html
>+      ok(ex == msg, "addAlarm() throws \"" + msg + "\" when no parameters");

  is(ex, msg, "addAlarm() throws \"" + msg + "\" when no parameters");

and similar in the other three places.

>+++ b/dom/network/tests/test_networkstats_enabled_no_perm.html

As Ehsan said, these changes break the feature detection story we have right this moment.  It might be better to go ahead and expose the object even if the permission is not available, but have it end up null in that case.

>+++ b/dom/network/tests/test_networkstats_enabled_perm.html
>+  ok(typeof navigator.mozNetworkStats === 'object',

That would test true for null as well.  Maybe better to test that it's instanceof the right thing?

>+++ b/dom/webidl/NetworkStatsManager.webidl
>+  DOMRequest getSamples(any network,
>+                        any start,
>+                        any end,
>+               optional any options /* NetworkStatsGetOptions */);

Are there really no more restrictive types than "any" here?  That seems a bit weird to me.

It seems like it would make more sense if start/end were Date and network were MozNetworkStatsInterface.  Then you can remove the corresponding checks from the implementation.

options should be a NetworkStatsGetOptions, for sure, like the comment says.

>+  DOMRequest addAlarm(any network,
>+                      long threshold,
>+             optional any options /* NetworkStatsAlarmOptions */);

Similar here: use better types for network and options.

>+  DOMRequest getAllAlarms(optional any network);

And here.

r-, since I think the uses of "any" there are wrong...

Comment 10

[John Shih]

(In reply to :Ehsan Akhgari (@work week, needinfo? me!) from comment #4)
> Please don't land this unless bug 983502 has been fixed, this will break the
> MarketPlace feature detection without that bug.

Hi Ehsan,

Can you elaborate more on the "break" you mentioned? Although bug 983502 is landed, I still not sure about the relation between that bug and this work. Thanks!

Comment 11

[(no longer active)]

(In reply to John Shih from comment #10)
> (In reply to :Ehsan Akhgari (@work week, needinfo? me!) from comment #4)
> > Please don't land this unless bug 983502 has been fixed, this will break the
> > MarketPlace feature detection without that bug.
> 
> Hi Ehsan,
> 
> Can you elaborate more on the "break" you mentioned? Although bug 983502 is
> landed, I still not sure about the relation between that bug and this work.
> Thanks!

Currently marketplace uses the fact that we expose some of these types of properties on navigator and set their values to null when you lack enough permissions to access it as an indication that the platform supports these feature if you had sufficient permissions.  Moving these APIs to WebIDL will properly hide them from the non-privileged pages, hence breaking marketplace.

We're trying to address this issue using navigator.getFeature().  Bug 983502 happened to implement the parts of that API which are not useful for this use case, I filed bug 1009645 for implementing the interesting bits, and moved the dependencies to that bug.

Sorry for the confusion!

Comment 12

[Bobby Holley (:bholley)]

Ehsan, is this ready to go, or do we also need to block on a server-side Marketplace bug?

Comment 13

[(no longer active)]

(In reply to Bobby Holley (:bholley) from comment #12)
> Ehsan, is this ready to go, or do we also need to block on a server-side
> Marketplace bug?

Wil?  I'm not sure how MarketPlace is going to use this, but from the Gecko perspective, I think it can land now.  But I'll let Wil confirm.

Comment 14

[Wil Clouser [:clouserw]]

Marking as blocked on the Marketplace bug

Comment 15

[(no longer active)]

We can now convert this all to WebIDL, there is no market place dependency to worry about any more.

Comment 16

[Peter Van der Beken [:peterv]]

Let's drive this in.

Comment 17

[Peter Van der Beken [:peterv]]

Attachment: Convert MozNetworkStatsManager to WebIDL

Comment 18

[Peter Van der Beken [:peterv]]

Comment on attachment 8713683 [details] [diff] [review]
Convert MozNetworkStatsManager to WebIDL

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

I've named the interface MozNetworkStatsManager, as all the related interfaces kept the Moz prefix when they were converted to WebIDL.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8713683 [details] [diff] [review]
Convert MozNetworkStatsManager to WebIDL

>+++ b/dom/network/NetworkStatsManager.js

>+    let appManifestURL = aOptions.appManifestURL;
>+    let serviceType = aOptions.serviceType;
>+    let browsingTrafficOnly = aOptions.browsingTrafficOnly;

This is a subtle behavior change.  These used to default to null, null, false.  now they default to undefined.

Seems to me like you should keep the old defaults, either explicitly here or via adding default values to the dictionary.

r=me with that addressed.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/216059cd4bca

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ddc1096f248

Comment 22

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

Landed https://hg.mozilla.org/integration/mozilla-inbound/rev/3ddc1096f248 for Gregor to fix up b2g builds after the webidl conversion.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/216059cd4bca
https://hg.mozilla.org/mozilla-central/rev/3ddc1096f248