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.

Sign up for GitHub

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

Jump to bottom

Report body interfaces #216

Open
domenic opened this issue Aug 27, 2020 · 17 comments · May be fixed by #258
Open

Report body interfaces #216

domenic opened this issue Aug 27, 2020 · 17 comments · May be fixed by #258
Assignees
@clelland

Comments

@domenic
Copy link
Contributor

domenic commented Aug 27, 2020

Looking at Chromium's implementation, I see a lot of "report body" interfaces:

However, most of these don't exist in specifications. And, many of them are using [NoInterfaceObject], which no longer exists on the web platform (it was removed from Web IDL).

I'm unsure how the spec is supposed to work here, and the specs definitely don't seem to match implementations.

  • The Reporting API's "queue a violation report" algorithm takes an "object". But, the ReportingObserver API uses ReportBody, so is there an implicit requirement that such objects be subclasses of ReportBody?

  • Some specs appear to fill this out using a generic "a new object" language, e.g. Fetch and COEP. This does not involve any interfaces. The proposed COOP reporting spec in Cross origin opener policy reporting whatwg/html#5518 does the same, although as noted above, the in-progress Chromium implementation seems to use an interface. How do these things show up in ReportingObserver?

  • I can't figure out what the CSP spec is doing, but it certainly doesn't use CSPViolationReportBody.

  • I can't find any spec for InterventionReportBody or DeprecationReportBody.

  • DocumentPolicyViolationReportBody doesn't exist in Document Policy, and I can't find where that spec queues reports. Maybe that part isn't written yet?

  • Permissions Policy seems to be fully on board the interface train: it declares a proper, non-NoInterfaceObject report body interface, and sends that through the "queue a violation report" algorithm.

This seems like a pretty big mess.

My recommendation if we were starting from scratch would be to not use interfaces for this, and have "queue a violation report" take either an object, or something more structured (like a record<USVString, any> or an Infra ordered map) to give guidance to callers. Implementations could unobservably use IDL dictionaries behind the scenes if they wanted, but specs would use the Fetch/COEP/COOP style of just supplying a table of keys/values. And we'd get rid of ReportBody entirely and make ReportingObserver's body just an object? instead of a ReportBody?.

However, we're not starting from scratch, so I'm unsure what the best path forward is.

/cc @clelland @camillelamy @mikewest
@jpchase
Copy link
Collaborator

jpchase commented Aug 27, 2020

Re not finding a spec for InterventionReportBody or DeprecationReportBody. In #189, crash, deprecation and intervention reports were moved to their own separate repos:
Crash reporting (spec)
Deprecation reporting (spec)
Intervention reporting (spec)

All three of these use the full interface approach, similar to Permissions Policy.

@domenic
Copy link
Contributor Author

domenic commented Aug 27, 2020

Thanks! That certainly tips the balance, at least in specs space, toward the full-interface approach. Although it feels a little unnecessary to create new constructor-less classes for things that are just data holders (see w3ctag/design-principles#11), it's not the end of the world. It's the current inconsistency that's troublesome, and in particular as HTML editor reviewing the COEP and COOP spec patches, I'm not sure what to do.

@clelland
Copy link
Contributor

  • Document Policy should define its report type; that's an omission.
  • Chrome needs to remove the "NoInterfaceObject" from the various body types. I just filed crbug.com/1122656 for that.
  • In ReportingObserver, these objects (as the 'body' property of the observed Report) are instances of their respective types, and are not simply ES dictionaries. The Reporting spec just says

Each report has a body, which is either null or an object which can be serialized into a JSON text. The fields contained in a report’s body are determined by the report’s type.

  • Crash reports are necessarily not visible to reporting observers; it doesn't really matter what we do with those as long as the serialized report is correct.

@clelland
Copy link
Contributor

We could switch to plain ES objects; there's really no benefit that I can see to using classes. It was added to support ReportingObserver; I suspect that it was originally so that we could use IDL to express the structure and the members that would be present.

@domenic
Copy link
Contributor Author

domenic commented Aug 28, 2020

You could still use dictionaries, either behind the scenes in implementations, or in specs if desired (although I'm not sure that would buy much).

@jpchase
Copy link
Collaborator

jpchase commented Aug 28, 2020

There was #164 where toJSON() wasn't working in Chromium, but there were also spec changes made. Just pointing it out, in case it matters here.

@domenic
Copy link
Contributor Author

domenic commented Aug 28, 2020

Yeah, using normal JS objects (dictionaries) would be a more natural way of solving that, instead of having the code generator generate a bunch of getters and a toJSON() method.

@domenic domenic mentioned this issue Feb 4, 2021
Open
@rachelandrew rachelandrew mentioned this issue Jun 10, 2021
Merged
@clelland clelland self-assigned this Jun 20, 2022
@RByers
Copy link
Collaborator

RByers commented Nov 25, 2022

Following up from discussion of this on blink-dev.

The principle of using dictionaries when there's only data makes sense to me. But dictionaries can't be used as attributes and Report has a body attribute.

@domenic
Copy link
Contributor Author

domenic commented Nov 25, 2022

I think Report itself can be a dictionary, looking at how it's used?

@RByers RByers mentioned this issue Nov 25, 2022
Open
@clelland
Copy link
Contributor

Yes, Report could also be a dictionary; I don't believe that there are any constraints on dictionaries being dictionary members.

@RByers
Copy link
Collaborator

RByers commented Nov 25, 2022

I think Report itself can be a dictionary, looking at how it's used?

Oh I see, make Report a dictionary and then ReportBody isn't an attribute anymore so all of them can be dictionaries too. Yeah that makes sense. I'm just looking over to try to understand if there would be any non-trivial web compat implications of this. @clelland thoughts?

@clelland
Copy link
Contributor

I can't see any compat issues -- without the interface object currently being exposed, there doesn't seem to be any way to get the type of a Report or ReportBody -- so there shouldn't be any type-detection going on right now that would fail if we switched to dictionaries.

Reports (and their bodies) currently look like Objects to a script, and that wouldn't change. (The "Report" and e.g. "DeprecationReportBody" names are visible in DevTools' representation of the reports, so I'm not 100% sure that there's no way to discover them currently, but I don't believe it's likely.

@clelland
Copy link
Contributor

(Not that anyone should be using something other than the type member to do report-type detection; I was just checking to see if it's currently even possible)

@RByers
Copy link
Collaborator

RByers commented Nov 25, 2022

Yeah it looks like it is possible in theory, eg:

> window.speechSynthesis.getVoices()[0][Symbol.toStringTag]
'SpeechSynthesisVoice'

But poking around at this stuff (like __proto__ etc.) has always been somewhat implementation defined AFAIK, so I'm not worried about it as a real compat risk in practice.

@RByers
Copy link
Collaborator

RByers commented Nov 25, 2022

Alright, sounds like we're converging on just moving all of these to dictionaries, right?

@clelland
Copy link
Contributor

That's the one that does it... rpt.body[Symbol.toStringTag] reports the internal type.

That would change to Object for a dictionary, I suspect, if anyone were actually relying on it. It does seem substantially less likely to be a compatibility risk, for sure.

clelland added a commit that referenced this issue Dec 4, 2022
@clelland
Change Report and ReportBody interfaces to dicts.
e894337 
This change removes any suggestion that the two interface names (and
the names of those interfaces which inherit from `ReportBody`) should
be exposed on the global object. `ReportBody` is purely abstract, and
exists so that it can be used as the type of the `body` member of
`Report`, while still allowing other specifications to define new
subtypes for their own report types.

Closes: #216
@clelland clelland linked a pull request Dec 5, 2022 that will close this issue
Open
@clelland
Copy link
Contributor

clelland commented Dec 5, 2022

Moving to dictionaries makes sense to me -- I've put up a PR at #258 for that.

@bakulf, do you think this would work for Gecko?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@clelland clelland

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Change Report and ReportBody interfaces to dicts. w3c/reporting
4 participants
@clelland @jpchase @domenic @RByers