[webkit-reviews] review granted: [Bug 224197] Safari not computing image aspect ratios from width and height attributes for lazy loaded images : [Attachment 426536] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted cathiechen <cathiechen at igalia.com>'s
request for review:
Bug 224197: Safari not computing image aspect ratios from width and height
attributes for lazy loaded images
https://bugs.webkit.org/show_bug.cgi?id=224197

Attachment 426536: Patch

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




--- Comment #10 from Darin Adler <darin at apple.com> ---
Comment on attachment 426536
  --> https://bugs.webkit.org/attachment.cgi?id=426536
Patch

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

This is OK, but I think it could be made better. Suggestions below.

> Source/WebCore/rendering/RenderImage.cpp:854
> +    if (!element() || !is<HTMLImageElement>(*element()))
> +	   return false;
> +
> +    if (isShowingAltText())
> +	   return false;
> +
> +    return true;

The is<> function includes built-in null checking, so this can be written like
this:

    return is<HTMLIMageElement>(element()) && isShowingAltText();

I think that’s clearer than the multiple return statements.

>> Source/WebCore/rendering/RenderReplaced.cpp:477
>> +bool RenderReplaced::getAspectRatioFromWidthHeight(double& intrinsicRatio)
const
> 
> This should probably return Optional<double>.

Yes!

And to follow WebKIt coding style we’d then remove the word "get" from the
name. Not even sure that "FromWidthHeight" is a good way to name this. I think
it should be named intrinsicAspectRatio.

> Source/WebCore/rendering/RenderReplaced.h:61
> +    virtual bool canMapWidthHeightAsAspectRatio() const { return false; }

I don’t understand the name of this function. I don’t know what "map width
height as aspect ratio" means. Maybe this should be named
hasIntrinsicAspectRatio?

I am not 100% sure what the concept is. For example, if this returns false does
that mean we have no intrinsic aspect ratio at all? Or does it just mean it
should be ignored in RenderImage::computeIntrinsicRatioInformation?

> LayoutTests/imported/w3c/ChangeLog:9
> +	   The test cases for error images and images without src in
img-aspect-ratio.html are passed. This patch
> +	   doesn't change the behavior of the original aspect ratio case, so
it's failed like before.

Before doing more work here, I think we should take the time to restructure
this test so it does multiple PASS/FAIL and not just a single FAIL. I expect
that won’t be too difficult.