Bug 230242 - Implement the borderBoxSize/contentBoxSize parts of ResizeObserver
Summary: Implement the borderBoxSize/contentBoxSize parts of ResizeObserver
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar, WebExposed, WPTImpact
Depends on:
Blocks:
 
Reported: 2021-09-13 21:00 PDT by Simon Fraser (smfr)
Modified: 2021-09-14 23:18 PDT (History)
15 users (show)

See Also:


Attachments
Patch (58.81 KB, patch)
2021-09-13 21:06 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (57.86 KB, patch)
2021-09-13 22:09 PDT, Simon Fraser (smfr)
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (58.85 KB, patch)
2021-09-13 22:18 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (63.04 KB, patch)
2021-09-14 09:49 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (63.82 KB, patch)
2021-09-14 13:26 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2021-09-13 21:00:50 PDT
Implement the borderBoxSize/contentBoxSize parts of ResizeObserver
Comment 1 Simon Fraser (smfr) 2021-09-13 21:06:10 PDT
Created attachment 438098 [details]
Patch
Comment 2 Simon Fraser (smfr) 2021-09-13 22:09:44 PDT
Created attachment 438102 [details]
Patch
Comment 3 Simon Fraser (smfr) 2021-09-13 22:18:52 PDT
Created attachment 438103 [details]
Patch
Comment 4 Simon Fraser (smfr) 2021-09-14 09:49:12 PDT
Created attachment 438152 [details]
Patch
Comment 5 zalan 2021-09-14 11:49:54 PDT
Comment on attachment 438152 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=438152&action=review

> Source/WebCore/rendering/RenderBox.h:141
> +    LayoutSize logicalSize() const { return style().isHorizontalWritingMode() ? m_frameRect.size() : m_frameRect.size().transposedSize(); }

any reason not relying on the existing logical functions e.g return { logicalWidth(), logicalHeight() };
Comment 6 Sam Weinig 2021-09-14 11:57:59 PDT
Comment on attachment 438152 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=438152&action=review

DOM stuff looks good.

> Source/WebCore/page/ResizeObservation.cpp:106
> +bool ResizeObservation::elementSizeChanged(BoxSizes& currentSize) const

I would return either a pair or a bespoke struct here instead of using an out parameter.

> Source/WebCore/page/ResizeObserverBoxOptions.idl:26
> +// https://wicg.github.io/ResizeObserver/

I would update this link to the more specific https://wicg.github.io/ResizeObserver/#enumdef-resizeobserverboxoptions or https://drafts.csswg.org/resize-observer/#enumdef-resizeobserverboxoptions. (I wish there were a way to point to a particular version, like based on the git sha), so that if the spec changes, we know which one our implementation was based on, but I don't think there is a way to do that.

> Source/WebCore/page/ResizeObserverBoxOptions.idl:30
> +    "content-box"

I would put "device-pixel-content-box" commented out here with a bugzilla link to a bug about it.

> Source/WebCore/page/ResizeObserverEntry.h:68
> +    // The spec is designed to allow mulitple boxes for multicol scenarios, but for now these vectors only ever contain one entry.
> +    Vector<Ref<ResizeObserverSize>> m_borderBoxSizes;
> +    Vector<Ref<ResizeObserverSize>> m_contentBoxSizes;

One day, we should really convert these types of immutable vectors that don't ever change size once constructed to use something more efficient like FixedVector (and we should fix FixedVector to not depend on RefCountedArray).

> Source/WebCore/page/ResizeObserverOptions.idl:26
> +// https://wicg.github.io/ResizeObserver/

Again, I would use https://drafts.csswg.org/resize-observer/#dictdef-resizeobserveroptions

> Source/WebCore/page/ResizeObserverSize.idl:26
> +// https://wicg.github.io/ResizeObserver/

Again, https://drafts.csswg.org/resize-observer/#resizeobserversize.
Comment 7 Simon Fraser (smfr) 2021-09-14 13:26:47 PDT
Created attachment 438161 [details]
Patch
Comment 8 EWS 2021-09-14 23:17:04 PDT
Committed r282441 (241694@main): <https://commits.webkit.org/241694@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438161 [details].
Comment 9 Radar WebKit Bug Importer 2021-09-14 23:18:16 PDT
<rdar://problem/83132690>