Bug 48277 – Fix canvas/philip/tests/2d.imageData.get.source.negative

• Component: Canvas
• Status: RESOLVED FIXED

[Mike Lawther]

Update behaviour as per http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#pixel-manipulation: 
"The getImageData(sx, sy, sw, sh) method must return an ImageData object representing the underlying pixel data for the area of the canvas denoted by the rectangle whose corners are the four points (sx, sy), (sx+sw, sy), (sx+sw, sy+sh), (sx, sy+sh), in canvas coordinate space units."

[Mike Lawther]

Added block on master bug

[Mike Lawther]

Created attachment 71821 [details]
Patch

[Mike Lawther]

Added jamesr to cc list.

[Matthew Delaney]

FWIW, this patch looks good to me. Though, does it end up making 2d.imageData.get.source.negative pass entirely on Mac? I note on my machine that the test fails 3 pixels of the imgdata1 and one of the imgdata2's pixels. Off the top of my head, I would guess that that imgdata2's pixel still fails.

[Mike Lawther]

I tested it on my Mac in Safari, and on Chromium Windows, and it passed entirely on both. run-webkit-tests also reported it passing correctly. 

Running the test in a current Chrome, I get:

Failed assertion imgdata1.data["0"] === 255 (got 0[number], expected 255[number])
Failed assertion imgdata1.data["1"] === 255 (got 0[number], expected 255[number])
Failed assertion imgdata1.data["2"] === 255 (got 0[number], expected 255[number])
Failed assertion imgdata2.data["3"] === 0 (got 255[number], expected 0[number])

But running it in a patched Safari, I get it saying 'Passed', as per the expectations.

[Matthew Delaney]

Ok great. Make sure then to remove the test from the LayoutTests/platform/mac/Skipped list and that it still passes from run-webkit-tests.

I had a patch out to unskip all the tests and reset the expectations, but it had some troubles with the bots and I need to resubmit. That should hopefully clear up future confusions about whether these tests are passing/failing and fixed or not.

[Mike Lawther]

Yep - the patch does remove the test from LayoutTests/platform/mac/Skipped (and also from chromium's test_expectations.txt, since I tested that platform too).

[Mike Lawther]

Adding Oliver to cc: 

Oliver: could you take a look at this patch? Thanks!

[James Robinson]

Comment on attachment 71821 [details]
Patch

R=me.  Thanks!

[WebKit Commit Bot]

Comment on attachment 71821 [details]
Patch

Rejecting patch 71821 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 71821]" exit_code: 1
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=71821&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=48277&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Updating working directory
Processing patch 71821 from bug 48277.
ERROR: /Projects/CommitQueue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/4746045

[Mike Lawther]

Created attachment 72402 [details]
Patch

[Matthew Delaney]

Since I landed the patch to (finally) stop skipping all those (including this) canvas tests on mac, if this patch fixes the mac failing expectations then you'll need to update that file as well. It's located at LayoutTests/platform/mac-snowleopard/canvas/philip…

The mac-leopard skipped list currently contains this test so there's no need to update those expectations. Though, if this certainly fixes the problem on both snowleopard and leopard (likely the case) you can just get the expectation (of passing) file into LayoutTests/platform/mac/canvas/philip… in which case it'll apple to both leopard and snowleopard.

[Mike Lawther]

The patch does remove the snowleopard expectations file, since I can confirm that the test passes on snowleopard. I can't see why it wouldn't work on leopard, but since I didn't test it, I haven't touched those.

[Matthew Delaney]

Are you on IRC? I can't find you. I can explain the Skipped lists in more detail.

Basically, if you have a fix (presumably) for all the ports, you'll just want to reset the project-wide expectations file (to now be passing) and then get rid of each port's expectation file if they have one. That way none of the test bots for those ports will show red on that test if the patch did indeed fix the bug.

[WebKit Commit Bot]

Comment on attachment 72402 [details]
Patch

Clearing flags on attachment: 72402

Committed r70957: <http://trac.webkit.org/changeset/70957>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mike Lawther]

Followup patch to remove entries in other ports Skipped files: https://bugs.webkit.org/show_bug.cgi?id=48702