Bug 91260 - [chromium] Remove incorrect debug assertion in LayerRendererChromium.cpp
Summary: [chromium] Remove incorrect debug assertion in LayerRendererChromium.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shawn Singh
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-13 10:00 PDT by Shawn Singh
Modified: 2012-07-13 12:40 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.33 KB, patch)
2012-07-13 10:05 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch for landing (3.36 KB, patch)
2012-07-13 10:20 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shawn Singh 2012-07-13 10:00:06 PDT
Bug originally reported in http://code.google.com/p/chromium/issues/detail?id=136017 ... The assertion is being triggered due to anti-aliasing inflation that causes clipping.  To our knowledge, this scenario of clipping is actually tolerable, and has not yet caused any known bugs.

In addition to fixing the assert, I updated comments that needed to be worded better.

Patch coming in a moment
Comment 1 Shawn Singh 2012-07-13 10:05:38 PDT
Created attachment 152284 [details]
Patch
Comment 2 Adrienne Walker 2012-07-13 10:13:33 PDT
Comment on attachment 152284 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152284&action=review

R=me, with comment nits.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:703
> +    // (Note, however, that it may still have perspective components in the 2d parts of the matrix.)

I don't know that it's important to add this superfluous comment about perspective here.  Somebody modifying this code in the future doesn't need to be made aware of this extra detail just because this comment was wrong in the past.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:884
> +        // (Note, however, that it may still have perspective components in the 2d parts of the matrix.)

Same with this perspective comment.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-885
> -        ASSERT(!clipped);

I do think you need a comment about why it's ok that this quad is clipped, since we assert on clipped pretty much everywhere else.
Comment 3 Shawn Singh 2012-07-13 10:20:52 PDT
Created attachment 152288 [details]
Patch for landing
Comment 4 Shawn Singh 2012-07-13 10:22:15 PDT
Thanks for the review!  I'm happy to tweak the comments more if they're not good enough yet, please feel free to cq- it if needed.
Comment 5 WebKit Review Bot 2012-07-13 12:40:46 PDT
Comment on attachment 152288 [details]
Patch for landing

Clearing flags on attachment: 152288

Committed r122619: <http://trac.webkit.org/changeset/122619>
Comment 6 WebKit Review Bot 2012-07-13 12:40:51 PDT
All reviewed patches have been landed.  Closing bug.