KEMBAR78
Move options to observe() by kenchris · Pull Request #261 · w3c/compute-pressure · GitHub
Skip to content

Move options to observe() #261

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 5, 2024
Merged

Move options to observe() #261

merged 1 commit into from
Apr 5, 2024

Conversation

kenchris
Copy link
Contributor

@kenchris kenchris commented Apr 5, 2024 •
edited by pr-preview bot
Loading

Fixes #259

Preview | Diff

@arskama
Copy link
Contributor

arskama commented Apr 5, 2024

LGTM, Would be good to have another reviewer on that one as well.

tomayac
tomayac reviewed Apr 5, 2024
View reviewed changes
Copy link
Contributor

@tomayac tomayac left a comment

Choose a reason for hiding this comment

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

The changes need to be reflected in the Explainer as well (two instances).

Do I read it correctly that the sampleInterval if no value is given is set to 0? What does this mean? That it will never run?

@Elchi3 Elchi3 mentioned this pull request Apr 5, 2024
Closed
@kenchris
Copy link
Contributor Author

kenchris commented Apr 5, 2024

@tomayac Arnaud has another PR up for changing the explainer etc.

If it is 0, it will get as fast as the system can handle:

If timeDeltaMilliseconds ≥ sampleInterval, return true, otherwise return false.

We could consider using something else, mind filing an issue?

@darktears
Copy link

LGTM.

@kenchris
Copy link
Contributor Author

kenchris commented Apr 5, 2024

https://wicg.github.io/periodic-background-sync/#periodic-sync-registration-minimum-interval <- also uses minInterval of 0 (as fast as system allows)

https://wicg.github.io/js-self-profiling/#the-profilersample-dictionary says it must be 0 or bigger, but doesn't even check whether it is not undefined

@kenchris kenchris merged commit 41ec61b into w3c:main Apr 5, 2024
rakuco
rakuco reviewed Apr 8, 2024
View reviewed changes
Copy link
Member

@rakuco rakuco left a comment

Choose a reason for hiding this comment

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

Moving options to observe() means that subsequent calls to observe() will overwrite [[SampleInterval]], is that intentional?

@kenchris
Copy link
Contributor Author

kenchris commented Apr 8, 2024 •
edited
Loading

Yes, for the same source type that makes sense, but we need to spec things differently when we support multiple sources. We do support two sources now in the spec, but makes sense to postpone this change till after shipping, to not give @arskama unneeded work as this is purely a spec change for now.

PerformanceObserver works the same way. A consecutive call to observe is seen as an update, and in the case some update is incompatible (like moving from one type to multiple) which is not a problem for us, then they throw "InvalidModificationError".

Anyway, here is a PR we can merge when we decide to #263

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 8, 2024
@arskama @chromium-wpt-export-bot
compute pressure: Move Options to Observe method
5895c1f 
Changing the implementation to reflect API specifications change.[1]

[1]: w3c/compute-pressure#261

Bug: 333276003
Change-Id: Iebed979da7644ce1033db563972ba65b197187f6
@chromium-wpt-export-bot chromium-wpt-export-bot mentioned this pull request Apr 8, 2024
Merged
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 8, 2024
@arskama @chromium-wpt-export-bot
compute pressure: Move Options to Observe method
71a5f25 
Changing the implementation to reflect API specifications change.[1]

[1]: w3c/compute-pressure#261

Bug: 333276003
Change-Id: Iebed979da7644ce1033db563972ba65b197187f6
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 8, 2024
@arskama @chromium-wpt-export-bot
compute pressure: Move Options to Observe method
d2e3f6f 
Changing the implementation to reflect API specifications change.[1]

[1]: w3c/compute-pressure#261

Bug: 333276003
Change-Id: Iebed979da7644ce1033db563972ba65b197187f6
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 9, 2024
@arskama @chromium-wpt-export-bot
compute pressure: Move Options to Observe method
a0e4a59 
Changing the implementation to reflect API specifications change.[1]

[1]: w3c/compute-pressure#261

Bug: 333276003
Change-Id: Iebed979da7644ce1033db563972ba65b197187f6
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 10, 2024
@arskama @chromium-wpt-export-bot
compute pressure: Move Options to Observe method
c5b08c0 
Changing the implementation to reflect API specifications change.[1]

[1]: w3c/compute-pressure#261

Bug: 333276003
Change-Id: Iebed979da7644ce1033db563972ba65b197187f6
aarongable pushed a commit to chromium/chromium that referenced this pull request Apr 10, 2024
@arskama
compute pressure: Move Options to Observe method
27e6aab 
Changing the implementation to reflect API specifications change.[1]

[1]: w3c/compute-pressure#261

Bug: 333276003
Change-Id: Iebed979da7644ce1033db563972ba65b197187f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5434554
Reviewed-by: Raphael Kubo Da Costa <raphael.kubo.da.costa@intel.com>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Arnaud Mandy <arnaud.mandy@intel.com>
Cr-Commit-Position: refs/heads/main@{#1284973}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 10, 2024
@arskama @chromium-wpt-export-bot
compute pressure: Move Options to Observe method
e54be54 
Changing the implementation to reflect API specifications change.[1]

[1]: w3c/compute-pressure#261

Bug: 333276003
Change-Id: Iebed979da7644ce1033db563972ba65b197187f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5434554
Reviewed-by: Raphael Kubo Da Costa <raphael.kubo.da.costa@intel.com>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Arnaud Mandy <arnaud.mandy@intel.com>
Cr-Commit-Position: refs/heads/main@{#1284973}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 10, 2024
@arskama @chromium-wpt-export-bot
compute pressure: Move Options to Observe method
b502725 
Changing the implementation to reflect API specifications change.[1]

[1]: w3c/compute-pressure#261

Bug: 333276003
Change-Id: Iebed979da7644ce1033db563972ba65b197187f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5434554
Reviewed-by: Raphael Kubo Da Costa <raphael.kubo.da.costa@intel.com>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Arnaud Mandy <arnaud.mandy@intel.com>
Cr-Commit-Position: refs/heads/main@{#1284973}
@Elchi3 Elchi3 mentioned this pull request Apr 12, 2024
Merged
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 19, 2024
@arskama @moz-wptsync-bot
Bug 1890280 [wpt PR 45608] - compute pressure: Move Options to Observ…
cb47d18 
…e method, a=testonly

Automatic update from web-platform-tests
compute pressure: Move Options to Observe method

Changing the implementation to reflect API specifications change.[1]

[1]: w3c/compute-pressure#261

Bug: 333276003
Change-Id: Iebed979da7644ce1033db563972ba65b197187f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5434554
Reviewed-by: Raphael Kubo Da Costa <raphael.kubo.da.costa@intel.com>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Arnaud Mandy <arnaud.mandy@intel.com>
Cr-Commit-Position: refs/heads/main@{#1284973}

--

wpt-commits: b502725c15ef80cc8179a31a1e868ad7112bcc28
wpt-pr: 45608
jwidar pushed a commit to jwidar/LatencyZeroGithub that referenced this pull request Sep 16, 2025
@arskama @moz-wptsync-bot
Bug 1890280 [wpt PR 45608] - compute pressure: Move Options to Observ…
27db51d 
…e method, a=testonly

Automatic update from web-platform-tests
compute pressure: Move Options to Observe method

Changing the implementation to reflect API specifications change.[1]

[1]: w3c/compute-pressure#261

Bug: 333276003
Change-Id: Iebed979da7644ce1033db563972ba65b197187f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5434554
Reviewed-by: Raphael Kubo Da Costa <raphael.kubo.da.costa@intel.com>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Arnaud Mandy <arnaud.mandy@intel.com>
Cr-Commit-Position: refs/heads/main@{#1284973}

--

wpt-commits: b502725c15ef80cc8179a31a1e868ad7112bcc28
wpt-pr: 45608
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tomayac tomayac tomayac left review comments

+1 more reviewer

@rakuco rakuco rakuco left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Move Options to observe?

5 participants

@kenchris @arskama @darktears @rakuco @tomayac