Try to fix build after fenced frame spec container size removal. #1270
Conversation
I think one of our uses of it was meant to be content size instead.
|
@gtanzer would appreciate your feedback on whether this is actually doing the right thing. |
|
Instead of asking Garrett, would be better to ask the owner of this commit that removed container size. |
|
I think Andrew removed too much; container size is implemented in Chrome and used by PA... That PR didn't just remove the IDL getters for the fields; it removed the container size fields themselves. It's possible that the container size fields weren't functional in the fenced frame spec though. They should be used to set the width/height fields of the fenced frame element upon loading a config, if the fields haven't previously been set manually. |
|
Hmm, was the intent of the spec here that the requested size is set as the container size, and the actual size as the content size? |
|
Yes, see the relevant line of the implementation here: |
|
OK, so this is incorrect and it's good that I checked with you. Hmm, I can't seem to be able to @VergeA; I guess I should follow up on the original commit. |
|
Hey folks, just catching up. When I was looking into the implementation, I missed the actual usage of container_size to set width and height; I thought container size was vestigial given that it wasn't provided explicitly in the internal config struct. I'll write up another patch to re-add the container_size internal struct field to un-break the build, and we can determine how to spec the actual behavior from there. |
|
The constructors don't mean a ton (that is a longstanding refactor todo task, to unify everything into one exhaustive constructor); PA fills in the fields separately. |
Follow-up to WICG#182 Along with the unused IDL size attributes, I also removed the internal fenced frame config container size definition. I was mistaken about it being vestigial (I missed a reference to it in the implementation when I was originally deciding to remove it, so I didn't see it was actively used). The Protected Audience spec depended on the field, and I broke their build: WICG/turtledove#1270 (comment) This change re-adds the container size definition to un-break things.
|
I've got a PR that re-adds the container size field here: WICG/fenced-frame#187 |
|
Thanks! Closing this one... |
Follow-up to #182 Along with the unused IDL size attributes, I also removed the internal fenced frame config container size definition. I was mistaken about it being vestigial (I missed a reference to it in the implementation when I was originally deciding to remove it, so I didn't see it was actively used). The Protected Audience spec depended on the field, and I broke their build: WICG/turtledove#1270 (comment) This change re-adds the container size definition to un-break things.
I think one of our uses of it was probably meant to be content size instead.
Preview | Diff