Bug 230242 – Implement the borderBoxSize/contentBoxSize parts of ResizeObserver

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Simon Fraser (smfr)]

Implement the borderBoxSize/contentBoxSize parts of ResizeObserver

[Simon Fraser (smfr)]

Created attachment 438098 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 438102 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 438103 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 438152 [details]
Patch

[zalan]

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() };

[Sam Weinig]

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.

[Simon Fraser (smfr)]

Created attachment 438161 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/83132690>