Summary: | [Skia] canvas/philip/tests/2d.imageData.get.source.outside.html crashes test_shell in Chromium Linux debug | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dominic Cooney <dominicc> | ||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED WONTFIX | ||||||||||
Severity: | Normal | CC: | brettw, darin, eric, schenney | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 20553 | ||||||||||
Attachments: |
|
Description
Dominic Cooney
2010-08-25 11:31:08 PDT
Created attachment 65452 [details]
Patch
Comment on attachment 65452 [details] Patch LGTM once the point below has been addressed. Note: I am not a WebKit Reviewer. You need a real review from someone else before landing this patch. > + In Chromium Skia getImageData don't retrieve the pointer into the > + canvas bitmap when there are no columns to copy, because this > + asserts when the source is outside the canvas bounds. This sentence doesn't make sense. Created attachment 65472 [details]
Patch
Who wrote chromium's getImageData? I suspect this fixes the crash, but is it the right fix? Comment on attachment 65472 [details] Patch > WebCore/platform/graphics/skia/ImageBufferSkia.cpp:181 > + int numRows = numColumns > 0 ? endY - originY : 0; If this function is meant to do nothing in this case (and I'm guessing it is), I'd put an early-return below the computation of numColumns, rather than making the numRows computation more confusing. Created attachment 65964 [details]
Patch with early return.
Comment on attachment 65964 [details]
Patch with early return.
Looks fine, but why wouldn't we update test_expectations.txt in the same commit?
Comment on attachment 65964 [details] Patch with early return. > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog > + > + Fixes getImageData in the Chromium Skia port so that it doesn't > + retrieve the pointer into the canvas bitmap when there are no > + columns to copy. Retrieving the pointer asserts when the source is > + outside the canvas bounds. > + The double negative here is confusing. Can you please reword this sentence? Otherwise, looks great. canvas/philip/ is still marked as SKIP in the chromium test_expectations so there's not much point in updating it at this point. Comment on attachment 65964 [details]
Patch with early return.
Tests?
I plan to baseline philip in a separate CL, then add updated expectations into this patch. But not actively working on this right now. |