Handles header-based directFromSellerSignals. #774

qingxinwu · 2023-08-31T20:40:38Z

Resolves auction config's directFromSellerSignalsHeaderAdSlot promise.
Passes directFromSellerSignals to worklets (generateBid(), scoreAd(),
reportWin(), reportResult()).

Preview | Diff

Resolves auction config's directFromSellerSignalsHeaderAdSlot promise. Passes directFromSellerSignals to worklets (generateBid(), scoreAd(), reportWin(), reportResult()).

spec.bs

caraitto

Thanks Qingxin for helping out with this!

I took a quick look and left some comments. By the way, how do you think we should integrate our 2 PRs?

qingxinwu · 2023-08-31T21:17:20Z

Thanks Qingxin for helping out with this!

I took a quick look and left some comments. By the way, how do you think we should integrate our 2 PRs?

Thanks Caleb. I'll fix those errors you pointed out.
About integrating the PRs, I only need the two struct definitions that I put in a todo block to remind myself to remove. So unless it's required to put them in a single PR, I think you can submit your PR with that TODO about my work, and I'll merge mine afterwards.

spec.bs

qingxinwu · 2023-09-05T18:09:01Z

@domfarolino Mind taking a look? Thanks.

qingxinwu · 2023-09-05T18:54:22Z

Thanks Qingxin for helping out with this!
I took a quick look and left some comments. By the way, how do you think we should integrate our 2 PRs?

Thanks Caleb. I'll fix those errors you pointed out. About integrating the PRs, I only need the two struct definitions that I put in a todo block to remind myself to remove. So unless it's required to put them in a single PR, I think you can submit your PR with that TODO about my work, and I'll merge mine afterwards.

Done. @caraitto Feel free to take another look. Thanks!

spec.bs

caraitto

Still LGTM, good catches Dominic. Qingxin, please wait for Dominic's approval.

spec.bs

domfarolino

Just about done I think!

spec.bs

caraitto

Still LGTM.

spec.bs

domfarolino

LGTM % #774 (comment).

Thanks for the patience with all the review churn, I hope it was helpful.

qingxinwu · 2023-10-12T15:05:25Z

LGTM % #774 (comment).

Thanks for the patience with all the review churn, I hope it was helpful.

Thanks so much for all the through reviews. That's super helpful! I'll address that, and link to the correct definitions once the other PR is merged.

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

WICG#774 landed, and this is done.

These default to null, not undefined, if not specified (or if there was an error loading). Practically, I this this only matters for perBuyerSignals (since the other fields get set from string or null fields), but it's probably good to be consistent. This is related to this thread from the review: WICG#774 (comment)

Qingxin Wu added 2 commits August 30, 2023 23:10

Handle directFromSellerSignalsHeaderAdSlot

538ac0d

Handles header-based directFromSellerSignals.

7cb8962

Resolves auction config's directFromSellerSignalsHeaderAdSlot promise. Passes directFromSellerSignals to worklets (generateBid(), scoreAd(), reportWin(), reportResult()).

qingxinwu requested a review from caraitto August 31, 2023 20:41

qingxinwu commented Aug 31, 2023

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

qingxinwu added the spec Relates to the spec label Aug 31, 2023

caraitto mentioned this pull request Aug 31, 2023

[Spec] Add directFromSellerSignals #771

Merged

caraitto reviewed Aug 31, 2023

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

caraitto reviewed Aug 31, 2023

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

caraitto reviewed Aug 31, 2023

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

caraitto reviewed Aug 31, 2023

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

caraitto reviewed Aug 31, 2023

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

caraitto reviewed Aug 31, 2023

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

caraitto suggested changes Aug 31, 2023

View reviewed changes

Qingxin Wu added 2 commits September 1, 2023 14:33

Address comments, and pass signal to top level auction.

a89a847

Small change.

d2fc28f

qingxinwu commented Sep 1, 2023

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

Qingxin Wu added 2 commits September 1, 2023 15:00

Return failure if ad slot promise resolves failure.

83af67a

Define IDLs for directFromSellerSignals and convert to JS.

7ec4402

qingxinwu requested review from caraitto and domfarolino September 5, 2023 17:49

Qingxin Wu added 2 commits September 5, 2023 14:17

Add or-undefined to direct seller signals parameter.

74d2c57

Cleanup

f92411f

Merge branch 'WICG:main' into direct

f3c8c46

domfarolino reviewed Sep 8, 2023

View reviewed changes

qingxinwu and others added 2 commits September 8, 2023 15:53

Merge branch 'WICG:main' into direct

f37c7f0

Address comments.

b26544f

qingxinwu mentioned this pull request Sep 8, 2023

Some trivial cleanups came up during another PR's review. #789

Merged

Merge branch 'WICG:main' into direct

b86b186

domfarolino reviewed Oct 2, 2023

View reviewed changes

spec.bs Show resolved Hide resolved

domfarolino reviewed Oct 2, 2023

View reviewed changes

spec.bs Show resolved Hide resolved

spec.bs Outdated Show resolved Hide resolved

spec.bs Show resolved Hide resolved

spec.bs Outdated Show resolved Hide resolved

spec.bs Outdated Show resolved Hide resolved

spec.bs Show resolved Hide resolved

spec.bs Outdated Show resolved Hide resolved

Address Dom's comments.

39ad2df

qingxinwu requested a review from domfarolino October 2, 2023 21:07

caraitto approved these changes Oct 5, 2023

View reviewed changes

domfarolino reviewed Oct 7, 2023

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

domfarolino reviewed Oct 7, 2023

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

spec.bs Outdated Show resolved Hide resolved

qingxinwu and others added 2 commits October 10, 2023 10:34

Merge branch 'WICG:main' into direct

e4dca01

Address Dom's comments.

a0fb991

caraitto approved these changes Oct 10, 2023

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

caraitto requested a review from domfarolino October 10, 2023 19:04

qingxinwu mentioned this pull request Oct 10, 2023

Fix a problem of setting promise fields to strings. #854

Merged

domfarolino approved these changes Oct 12, 2023

View reviewed changes

qingxinwu and others added 7 commits October 12, 2023 14:00

Merge branch 'main' into direct

608c6ae

address nit

1d52533

Merge branch 'main' into direct

0240903

remove section redundant with 771

01fca70

fix name of captured headers

3149981

add link

01d73c8

fix link syntax

9f6fe41

JensenPaul merged commit ffc0e8d into WICG:main Oct 13, 2023
2 checks passed

github-actions bot added a commit that referenced this pull request Oct 13, 2023

Handles header-based directFromSellerSignals. (#774)

821249d

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

caraitto added a commit to caraitto/turtledove that referenced this pull request Oct 13, 2023

Remove TODO that is done

68827f6

WICG#774 landed, and this is done.

caraitto mentioned this pull request Oct 13, 2023

Remove TODO that is done #860

Merged

caraitto mentioned this pull request Oct 13, 2023

Set directFromSellerSignals dict members to null #861

Merged

qingxinwu deleted the direct branch October 16, 2023 17:29

qingxinwu mentioned this pull request Oct 16, 2023

Link adAuctionHeaders #863

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handles header-based directFromSellerSignals. #774

Handles header-based directFromSellerSignals. #774

qingxinwu commented Aug 31, 2023 •

edited by pr-preview bot

Loading

caraitto left a comment

qingxinwu commented Aug 31, 2023 •

edited

Loading

qingxinwu commented Sep 5, 2023

qingxinwu commented Sep 5, 2023

caraitto left a comment

domfarolino left a comment

caraitto left a comment

domfarolino left a comment

qingxinwu commented Oct 12, 2023

Handles header-based directFromSellerSignals. #774

Handles header-based directFromSellerSignals. #774

Conversation

qingxinwu commented Aug 31, 2023 • edited by pr-preview bot Loading

caraitto left a comment

Choose a reason for hiding this comment

qingxinwu commented Aug 31, 2023 • edited Loading

qingxinwu commented Sep 5, 2023

qingxinwu commented Sep 5, 2023

caraitto left a comment

Choose a reason for hiding this comment

domfarolino left a comment

Choose a reason for hiding this comment

caraitto left a comment

Choose a reason for hiding this comment

domfarolino left a comment

Choose a reason for hiding this comment

qingxinwu commented Oct 12, 2023

qingxinwu commented Aug 31, 2023 •

edited by pr-preview bot

Loading

qingxinwu commented Aug 31, 2023 •

edited

Loading