Bug 51186 - [chromium] Add asserts to test for contiguous-pixel Skia bitmaps.
Summary: [chromium] Add asserts to test for contiguous-pixel Skia bitmaps.
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: W. James MacLean
URL:
Keywords:
Depends on: 51513 51565
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-16 07:26 PST by W. James MacLean
Modified: 2013-04-09 12:55 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.63 KB, patch)
2010-12-16 07:29 PST, W. James MacLean
no flags Details | Formatted Diff | Diff
Patch (4.75 KB, patch)
2010-12-23 07:56 PST, W. James MacLean
no flags Details | Formatted Diff | Diff
Patch (4.74 KB, patch)
2010-12-23 08:26 PST, W. James MacLean
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description W. James MacLean 2010-12-16 07:26:44 PST
[chromium] Add asserts to test for contiguous-pixel Skia bitmaps.
Comment 1 W. James MacLean 2010-12-16 07:29:37 PST
Created attachment 76768 [details]
Patch
Comment 2 W. James MacLean 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.
Comment 3 W. James MacLean 2010-12-16 07:38:21 PST
James R, can you please review this? It's a very short (and hopefully straightforward) CL.
Comment 4 Kenneth Russell 2010-12-22 11:58:48 PST
Comment on attachment 76768 [details]
Patch

Looks OK.
Comment 5 WebKit Commit Bot 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>
Comment 6 WebKit Commit Bot 2010-12-22 14:03:46 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 W. James MacLean 2010-12-23 07:56:10 PST
Created attachment 77333 [details]
Patch
Comment 8 W. James MacLean 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.
Comment 9 W. James MacLean 2010-12-23 07:59:47 PST
Patch reverted due to not compiling on mac.
Comment 10 Kenneth Russell 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() == ...))
Comment 11 W. James MacLean 2010-12-23 08:26:21 PST
Created attachment 77335 [details]
Patch
Comment 12 W. James MacLean 2010-12-23 08:27:20 PST
(In reply to comment #10)
> 
> ASSERT(!pixels || (skiaBitmap->rowBytes() == ...))

This is much better, thanks.
Comment 13 Kenneth Russell 2010-12-23 08:28:27 PST
Comment on attachment 77335 [details]
Patch

OK.
Comment 14 WebKit Commit Bot 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>
Comment 15 WebKit Commit Bot 2010-12-23 08:56:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Kenneth Russell 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.
Comment 17 Kenneth Russell 2010-12-23 15:23:40 PST
See above comment.