Revise descriptions for ReportingObserver() report types#29475
Conversation
|
Tip: Review these changes grouped by change (recommended for most PRs), or grouped by feature (for large PRs). |
| ], | ||
| "support": { | ||
| "chrome": { | ||
| "version_added": "145" |
There was a problem hiding this comment.
This is a bit weird, as
crashreports aren't observed by reporting observers, but I think you can still specify them as a valid report type in the constructortypeslist.
If this report is never observed in the only implementing browser, then we should probably mention this in the notes? However, it is also not really clear what support means then if there is no effect?
There was a problem hiding this comment.
I've tried adding a note to explain this. Let me know what you think. This is a weird situation, but it is still useful to know that the report type is not observed.
| "support": { | ||
| "chrome": { | ||
| "version_added": "145", | ||
| "notes": "The `crash` report type can be specified in the `types` array, but crash reports are not observed by reporting observers. They can be sent to reporting endpoints." | ||
| }, |
There was a problem hiding this comment.
I think this note can be helpful, but I'm still wondering what "supported" means here versus not supported. Does setting the type to crash in an earlier version have a different behavior, for example throwing an error?
/cc @ddbeck for a 2nd opinion
There was a problem hiding this comment.
It basically just means that crash is a recognised type. I've tested it, and unfortunately, it doesn't throw if an unrecognized type is included in the options object.
The main reaosn I'm adding this here is because of this note in the description:
This data is also used as the BCD for each report type dictionary; I'll need it when documenting CrashReportBody.
Because we don't record BCD separately for dictionaries, I need this data point to show BCD in the new dictionary page I'm documenting. This is how it was decided for the reporting dictionaries.
There was a problem hiding this comment.
Why will you need it when documenting crash report bodies—can you say more about this? If you need a way to show support for receiving crash report bodies, would http.headers.Reporting-Endpoints.crash-reporting suffice (see #29399)?
There was a problem hiding this comment.
I guess it is for consistency more than anything. This is what we've used to represent the BCD for all the other reporting dictionaries.
Also, there is a bit of a version mismatch. The data you've added says Chrome 139, but the bits I've just bene documenting say Chrome 145. https://developer.mozilla.org/en-US/docs/Web/API/CrashReportContext#browser_compatibility, for example.
There was a problem hiding this comment.
I'd say it's inconsistent to say that ReportingObserver() has anything to do with crash reporting. 🤷
As for the header data, there's no inconsistency but there is a part missing:
CrashReportContextis part of the crash report context key-value API. It's new in Chrome 145.http.headers.Reporting-Endpoints.crash-reportingis the named endpoint for crash reports. It's new in Chrome 139.- Here's the missing part: Since Chrome 73, crash reports go to the
defaultcrash reporting endpoint (citation). We could have some keys likehttp.headers.Reporting-Endpoints.default,http.headers.Reporting-Endpoints.default.receives_crash_type, andhttp.headers.Reporting-Endpoints.default.receives_deprecation_typeto track this.
On the last point, would that solve the problem for crash report bodies?
See also mdn/content#43688 and cc: @hamishwillee.
There was a problem hiding this comment.
I'd say it's inconsistent to say that
ReportingObserver()has anything to do with crash reporting. 🤷
Yeah, I guess it is. I was just trying to follow the pattern laid out for the other reporting body dictionaries, but I see your point.
As for the header data, there's no inconsistency but there is a part missing:
* `CrashReportContext` is part of the crash report context key-value API. It's new in Chrome 145. * `http.headers.Reporting-Endpoints.crash-reporting` is the named endpoint for crash reports. It's new in Chrome 139.
Hrm, neither of those sounds like the data that should be used to represent CrashReportBody.
* Here's the missing part: Since Chrome 73, crash reports go to the `default` crash reporting endpoint ([citation](https://chromestatus.com/feature/6683949661159424)). We could have some keys like `http.headers.Reporting-Endpoints.default`, `http.headers.Reporting-Endpoints.default.receives_crash_type`, and `http.headers.Reporting-Endpoints.default.receives_deprecation_type` to track this.On the last point, would that solve the problem for crash report bodies?
This sounds more like the data we want to use for CrashReportBody. Shall I remove the crash data point from this PR (I think the type fix on line 107 could still do with fixing), and open a new PR to add the data you're proposing here?
See also mdn/content#43688 and cc: @hamishwillee.
There was a problem hiding this comment.
Shall I remove the
crashdata point from this PR (I think the type fix on line 107 could still do with fixing), and open a new PR to add the data you're proposing here?
That sounds good to me. Feel free to tag me for review on it when it opens.
There was a problem hiding this comment.
That sounds good to me. Feel free to tag me for review on it when it opens.
OK, I'll get on it. Thanks for the help in resolving this, @ddbeck.
|
Approved, but before merging I'd recommend renaming this PR to something like "Revise descriptions for |
Done. Thanks, @ddbeck! |
Summary
Chrome 145 adds support for the Crash Reporting API; see https://chromestatus.com/feature/6228675846209536.
Most of the BCD for this API has already been added; however, I wanted to add a data point for the
crashreport type. The data for the different report types is contained under api.ReportingObserver.ReportingObserver.options_parameter.types_property, so I added it under there. This data is also used as the BCD for each report type dictionary; I'll need it when documentingCrashReportBody.This is a bit weird, as
crashreports aren't observed by reporting observers, but I think you can still specify them as a valid report type in the constructortypeslist.Test results and supporting details
Related issues