-
Notifications
You must be signed in to change notification settings - Fork 31
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
Remove unused config size attributes #182
Conversation
The IDL containerWidth, containerHeight, contentWidth, and contentHeight attributes not actually implemented in Chromuim, so we should remove them. Additionally, the internal fenced frame config struct does not implement container size, only content size. So we should remove that as well
Is this ready for review? |
Yes, sorry! I don't think I have the proper permissions on the repo to add reviewers myself. |
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.
Very nice, thanks for this cleanup!
Before I merge, can I ask @blu25 to take a quick look too? Just since you all are more familiar with the latest FF implementation than me at the moment. |
Thanks for the review Dom! And yep, sounds good. |
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.
Changes LGTM, thanks!
SHA: a8ec772 Reason: push, by blu25 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.
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.
The IDL containerWidth, containerHeight, contentWidth, and contentHeight attributes not actually implemented in Chromuim, so we should remove them.
Additionally, the internal fenced frame config struct does not implement container size, only content size. So we should remove that as well.
Preview | Diff