[webkit-reviews] review granted: [Bug 217529] Define image intrinsic aspect ratio by mapping to the `aspect-ratio` property. : [Attachment 429304] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted cathiechen
<cathiechen at igalia.com>'s request for review:
Bug 217529: Define image intrinsic aspect ratio by mapping to the
`aspect-ratio` property.
https://bugs.webkit.org/show_bug.cgi?id=217529

Attachment 429304: Patch

https://bugs.webkit.org/attachment.cgi?id=429304&action=review




--- Comment #19 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 429304
  --> https://bugs.webkit.org/attachment.cgi?id=429304
Patch

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

Much better!

> Source/WebCore/html/HTMLElement.cpp:641
> +void HTMLElement::applyAspectRatioToStyle(const AtomString& widthAttribute,
const AtomString& heightAttribute, MutableStyleProperties& style)
> +{
> +    if (!document().settings().aspectRatioOfImgFromWidthAndHeightEnabled())
> +	   return;
> +
> +    double width =
parseValidHTMLFloatingPointNumber(widthAttribute).valueOr(-1);
> +    double height =
parseValidHTMLFloatingPointNumber(heightAttribute).valueOr(-1);
> +    if (width < 0 || height < 0)
> +	   return;
> +    auto ratioList = CSSValueList::createSlashSeparated();
> +    ratioList->append(CSSValuePool::singleton().createValue(width,
CSSUnitType::CSS_NUMBER));
> +    ratioList->append(CSSValuePool::singleton().createValue(height,
CSSUnitType::CSS_NUMBER));
> +    auto list = CSSValueList::createSpaceSeparated();
> +   
list->append(CSSValuePool::singleton().createIdentifierValue(CSSValueAuto));
> +    list->append(ratioList);
> +
> +    style.setProperty(CSSPropertyAspectRatio,
RefPtr<CSSValue>(WTFMove(list)));
> +}

This could test in the beginning if CSSPropertyAspectRatio is already set and
bail out (since it gets called twice, separately for both and width and
height).

It would look nicer (but be slightly less efficient) in the call sites if it
didn't take width/height as parameters but grabbed both here
('applyAspectRatioFromWidthAndHeightAttributesToStyle')

> Source/WebCore/html/HTMLImageElement.cpp:121
> +	   if (hasAttribute(heightAttr))
> +	       applyAspectRatioToStyle(value, getAttribute(heightAttr), style);

The hasAttribute test here is not needed, applyAspectRatioToStyle will just
bail out if one is missing.
This should use attributeWithoutSynchronization() instead of getAttribute.

> Source/WebCore/html/HTMLImageElement.cpp:125
> +	   if (hasAttribute(widthAttr))
> +	       applyAspectRatioToStyle(getAttribute(widthAttr), value, style);

Same here.

> Source/WebCore/html/HTMLInputElement.cpp:711
> +	   if (hasAttribute(heightAttr) && isImageButton())
> +	       applyAspectRatioToStyle(value, getAttribute(heightAttr), style);

Same here.

> Source/WebCore/html/HTMLInputElement.cpp:716
> +	   if (hasAttribute(widthAttr) && isImageButton())
> +	       applyAspectRatioToStyle(getAttribute(widthAttr), value, style);

Same here.

> Source/WebCore/html/HTMLVideoElement.cpp:119
> +	   if (hasAttribute(heightAttr))
> +	       applyAspectRatioToStyle(value, getAttribute(heightAttr), style);

Same here.

> Source/WebCore/html/HTMLVideoElement.cpp:123
> +	   if (hasAttribute(widthAttr))
> +	       applyAspectRatioToStyle(getAttribute(widthAttr), value, style);

Same here.