Add a layout test for bug 320244

Add a layout test for bug 320244

Images with EXIF orientation are sometimes shown with the wrong
orientation inside ImageDocument. The bug does not reproduce in
an <iframe> ImageDocument. Add a direct-load test that exhibits
the bug. Add a failure expectation to TestExpections, pending a
fix to http://crbug/320244

The test image is 82 x 1281 after orientation, and layout tests
use an 800 x 600 screen. In ImageDocument the test image should
be shown at 38 x 600 (w x h) to maintain aspect ratio.

TEST=fast/images/exif-orientation-height-image-document.html
BUG=320244
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167497

[pdr., 2014-02-18 02:59:18]

https://codereview.chromium.org/168583003/diff/200001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/168583003/diff/200001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:690: crbug.com/320244
fast/images/exif-orientation-height-image-document.html [ Failure ]
Why are these marked as failure? Did you mean to use [ NeedsRebaseline ]?

https://codereview.chromium.org/168583003/diff/200001/LayoutTests/fast/images...
File LayoutTests/fast/images/exif-orientation-height-image-document.html
(right):

https://codereview.chromium.org/168583003/diff/200001/LayoutTests/fast/images...
LayoutTests/fast/images/exif-orientation-height-image-document.html:5:
testRunner.queueLoad('resources/jpeg-height-exif-orientation.jpg?' +
Math.random());
Math.random() shouldn't be required here.

Can you use an existing image that's smaller (such as
resources/exif-orientation-1-ul.jpg)? This will perform better on the slow debug
bots, and will not make future reviewers hungry.

[pdr., 2014-02-18 03:02:15]

On 2014/02/18 02:59:18, pdr wrote:
>
https://codereview.chromium.org/168583003/diff/200001/LayoutTests/TestExpecta...
> File LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/168583003/diff/200001/LayoutTests/TestExpecta...
> LayoutTests/TestExpectations:690: crbug.com/320244
> fast/images/exif-orientation-height-image-document.html [ Failure ]
> Why are these marked as failure? Did you mean to use [ NeedsRebaseline ]?

Doh, because the code is not fixed.

I don't think we should check in a new failing test. Instead, lets just attach
it to the bug so whoever fixes this bug can use the test.

[Noel Gordon, 2014-02-18 03:32:01]

https://codereview.chromium.org/168583003/diff/200001/LayoutTests/fast/images...
File LayoutTests/fast/images/exif-orientation-height-image-document.html
(right):

https://codereview.chromium.org/168583003/diff/200001/LayoutTests/fast/images...
LayoutTests/fast/images/exif-orientation-height-image-document.html:5:
testRunner.queueLoad('resources/jpeg-height-exif-orientation.jpg?' +
Math.random());
On 2014/02/18 02:59:18, pdr wrote:
> Math.random() shouldn't be required here.

Lemme remove random() ...
 
> Can you use an existing image that's smaller (such as
> resources/exif-orientation-1-ul.jpg)? This will perform better on the slow
debug
> bots, and will not make future reviewers hungry.

Yeah sorry for the large image, but smaller images don't reproduce the bug if I
recall correctly.

[Noel Gordon, 2014-02-18 03:32:38]

> pdr wrote:
> Doh, because the code is not fixed.

Yeah. I first needed a find a reliable test that reproduces the bug.

"The bug does not reproduce in an <iframe> ImageDocument or a
window.open() ImageDocument. Add a direct-loading test case to
reproduce this bug, add a failure expectation to TestExpections."
 
> I don't think we should check in a new failing test. Instead, lets just attach
> it to the bug so whoever fixes this bug can use the test.

and let the bots have at it to confirm that the new test does in
fact reliably reproduce the bug.

[pdr., 2014-02-18 04:18:10]

not lgtm. This test depends on the image being large due to a race with the
renderer. This will be a flaky test on the bots.

I would recommend using the slow loading test infrastructure found in
LayoutTests/http/tests/misc/resources/slow-png-load.pl

[Noel Gordon, 2014-02-18 07:45:01]

- copied slow-png-load.pl to slow-jpeg-load.pl
- made slow-jpeg-load.pl return Content-Type: image/jpeg (not that it matters).
- moved my test into http/tests/misc, modify it read the image as follows
   testRunner.queueLoad('resources/slow-jpeg-load.pl?name=test.jpg&loadtime=1');
- tested small/large exif images, loadtime={0, 1, 2, etc}, in content shell

Result: bug reproduced only for the large exif image at loadtime=0.

[Noel Gordon, 2014-02-19 01:22:28]

Seems we do need to test a large image.

> not lgtm. This test depends on the image being large due to a race with the
> renderer.

Blink should draw the test correctly in all cases, regardless of bugs.

> This will be a flaky test on the bots.

There's no evidence of that.

[pdr., 2014-02-19 18:53:44]

On 2014/02/19 01:22:28, Noel Gordon (Google) wrote:
> > This will be a flaky test on the bots.
> 
> There's no evidence of that.

Please explain why this test won't pass with the small image. This should be
trivial to determine.

My intuition is that the large image takes extra time to pull from disk and we
are racing the renderer. This is a recipe for flaky tests. I suspect you think
similarly which is why you used Math.random() to avoid the cache in the first
patch.

[Noel Gordon, 2014-02-20 01:56:04]

On 2014/02/19 18:53:44, pdr wrote:
> Please explain why this test won't pass with the small image. This should be
> trivial to determine.

The small image fits in the window, so the resize code paths in ImageDocument
are never invoked.

> My intuition is that the large image takes extra time to pull from disk and we
> are racing the renderer. This is a recipe for flaky tests. I suspect you think
> similarly which is why you used Math.random() to avoid the cache in the first
> patch.

Our intuition is sometimes misleading.  Nothing to do with disk loading as it
turns out :/

[Noel Gordon, 2014-02-20 01:59:02]

> On 2014/02/19 18:53:44, pdr wrote:
> I suspect you think similarly which is why you used Math.random() to
> avoid the cache in the first patch.

and yeap, I had that view, and turns out I was wrong about that.  Glad
you suggested that I remove that Math.random().

[pdr., 2014-02-20 04:35:25]

On 2014/02/20 01:59:02, Noel Gordon (Google) wrote:
> > On 2014/02/19 18:53:44, pdr wrote:
> > I suspect you think similarly which is why you used Math.random() to
> > avoid the cache in the first patch.
> 
> and yeap, I had that view, and turns out I was wrong about that.  Glad
> you suggested that I remove that Math.random().

Excellent!

Thank you for debugging this. I'm convinced this will not be flaky. LGTM

[Noel Gordon, 2014-02-20 04:45:59]

> On 2014/02/18 02:59:18, pdr wrote:

> > Can you use an existing image that's smaller ... This will perform
> > better on the slow debug bots, and will not make future reviewers
> > hungry.

So if I make an image with one side bigger than 800, it won't fit in
the test window, and that'll tickle the bug. Tried that and it seems
to work. That makes the test image much much smaller and removes all
traces of PII.

[pdr., 2014-02-20 04:47:06]

On 2014/02/20 04:45:59, Noel Gordon (Google) wrote:
> > On 2014/02/18 02:59:18, pdr wrote:
> 
> > > Can you use an existing image that's smaller ... This will perform
> > > better on the slow debug bots, and will not make future reviewers
> > > hungry.
> 
> So if I make an image with one side bigger than 800, it won't fit in
> the test window, and that'll tickle the bug. Tried that and it seems
> to work. That makes the test image much much smaller and removes all
> traces of PII.

<3 <3 <3

Thanks for going the extra mile.

[Noel Gordon, 2014-02-20 06:35:28]

The CQ bit was checked by noel@chromium.org

[commit-bot: I haz the power, 2014-02-20 06:35:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noel@chromium.org/168583003/410001

[commit-bot: I haz the power, 2014-02-20 09:31:53]

Change committed as 167497