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

Add description of ad object fields #558

Merged
merged 7 commits into from
Jun 1, 2023
Merged

Conversation

brusshamilton
Copy link
Contributor

Adds descriptions for the adRenderId, sizeGroup, buyerReportingId, and buyerAndSellerReportingId fields and makes the exact field names more clear.

FLEDGE.md Show resolved Hide resolved
FLEDGE.md Outdated Show resolved Hide resolved
FLEDGE.md Outdated Show resolved Hide resolved
FLEDGE.md Outdated Show resolved Hide resolved
FLEDGE.md Outdated Show resolved Hide resolved
FLEDGE.md Outdated

* `sizeGroup`: The name of the sizeGroup that this ad is associated with.

The `adComponents` field contains the various ad components (or "products") that can be used to construct ["Ads Composed of Multiple Pieces"](https://github.com/WICG/turtledove/blob/main/FLEDGE.md#34-ads-composed-of-multiple-pieces)). Similarly to the `ads` field, each entry is an object that includes both a rendering URL, `adRenderId`, `sizeGroup`, and `metadata`. Thanks to `ads` and `adsComponents` being separate fields, the buyer is able to update the `ads` field via the `updateUrl` without losing `adComponents` stored in the interest group.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The `adComponents` field contains the various ad components (or "products") that can be used to construct ["Ads Composed of Multiple Pieces"](https://github.com/WICG/turtledove/blob/main/FLEDGE.md#34-ads-composed-of-multiple-pieces)). Similarly to the `ads` field, each entry is an object that includes both a rendering URL, `adRenderId`, `sizeGroup`, and `metadata`. Thanks to `ads` and `adsComponents` being separate fields, the buyer is able to update the `ads` field via the `updateUrl` without losing `adComponents` stored in the interest group.
The `adComponents` field contains the various ad components (or "products") that can be used to construct ["Ads Composed of Multiple Pieces"](https://github.com/WICG/turtledove/blob/main/FLEDGE.md#34-ads-composed-of-multiple-pieces)). Similar to the `ads` field, each entry is an object that includes `renderURL`, and an optional `adRenderId` and `metadata`. Thanks to `ads` and `adsComponents` being separate fields, the buyer is able to update the `ads` field via the `updateUrl` without losing `adComponents` stored in the interest group.

Copy link
Contributor

Choose a reason for hiding this comment

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

each entry is an object that includes renderURL, and an optional adRenderId and metadata.

These three values (plus sizeGroup) are the only adComponent attributes that are actually used by PAAPI, yes?

If a component includes an optional adRenderId will only the id be sent in lieu of the entire serialised object for B&A auctions?

adsComponents --> adComponents

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the typo. Yes, the optimization for B&A currently specifies that the adRenderId will be sent instead of the ad object.

FLEDGE.md Outdated

The `adComponents` field contains the various ad components (or "products") that can be used to construct ["Ads Composed of Multiple Pieces"](https://github.com/WICG/turtledove/blob/main/FLEDGE.md#34-ads-composed-of-multiple-pieces)). Similarly to the `ads` field, each entry is an object that includes both a rendering URL and arbitrary metadata that can be used at bidding time. Thanks to `ads` and `adsComponents` being separate fields, the buyer is able to update the `ads` field via the `updateUrl` without losing `adComponents` stored in the interest group.
* `adRenderId`: A short identifier for this ad in this context. When this field is specified it will be sent instead of the full ad object for [B&A server auctions](https://github.com/WICG/turtledove/blob/main/FLEDGE_browser_bidding_and_auction_API.md).
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to specify a type here, e.g. string or BigInt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. It has been specified to be an 8 byte string.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done. It has been specified to be an 8 byte string.

Is PAAPI UTF-8 all through?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm that's an interesting point. JavaScript and ECMAScript strings are UTF-16 (e.g. charCodeAt() returns a 16-bit number). So does an 8-byte string imply at most 4 characters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Chrome uses UTF-8 for strings internally, but that's a detail we shouldn't leak.

The intent was not to only be 4 characters, but rather to be 8 characters. Perhaps it would be better for us to define it as a ByteString instead of a DOMString. Then the IDL would perform the correct conversion and would enforce that each character's code was <= 255. That would ensure that the length of the JavaScript string and the length of the internal Chrome string was the same.

Copy link
Contributor

@dmdabbs dmdabbs May 19, 2023

Choose a reason for hiding this comment

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

enforce that each character's code was <= 255

So a string value (whether in the adRenderId or some other string attribute such as a IG name, &c) containing say "Ā" (256) would cause Chrome to reject the entire input?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

I could see IG owners perhaps using accented characters with codes > 255 in interest group names.
If there is not a warning about this constraint would you please add one in the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're not going to enforce that for interest group names, just for adRenderId.

FLEDGE.md Outdated
@@ -155,9 +155,19 @@ The `executionMode` attribute is optional, and may contain one of the following
mode does not have the same limitations on what top-level sites can join or leave
the interest group.

The `ads` list contains the various ads that the interest group might show. Each entry is an object that includes both a rendering URL and arbitrary metadata that can be used at bidding time.
The `ads` list contains the various ads that the interest group might show. Each entry is an object that must contain an `renderUrl` property with the URL for the ad that would be shown. An ad may also have the following optional properties:
Copy link
Contributor

@dmdabbs dmdabbs May 17, 2023

Choose a reason for hiding this comment

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

An ad may also have the following optional properties:

Wouldn't at least one sizeGroup be needed (it is part of the k-anon computation), else how would the auction determine an ad's suitability?

If an ad supports a single adSize, the sizeGroup value may be the name of that adSize, yes? I recall prior wording that is was unnecessary to declare single-size sizeGroups.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be needed eventually, but is not currently required. The timeline for including ad size in the k-anonymity calculation is yet to be determined.

Copy link
Contributor

@dmdabbs dmdabbs May 17, 2023

Choose a reason for hiding this comment

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

Sorry I muddled two things there @brusshamilton. My

how would the auction determine an ad's suitability

intended to ask

how would the auction determine the ad's suitability to fit the ad slot dimensions absent a sizeGroup attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Presumably the bidding script has some way of knowing. Either through the metadata associated with an ad or other data in the interest group.

We are going to be adding the sizeGroup eventually (see pull request #417), but it doesn't make sense to ad a reference to it before that pull request lands.

FLEDGE.md Outdated Show resolved Hide resolved
FLEDGE.md Outdated Show resolved Hide resolved
FLEDGE.md Outdated Show resolved Hide resolved
Co-authored-by: Paul Jensen <JensenPaul@users.noreply.github.com>
@JensenPaul JensenPaul merged commit 32e8f75 into WICG:main Jun 1, 2023
github-actions bot added a commit that referenced this pull request Jun 1, 2023
SHA: 32e8f75
Reason: push, by JensenPaul

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to qingxinwu/turtledove that referenced this pull request Jun 1, 2023
SHA: 32e8f75
Reason: push, by qingxinwu

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@brusshamilton brusshamilton deleted the AdObj branch June 16, 2023 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants