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 <dfn>requested sample
+      interval</dfn> 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 @@ <h3>The <dfn>toJSON</dfn> member</h3>
  <h3>The <dfn>PressureObserverOptions</dfn> dictionary</h3>
  <pre class="idl">
    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 @@ <h3>The <dfn>constructor()</dfn> method</h3>
          Set |this|.{{PressureObserver/[[Callback]]}} to |callback:PressureUpdateCallback|.
        </li>
        <li>
-          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).
+    </p>
+    <p>
+      The [=requested sampling rate=] can be derived from the [=requested sample interval=]


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

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

Should be fixed now

I still think the current <dfn> 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 <dfn>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 <[email protected]>

rakuco

lgtm with a nit!

index.html

Co-authored-by: Raphael Kubo da Costa <[email protected]>

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 <[email protected]> Reviewed-by: Raphael Kubo Da Costa <[email protected]> Commit-Queue: Arnaud Mandy <[email protected]> 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 <[email protected]> Reviewed-by: Raphael Kubo Da Costa <[email protected]> Commit-Queue: Arnaud Mandy <[email protected]> Cr-Commit-Position: refs/heads/main@{#1275582} Co-authored-by: Arnaud Mandy <[email protected]>

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 <[email protected]> Reviewed-by: Raphael Kubo Da Costa <[email protected]> Commit-Queue: Arnaud Mandy <[email protected]> Cr-Commit-Position: refs/heads/main@{#1275582} Co-authored-by: Arnaud Mandy <[email protected]>

…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 <[email protected]> Reviewed-by: Raphael Kubo Da Costa <[email protected]> Commit-Queue: Arnaud Mandy <[email protected]> Cr-Commit-Position: refs/heads/main@{#1275582} Co-authored-by: Arnaud Mandy <[email protected]> -- 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 <[email protected]>

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 <[email protected]>

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

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.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change sampleRate to sampleInterval #254

Change sampleRate to sampleInterval #254

kenchris commented Mar 18, 2024 •

edited by pr-preview bot

Loading

arskama Mar 18, 2024 •

edited

Loading

kenchris Mar 19, 2024

kenchris Mar 19, 2024

kenchris Mar 19, 2024

arskama commented Mar 19, 2024

arskama commented Mar 19, 2024

kenchris commented Mar 19, 2024

rakuco Mar 19, 2024

kenchris Mar 19, 2024

rakuco Mar 19, 2024

kenchris Mar 19, 2024

kenchris Mar 19, 2024

rakuco Mar 19, 2024

kenchris Mar 19, 2024

rakuco Mar 19, 2024

kenchris Mar 19, 2024

rakuco Mar 19, 2024

kenchris Mar 19, 2024

rakuco left a comment

foolip commented Mar 19, 2024

Change sampleRate to sampleInterval #254

Change sampleRate to sampleInterval #254

Conversation

kenchris commented Mar 18, 2024 • edited by pr-preview bot Loading

arskama Mar 18, 2024 • edited Loading

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

arskama commented Mar 19, 2024

arskama commented Mar 19, 2024

kenchris commented Mar 19, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

The sampleInterval member

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakuco left a comment

Choose a reason for hiding this comment

foolip commented Mar 19, 2024

kenchris commented Mar 18, 2024 •

edited by pr-preview bot

Loading

arskama Mar 18, 2024 •

edited

Loading