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

[Spec] Additional bids and negative targeting feature #796

Merged
merged 41 commits into from
Oct 20, 2023

Conversation

qingxinwu
Copy link
Collaborator

@qingxinwu qingxinwu commented Sep 12, 2023

See the explainer PR for the feature.

Patching the fetch method and handling the response header are still todos, waiting for https://github.com/WICG/turtledove/pull/771/files to land, which does similar things for direct from seller signals.


Preview | Diff

@qingxinwu qingxinwu added the spec Relates to the spec label Sep 12, 2023
@qingxinwu qingxinwu marked this pull request as draft September 12, 2023 19:30
@qingxinwu qingxinwu requested a review from morlovich September 19, 2023 19:51
@qingxinwu qingxinwu marked this pull request as ready for review September 19, 2023 19:52
@qingxinwu
Copy link
Collaborator Author

@morlovich Mind taking a look at the huge spec change for additional bid and negative targeting? Thanks! It might take some time to do a full review, so feel free to send in some comments so that I can start addressing those ealier, while you continue reviewing.

@orrb1 FYI, and feel free to review as well, especially the explaination paragraphs.

@qingxinwu qingxinwu changed the title (WIP) Spec for additional bids and negative targeting feature Spec for additional bids and negative targeting feature Sep 19, 2023
@qingxinwu qingxinwu changed the title Spec for additional bids and negative targeting feature [Spec] Additional bids and negative targeting feature Sep 19, 2023
Copy link
Collaborator

@orrb1 orrb1 left a comment

Choose a reason for hiding this comment

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

Still working through it, just sending first round of comments. Thanks, Qingxin!

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
Copy link
Collaborator

@orrb1 orrb1 left a comment

Choose a reason for hiding this comment

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

Thanks again, Qingxin!

spec.bs Outdated 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 Show resolved Hide resolved
@qingxinwu qingxinwu requested review from domfarolino and removed request for orrb1 September 25, 2023 17:39
@qingxinwu
Copy link
Collaborator Author

I'll be able to review this but might be a tiny bit slow, so I'll want for some of the outstanding comments to get resolved first.

Done.

Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

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

Back from vacation. I made it through most of the PR, so I'll submit what I have now.

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@qingxinwu qingxinwu mentioned this pull request Oct 16, 2023
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
Co-authored-by: Dominic Farolino <[email protected]>
@qingxinwu qingxinwu requested a review from domfarolino October 18, 2023 14:55
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
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 Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@qingxinwu
Copy link
Collaborator Author

qingxinwu commented Oct 19, 2023

Addressed all comments.
(strikethrough) Will address other open comments a little bit later today.

Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks. Feel free to merge.

@JensenPaul JensenPaul merged commit 433120f into WICG:main Oct 20, 2023
1 check passed
github-actions bot added a commit that referenced this pull request Oct 20, 2023
SHA: 433120f
Reason: push, by JensenPaul

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@qingxinwu qingxinwu deleted the negative branch October 20, 2023 15:04
github-actions bot added a commit to qingxinwu/turtledove that referenced this pull request Oct 20, 2023
SHA: 433120f
Reason: push, by qingxinwu

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec Relates to the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants