Sensor objects fire 'change' event considering their own frequency hint #210

pozdnyakov · 2017-05-23T11:52:17Z

Fixes #152. Each Sensor instance fires 'change' event considering its individual
frequency hint. Thus we achieve that appearance of a new Sensor instance with a
higher frequency hint does not affect the 'onchange' notification of the existing
Sensor instances of the same type.

Preview | Diff

pozdnyakov · 2017-05-23T12:44:26Z

@tobie @anssiko please take a look. Here is the extract from #197 , should be easier to review :)

pozdnyakov · 2017-05-24T11:02:49Z

gentle ping for review

tobie · 2017-05-24T11:14:22Z

I won't be able to get to this right away. And I'd like to review it personally before merging it in. Sorry.

anssiko

LGTM with one issue identified.

anssiko · 2017-05-24T11:38:15Z

index.bs

+        1.  Abort these steps.
+    1.  Let |reportingInterval| be the result of 1 / |reportingFrequency|.
+    1.  Let |timestampDelta| be the result of [=latest reading=]["timestamp"] - |lastReportedTimestamp|.
+    1.  If  |timestampDelta| is less than or equal to |reportingInterval|


s/is less than or equal to/is greater than or equal to/

Thanks for the catch!

alexshalamov · 2017-05-24T16:59:34Z

index.bs

+        1.  Abort these steps.
+    1.  Let |reportingFrequency| be result of invoking the [=Find the reporting frequency of a Sensor=]
+        abstract operation.
+    1.  If |reportingFrequency| is not set


Find the reporting frequency of a Sensor algorithm always returns something, so, branch If |reportingFrequency| is not set should never be executed.

The Find the reporting frequency of a Sensor operation can return null

:) not set =! set to null
Maybe If |reportingFrequency| is null ? wdyt?

alexshalamov

lgtm

alexshalamov · 2017-05-29T14:21:10Z

index.bs

+            passing it |sensor_instance| as argument.
+        1.  Abort these steps.
+    1.  Let |deferUpdateTime| be the result of |reportingInterval| - |timestampDelta|.
+    1.  User agent must defer queueing a task to run the [=Notify Sensor Object about new reading=] abstract


Should there be a way to cancel this, e.g., if stop() is called?

Queuing task == sensor_task_source.task_queue.add(task);
Do we need to save 'deferred notify' and then, if stop is called, we would not queue a task?

anssiko

Reviewed the delta to the previous, LGTM with minor nits.

anssiko · 2017-05-30T06:59:32Z

index.bs

@@ -1175,6 +1175,9 @@ Gets the {{Error}} object passed to {{SensorErrorEventInit}}.
        value of |reading| to see if there's a change
        that needs to be propagated.
    1.  Unset |sensor|’s [=reporting flag=].
+    1.  [=set/For each=] |s| of |activated_sensors|,


To iterate over a list, use "For each X in Y".

anssiko · 2017-05-30T06:59:34Z

index.bs

@@ -1175,6 +1175,9 @@ Gets the {{Error}} object passed to {{SensorErrorEventInit}}.
        value of |reading| to see if there's a change
        that needs to be propagated.
    1.  Unset |sensor|’s [=reporting flag=].
+    1.  [=set/For each=] |s| of |activated_sensors|,
+        1.  Invoke the [=Report Latest Reading updated=] abstract operation,
+            passing it |s| as argument.


as an argument

pozdnyakov · 2017-05-31T05:55:21Z

@tobie, could you PTAL ? this implements the consensus approach from #152

tobie · 2017-05-31T08:25:51Z

@pozdnyakov: I did take a look. I'd like to clean-up parts of the spec before integrating this. I also don't think this is the cleanest way to do this, tbh. Let me come back to it later.

alexshalamov

As the specification has clear definition for sensor task source, I think this PR can be merged.

anssiko · 2017-06-22T15:40:50Z

LGTM.

anssiko · 2017-06-26T09:09:48Z

@alexshalamov @pozdnyakov, there has been plenty of time (>1 month) for the group to review this PR, and all feedback that suggests concrete changes have been incorporated. You should be able to merge this now.

…its individual frequency hint. Thus we achieve that appearance of a new Sensor instance with a higher frequency hint does not affect the 'onchange' notification of the existing Sensor instances of the same type.

…uency hint" This relands w3c#210

pozdnyakov requested a review from tobie May 23, 2017 11:52

pozdnyakov mentioned this pull request May 23, 2017

Rewrite Abstract Operations #197

Merged

anssiko changed the title ~~Sensor obects have their own a reading copies.~~ Sensor objects have their own a reading copies. May 24, 2017

anssiko changed the title ~~Sensor objects have their own a reading copies.~~ Sensor objects have their own reading copies May 24, 2017

anssiko self-requested a review May 24, 2017 09:55

anssiko approved these changes May 24, 2017

View reviewed changes

alexshalamov reviewed May 24, 2017

View reviewed changes

pozdnyakov mentioned this pull request May 26, 2017

Call 'onchange' on a Senor instance considering its own frequency hint #152

Closed

pozdnyakov force-pushed the Sensor.reading branch from 70bd09c to 795b489 Compare May 26, 2017 10:53

alexshalamov mentioned this pull request May 26, 2017

Use task source mechanism to dispatch events instead of firing them synchronously #215

Closed

alexshalamov approved these changes May 26, 2017

View reviewed changes

pozdnyakov force-pushed the Sensor.reading branch from 795b489 to 352d8f5 Compare May 29, 2017 14:01

pozdnyakov changed the title ~~Sensor objects have their own reading copies~~ Sensor objects fire 'change' event considering their own frequency hint May 29, 2017

pozdnyakov requested a review from anssiko May 29, 2017 14:04

alexshalamov reviewed May 29, 2017

View reviewed changes

anssiko approved these changes May 30, 2017

View reviewed changes

alexshalamov approved these changes May 30, 2017

View reviewed changes

pozdnyakov force-pushed the Sensor.reading branch from 76b2946 to 30c872e Compare June 13, 2017 13:43

alexshalamov approved these changes Jun 22, 2017

View reviewed changes

Mikhail Pozdnyakov added 2 commits June 26, 2017 12:43

Review comments from @anssiko and @alexshalamov

8c213fa

pozdnyakov force-pushed the Sensor.reading branch from 30c872e to dcc6018 Compare June 26, 2017 11:47

Rebased. Editorial changes to make the added prose consistent.

70e9f8f

pozdnyakov force-pushed the Sensor.reading branch from dcc6018 to 70e9f8f Compare June 26, 2017 11:50

pozdnyakov merged commit 177f954 into w3c:master Jun 26, 2017

pozdnyakov mentioned this pull request Jun 26, 2017

Revert "Sensor objects fire 'change' event considering their own frequency hint" #235

Merged

pozdnyakov pushed a commit to pozdnyakov/sensors that referenced this pull request Jun 26, 2017

Reland "Sensor objects fire 'change' event considering their own freq…

e0f4250

…uency hint" This relands w3c#210

This was referenced Jun 26, 2017

Reland "Sensor objects fire 'change' event considering their own freq… #236

Merged

The Sensor.lastEventFiredAt internal slot is useless #199

Closed

The Sensor.waitingForUpdate internal slot is useless #201

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sensor objects fire 'change' event considering their own frequency hint #210

Sensor objects fire 'change' event considering their own frequency hint #210

pozdnyakov commented May 23, 2017 •

edited by pr-preview bot

Loading

pozdnyakov commented May 23, 2017

pozdnyakov commented May 24, 2017

tobie commented May 24, 2017

anssiko left a comment

anssiko May 24, 2017

pozdnyakov May 24, 2017

alexshalamov May 24, 2017

pozdnyakov May 26, 2017

alexshalamov May 26, 2017

pozdnyakov May 26, 2017

alexshalamov left a comment

alexshalamov May 29, 2017

anssiko left a comment

anssiko May 30, 2017

anssiko May 30, 2017

pozdnyakov commented May 31, 2017

tobie commented May 31, 2017

alexshalamov left a comment

anssiko commented Jun 22, 2017

anssiko commented Jun 26, 2017

Sensor objects fire 'change' event considering their own frequency hint #210

Sensor objects fire 'change' event considering their own frequency hint #210

Conversation

pozdnyakov commented May 23, 2017 • edited by pr-preview bot Loading

pozdnyakov commented May 23, 2017

pozdnyakov commented May 24, 2017

tobie commented May 24, 2017

anssiko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexshalamov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anssiko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pozdnyakov commented May 31, 2017

tobie commented May 31, 2017

alexshalamov left a comment

Choose a reason for hiding this comment

anssiko commented Jun 22, 2017

anssiko commented Jun 26, 2017

pozdnyakov commented May 23, 2017 •

edited by pr-preview bot

Loading