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

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request 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 Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=209684&action=review


r=me

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

This is compact but the logic is hard to follow. It would be helpful to
separate the AspectRatioFitShrink/AspectRatioFitGrow cases so both get
explicitly named.

> Source/WebCore/rendering/RenderHTMLCanvas.cpp:74
> +    bool clip = !contentRect.contains(paintRect);
> +    if (clip) {
> +	   // Not allowed to overflow the content box.
> +	   paintInfo.context->save();
> +	   paintInfo.context->clip(pixelSnappedIntRect(contentRect));
> +    }

If this is a common pattern it might be good to add a RAII helper.

> Source/WebCore/rendering/RenderImage.cpp:425
> +	   LayoutRect contentRect = contentBoxRect();
> +	   contentRect.moveBy(paintOffset);
> +	   LayoutRect paintRect = replacedContentRect();
> +	   paintRect.moveBy(paintOffset);
> +	   bool clip = !contentRect.contains(paintRect);
> +	   if (clip) {
> +	       context->save();
> +	       context->clip(contentRect);
> +	   }

The pattern seems to be repeating.

> Source/WebCore/rendering/style/RenderStyleConstants.h:215
> +enum EObjectFit {
> +    ObjectFitFill, ObjectFitContain, ObjectFitCover, ObjectFitNone,
ObjectFitScaleDown
> +};

Can't we stop using this ancient EFoo naming convention?