this definition matches the implementation better than the record<DOMString, any> contribution defined in PAgg spec. If I'm not missing something, shared storage (but its bucket can only be bigInt), and the new error reporting will both fit in this IDL, without needing the more general record<> one. If that's true, should we upadte the PAgg spec's IDL?

Agreed that this way of 'overloading' (without actually overloading) isn't ideal. (And both options should be functionally similar.)

But, from a layering perspective, I think that kind of generic object/map type maybe makes more sense than having PAExtendedHistogramContribution be defined in the Private Aggregation spec as the 'filling in'/signals concept is Protected Audience-specific.

isn't the IDL "inaccurate" then? We only allow bucket/value/filteringId as the contribution's fields, and the IDL layer will reject the input if it has other fields, but with this record<> thing, it will not. The filling in logic should be in PA spec, but wondering if it makes sense to put the PAExtendedHistogramContribution definition in PAgg spec.
@domfarolino WDYT? Another question, the bucket field will be bigint for shared storage, but (PASignal or bigint) for protected audience. If we do want to change this "record<>" definition to a stricter one, how would we define a single IDL for both cases?

The actual IDL conversion/handling is still present -- e.g. see line 5 of the new algorithm.

But agreed that this is definitely a bit weird/hacky, so would be interested to hear if people have suggestions on a better approach.

I guess one in-between option could be to define a Private Aggregation layer extended IDL type of the form:

dictionary ExtendedHistogramContribution {
  required any bucket;
  required any value;
  bigint filteringId = 0;  // maybe this should be any too.
};

or even

dictionary ExtendedHistogramContribution {
  required (bigint or record<DOMString, any>) bucket;
  required (long or record<DOMString, any>) value;
  bigint filteringId = 0;  // maybe this should have its type modified too.
};

Then Protected Audience and Shared Storage would (in their algorithm) define what buckets/values they actually permit.

cc @dmcardle

oh wait, I think I was wrong. I thought we had an IDL for it in code, but just noticed that in our implementation, it was not restricted by IDL, but by our own code instead which mimics the IDL's behaviour. So I think your spec is fine, as long as the algorithms handle the type check in the same way as the code does. cc: @morlovich

@domfarolino WDYT?

If I understand correctly, we're using overly-loose IDL and then tightening it up with spec logic in an algorithm that actually operates on the IDL, doing more conversions and rethrowing exceptions, right? (See #1412 (review)). That sounds fine to me, especially if it matches the implementation. Let me know if I'm not following correctly though.

Thanks, yep I think that's an accurate description. So maybe we land this as is, but I'll add an issue to the Private Aggregation spec about considering other options for the overly loose IDLs.

Thanks. SGTM.

Agreed that this way of 'overloading' (without actually overloading) isn't ideal. (And both options should be functionally similar.)

But, from a layering perspective, I think that kind of generic object/map type maybe makes more sense than having PAExtendedHistogramContribution be defined in the Private Aggregation spec as the 'filling in'/signals concept is Protected Audience-specific.

if we want this algorithm to cover the PA's contributeToHistogramOnEvent(), then it needs to be updated, like its step 8, which throws when the event is not "reserved.xx", but it should also allow events that does not start with "reserved.".

oh never mind, seems [=determine whether to perform default contributeToHistogramOnEvent() processing=] never returns true, so it will not reach after step 3 of PAgg spec's [=contributeToHistogramOnEvent=]

The Protected Audience implementation is covered by steps 1-3. (Note that the "determine whether to perform default contributeToHistogramOnEvent() processing" algorithm for Protected Audience never returns true.) Maybe I should add a better note explaining this behavior to the Protected Audience spec.

yeah probably explain that the algorithm will never return true or never perform default xxx on the next line, or in the algirhtm's definition, something like they return a [=boolean=] (only false)

Yep, makes sense -- added the parenthetical and a note.

rethrowing any [=exceptions=] should be fine, as that's what the Web IDL standard does in many places.