[webkit-reviews] review granted: [Bug 19688] Add autodetection of image orientation from EXIF information : [Attachment 135784] another round of platform specific things and a lying compiler

Thu Apr 5 10:38:46 PDT 2012

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

Attachment 135784: another round of platform specific things and a lying
compiler
https://bugs.webkit.org/attachment.cgi?id=135784&action=review

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

r=me but please consider using an enum in place of the bool parameter in some
places.

> LayoutTests/fast/images/exif-orientation.html:22
> +<head>
> +<title>This test can only be run with layoutTestController (or by manually
setting WebKitShouldRespectImageOrientation to true).</title>
> +<script>
> +if (window.layoutTestController)
> +   
layoutTestController.overridePreference('WebKitShouldRespectImageOrientation',
1);
> +</script>
> +<style>
> +img { border: 1px solid black; }
> +div { display: inline-block; margin-right: 20px; margin-bottom: 10px; width:
100px; valign: top; }
> +</style>
> +</head>
> +<div><img src="resources/exif-orientation-1-ul.jpg"><br>Normal</div>
> +<div><img src="resources/exif-orientation-2-ur.jpg"><br>Flipped
horizontally</div>
> +<div><img src="resources/exif-orientation-3-lr.jpg"><br>Rotated
180°</div>
> +<div><img src="resources/exif-orientation-4-lol.jpg"><br>Flipped
vertically</div>
> +<br>
> +<div><img src="resources/exif-orientation-5-lu.jpg"><br>Rotated 90° CCW
and flipped vertically</div>
> +<div><img src="resources/exif-orientation-6-ru.jpg"><br>Rotated 90°
CCW</div>
> +<div><img src="resources/exif-orientation-7-rl.jpg"><br>Rotated 90° CW
and flipped vertically </div>
> +<div><img src="resources/exif-orientation-8-llo.jpg"><br>Rotated 90°
CW</div>
> +<br>
> +<div><img src="resources/exif-orientation-9-u.jpg"><br>Undefined (invalid
value)</div>

Could this be a dumpAsText(true), maybe also getting the image dimensions in JS
to check for rotation. Otherwise the text in the test will make it break with
every OS rev.

How about another test that that uses images with EXIF as CSS background and
content images to check for rotation?

> Source/WebCore/platform/graphics/BitmapImage.cpp:186
> +	   m_sizeRespectingOrientation = m_source.size(true);

Boo for unreadable bools at call sites. Maybe use an enum for the argument?

> Source/WebCore/platform/graphics/BitmapImage.cpp:197
> +	   m_sizeRespectingOrientation = m_source.size(true);

Ditto.