Closed Bug 993311 Opened 6 years ago Closed 4 years ago

Convert Network Stats API to WebIDL

Categories

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

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: johnshih.bugs, Assigned: peterv)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 6 obsolete files)

This bug is a follow-up of Bug 986837, which is to convent the remaining part of NetworkStats API to WebIDL.
Attachment #8403137 - Attachment is obsolete: true
Depends on: 986837
Blocks: 981845
Attachment #8403138 - Attachment is obsolete: true
Please don't land this unless bug 983502 has been fixed, this will break the MarketPlace feature detection without that bug.
Depends on: 983502
Attachment #8403885 - Attachment is obsolete: true
Attachment #8404619 - Flags: review?(bzbarsky)
fix typo
Attachment #8404619 - Attachment is obsolete: true
Attachment #8404619 - Flags: review?(bzbarsky)
Attachment #8405145 - Flags: review?(bzbarsky)
Attachment #8405145 - Attachment is obsolete: true
Attachment #8405145 - Flags: review?(bzbarsky)
Attachment #8405216 - Flags: review?(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...
Attachment #8405216 - Flags: review?(bzbarsky) → review-
(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!
Flags: needinfo?(ehsan)
Depends on: 1009645
No longer depends on: 983502
(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!
Flags: needinfo?(ehsan)
Ehsan, is this ready to go, or do we also need to block on a server-side Marketplace bug?
Flags: needinfo?(ehsan)
(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.
Flags: needinfo?(ehsan) → needinfo?(clouserw)
Marking as blocked on the Marketplace bug
Depends on: 983880
Flags: needinfo?(clouserw)
Depends on: 1240865
We can now convert this all to WebIDL, there is no market place dependency to worry about any more.
Let's drive this in.
Assignee: johnshih.bugs → peterv
Status: NEW → ASSIGNED
Attachment #8405216 - Attachment is obsolete: true
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.
Attachment #8713683 - Flags: review?(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.
Attachment #8713683 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/216059cd4bca
https://hg.mozilla.org/mozilla-central/rev/3ddc1096f248
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Blocks: 1245091
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.