Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop exposing Sensor state. #180

Merged
merged 4 commits into from
Apr 5, 2017
Merged

Stop exposing Sensor state. #180

merged 4 commits into from
Apr 5, 2017

Conversation

tobie
Copy link
Member

@tobie tobie commented Apr 4, 2017

References #160.


Preview | Diff

@@ -592,7 +592,6 @@ A [=sensor=] has an associated <dfn>current polling frequency</dfn> which is ini
<pre class="idl">
[SecureContext]
interface Sensor : EventTarget {
readonly attribute SensorState state;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it be modified into boolean activated, like you and @rwaldron discussed at #160 (comment) or we'll leave it not exposed for now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just avoid exposing it for now. That's the kind of precise feedback I'd like to see from origin trials, though (maybe we could even expose both there and see which ones devs end up using?)

Copy link

@pozdnyakov pozdnyakov Apr 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In origin trials the API call statistics is collected, so IMO it is preferable to leave something and see whether it is called or not. I would personally leave the activated attribute: it is minimalistic and perfectly aligned with the rest of API. Exposing both is also possible, so I'm fine with any decision regarding this :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, my worry about basing this on stats is that actual usage will depend highly on what documentation we release before we go to origin trial. @kenchris any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with exposing the activated attribute. Then we can also see whether people use it or not or need something more powerful

index.bs Outdated
The getter of the {{Sensor/activated!!attribute}} attribute must run these steps
or their [=equivalent=]:

1. If <emu-val>this</emu-val>.{{[[state]]}} is "activating",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"activated"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh. Wednesday is the day I take care of the kids. It shows. :D

@pozdnyakov
Copy link

lgtm!

@tobie tobie merged commit 77710e9 into w3c:master Apr 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants