Skip to content
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

Fix issue 033506: correct droppedEntriesCount #33538

Merged
merged 10 commits into from
May 28, 2024
Merged

Conversation

wbamberg
Copy link
Collaborator

@wbamberg wbamberg commented May 10, 2024

Fixes #33506.

@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)

@wbamberg wbamberg requested a review from a team as a code owner May 10, 2024 16:23
@wbamberg wbamberg requested review from sideshowbarker and removed request for a team May 10, 2024 16:24
@github-actions github-actions bot added Content:WebAPI Web API docs size/s [PR only] 6-50 LoC changed labels May 10, 2024
Copy link
Contributor

github-actions bot commented May 10, 2024

Preview URLs

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

@wbamberg
Copy link
Collaborator Author

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.

Copy link
Contributor

@tunetheweb tunetheweb left a comment

Choose a reason for hiding this comment

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

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.

- : 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:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe optional here?

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

  1. 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.

  2. 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:

  1. 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@wbamberg
Copy link
Collaborator Author

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

@wbamberg wbamberg requested a review from tunetheweb May 14, 2024 00:21
@wbamberg
Copy link
Collaborator Author

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

@wbamberg wbamberg requested a review from a team as a code owner May 14, 2024 05:01
@github-actions github-actions bot added the Content:Glossary Glossary entries label May 14, 2024
@github-actions github-actions bot removed the Content:Glossary Glossary entries label May 14, 2024
Copy link
Contributor

@tunetheweb tunetheweb left a comment

Choose a reason for hiding this comment

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

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


- : 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 (`{}`).
Copy link
Contributor

Choose a reason for hiding this comment

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

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 (`{}`).

Copy link
Collaborator Author

@wbamberg wbamberg May 20, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 wbamberg requested review from hamishwillee and removed request for sideshowbarker May 21, 2024 01:55
@wbamberg
Copy link
Collaborator Author

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

Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

Thanks @wbamberg and @tunetheweb 👍

@Elchi3 Elchi3 merged commit 65fc487 into mdn:main May 28, 2024
8 checks passed
wbamberg added a commit to wbamberg/content that referenced this pull request May 28, 2024
* upstream/main: (55 commits)
  Replace `.` with `#` in example given selectors are `#ids` (mdn#33791)
  update info in cross browser testing strategies (mdn#33730)
  Clarify that `navigator.storage.persist()` depends on heuristics (mdn#33780)
  fix typo (mdn#33785)
  feat: improvements on Glossary/Hoisting (mdn#33787)
  CSS update: overview of shapes guide (mdn#33771)
  CSS update: Shapes from box values (mdn#33770)
  Fix issue 033506: correct droppedEntriesCount (mdn#33538)
  Revert "=== Symbol("foo")" (mdn#33782)
  docs(css): FF126 - Support for `shape()` function (mdn#33446)
  Bump lint-staged from 15.2.4 to 15.2.5 (mdn#33777)
  Bump ajv from 8.13.0 to 8.14.0 (mdn#33776)
  Add missing spaces for `subtlecrypto` (mdn#33774)
  fix: typo in `color_and_luminca` (mdn#33775)
  feat: improvments on gutters (mdn#33751)
  FF127Relnote- data: and javascript: URLS forbidden in base HREF (mdn#33738)
  update the content of SVG `<view>` element (mdn#33710)
  Clipboard.write() - log and fixes (mdn#33769)
  updated ClipboardItem and Clipboard documentation and examples using … (mdn#33262)
  Fix error in the code snippet for Symbol (mdn#33765)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs size/s [PR only] 6-50 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

droppedEntriesCount documentation incorrect
3 participants