Move options to observe() - #261
Conversation
|
LGTM, Would be good to have another reviewer on that one as well. |
tomayac
left a comment
There was a problem hiding this comment.
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?
|
@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:
We could consider using something else, mind filing an issue? |
|
LGTM. |
|
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 |
rakuco
left a comment
There was a problem hiding this comment.
Moving options to observe() means that subsequent calls to observe() will overwrite [[SampleInterval]], is that intentional?
|
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 |
Changing the implementation to reflect API specifications change.[1] [1]: w3c/compute-pressure#261 Bug: 333276003 Change-Id: Iebed979da7644ce1033db563972ba65b197187f6
Changing the implementation to reflect API specifications change.[1] [1]: w3c/compute-pressure#261 Bug: 333276003 Change-Id: Iebed979da7644ce1033db563972ba65b197187f6
Changing the implementation to reflect API specifications change.[1] [1]: w3c/compute-pressure#261 Bug: 333276003 Change-Id: Iebed979da7644ce1033db563972ba65b197187f6
Changing the implementation to reflect API specifications change.[1] [1]: w3c/compute-pressure#261 Bug: 333276003 Change-Id: Iebed979da7644ce1033db563972ba65b197187f6
Changing the implementation to reflect API specifications change.[1] [1]: w3c/compute-pressure#261 Bug: 333276003 Change-Id: Iebed979da7644ce1033db563972ba65b197187f6
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}
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}
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}
…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
…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
Fixes #259
Preview | Diff