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 webIDL dictionaries for reporting browser signals #678

Merged
merged 12 commits into from
Jul 12, 2023

Conversation

qingxinwu
Copy link
Collaborator

@qingxinwu qingxinwu commented Jun 28, 2023

Change reportWin and reportResult's browser signals from ordered maps to dictionaries, so that they can be converted to JS values and passed to javascript methods.

Also fixed an error about ad cost, which will be the last 8 bits instead of 12 bits. See the explainer


💥 Error: 400 Bad Request 💥

PR Preview failed to build. (Last tried on Jul 11, 2023, 11:23 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 2082:81 in <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.

@qingxinwu qingxinwu requested a review from jyasskin June 28, 2023 02:07
@qingxinwu
Copy link
Collaborator Author

@jyasskin Would you mind taking a look? Thanks. I believe this is still needed after changing script runner to use WebIDL callback invocation

@JensenPaul JensenPaul added the spec Relates to the spec label Jun 28, 2023
@qingxinwu
Copy link
Collaborator Author

@jyasskin friendly ping.

Copy link
Member

@jyasskin jyasskin left a comment

Choose a reason for hiding this comment

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

Sorry I took so long to get to this.

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 Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated
Comment on lines 2913 to 2914
<dd>Advertiser click or conversion cost passed from `generateBid()` to `reportWin()`. Non-negative
and only the lowest 8 bits will be passed
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the "lowest 8 bits" means for a double. Is this the rounding operation from above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated.
yes, it's the rounding operation from above.
I copied from the existing "lowest 12 bits", but after double check, that was wrong. It is 8 bits of precision in the mantissa (not including sign) and 8 bits in the exponent(code).
Another thing we need to update about the [=round a value=] algorithm is letting it take a parameter (K) about the number of precision in the mantissa. Different Ks are used for different fields in our implementation.

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@qingxinwu qingxinwu requested a review from jyasskin July 11, 2023 05:17
Copy link
Member

@jyasskin jyasskin left a comment

Choose a reason for hiding this comment

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

Thanks!

spec.bs Outdated
not null, and |buyer| is [=same origin=] with |leadingBidInfo|'s
[=leading bid info/highest scoring other bid owner=], false otherwise
<dt>{{ReportWinBrowserSignals/interestGroupName}}
<dd>|interestGroupName| if it is not null, {{undefined}} otherwise
Copy link
Member

Choose a reason for hiding this comment

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

I think this can't be null anymore.

Suggested change
<dd>|interestGroupName| if it is not null, {{undefined}} otherwise
<dd>|interestGroupName|

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This turns out to be much more complicated than just passing the IG name.
There's an open PR handling it (it's the report id stuff), so I'll just leave a todo here

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@qingxinwu qingxinwu merged commit 77601ee into WICG:main Jul 12, 2023
1 check passed
@qingxinwu qingxinwu deleted the reportsignals branch July 12, 2023 13:42
github-actions bot added a commit that referenced this pull request Jul 12, 2023
SHA: 77601ee
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 morlovich/turtledove that referenced this pull request Jul 12, 2023
SHA: 77601ee
Reason: push, by morlovich

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
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