Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update explainer to talk about configs and use cases, rather than urn:uuids and modes #56
Update explainer to talk about configs and use cases, rather than urn:uuids and modes #56
Changes from 22 commits
6f4ae57
0ba427d
63903c2
fa25fdd
b745590
d27f6e8
d188c73
83cf282
21509f0
ba59755
e209010
89e4669
583dfb5
9413947
319fcc9
c5728f6
6b3ce12
f334f5b
f39977e
1055c08
f233f19
3d9dc87
f406e08
3883b40
aa7f8c1
a23153f
482d34c
52ef66f
8ebbf63
3b6807e
123e716
97f6725
3f09aed
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now the API is still
canLoadOpaqueURL()
. Should we make a reference to that, saying thatcanLoadConfig()
will replace it in the future, and in the meantime both will exist?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed maybe removing this section from the explainer for now. Do you still plan on doing that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I semi-reverted the changes here. Now it describes the same HTMLFencedFrameElement.canLoadOpaqueURL() function, but as "would a fenced frame config with an opaque mapped url be able to load in this context?"
We can fully replace it when the replacement is fully determined/implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two issues here:
setAttribute()
won't actually work here since theconfig
attribute is not a content attribute (string), but only a WebIDL attribute, which are only settable via JS. You can tell this because we don't have[Reflect]
in our WebIDL: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/fenced_frame/html_fenced_frame_element.idl;l=10;drc=d99163d64d43b85a9efcf0110a0cfa81886b4616There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FF Opaque Source explainer example shows the auction result promise resolving to an opaque url...
One chooses this path 'upstream' by returning a simple
'render': url
fromgenerateBid()
.And the means to opt into the new way is to return
'render': {url: renderUrl, size: {width: renderWidth, height: renderHeight}},
and then use
Is that correct?
Or does the embedder set dimensions here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the way you'll get FLEDGE's promise to resolve a
FencedFrameConfig
(instead of a URN) is by havingresolveToConfig: true
inside themyAuctionConfig
(that you pass intorunAdAuction()
). That's how you communicate your preference to deal in terms of configs (instead of URNs) to FLEDGE. This behavior isn't enabled yet though, so if you do it today you'll still get a URN. In fact, even once the behavior is enabled, it will only be rolled out gradually to more users for testing, so if you pass inresolveToConfig
there will be a transition period where FLEDGE may still give you a URN. Long-term, FLEDGE will always resolve to aFencedFrameConfig
with or withoutresolveToConfig: true
in the auction configuration.The reason all of this is not super clear yet is because I have not made the PR to the FLEDGE repository which describes exactly how to take advantage of this, but I'll be doing that soon.
Regarding your first question:
I am not sure about this (not as familiar with FLEDGE). @gtanzer do you know?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the combination of those things is correct. In order to hook into the new size behavior in FLEDGE, you will need to declare ad sizes in the interest group declaration, make a bid that includes both url and size, put
resolveToConfig
in the auction config, and then store the winning config in.config
as @dmdabbs's first example shows (to the extent that the fenced frames changes are rolled out). (You can't/don't need to modify a config that gets returned from FLEDGE; it already has everything baked in.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it may not be necessary to use configs to hook into the FLEDGE size changes: we would just need to do some extra implementation work to make urns work. We'll get back to you on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An IG ad's size metdata may be percentages,
The penny hasn't dropped for me how the internal size (I forget your terminology) is set and the external (page-visible dimensions).
Also how pubs/sellers deal with the URN->Newfangled Config rollout/transition.
I appreciate your explanations, @domfarolino and @gtanzer .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@domfarolino Re the original comment, updated to
adFrame.config = auctionWinnerConfig;