Skip to content

Associate sample interval with source #263

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.

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

Merged
merged 2 commits into from
Apr 9, 2024
Merged

Associate sample interval with source #263

merged 2 commits into from
Apr 9, 2024

Conversation

kenchris
Copy link
Contributor

@kenchris kenchris commented Apr 8, 2024

Sample interval should be per source, and updated when observe() is called with the same source again.

This is consistent with what PerformanceObserver does


Preview | Diff

@arskama arskama requested a review from rakuco April 8, 2024 13:34
@@ -1041,7 +1049,7 @@ <h3>Supporting algorithms</h3>
Let |record:PressureRecord| be |observer|.{{PressureObserver/[[LastRecordMap]]}}[|source|].
</li>
<li>
Let |sampleInterval| be |observer|.{{PressureObserver/[[SampleInterval]]}}.
Let |sampleInterval| be |observer|.{{PressureObserver/[[SampleIntervalMap]]}}[|source|].
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to check if this key exists just like you do for [[LastRecordMap]]? I don't fully remember if there can be a condition where the corresponding entry is removed before this algorithm is called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as far as I can see, no. It always exists as there is a default value and we only remove it as we stop observing the source.

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.

One other thing: you probably mean "Associate" instead of "Associated" in the title.

@arskama
Copy link
Contributor

arskama commented Apr 9, 2024

This is just an internal change? doesn't modify yet the way the user enter the source in observe method? right?

@kenchris kenchris changed the title Associated sample interval with source Associate sample interval with source Apr 9, 2024
@arskama
Copy link
Contributor

arskama commented Apr 9, 2024

LGTM

@kenchris kenchris merged commit bfe7f14 into w3c:main Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants