Bug 100698

Created attachment 171313 [details] Patch

Comment on attachment 171313 [details] Patch Could we make this say PASS/FAIL instead? You, the test writer, have that information better than someone bringing up a new port and staring at this for the first time. :)

Created attachment 171334 [details] Patch

(In reply to comment #2) > (From update of attachment 171313 [details]) > Could we make this say PASS/FAIL instead? You, the test writer, have that information better than someone bringing up a new port and staring at this for the first time. :) Sure thing, done. (I copied the behavior of exif-orientation.html, but I agree that it's better now.)

Created attachment 171335 [details] Patch

Comment on attachment 171334 [details] Patch I would have done something like expectHorizontal(1); expectVertical(2); for each of the cases. But this is fine too.

Comment on attachment 171335 [details] Patch Rejecting attachment 171335 [details] from commit-queue. New failing tests: platform/chromium/virtual/deferred/fast/images/exif-orientation-image-document.html Full output: http://queues.webkit.org/results/14631303

Comment on attachment 171334 [details] Patch Attachment 171334 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14623550 New failing tests: platform/chromium/virtual/deferred/fast/images/exif-orientation-image-document.html

Created attachment 171364 [details] Patch for landing

hclam: The test I'm adding in this CL fails with in the deferred image decoding suite. I'm adding it to TestExpectations to the block of other failing tests. It'll need to be fixed before deferred decoding can become turned on by default.

(In reply to comment #10) > hclam: The test I'm adding in this CL fails with in the deferred image decoding suite. I'm adding it to TestExpectations to the block of other failing tests. It'll need to be fixed before deferred decoding can become turned on by default. okay sounds good to me.

Comment on attachment 171364 [details] Patch for landing Rejecting attachment 171364 [details] from commit-queue. New failing tests: platform/chromium/virtual/deferred/fast/images/exif-orientation-image-document.html Full output: http://queues.webkit.org/results/14642307

Created attachment 171368 [details] Patch for landing

Comment on attachment 171368 [details] Patch for landing Clearing flags on attachment: 171368 Committed r132877: <http://trac.webkit.org/changeset/132877>

All reviewed patches have been landed. Closing bug.

Chromium Expectations added in r132989 <http://trac.webkit.org/changeset/132989>

Nico, are you sure this should work without overriding the preference? My suspicion is, it passes on Chromium since the preference is default on, isn't it? I would suggest to add testRunner.overridePreference('WebKitShouldRespectImageOrientation', 1);

Hi Dominik, (In reply to comment #17) > Nico, are you sure this should work without overriding the preference? Yes, that's correct: The preference controls if exif information is used for <img> elements in html documents. For image documents, exif information is always used on ports that support it. > My suspicion is, it passes on Chromium since the preference is default on, isn't it? No, the pref is off by default in chromium, and we don't plan to turn it on in the close future. > I would suggest to add > testRunner.overridePreference('WebKitShouldRespectImageOrientation', 1); That won't have an effect on the test. For the exif stuff to work, your port needs to have explicit code to support drawing of rotated images. For that reason, RenderObject::shouldRespectImageOrientation() has an #ifdef for the imagedocument code that's currently only on for mac and chromium. If your port's drawing code doesn't support honoring image orientation, then without that ifdef images would be laid out according to their exif information, but then only stretched (instead of rotated) at paint time – which is worse than them being 90 degree rotated. (To see what I mean, remove the ifdef in RenderObject::shouldRespectImageOrientation() and look at the test this bug added in your port). If you want to get this test passing on your port, you need to implement drawing support first and then enable the ifdef for your port.

Comment on attachment 171368 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=171368&action=review Isn't this a text test? > LayoutTests/platform/chromium/TestExpectations:3829 > +Bug(thakis) fast/images/exif-orientation-image-document.html [ ImageOnlyFailure ] image?

(In reply to comment #19) > (From update of attachment 171368 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171368&action=review > > Isn't this a text test? No. The test says `testRunner.dumpAsText(true);`, and the `true` means "generate pixel dumps" as far as I know. If your question is "shouldn't it be sufficient to make this a text-only test?": No, because the image bounds alone don't tell you if an image is upside down. > > > LayoutTests/platform/chromium/TestExpectations:3829 > > +Bug(thakis) fast/images/exif-orientation-image-document.html [ ImageOnlyFailure ] > > image?

(In reply to comment #18) Thanks Nico for the detailed explanations. I did implement the rotation/flipping code in bug 101207 for Cairo, however, i didn't find that RenderObject #ifdef earlier. > > I would suggest to add > > testRunner.overridePreference('WebKitShouldRespectImageOrientation', 1); > > That won't have an effect on the test. Actually, it does. If I enable the setting inside the test, it passes on Cairo/EFL since the second line of the if-condition in RenderObject::shouldRespectImageOrientation checks for the setting. > For the exif stuff to work, your port needs to have explicit code to support drawing of rotated images. For that reason, RenderObject::shouldRespectImageOrientation() has an #ifdef for the imagedocument code that's currently only on for mac and chromium. [...] If you want to get this test passing on your port, you need to implement drawing support first and then enable the ifdef for your port. Yep, I'll add USE(CAIRO) to the ifdef in RenderObject that you mentioned. Thanks again.