Bug 217294

Equivalent chromium bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1110330

Created attachment 410502 [details]
Patch

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

Created attachment 410622 [details]
Patch

Created attachment 410623 [details]
Patch

The iOS layout test failure is a slight image-diff... I think the patch can be reviewed.

Comment on attachment 410623 [details]
Patch

iOS bot fails http/tests/security/canvas-remote-image-orientation.html.

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

> Source/WebCore/html/HTMLImageElement.h:143
> +    bool allowsOrientationOverride() const override;

final?

> Source/WebCore/rendering/RenderElement.cpp:2151
> +    return orientation;

Why not:
return element() && !element()->allowsOrientationOverride() ? ImageOrientation::FromImage : style().imageOrientation();

Created attachment 410831 [details]
Patch

Created attachment 410847 [details]
Patch

Comment on attachment 410847 [details]
Patch

LGTM overall but I hope we can change http/tests/security/canvas-remote-image-orientation.html so that it passes on iOS.

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

> Source/WebCore/ChangeLog:31
> +                Only apply orientation for eligible images. 

Strange indentation here and above.

> Source/WebCore/html/HTMLImageElement.cpp:690
> +    return image->isOriginClean(&(document().securityOrigin()));

Could be a one liner too.

> LayoutTests/ChangeLog:11
> +                Added a test to ensure image-orientation: none is not respected for remote images in canvas

Strange indentation.

> LayoutTests/imported/w3c/ChangeLog:10
> +                Imported a new W3C test for remote image with image-orientation.

Strange indentation.

> LayoutTests/platform/ios-simulator-wk2/TestExpectations:11
> +http/tests/security/canvas-remote-image-orientation.html [ ImageOnlyFailure ]

Isn't there a way we could change the test slightly?
It seems the WPT tests are passing, so maybe the idea is to change the canvas size?

Also, we could migrate that test to http/wpt so that we would more easily reuse WPT exif-orientation-3-lr.jpg (or even contribute it to WPT at some point).

Comment on attachment 410847 [details]
Patch

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

>> LayoutTests/platform/ios-simulator-wk2/TestExpectations:11
>> +http/tests/security/canvas-remote-image-orientation.html [ ImageOnlyFailure ]
> 
> Isn't there a way we could change the test slightly?
> It seems the WPT tests are passing, so maybe the idea is to change the canvas size?
> 
> Also, we could migrate that test to http/wpt so that we would more easily reuse WPT exif-orientation-3-lr.jpg (or even contribute it to WPT at some point).

There is already a wpt test for this. I wasn't aware of http/wpt, I'll import it there instead of the webkit-specific test. Should I land this with the test working on ios, or do you want to re-review once I have that ready?

> There is already a wpt test for this. I wasn't aware of http/wpt, I'll
> import it there instead of the webkit-specific test. Should I land this with
> the test working on ios, or do you want to re-review once I have that ready?

I think the patch is good to land once the test works on iOS.

Created attachment 410861 [details]
Patch

Comment on attachment 410861 [details]
Patch

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

> Source/WebCore/dom/Element.h:340
> +    virtual bool allowsOrientationOverride() const { return true; }

I do not think it is worthy adding this as a virtual method because it is overridden by one superclass which is HTMLImageElement. All it does is it saves you a casting to HTMLImageElement in RenderElement::imageOrientation().

> Source/WebCore/rendering/RenderElement.cpp:2143
> +    return (element() && !element()->allowsOrientationOverride()) ? ImageOrientation(ImageOrientation::FromImage) : style().imageOrientation();

Can be written like this:

    auto* imageElement = is<HTMLImageElement>(element()) ? downcast<HTMLImageElement>(element()) : nullptr;
    return (imageElement && ...

Created attachment 410920 [details]
Patch for landing

Committed r268249: <https://trac.webkit.org/changeset/268249>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410920 [details].

<rdar://problem/70130571>