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 is ad component field to fenced frame config. #115

Merged
merged 16 commits into from
Nov 12, 2023

Conversation

xiaochen-z
Copy link
Collaborator

@xiaochen-z xiaochen-z commented Aug 24, 2023

This is the first step to spec the ad component reportEvent change.


Preview | Diff

@domfarolino
Copy link
Collaborator

How will this be used? I'm just trying to get a little context to review this, by understanding how it'll need to look in the future?

the ad component reportEvent change.

... is a little vague.

@xiaochen-z
Copy link
Collaborator Author

How will this be used? I'm just trying to get a little context to review this, by understanding how it'll need to look in the future?

the ad component reportEvent change.

... is a little vague.

For ad component, event reporting works different than the main ad. This boolean field allows related event reporting algorithms to know whether this ad is an ad component or not. Please see the explainer for ad component event reporting.

@domfarolino
Copy link
Collaborator

OK, could you include some of that context directly in the PR as a non-normative note, because that context is very useful? This seems to be the first time "component ads" are really referenced in the spec, so at the very least linking to that explainer content from the PR would be wonderful.

@xiaochen-z
Copy link
Collaborator Author

Thanks. I've added a note on ad components and the special handling on event reporting.

@gtanzer
Copy link
Collaborator

gtanzer commented Sep 6, 2023

I'm not sure that this field is necessary in spec. I think it's used in the implementation only because the renderer can't access the entire frame tree with ancestors' fenced frame properties. But I might be misremembering.

@domfarolino
Copy link
Collaborator

I'm not sure that this field is necessary in spec. I think it's used in the implementation only because the renderer can't access the entire frame tree with ancestors' fenced frame properties. But I might be misremembering.

Should we abandon this or...? I'll trust people more familiar with the implementation these days to make the call.

@xiaochen-z
Copy link
Collaborator Author

This field is used at browser to identify an ad component frame. If we do not mention this field in the spec, is it sufficient to provide a definition of ad component and then describe the algorithm like "if this is an ad component, then ..."?

@gtanzer
Copy link
Collaborator

gtanzer commented Sep 12, 2023

Actually maybe "is ad component" is necessary because you can (maybe not right now, but at some point) created nested fenced frames with different constructors, and you want to treat the actual ad component frames differently...

@domfarolino
Copy link
Collaborator

So shall we land this @gtanzer?

@gtanzer
Copy link
Collaborator

gtanzer commented Sep 13, 2023

Sgtm, though maybe "is ad component" should have a default value of false.

@xiaochen-z
Copy link
Collaborator Author

Added default values.

@domfarolino
Copy link
Collaborator

Still look good @gtanzer ?

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.

Ping @gtanzer

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

gtanzer commented Nov 7, 2023

still lgtm

Co-authored-by: Dominic Farolino <[email protected]>
@domfarolino domfarolino merged commit c4e4819 into WICG:master Nov 12, 2023
2 checks passed
github-actions bot added a commit that referenced this pull request Nov 12, 2023
SHA: c4e4819
Reason: push, by domfarolino

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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants