Bug 51186

Summary: [chromium] Add asserts to test for contiguous-pixel Skia bitmaps.
Product: WebKit Reporter: W. James MacLean <wjmaclean>
Component: New BugsAssignee: W. James MacLean <wjmaclean>
Status: RESOLVED WONTFIX    
Severity: Normal CC: commit-queue, jamesr, kbr, schenney, senorblanco
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 51513, 51565    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

W. James MacLean
Reported 2010-12-16 07:26:44 PST
[chromium] Add asserts to test for contiguous-pixel Skia bitmaps.
Attachments
Patch (4.63 KB, patch)
2010-12-16 07:29 PST, W. James MacLean 		no flags
DetailsFormatted DiffDiff
Patch (4.75 KB, patch)
2010-12-23 07:56 PST, W. James MacLean 		no flags
DetailsFormatted DiffDiff
Patch (4.74 KB, patch)
2010-12-23 08:26 PST, W. James MacLean 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
W. James MacLean
Comment 1 2010-12-16 07:29:37 PST
Created attachment 76768 [details] Patch
W. James MacLean
Comment 2 2010-12-16 07:33:12 PST
Since Skia bitmaps may have rowBytes() >= bytesPerPixel() * width(), and since some places in the code assume the equality above is enforced, add asserts() to detect when there are extra bytes at the end of each line where padding=0 is assumed.
W. James MacLean
Comment 3 2010-12-16 07:38:21 PST
James R, can you please review this? It's a very short (and hopefully straightforward) CL.
Kenneth Russell
Comment 4 2010-12-22 11:58:48 PST
Comment on attachment 76768 [details] Patch Looks OK.
WebKit Commit Bot
Comment 5 2010-12-22 14:03:39 PST
Comment on attachment 76768 [details] Patch Clearing flags on attachment: 76768 Committed r74503: <http://trac.webkit.org/changeset/74503>
WebKit Commit Bot
Comment 6 2010-12-22 14:03:46 PST
All reviewed patches have been landed. Closing bug.
W. James MacLean
Comment 7 2010-12-23 07:56:10 PST
Created attachment 77333 [details] Patch
W. James MacLean
Comment 8 2010-12-23 07:58:22 PST
Comment on attachment 77333 [details] Patch The patch failed on mac compile (I thought I had tested this, but apparently not). I have revised to correct, and this patch has been re-tested to make sure it compiles on Mac.
W. James MacLean
Comment 9 2010-12-23 07:59:47 PST
Patch reverted due to not compiling on mac.
Kenneth Russell
Comment 10 2010-12-23 08:01:53 PST
Comment on attachment 77333 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77333&action=review The "if" with an ASSERT as its sole arm is sloppy. > WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:125 > + ASSERT(skiaBitmap->rowBytes() == SkBitmap::ComputeRowBytes(skiaConfig, skiaBitmap->width())); ASSERT(!pixels || (skiaBitmap->rowBytes() == ...))
W. James MacLean
Comment 11 2010-12-23 08:26:21 PST
Created attachment 77335 [details] Patch
W. James MacLean
Comment 12 2010-12-23 08:27:20 PST
(In reply to comment #10) > > ASSERT(!pixels || (skiaBitmap->rowBytes() == ...)) This is much better, thanks.
Kenneth Russell
Comment 13 2010-12-23 08:28:27 PST
Comment on attachment 77335 [details] Patch OK.
WebKit Commit Bot
Comment 14 2010-12-23 08:56:48 PST
Comment on attachment 77335 [details] Patch Clearing flags on attachment: 77335 Committed r74561: <http://trac.webkit.org/changeset/74561>
WebKit Commit Bot
Comment 15 2010-12-23 08:56:54 PST
All reviewed patches have been landed. Closing bug.
Kenneth Russell
Comment 16 2010-12-23 15:23:11 PST
These assertions seem to have broken the Chromium UI tests. See https://bugs.webkit.org/show_bug.cgi?id=51565 , but in particular: http://build.chromium.org/p/chromium/builders/Vista%20Tests%20(dbg)(2)/builds/3052 the failure of UITestCanLaunchWithOSMesa, and: http://build.chromium.org/p/chromium/builders/Vista%20Tests%20(dbg)(2)/builds/3052/steps/ui_tests/logs/stdio http://build.chromium.org/p/chromium/builders/Vista%20Tests%20(dbg)(2)/builds/3052/steps/Process%20Dumps/logs/stdio Reopening. Please test this patch with this test on Windows before resubmitting.
Kenneth Russell
Comment 17 2010-12-23 15:23:40 PST
See above comment.
Note You need to log in before you can comment on or make changes to this bug.