Change API layering in spec #51

alexmturner · 2023-05-30T20:59:15Z

Creates new exported algorithms and one algorithm to be overriden. This cl doesn't actually move any content to other specs, but is a step to making that simpler. This change also addresses batching, by defining the batching scopes more completely.

Preview | Diff

alexmturner · 2023-05-30T21:01:59Z

@yoavweiss could you take a quick look? I wasn't sure if this was the best direction towards fixing the layering, but I'm trying to allow each implementing API to define its own 'batching scope'. Specifically, I wasn't really sure whether functions being 'overridden' was a well-supported spec concept. Thanks!

alexmturner · 2023-06-01T21:28:14Z

I've reworked this to separate out the parts that will (eventually) be moved to other specs into separate sections at the end as well as moved from 'overrides' to having a field that gets set by implementing specs

cc @linnan-github

yoavweiss

Looks good overall. A few comments

yoavweiss · 2023-06-01T23:17:45Z

spec.bs

-1. [=list/Append=] |contribution| to the [=contributions cache=].
+1. Let |batchingScopeSteps| be the {{PrivateAggregation}} object's
+    [=PrivateAggregation/get batching scope steps=].
+1. If |batchingScopeSteps| is null, throw a new {{DOMException}} with name


Can that happen? If not, it's better to assert it doesn't, rather than throwing

Added a note here -- below we say "The global scope may wait to set the field to indicate that the API is not yet available."

What would be the behavior expected from developers here? Is there a way for them to check availability before hitting an exception?

This is really just to prevent the API being available during the initial execution of a script (e.g. while addModule() runs). We only want the API to be available after that point. Definitely open to suggestions for better structuring this, but for now I've updated the description slightly to clarify.

Added an example to better illustrate this, lmk if you think it's clear enough

It's fine as far as the spec go, but worthwhile thinking about the developer ergonomics here:

How would developers know when it's fine to run this API call and when it isn't? Is that part of our developer documentation?

If I'm a library provider enabling folks to send reports, how should I write my code so that it never runs too early? (regardless of when callers may call it)

Yep, makes sense. Added an issue to think about this a bit further/follow up.

yoavweiss · 2023-06-01T23:22:58Z

spec.bs


 Structures {#structures}
 ========================

 General {#general-structures}
 -----------------------------

-<h4 dfn-type=dfn>Aggregatable report</h3>
+<h4 dfn-type=dfn>Batching scope</h4>
+A batching scope is a unique internal value that identifies which


s/unique internal value/implementation-defined value/

https://infra.spec.whatwg.org/#implementation-defined

For this, I was trying to emulate the "opaque origin" kind of concept. I saw "unique internal value" in the HTML spec (https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#unique-values), but I don't think it's exported, so I might need to move that definition in here.

@domenic - thoughts? Should we try to export "unique internal value"?

Huh I didn't realize that we made that into a whole concept with a definition. Probably we should move that to Infra and make it exported.

In the meantime, you could do <a spec=HTML>unique internal value</a> to link to the concept despite it not being exported.

Thanks, moved to the tag as suggested. Also added an issue and filed whatwg/infra#583 to track this.

spec.bs

alexmturner · 2023-06-02T17:49:18Z

@linnan-github, could you PTAL? Thanks!

spec.bs

yoavweiss · 2023-06-06T02:55:34Z

spec.bs

-1. [=list/Append=] |contribution| to the [=contributions cache=].
+1. Let |batchingScopeSteps| be the {{PrivateAggregation}} object's
+    [=PrivateAggregation/get batching scope steps=].
+1. If |batchingScopeSteps| is null, throw a new {{DOMException}} with name


It's fine as far as the spec go, but worthwhile thinking about the developer ergonomics here:

How would developers know when it's fine to run this API call and when it isn't? Is that part of our developer documentation?

If I'm a library provider enabling folks to send reports, how should I write my code so that it never runs too early? (regardless of when callers may call it)

yoavweiss · 2023-06-07T07:36:29Z

spec.bs

+`run()` or to `selectURL()`), create a new [=batching scope=] and associate it
+with that invocation.
+
+When a {{PrivateAggregation}} object is exposed on a


The "when..." here doesn't make sense, as you're defining that exposure above, with the partial interface. It'd be better to actually find algorithmic hooks that indicate that the addModule execution is done, and set the algorithm there.

yoavweiss · 2023-06-07T07:37:15Z

spec.bs

+Note: Multiple operation invocations can be in-progress at the same time, each
+with a different batching scope. However, only one can be currently executing.
+
+When an operation invocation completes, [=mark a batching scope complete=] given


Same here - is there an algorithm that runs the operation invocation, that can call this logic?

yoavweiss · 2023-06-07T07:38:46Z

spec.bs

+Issue: Do we want to align naming with implementation?
+
+At the start of each Protected Audience auction (i.e. a call to
+`runAdAuction()`), create a new [=batching scope=] for each unique origin


Can you state that in terms of an algorithm step? (e.g. before step 1 of https://wicg.github.io/turtledove/#dom-navigator-runadauction)

yoavweiss · 2023-06-07T07:39:05Z

spec.bs

+`runAdAuction()`), create a new [=batching scope=] for each unique origin
+participating in the auction (including both bidders and the seller).
+
+When a {{PrivateAggregation}} object is exposed on a


Same comment as for SharedStorage

yoavweiss · 2023-06-07T07:40:17Z

spec.bs

+1. Return the [=batching scope=] associated with the auction and the |scope|'s
+    [=relevant settings object=]'s [=environment settings object/origin=].
+
+When an auction completes, perform the following steps:


Similarly, would be better to hook onto a specific algorithm.

alexmturner · 2023-06-07T21:09:30Z

@yoavweiss Thanks for the feedback! Had a go reworking to better integrate with the specs. Ran into a few issues with the other specs, so opened issues and noted those in this spec too where relevant. Some of the integrations I've put here are a little more handwavy than I'd like, but that should be fixable once those issues are resolved.

spec.bs

linnan-github · 2023-06-08T14:31:52Z

spec.bs


+Note: This last requirement means that global scopes with different origins


Probably can mention or link to the Same-Origin policy?

Added a comment (and an issue to actually link the section in the security considerations once that PR lands)

linnan-github · 2023-06-08T14:34:30Z

spec.bs

-    See infra/201
+    See <a href="https://github.com/whatwg/infra/issues/201">infra/201</a>.
+
+To <dfn algorithm export>append an entry to the contribution cache</dfn> given a


Given this is only one step, does it need to be a standalone algorithm?

I did it this way to allow other APIs to integrate, e.g. for the Protected Audience extensions. I put these under a new heading "Exported algorithms" to make this clearer

spec.bs

Creates new exported algorithms and one algorithm to be overriden. This cl doesn't actually move any content to other specs, but is a step to making that simpler. This change also addresses batching, by defining the batching scopes more completely.

spec.bs

alexmturner · 2023-06-09T19:47:59Z

Thanks for all the improvements!

SHA: 3d32307 Reason: push, by alexmturner Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

yoavweiss reviewed Jun 1, 2023

View reviewed changes

yoavweiss reviewed Jun 6, 2023

View reviewed changes

alexmturner mentioned this pull request Jun 6, 2023

Consider defining "unique internal value" whatwg/infra#583

Open

alexmturner force-pushed the layering-batching branch from 39ead42 to af27a6d Compare June 6, 2023 16:35

yoavweiss reviewed Jun 7, 2023

View reviewed changes

linnan-github reviewed Jun 8, 2023

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

linnan-github reviewed Jun 8, 2023

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

linnan-github reviewed Jun 8, 2023

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

linnan-github reviewed Jun 8, 2023

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

linnan-github reviewed Jun 8, 2023

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

alexmturner added 11 commits June 8, 2023 21:52

Change API layering in spec

0f246e4

Creates new exported algorithms and one algorithm to be overriden. This cl doesn't actually move any content to other specs, but is a step to making that simpler. This change also addresses batching, by defining the batching scopes more completely.

move API-specific content into particular sections, rework overrides

624c636

add note, remove stale issue

1739a0c

better explain null algorithm

65986e5

add shared storage example

c5b0fef

link unique internal value, add new line

b86994b

Link infra issues

e9fff84

rework monkeypatches

8b862a0

review comments

0d3b681

monkeypatch -> monkey patch

cfd5402

fix note

8a390d8

linnan-github approved these changes Jun 9, 2023

View reviewed changes

linnan-github reviewed Jun 9, 2023

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

milliseconds

ada0439

alexmturner force-pushed the layering-batching branch from 3a54c0e to ada0439 Compare June 9, 2023 19:47

alexmturner merged commit 3d32307 into main Jun 9, 2023

alexmturner deleted the layering-batching branch June 9, 2023 19:53

github-actions bot added a commit that referenced this pull request Jun 9, 2023

Change API layering in spec (#51)

2878500

SHA: 3d32307 Reason: push, by alexmturner Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

alexmturner mentioned this pull request Jun 12, 2023

Reliant specs and their layering #43

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change API layering in spec #51

Change API layering in spec #51

alexmturner commented May 30, 2023 •

edited by pr-preview bot

Loading

alexmturner commented May 30, 2023

alexmturner commented Jun 1, 2023

yoavweiss left a comment

yoavweiss Jun 1, 2023

alexmturner Jun 2, 2023

yoavweiss Jun 5, 2023

alexmturner Jun 5, 2023

alexmturner Jun 5, 2023

yoavweiss Jun 6, 2023

alexmturner Jun 6, 2023

yoavweiss Jun 1, 2023

alexmturner Jun 2, 2023

yoavweiss Jun 5, 2023

domenic Jun 6, 2023

alexmturner Jun 6, 2023

alexmturner commented Jun 2, 2023

yoavweiss Jun 6, 2023

yoavweiss Jun 7, 2023

yoavweiss Jun 7, 2023

yoavweiss Jun 7, 2023

yoavweiss Jun 7, 2023

yoavweiss Jun 7, 2023

alexmturner commented Jun 7, 2023 •

edited

Loading

linnan-github Jun 8, 2023

alexmturner Jun 8, 2023

linnan-github Jun 8, 2023

alexmturner Jun 8, 2023

alexmturner commented Jun 9, 2023


		Note: This last requirement means that global scopes with different origins

Change API layering in spec #51

Change API layering in spec #51

Conversation

alexmturner commented May 30, 2023 • edited by pr-preview bot Loading

alexmturner commented May 30, 2023

alexmturner commented Jun 1, 2023

yoavweiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexmturner commented Jun 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexmturner commented Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexmturner commented Jun 9, 2023

alexmturner commented May 30, 2023 •

edited by pr-preview bot

Loading

alexmturner commented Jun 7, 2023 •

edited

Loading