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

Rewrite Abstract Operations #197

Merged
merged 1 commit into from Jul 5, 2017
Merged

Rewrite Abstract Operations #197

merged 1 commit into from Jul 5, 2017

Conversation

@pozdnyakov
Copy link
Contributor

pozdnyakov commented May 18, 2017

This patch fixes several issues in the abstract operations and internal slots used in the Generic Sensor API specification.

The fixes include the following:

  • Stop firing events synchronously.
  • Reset internal slots values after stop.
  • Fix logical errors in the abstract operations.
  • Recompose abstract operations so that duplication of the algorithms steps is removed.
  • Drop the unused abstract operations and internal slots.

Fixes #203
Fixes #218
Fixes #215
Fixes #204
Fixes #126
Fixes #243


Preview | Diff

@pozdnyakov pozdnyakov requested review from tobie and anssiko May 18, 2017
@tobie

This comment has been minimized.

Copy link
Member

tobie commented May 18, 2017

Overall, this changes a lot of things but doesn't hook-up into the event loop/rAF processing-model, which is going to require reorganizing a lot of stuff. I wasn't expecting such a big diff given your original PR. I really can't include it as is. Can you hold on to it until we've reorganized the spec's processing model and see if there are bits you can salvage?

@pozdnyakov

This comment has been minimized.

Copy link
Contributor Author

pozdnyakov commented May 18, 2017

Recently we were recommended [1] to decouple 'onchange' notification from rAF : / therefore I think it's not advisable any more to mention rAF processing-model in the spec...

All abstract operations are linked to each other, so I had to update most of them in order to keep the specification consistent.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=715872

@tobie

This comment has been minimized.

Copy link
Member

tobie commented May 18, 2017

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=715872

Would be nice to bring that implementer feedback to the table, so the group can discuss the different options suggested and get consensus around them.

From what I see, the conversation seems focused on 60Hz, but we do want to get faster than that. What will happen then? Etc.

@tobie

This comment has been minimized.

Copy link
Member

tobie commented May 18, 2017

I think it would be useful to open a new issue with this. Sumarize the issues, the different proposals, etc.

@pozdnyakov

This comment has been minimized.

Copy link
Contributor Author

pozdnyakov commented May 19, 2017

Would be nice to bring that implementer feedback to the table, so the group can discuss the different options suggested and get consensus around them.

yeah, I notified the group on the call yesterday.

From what I see, the conversation seems focused on 60Hz, but we do want to get faster than that. What will happen then? Etc.

The problem described in the bug is that we cannot use rAF mechanism to sync sending updates for a sensor regardless of the frequency.

60Hz limit is an implementation detail in Chromium and it is not related to this PR. This PR is fixing concrete problems for the existing operations.

I think it would be useful to open a new issue with this. Sumarize the issues, the different proposals, etc.

Wouldn't the existing #171 fit?

@tobie

This comment has been minimized.

Copy link
Member

tobie commented May 19, 2017

Filed #198.

Copy link
Member

tobie left a comment

Let's move forward with #198 before we decide on what course of action to take here.

@pozdnyakov

This comment has been minimized.

Copy link
Contributor Author

pozdnyakov commented May 19, 2017

I don't think we should defer reviewing of this PR it fixes multiple issues (see the description) that make the specification unusable at the moment.

Work on #198 can be done in parallel.

@tobie

This comment has been minimized.

Copy link
Member

tobie commented May 19, 2017

I don't think we should defer reviewing of this PR it fixes multiple issues (see the description) that make the specification unusable at the moment.

Well, the consensus of the editors here seems it's better to wait.

That said, you seemed to have uncovered a number of small issues with the spec which you've just filed. I'm happy to accept PRs for those in the meantime.

@tobie

This comment has been minimized.

Copy link
Member

tobie commented May 22, 2017

Quoting @pozdnyakov in #198:

Please note, #197 does not add or remove anything related to tying sensor polling to animation frames -- it just fixes the existing issues in abstract operations improving the current ED.

I disagree. This PR notably removes a number of inline issues that I don't think it resolves. It also introduces a large amount of changes to the ED which the current editors would prefer to see as a number of smaller pull requests, notably because the editors currently disagree with some of the issues you've filed and which this pull request claims to fix.

@pozdnyakov

This comment has been minimized.

Copy link
Contributor Author

pozdnyakov commented May 22, 2017

I disagree. This PR notably removes a number of inline issues that I don't think it resolves.

Oh, I did not mean to drop these inline issues. Would you mind converting them into issues at https://github.com/w3c/sensors/issues? If you want I can file new issues corresponding to the disappeared inline ones, think this way they'd be easier to track.

@pozdnyakov

This comment has been minimized.

Copy link
Contributor Author

pozdnyakov commented May 22, 2017

It also introduces a large amount of changes to the ED which the current editors would prefer to see as a number of smaller pull requests, notably because the editors currently disagree with some of the issues you've filed and which this pull request claims to fix.

Could you point out the changes that you think are controversial in the diff? I'll try to split them into separate PRs if possible (operations are linked together so it might be difficult to fix one of them without touching the others).
So far I see #202 to split from this PR, what would be the other items?

@tobie

This comment has been minimized.

Copy link
Member

tobie commented May 22, 2017

If you want I can file new issues corresponding to the disappeared inline ones, think this way they'd be easier to track.

That was my plan initially, but the lack of context of the issue tracker makes the issues a lot more verbose. I thought it would easier to go through those f2f with the spec in hand.

@tobie tobie added this to Needs discussion in Level 1 May 23, 2017
@tobie tobie removed this from Needs discussion in Level 1 May 23, 2017
@pozdnyakov pozdnyakov force-pushed the pozdnyakov:new_operations branch from 6583416 to 6dbf88b May 23, 2017
@pozdnyakov

This comment has been minimized.

Copy link
Contributor Author

pozdnyakov commented May 23, 2017

That was my plan initially, but the lack of context of the issue tracker makes the issues a lot more verbose. I thought it would easier to go through those f2f with the spec in hand.

I put the (non-editorial) inline issues back, and also removed #202 from the scope of this PR.
Could you please take a look?

@tobie

This comment has been minimized.

Copy link
Member

tobie commented May 23, 2017

As mentioned above, this PR introduces a larger number of changes to the spec that neither of its editors are comfortable with dealing as a bundle.

I think it would be more productive to explain what it is you want to achieve in a separate discussion, get consensus there and then proceed from it, rather than produce a huge diff and file a bunch of issues after the fact that try and justify it.

Basically, dealing with this makes a lot more work for me than understanding what it is you want to do and baking it in the spec in a way that makes sense to the spec's current organization.

As you know, PR get merged in much more quickly when they're small, focused, and have garnered agreement/consensus beforehand. This one is neither, really, and as such is a bit of a time sink. :(

@pozdnyakov

This comment has been minimized.

Copy link
Contributor Author

pozdnyakov commented May 23, 2017

As mentioned above, this PR introduces a larger number of changes to the spec that neither of its editors are comfortable with dealing as a bundle.

I think it would be more productive to explain what it is you want to achieve in a separate discussion, get consensus there and then proceed from it, rather than produce a huge diff and file a bunch of issues after the fact that try and justify it.

I supposed (possibly erroneously) that giving a final picture could make it easier to understand but, sure, we can make it a different way so, let's split this PR into smaller ones, the first one is #210. It only contains fixes for #152 and #168 and no other changes/refactoring. Please tell if this it is good enough to start review, if so I'll put the rest of the changes in follow-up patches. Thanks!

@anssiko

This comment has been minimized.

Copy link
Member

anssiko commented May 23, 2017

@tobie, I'm happy to look for volunteers to help you with the review.

@tobie

This comment has been minimized.

Copy link
Member

tobie commented May 23, 2017

I'm happy to look for volunteers to help you with the review.

I'm pretty sure you're well aware the cost of a pull request goes beyond the time to review it. As I said above, it's easier for me to take smaller PRs which stem from issues where we arrived at a consensus.

@anssiko

This comment has been minimized.

Copy link
Member

anssiko commented May 26, 2017

As I said above, it's easier for me to take smaller PRs which stem from issues where we arrived at a consensus.

The changes in this PR have interdependencies (the newly introduced abstract operations are linked together) so splitting would not work very well, and you'd be left with a PR that is harder to review.

I'd compare this PR to #156 that landed without issues and everyone was happy afterwards, and feel splitting this PR is make-work. If reviewing this PR is a bandwidth issue for the current editors (and that's fine, it just needs to be acknowledged), we should look at expanding the editorial team, seek for volunteers to review this particular PR.

Here's my proposed way forward with this PR:

  • Everyone (re-)reviews the issues this PR fixes documented in #197 (comment) and notes in the respective issue(s) their concerns. Silence is considered consent as usual. I personally reviewed the issues and saw no disagreement. I noticed we did not document the proposed solution for #152 and have asked @pozdnyakov to address that.

  • When we've addressed all the concerns raised in the issues documented in #197 (comment), the editors, reinforced with volunteers, will review this PR, land #210 that was already split out, rebase this PR and land it too.

Any concerns? Thanks for your help!

@tobie

This comment has been minimized.

Copy link
Member

tobie commented May 26, 2017

I mostly think that given the conversations happening in other issues (eg.#198, #209, etc.), this PR is premature. I also said that it would be easier for me, the editor, to have a good understanding of the architecture (which, let's be honest, is still completely in flux) and edit the spec myself, rather than pull in a huge spec change that doesn't say clearly what issues it is addressing.

So the bandwidth problem is a red herring.

If you disagree with how the editor chooses to edit the spec, you're free to bring that up with the chair, but please stop pressuring the editor into accepting changes he feels aren't appropriate at this time. It makes for a tense and unproductive work environment.

@alexshalamov

This comment has been minimized.

Copy link

alexshalamov commented May 26, 2017

The #209 is not related to this issue, lets discuss #209 in #209
I don't see any technical objections related to decoupling onchange from rAF in #198

I agree with Anssi that we need to research rAF synchronization as an opt-in feature.

@tobie

This comment has been minimized.

Copy link
Member

tobie commented May 26, 2017

The #209 is not related to this issue

That's hard to say, actually, given how big the PR is.

I don't see any technical objections related to decoupling onchange from rAF in #198

It's currently not coupled.

Ironically, the design proposed in this PR is one of the two possible designs I outline in #209. I do think this one is the right one to choose, but it would be nice to do so explicitly, rather than implicitly, hence my continuous push to edit this document once the issues a clear and there's consensus around them, i.e. as per W3C process, rather than in a pushy and rushed manner.

I agree with Anssi that we need to research rAF synchronization as an opt-in feature.

Likewise.

@@ -917,7 +913,7 @@ and "timestamp" as arguments.
1. Let |permission_state| be the result of invoking
[=request sensor access=] with |sensor_instance| as argument.
1. If |permission_state| is "granted",
1. Invoke [=register a sensor object=] with |sensor_instance| as argument.
1. Invoke [=activate a sensor object=] with |sensor_instance| as argument.

This comment has been minimized.

Copy link
@alexshalamov

alexshalamov Jun 28, 2017

[=activate a sensor object=] might be asynchronous, maybe it is better to run it as a task?

This comment has been minimized.

Copy link
@pozdnyakov

pozdnyakov Jun 30, 2017

Author Contributor

event sends together with the state update are packed into tasks.

index.bs Outdated
1. Run these sub-steps [=in parallel=]:
1. Invoke [=unregister a sensor object=] with |sensor_instance| as argument.
1. Invoke [=deactivate a sensor object=] with |sensor_instance| as argument.

This comment has been minimized.

Copy link
@alexshalamov

alexshalamov Jun 28, 2017

Same as [=activate a sensor object=], might be better to run it inside task scope.

@@ -1020,8 +1015,7 @@ Gets the {{Error}} object passed to {{SensorErrorEventInit}}.
1. If the [=browsing context=] is not a [=top-level browsing context=], then:
1. [=throw=] a {{SecurityError}}.
1. Let |sensor_instance| be a new {{Sensor}} object,
1. If [=sensor=] [=supports periodic reporting mode=] and
|options|.{{frequency!!dict-member}} is [=present=], then
1. If |options|.{{frequency!!dict-member}} is [=present=], then

This comment has been minimized.

Copy link
@alexshalamov

alexshalamov Jun 28, 2017

Do we need 'else' branch? UA may select default frequency.

index.bs Outdated
@@ -1400,7 +1245,9 @@ in order to reduce resource consumption, notably battery usage.
: output
:: None

1. Set |sensor_instance|.{{[[state]]}} to "idle".
1. If |sensor_instance|.{{[[state]]}} is "activated",
Invoke [=deactivate a sensor object=] with |s| as argument.

This comment has been minimized.

Copy link
@alexshalamov

alexshalamov Jun 28, 2017

Enqueue [=deactivate a sensor object=] task?

index.bs Outdated
@@ -678,6 +678,9 @@ This set is initially [=set/is empty|empty=].
A [=sensor=] has an associated <dfn>latest reading</dfn> [=ordered map|map=]
which holds the latest available [=sensor readings=].

Any time the [=latest reading=] is updated, the UA invokes [=Report latest reading updated=]

This comment has been minimized.

Copy link
@alexshalamov

alexshalamov Jun 28, 2017

invokes [=Report latest reading updated=] (operation | algorithm) for every... wdyt?

This comment has been minimized.

Copy link
@pozdnyakov

pozdnyakov Jun 30, 2017

Author Contributor

rephrased in the latest patch.

index.bs Outdated
@@ -867,8 +863,8 @@ with the internal slots described in the following table:
</tr>
<tr>
<td><dfn attribute for=Sensor>\[[waitingForUpdate]]</dfn></td>
<td>A boolean which indicates whether the observers have been updated
or whether the object is waiting for a new reading to do so.
<td>A boolean which indicates whether the observers need to be

This comment has been minimized.

Copy link
@alexshalamov

alexshalamov Jun 28, 2017

I think, semantics has changed a bit. It's not "waiting for update" flag, it is "Notify sensor object about new reading task is still running". Maybe we can rename this internal slot?

This comment has been minimized.

Copy link
@pozdnyakov

pozdnyakov Jun 29, 2017

Author Contributor

"pendingReadingNotification" ?

This comment has been minimized.

Copy link
@pozdnyakov

pozdnyakov Jun 30, 2017

Author Contributor

done.

@alexshalamov

This comment has been minimized.

Copy link

alexshalamov commented Jun 29, 2017

I updated PR description. Some issues are fixed already.

@pozdnyakov pozdnyakov force-pushed the pozdnyakov:new_operations branch from e635e0e to 43fc46a Jun 30, 2017
index.bs Outdated
or whether the object is waiting for a new reading to do so.
<td><dfn attribute for=Sensor>\[[pendingReadingNotification]]</dfn></td>
<td>A boolean which indicates whether the observers need to be
updated after a new [=sensor reading=] was reported.

This comment has been minimized.

Copy link
@alexshalamov

alexshalamov Jun 30, 2017

updated after => notified after?

This comment has been minimized.

Copy link
@pozdnyakov

pozdnyakov Jun 30, 2017

Author Contributor

done.

index.bs Outdated
correctly diagnosing the reason for the rejection
and might lead to confusing instructions to the user,
but it is a tradeoff some User Agent might choose to make. -->
1. Queue a task to run [=handle Errors=] with |e| and |sensor_instance| as arguments.

This comment has been minimized.

Copy link
@alexshalamov

alexshalamov Jun 30, 2017

handle errors => notify error?

This comment has been minimized.

Copy link
@pozdnyakov

pozdnyakov Jun 30, 2017

Author Contributor

done.

index.bs Outdated

: input
:: |sensor_instance|, a {{Sensor}} object.
: output
:: None

1. Remove all [=tasks=] with |sensor_instance| argument from the [=task queue=] associated

This comment has been minimized.

Copy link
@alexshalamov

alexshalamov Jun 30, 2017

Remove all tasks with sensor_instance argument from ...

Remove all tasks associated with sensor_instance from ...?

This comment has been minimized.

Copy link
@pozdnyakov

pozdnyakov Jun 30, 2017

Author Contributor

done.

…d internal slots used in the Sensor interface specification.

The refactoring goals are:

- Stop firing events synchronously.
- Reset internal slots values after stop.
- Fix logical errors in the abstract operations.
- Recompose abstract operations so that duplication of the algorithms steps is removed.
- Drop the unused abstract operations and internal slots.

Fixes #203
Fixes #218
Fixes #215
Fixes #204
Fixes #126
Fixes #243
@pozdnyakov pozdnyakov force-pushed the pozdnyakov:new_operations branch from 43fc46a to ed5d3d3 Jun 30, 2017
@pozdnyakov

This comment has been minimized.

Copy link
Contributor Author

pozdnyakov commented Jun 30, 2017

@rwaldron @reillyeon could you take a look?

@alexshalamov alexshalamov requested a review from rwaldron Jul 3, 2017
@pozdnyakov

This comment has been minimized.

Copy link
Contributor Author

pozdnyakov commented Jul 5, 2017

Link to a dedicated thread in the WG mailing list https://lists.w3.org/Archives/Public/public-device-apis/2017Jul/0000.html

@pozdnyakov pozdnyakov merged commit 833880d into w3c:master Jul 5, 2017
1 check passed
1 check passed
ipr PR deemed acceptable.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.