[webkit-reviews] review granted: [Bug 19688] Add autodetection of image orientation from EXIF information : [Attachment 136000] patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted  review:
Bug 19688: Add autodetection of image orientation from EXIF information
https://bugs.webkit.org/show_bug.cgi?id=19688

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=136000&action=review


> LayoutTests/platform/mac/fast/images/exif-orientation-css-expected.txt:14
> +Undefined (invalid value)

Should this be here?

> LayoutTests/platform/mac/fast/images/exif-orientation-expected.txt:14
> +Undefined (invalid value)

Ditto.

> Source/WebCore/platform/graphics/GraphicsContext.cpp:486
>	   // FIXME: Should be InterpolationLow
>	   setImageInterpolationQuality(InterpolationNone);

/aside Is there a bug for this FIXME?

> Source/WebCore/platform/graphics/ImageSource.cpp:109
> +    if (shouldRespectOrientation == RespectImageOrientation)
> +	   notImplemented();

Could you explain this notImplemented() with a comment?

> Source/WebCore/platform/graphics/ImageSource.cpp:117
> +    if (shouldRespectOrientation == RespectImageOrientation)
> +	   notImplemented();

Ditto.

> Source/WebCore/rendering/RenderObject.h:38
> +#include "Settings.h"

Why this include in the header?

> Source/WebCore/rendering/RenderObject.h:860
> +    RespectImageOrientationEnum shouldRespectImageOrientation() const;

Maybe this should be virtual, and have an override on RenderImage? I think the
name is a bit too general too. It's not clear in what situations this is
consulted.