Fix issue 033506: correct droppedEntriesCount #33538

wbamberg · 2024-05-10T16:23:59Z

@tunetheweb , here's a PR for your issue.

I wasn't sure how to talk about optionality though. Usually on MDN "optional" applies to things like function parameters and indicates to developers that they don't have to pass a value (and that there will be some default behavior). But for callbacks things are a little different, because it's more like "will my callback receive this value". So I'm always a bit uncomfortable using the same construct here and think we should try to explain it.

Although options is marked optional in the IDL (https://w3c.github.io/performance-timeline/#ref-for-dom-performanceobservercallback-3) I wasn't sure how to interpret that. As a developer, I'd want to know: "if it is optional, under what circumstances would it be omitted?".

Also, it reads to me as if options itself is always passed, but the droppedEntriesCount property might be omitted:

Let callbackOptions be a PerformanceObserverCallbackOptions with its droppedEntriesCount set to droppedEntriesCount if droppedEntriesCount is not null, otherwise unset.

Call po’s observer callback with observerEntryList as the first argument, with po as the second argument and as callback this value, and with callbackOptions as the third argument. If this throws an exception, report the exception.

(https://w3c.github.io/performance-timeline/#queue-the-performanceobserver-task)

github-actions · 2024-05-10T16:25:26Z

Preview URLs

/en-US/docs/Web/API/PerformanceObserver/PerformanceObserver

(comment last updated: 2024-05-14 05:24:03)

wbamberg · 2024-05-10T16:38:30Z

Ah, I just saw #33506 (comment), that is helpful!

Pushed 6f58c70 to cover this. I'm not sure the example is ideal but it at least reinforces the first-time-ness of it.

tunetheweb

Thanks! Glad I didn't try to tackle this myself as would have created all sorts of extra pages instead of trying to do it in this one page!!

Made some comments.

tunetheweb · 2024-05-10T17:35:45Z

files/en-us/web/api/performanceobserver/performanceobserver/index.md

-      - : The number of buffered entries which got dropped from the buffer due to the buffer being full. See the [`buffered`](/en-US/docs/Web/API/PerformanceObserver/observe#parameters) flag.
+    - `options`
+
+      - : An object with the following properties:


Maybe optional here?

Suggested change

- : An object with the following properties:

- : An optional object with the following properties:

If we want to do this we should instead use the {{optional_inline}} macro after the parameter name (like this: https://developer.mozilla.org/en-US/docs/Web/API/Request/Request#options).

But, 2 things.

I talked about this a bit in the PR description: "optional" is weird when we're talking about callback parameters. When we use "optional" for a normal API method the semantics is clear: developers don't have to supply a value, and if they don't, some default is used. But with a callback, the developers aren't calling it, so how are developers supposed to interpret "optional"? I think it is probably better, rather than "optional", to describe exactly why/under which circumstances the parameter is not passed, so developers know when they can access it.

Although this parameter is marked optional in the IDL, I can't understand from the spec how it is optional. AFAICS the relevant bit is https://w3c.github.io/performance-timeline/#queue-the-performanceobserver-task, which has:

Call po’s observer callback with observerEntryList as the first argument, with po as the second argument and as callback this value, and with callbackOptions as the third argument. If this throws an exception, report the exception.

I don't see any indication there that options may not be passed. It will be empty, for all but the first callback invocation, based on steps 7 and 8. So what am I missing here?

That's fair that it will be empty rather than missing. I guess I'm just trying to avoid the original confusion that was raised in #33506 where the user was confused why it wasn't there as they expected it to contain a value every time (where the droppedEntriesCount would be 0 every time after the first—that's not the case). I can understand that confusion. But maybe with all your other changes it's sufficient.

files/en-us/web/api/performanceobserver/performanceobserver/index.md

wbamberg · 2024-05-10T18:16:59Z

Thank you for the comments! I'm out for the rest of the weekend but will take a look on Monday! 👍

…dex.md Co-authored-by: Barry Pollard <barrypollard@google.com>

…dex.md

…dex.md Co-authored-by: Barry Pollard <barrypollard@google.com>

wbamberg · 2024-05-14T00:22:15Z

Thanks for your comments, please take another look :).

files/en-us/web/api/performanceobserver/performanceobserver/index.md

…dex.md

tunetheweb

LGTM with one more suggestion. But good to merge either way. Thanks fow working on this!

tunetheweb · 2024-05-20T10:35:07Z

files/en-us/web/api/performanceobserver/performanceobserver/index.md

+
+          - : The number of entries which could not be recorded because the {{domxref("Performance")}} object's internal buffer was full.
+
+            Note that this is only provided the first time the observer calls the callback, when the buffered entries are replayed. Once the observer starts making future observations, it no longer needs to use the buffer. After the first time, `options` will be an empty object (`{}`).


Shoudl this note be de-indented as it applies more to the whole options rather than droppedEntriesCount?

Suggested change

Note that this is only provided the first time the observer calls the callback, when the buffered entries are replayed. Once the observer starts making future observations, it no longer needs to use the buffer. After the first time, `options` will be an empty object (`{}`).

Note that this is only provided the first time the observer calls the callback, when the buffered entries are replayed. Once the observer starts making future observations, it no longer needs to use the buffer. After the first time, `options` will be an empty object (`{}`).

I think it applies more to droppedEntriesCount specifically though. When it says "Note that this is only provided", the referent of "this" is droppedEntriesCount, not options.

And the second sentence is only about droppedEntriesCount as well - we might imagine a future in which other properties get added to options (which is presumably the point of making this an object), and they might have nothing to do with buffering, and then tying this sentence to options as a whole would look wrong.

Yeah agree it's a little ambiguous and kinda applies to both at the moment. It's the last sentence that specifically applies to the options section rather than droppedEntriesCount .

Anyway, let's go with what's there now.

Yeah, I agree, the last sentence still bothers me a bit here. I would happily omit it, but am OK with it like this as well.

I agree that the last sentence is a little awkward and would be happy to omit it - it's implicitly true from the other stuff, and has the chance of becoming false if more options are ever added. But I also think it's OK to keep it.

wbamberg · 2024-05-21T01:56:14Z

@hamishwillee , would you mind taking a look at this?

Fix issue 033506: correct droppedEntriesCount

608b487

wbamberg requested a review from a team as a code owner May 10, 2024 16:23

wbamberg requested review from sideshowbarker and removed request for a team May 10, 2024 16:24

github-actions bot added Content:WebAPI Web API docs size/s 6-50 LoC changed labels May 10, 2024

Clarify when dropped entries are provided

6f58c70

tunetheweb reviewed May 10, 2024

View reviewed changes

wbamberg and others added 5 commits May 13, 2024 16:07

Update files/en-us/web/api/performanceobserver/performanceobserver/in…

c02d7f2

…dex.md Co-authored-by: Barry Pollard <barrypollard@google.com>

Update files/en-us/web/api/performanceobserver/performanceobserver/in…

7dfecd9

…dex.md Co-authored-by: Barry Pollard <barrypollard@google.com>

Update files/en-us/web/api/performanceobserver/performanceobserver/in…

2d0785c

…dex.md

Update files/en-us/web/api/performanceobserver/performanceobserver/in…

faeb8ee

…dex.md Co-authored-by: Barry Pollard <barrypollard@google.com>

Fix dl markup

a67561b

wbamberg requested a review from tunetheweb May 14, 2024 00:21

Fix issue 033506: add a page on CLS

e426e52

wbamberg requested a review from a team as a code owner May 14, 2024 05:01

github-actions bot added the Content:Glossary Glossary entries label May 14, 2024

Remove CLS page

4cdceca

github-actions bot removed the Content:Glossary Glossary entries label May 14, 2024

wbamberg commented May 14, 2024

View reviewed changes

files/en-us/web/api/performanceobserver/performanceobserver/index.md Outdated Show resolved Hide resolved

Update files/en-us/web/api/performanceobserver/performanceobserver/in…

cb93001

…dex.md

tunetheweb reviewed May 20, 2024

View reviewed changes

wbamberg requested review from hamishwillee and removed request for sideshowbarker May 21, 2024 01:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue 033506: correct droppedEntriesCount #33538

Fix issue 033506: correct droppedEntriesCount #33538

wbamberg commented May 10, 2024 •

edited

github-actions bot commented May 10, 2024 •

edited

wbamberg commented May 10, 2024

tunetheweb left a comment

tunetheweb May 10, 2024

wbamberg May 13, 2024

tunetheweb May 13, 2024

wbamberg commented May 10, 2024

wbamberg commented May 14, 2024

tunetheweb left a comment

tunetheweb May 20, 2024

wbamberg May 20, 2024 •

edited

tunetheweb May 20, 2024

wbamberg May 21, 2024

wbamberg May 21, 2024

wbamberg commented May 21, 2024

	- : An object with the following properties:
	- : An optional object with the following properties:


		- : The number of entries which could not be recorded because the {{domxref("Performance")}} object's internal buffer was full.

		Note that this is only provided the first time the observer calls the callback, when the buffered entries are replayed. Once the observer starts making future observations, it no longer needs to use the buffer. After the first time, `options` will be an empty object (`{}`).

Fix issue 033506: correct droppedEntriesCount #33538

Are you sure you want to change the base?

Fix issue 033506: correct droppedEntriesCount #33538

Conversation

wbamberg commented May 10, 2024 • edited

github-actions bot commented May 10, 2024 • edited

wbamberg commented May 10, 2024

tunetheweb left a comment

Choose a reason for hiding this comment

tunetheweb May 10, 2024

Choose a reason for hiding this comment

wbamberg May 13, 2024

Choose a reason for hiding this comment

tunetheweb May 13, 2024

Choose a reason for hiding this comment

wbamberg commented May 10, 2024

wbamberg commented May 14, 2024

tunetheweb left a comment

Choose a reason for hiding this comment

tunetheweb May 20, 2024

Choose a reason for hiding this comment

wbamberg May 20, 2024 • edited

Choose a reason for hiding this comment

tunetheweb May 20, 2024

Choose a reason for hiding this comment

wbamberg May 21, 2024

Choose a reason for hiding this comment

wbamberg May 21, 2024

Choose a reason for hiding this comment

wbamberg commented May 21, 2024

wbamberg commented May 10, 2024 •

edited

github-actions bot commented May 10, 2024 •

edited

wbamberg May 20, 2024 •

edited