[webkit-reviews] review requested: [Bug 52040] Implement object-fit CSS property : [Attachment 209684] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has asked  for review:
Bug 52040: Implement object-fit CSS property
https://bugs.webkit.org/show_bug.cgi?id=52040

Attachment 209684: Patch
https://bugs.webkit.org/attachment.cgi?id=209684&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=209684&action=review


> Source/WebCore/css/CSSPrimitiveValueMappings.h:3597
> +    switch (m_value.valueID) {

Please add an ASSERT(isValueID());

> Source/WebCore/loader/cache/CachedImage.h:73
> +    enum SizeType {
> +	   NormalSize, // Report the size of the image associated with a
certain renderer
> +	   IntrinsicSize // Report the intrinsic size, i.e. ignore whatever has
been set extrinsically.
> +    };

These comments are a bit confusing. Can you clarify what these do.

> Source/WebCore/platform/graphics/LayoutRect.h:163
> +    void scale(float xAxisScale, float yAxisScale);

The parameters aren't great.  How about xScale, yScale.

> Source/WebCore/platform/graphics/LayoutSize.h:126
> +	   if ((widthScale > heightScale) != (fit == AspectRatioFitGrow))

This if-statement is not easy to understand.

> Source/WebCore/rendering/RenderHTMLCanvas.cpp:85
> +    if (clip)
> +	   context->restore();

Can you use a stack based protector here?

> Source/WebCore/rendering/RenderImage.cpp:277
> +	   if (everHadLayout() && !selfNeedsLayout()) {
> +	       // The inner content rectangle is calculated during layout, but
may need an update now
> +	       // (unless the box has already been scheduled for layout). In
order to calculate it, we
> +	       // may need values from the containing block, though, so make
sure that we're not too
> +	       // early. It may be that layout hasn't even taken place once
yet.
> +
> +	       // FIXME: we should not have to trigger another call to
setContainerSizeForRenderer()
> +	       // from here, since it's already being done during layout.
> +	       updateInnerContentRect();
> +	   }

This looks odd.  Is this just an optimization?

> Source/WebCore/rendering/RenderImage.cpp:440
> +	   if (clip)
> +	       context->restore();

Can we use stack based protection.

> Source/WebCore/rendering/style/RenderStyleConstants.h:213
> +enum EObjectFit {

No E!