[webkit-changes] [WebKit/WebKit] e6a319: RenderReplaced::computeAspectRatioInformationForRe...

[Sammy Gill]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: e6a31953a657cbee20e39b544e0bbd5691de95de
      https://github.com/WebKit/WebKit/commit/e6a31953a657cbee20e39b544e0bbd5691de95de
  Author: Sammy Gill <sammy.gill at apple.com>
  Date:   2023-02-26 (Sun, 26 Feb 2023)

  Changed paths:
    M LayoutTests/TestExpectations
    A LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-height-constrained-by-max-width-expected.html
    A LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-height-constrained-by-max-width.html
    A LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-width-constrained-by-max-height-expected.html
    A LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-width-constrained-by-max-height.html
    A LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-inline-size-containment-no-crash.html
    M LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-intrinsic-width-height.html
    M LayoutTests/platform/gtk/TestExpectations
    M LayoutTests/platform/wpe/TestExpectations
    M Source/WebCore/rendering/RenderReplaced.cpp

  Log Message:
  -----------
  RenderReplaced::computeAspectRatioInformationForRenderBox does not need to call into RenderBox::computeReplacedLogicalWidth
https://bugs.webkit.org/show_bug.cgi?id=252589
rdar://105489410

Reviewed by Alan Baradlay.

The call to RenderBox::computeReplacedLogicalWidth from RenderReplaced::computeAspectRatioInformationForRenderBox
when the object was a video with a default object size was extraneous
and can be safely removed without changing any functionality. It could
also result in a stack overflow in certain conditions. At this
point we know the intrinsic sizes of the object anyway since it is a
video with a default object size (300px x 150px). This is
because:

1.) RenderBox::computeReplacedLogicalWidth would call into computeReplacedLogicalWidthUsing(MainOrPreferredSize, style().logicalWidth())
    com compute the width value to use
2.) The caller (RenderReplaced::computeReplacedLogicalWidth in this case)
    will only use the width value of style().logicalWidth().isAuto()
    returns true.
3.) If this is true RenderBox::computeReplacedLogicalWidth would have
    returned the intrinsic width but constrained to min/max sizes. However,
    the caller constrains the intrinsic width anyway.
4.) If this is false (style().logicalWidth().isAuto() returns false),
    then the caller (RenderReplaced::computeReplacedLogicalWidth) will
    use the intrinsic width anyway by calling into intrinsicLogicalWidth.

We can also take advantage of the existing logic to constrain the
intrinsic sizes in each dimension by transferred constraints from the
opposite one.

Here is an example of when this extra call could cause a stack overflow:

<style>
video {
  aspect-ratio: 1;
  container-type: inline-size;
  inset: 0 auto;
  min-width: min-content;
  position: fixed;
}
</style>
<video></video>

WebCore::RenderBox::computeReplacedLogicalWidthRespectingMinMaxWidth(WebCore::LayoutUnit, WebCore::ShouldComputePreferred) const
WebCore::RenderBox::computeReplacedLogicalWidth(WebCore::ShouldComputePreferred) const
WebCore::RenderReplaced::computeIntrinsicSizesConstrainedByTransferredMinMaxSizes(WebCore::RenderBox*, WebCore::FloatSize&, WebCore::FloatSize&) const
WebCore::RenderReplaced::computeReplacedLogicalHeight(std::__1::optional<WebCore::LayoutUnit>) const
WebCore::RenderImage::computeReplacedLogicalHeight(std::__1::optional<WebCore::LayoutUnit>) const
WebCore::RenderBox::computePositionedLogicalHeightReplaced(WebCore::RenderBox::LogicalExtentComputedValues&) const
WebCore::RenderBox::computePositionedLogicalHeight(WebCore::RenderBox::LogicalExtentComputedValues&) const
WebCore::RenderBox::computeLogicalHeight(WebCore::LayoutUnit, WebCore::LayoutUnit) const
WebCore::RenderBox::computeLogicalWidthFromAspectRatioInternal() const
WebCore::RenderBox::computeIntrinsicLogicalWidthUsing(WebCore::Length, WebCore::LayoutUnit, WebCore::LayoutUnit) const
WebCore::RenderBox::computeReplacedLogicalWidthUsing(WebCore::SizeType, WebCore::Length) const
WebCore::RenderBox::computeReplacedLogicalWidthRespectingMinMaxWidth(WebCore::LayoutUnit, WebCore::ShouldComputePreferred) const
WebCore::RenderVideo::computeReplacedLogicalWidth(WebCore::ShouldComputePreferred) const
WebCore::RenderBox::computePositionedLogicalWidthReplaced(WebCore::RenderBox::LogicalExtentComputedValues&) const
WebCore::RenderBox::computePositionedLogicalWidth(WebCore::RenderBox::LogicalExtentComputedValues&, WebCore::RenderFragmentContainer*) const
WebCore::RenderBox::computeLogicalWidthInFragment(WebCore::RenderBox::LogicalExtentComputedValues&, WebCore::RenderFragmentContainer*) const
WebCore::RenderBox::updateLogicalWidth()
WebCore::RenderReplaced::layout()
WebCore::RenderImage::layout()
WebCore::RenderMedia::layout()
WebCore::RenderVideo::layout()

After we have computed the width for the video, we call RenderBox::RenderBox::computeReplacedLogicalWidthRespectingMinMaxWidth
to restrict it within any min and max constraints. This eventually calls into RenderBox::computeIntrinsicLogicalWidthUsing to compute the
min-width using MinContent as the width type.

The container-type: inline-size; and inset: 0 auto; properties cause RenderBox::shouldComputeLogicalWidthFromAspectRatio to return true
because the call to shouldComputeLogicalWidthFromAspectRatioAndInsets returns true. This last method returns true because:
1.) Applying the inline size containment causes hasConstrainedWidth to be false when it would normally be true from the video’s intrinsic width.
2.) hasConstrainedHeight gets set to false because inset causes the logical top and property values to get set to a fixed value
3.) The final call to style.logicalHeight().isAuto() returns true

Due to these series of events we end up calling computeLogicalWidthFromAspectRatioInternal inside RenderBox::computeIntrinsicLogicalWidthUsing.
This eventually recurses its way back into RenderBox::computeReplacedLogicalWidthRespectingMinMaxWidth through the reset of the stack trace.

* LayoutTests/TestExpectations:
* LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-height-constrained-by-max-width-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-height-constrained-by-max-width.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-width-constrained-by-max-height-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-default-object-width-constrained-by-max-height.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-inline-size-containment-no-crash.html: Added.
This test currently crashes due to a different reason (webkit.org/b/252594),
but is still useful to this patch as it was causing the stack
overflow.

* LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/video-intrinsic-width-height.html:
One of the test cases was incorrectly changed in a previous patch and
is being restored to its previous version. The height for this test
should be 150px and not 300px because it falls under the case:

If 'height' and 'width' both have computed values of 'auto' and the
element also has an intrinsic height, then that intrinsic height is
the used value of 'height'.
https://www.w3.org/TR/CSS22/visudet.html#inline-replaced-height

* LayoutTests/platform/gtk/TestExpectations:
* LayoutTests/platform/wpe/TestExpectations:
* Source/WebCore/rendering/RenderReplaced.cpp:
(WebCore::RenderReplaced::computeIntrinsicSizesConstrainedByTransferredMinMaxSizes const):

Canonical link: https://commits.webkit.org/260851@main