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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Apr 6 10:48:13 PDT 2012

Simon Fraser (smfr) <simon.fraser at apple.com> has granted  review:
Bug 19688: Add autodetection of image orientation from EXIF information

Attachment 136000: patch

------- 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)


> 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();


> 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

More information about the webkit-reviews mailing list