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

Fix a number of imprecisions in GenerateBidOutput handling: #664

Merged
merged 9 commits into from
Jul 12, 2023

Conversation

morlovich
Copy link
Collaborator

@morlovich morlovich commented Jun 23, 2023

  1. The return value of generateBid() always wins over setBid if the function completed normally, even if it's invalid.
  2. setBid doesn't return true/false, it throws on invalid input.
  3. 'render' is not strictly required: if the bid value is <= 0 the object is a valid representation of not bidding regardless of other fields; this is relevant because doing setBid() with it does not throw.
  4. modelingSignals is actually an unrestricted double: the explainer has NaN/inf/-inf as ignored, and conveniently none of these are in the [0, 4095) range anyway.
  5. width/height in AdRender are optional, not required, so update IDL to match. The algorithm was already handling them correctly; OTOH url is required, so no need to check for its presense.

💥 Error: 400 Bad Request 💥

PR Preview failed to build. (Last tried on Jul 12, 2023, 2:53 PM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 CSS Spec Preprocessor - CSS Spec Preprocessor is the web service used to build Bikeshed specs.

🔗 Related URL

Error running preprocessor, returned code: 2.
FATAL ERROR: Garbage at 2076:81 in &lt;a>.
 ✘  Did not generate, due to fatal errors

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

1. The return value of generateBid() always wins over setBid if the function completed
   normally; even if it's invalid.
2. setBid doesn't return true/false, it throws on invalid input.
3. 'render' is not strictly required: if the bid value is <= 0 the
   object is a valid representation of not bidding regardless of other
   fields; this is relevant because doing setBid() with it does not throw.
4. modelingSignals is actually an unrestricted double: the explainer has
   NaN/inf/-inf as ignored, and conveniently none of these are in the
   [0, 4095) range anyway.

I've also scooted over the zero-argument setBid() next to its
full-functioned sibling, rather than separated by a giant algorithm.
@morlovich morlovich added the spec Relates to the spec label Jun 23, 2023
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
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 % build failure. Seems to be something wrong with the attribution reporting references? Locally I would take a stab at it but it looks like this PR was made on a fork so that's harder.

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 merged commit 66cc6c2 into WICG:main Jul 12, 2023
1 check passed
github-actions bot added a commit that referenced this pull request Jul 12, 2023
SHA: 66cc6c2
Reason: push, by qingxinwu

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 Jul 13, 2023
SHA: 66cc6c2
Reason: push, by qingxinwu

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
domfarolino added a commit that referenced this pull request Jul 18, 2023
domfarolino added a commit that referenced this pull request Jul 18, 2023
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.

3 participants