Change sampleRate to sampleInterval #254

kenchris · 2024-03-18T13:53:52Z

Based on Origin Trial feedback pointed out during intent to ship

Closes #253

arskama · 2024-03-18T16:09:04Z

index.html

+      The {{PressureObserverOptions/sampleInterval}} member represents the requested sample
+      interval expressed in milliseconds, ie. it represents the requested interval between samples
+      to be obtained from the hardware. The [=reporting rate=] will never exceed the sampling rate which
+      is obtained by dividing 1000 by the [=requested sample interval=].


" The [=reporting rate=] will never exceed the sampling rate which is obtained by dividing 1000 by the [=requested sample interval=]."

I think there is a wrong order of words or too many "by" in the "by dividing 1000 by ..."

but you "divide by" something: https://dictionary.cambridge.org/dictionary/english/divide-by

Let me ask a native english speaker

Native speaker said "looks good to me, like literally spot on" - so I guess we are fine :)

arskama · 2024-03-19T08:38:02Z

title: %s/samplerate/sampleRate/

arskama · 2024-03-19T09:14:36Z

LGTM, but I believe another reviewer is as least needed.

kenchris · 2024-03-19T10:03:39Z

I am wondering whether minInterval would be a better name?

rakuco · 2024-03-19T13:23:15Z

index.html

@@ -811,25 +810,20 @@ The toJSON member
  The PressureObserverOptions dictionary
  

            
    dictionary PressureObserverOptions {
-      double sampleRate = 1.0;
+      [EnforceRange] unsigned long long sampleInterval = 0;


I think an unsigned long is enough here. 4294967295 (its maximum value) corresponds to something like 1190 hours.

It was mostly done for consistency with the brackground sync spec

rakuco · 2024-03-19T13:28:50Z

index.html

@@ -531,10 +533,7 @@ The constructor() method
          Set |this|.{{PressureObserver/[[Callback]]}} to |callback:PressureUpdateCallback|.
        
        

            
-          If |options|["sampleRate"] is less than or equal to 0, throw a {{RangeError}}.


0 is a valid value for unsigned long and unsigned long long. If is allowed here, it means users can make the "passes rate test" check always return true. It's not exactly wrong, so it's more like what you'd like to communicate here.

It is a bit weird that it becomes a RangeError, so I more consider it the fastest frequency possible, which the implementation can limit

Basically 0 means, give me events as soon/fast as possible

index.html

rakuco · 2024-03-19T13:57:18Z

index.html

+      The rate is measured in Hertz (cycles per second).
+    
+    

            
+      The [=requested sampling rate=] can be derived from the [=requested sample interval=]


"requested sampling rate" was ed in PressureObserverOptions, now it's not defined anywhere.

My suggestion was to move the "requested sampling interval"'s to this section and also "requested sampling rate" here, and explain how they are basically the same thing.

Should be fixed now

I still think the current for "requested sample interval" that is currently in PressureObserverOptions should be moved to this section, so that you group all related concepts in one place.

I am trying to follow you, so you don't want the dfn in the
The sampleInterval member
section as it is now (and the old one previously was)?

Correct. IMO it makes more sense to put the s for both "requested sampling rate/interval" in the "Sampling and Reporting Rate" section, so that both concepts are defined next to each other. The "sampleInterval member" section then just references the concept you defined in the "Concepts" section.

OK check now!

index.html

Integrate Raphael's changes Co-authored-by: Raphael Kubo da Costa

rakuco

lgtm with a nit!

index.html

Co-authored-by: Raphael Kubo da Costa

foolip · 2024-03-19T17:34:53Z

Thank you sorting this out, @kenchris!

Following specifications change [1], sampleRate in hertz is changed to sampleInterval in millisecond. w3c/compute-pressure#254 Bug: 330376756 Change-Id: Ibe909d537e462cd251d28c7d4d4702341dc33c0c

Following specifications change[1], sampleRate in hertz is changed to sampleInterval in millisecond. [1]:w3c/compute-pressure#254 Bug: 330376756 Change-Id: Ibe909d537e462cd251d28c7d4d4702341dc33c0c

After the change w3c#254, documentations in this repository need update.

Following specifications change [1], sampleRate in hertz is changed to sampleInterval in millisecond. w3c/compute-pressure#254 Bug: 330376756 Change-Id: Ibe909d537e462cd251d28c7d4d4702341dc33c0c

Following specifications change[1], sampleRate in hertz is changed to sampleInterval in millisecond. w3c/compute-pressure#254 Bug: 330376756 Change-Id: Ibe909d537e462cd251d28c7d4d4702341dc33c0c

Following specifications change[1], sampleRate in hertz is changed to sampleInterval in millisecond. w3c/compute-pressure#254 Bug: 330376756 Change-Id: Ibe909d537e462cd251d28c7d4d4702341dc33c0c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5381598 Reviewed-by: Fr Reviewed-by: Raphael Kubo Da Costa Commit-Queue: Arnaud Mandy Cr-Commit-Position: refs/heads/main@{#1275582}

Following specifications change[1], sampleRate in hertz is changed to sampleInterval in millisecond. w3c/compute-pressure#254 Bug: 330376756 Change-Id: Ibe909d537e462cd251d28c7d4d4702341dc33c0c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5381598 Reviewed-by: Fr Reviewed-by: Raphael Kubo Da Costa Commit-Queue: Arnaud Mandy Cr-Commit-Position: refs/heads/main@{#1275582} Co-authored-by: Arnaud Mandy

After the change #254, documentations in this repository need update.

…sts#45213) Following specifications change[1], sampleRate in hertz is changed to sampleInterval in millisecond. w3c/compute-pressure#254 Bug: 330376756 Change-Id: Ibe909d537e462cd251d28c7d4d4702341dc33c0c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5381598 Reviewed-by: Fr Reviewed-by: Raphael Kubo Da Costa Commit-Queue: Arnaud Mandy Cr-Commit-Position: refs/heads/main@{#1275582} Co-authored-by: Arnaud Mandy

…mpleInterval, a=testonly Automatic update from web-platform-tests ComputePressure: Turn sampleRate into sampleInterval (#45213) Following specifications change[1], sampleRate in hertz is changed to sampleInterval in millisecond. w3c/compute-pressure#254 Bug: 330376756 Change-Id: Ibe909d537e462cd251d28c7d4d4702341dc33c0c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5381598 Reviewed-by: Fr Reviewed-by: Raphael Kubo Da Costa Commit-Queue: Arnaud Mandy Cr-Commit-Position: refs/heads/main@{#1275582} Co-authored-by: Arnaud Mandy -- wpt-commits: 6ec99e59d4f549f28e838ec04e0aaeac43488ed1 wpt-pr: 45213

Change samplerate to sampleInterval

2d9d134

arskama reviewed Mar 18, 2024

View reviewed changes

kenchris changed the title ~~Change samplerate to sampleInterval~~ Change sampleRate to sampleInterval Mar 19, 2024

rakuco reviewed Mar 19, 2024

View reviewed changes

kenchris force-pushed the main branch from 3242364 to 15dd353 Compare March 19, 2024 14:01

Fix comments from Raphael

f469c0a

kenchris force-pushed the main branch from 15dd353 to f469c0a Compare March 19, 2024 14:31

rakuco reviewed Mar 19, 2024

View reviewed changes

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

Apply suggestions from code review

ebc1921

Integrate Raphael's changes Co-authored-by: Raphael Kubo da Costa

rakuco approved these changes Mar 19, 2024

View reviewed changes

index.html Outdated Show resolved Hide resolved

Add part that was removed

b4b254b

Co-authored-by: Raphael Kubo da Costa

kenchris merged commit 3f35f03 into w3c:main Mar 19, 2024

chromium-wpt-export-bot mentioned this pull request Mar 20, 2024

ComputePressure: Turn sampleRate into sampleInterval web-platform-tests/wpt#45213

Merged

arskama added a commit to arskama/compute-pressure that referenced this pull request Mar 20, 2024

Updating documentation from sampleRate to sampleInterval.

fd1d731

After the change w3c#254, documentations in this repository need update.

arskama mentioned this pull request Mar 20, 2024

Updating documentation from sampleRate to sampleInterval. #255

Merged

arskama added a commit that referenced this pull request Mar 21, 2024

Updating documentation from sampleRate to sampleInterval. (#255)

42f1fcd

After the change #254, documentations in this repository need update.

		@@ -531,10 +533,7 @@ The constructor() method
		Set \|this\|.{{PressureObserver/[[Callback]]}} to \|callback:PressureUpdateCallback\|.


		If \|options\|["sampleRate"] is less than or equal to 0, throw a {{RangeError}}.

		The rate is measured in Hertz (cycles per second).


		The [=requested sampling rate=] can be derived from the [=requested sample interval=]

Change sampleRate to sampleInterval #254

Change sampleRate to sampleInterval #254

Uh oh!

Conversation

kenchris commented Mar 18, 2024 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arskama Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arskama commented Mar 19, 2024

Uh oh!

arskama commented Mar 19, 2024

Uh oh!

kenchris commented Mar 19, 2024

Uh oh!

The toJSON member

The PressureObserverOptions dictionary

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

The constructor() method

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

The sampleInterval member

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rakuco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

foolip commented Mar 19, 2024

Uh oh!

Uh oh!

kenchris commented Mar 18, 2024 •

edited by pr-preview bot

Loading

arskama Mar 18, 2024 •

edited

Loading