Bug 77582

Summary: [Chromium] Use the current clip when marking paints as opaque
Product: WebKit Reporter: Dana Jansens <danakj>
Component: New BugsAssignee: Dana Jansens <danakj>
Status: RESOLVED FIXED    
Severity: Normal CC: backer, danakj, jamesr, piman, reed, reveman, rjkroege, senorblanco, webkit.review.bot, wjmaclean
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Dana Jansens 2012-02-01 16:26:19 PST
[Chromium] Use the current clip when marking paints as opaque
Comment 1 Dana Jansens 2012-02-01 16:30:05 PST
Created attachment 125040 [details]
Patch
Comment 2 Dana Jansens 2012-02-01 16:31:54 PST
Fixes http://code.google.com/p/chromium/issues/detail?id=112356
Comment 3 Dana Jansens 2012-02-01 16:35:04 PST
Summary: gmail was painting the rounded box with drop shadow by applying a complex clip and filling a rect. The clip was not being considered and the opaque rect was being tracked without it.

Solution: for rect clips, intersect the paint rect with the clip. for more complex clips, it's not worth doing anything so don't track the paint.
Comment 4 Stephen White 2012-02-02 10:17:08 PST
Comment on attachment 125040 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:154
> +    SkCanvas* canvas() const { return m_canvas; }

I'm a fan of constness being transitive, if possible (although it's not in the WebKit coding style).  If you can deal with a const SkCanvas* at your call site, you should provide two accessors here:  one const method that returns a const SkCanvas*, and one non-const accessor that returns a non-const SkCanvas*.  Then constness is transitively preserved.
Comment 5 Dana Jansens 2012-02-02 13:01:04 PST
Yep that should be fine. I like the idea too.
Comment 6 Dana Jansens 2012-02-02 23:28:18 PST
Created attachment 125272 [details]
Patch

- const the canvas pointer with a 2nd accessor

@senorblanco can you be reviewer for this?
Comment 7 Stephen White 2012-02-03 07:07:46 PST
Comment on attachment 125272 [details]
Patch

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

Looks good as-is; the FYI is optional.  r=me

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:260
> +    SkIRect deviceClip;
> +    context->canvas()->getClipDeviceBounds(&deviceClip);
> +    SkRect rect = paintRect;
> +    if (!rect.intersect(SkIntToScalar(deviceClip.fLeft), SkIntToScalar(deviceClip.fTop), SkIntToScalar(deviceClip.fRight), SkIntToScalar(deviceClip.fBottom)))

FYI, you could also do something like this:

SkIRect deviceIClip;
context->canvas()->getClipDeviceBounds(&deviceIClip);
SkRect deviceClip;
deviceClip.set(deviceIClip);
if (!rect.intersect(deviceClip))

...

Not shorter in line count, but it does save you doing all those SkIntToScalars yourself.
Comment 8 Dana Jansens 2012-02-03 07:12:50 PST
Comment on attachment 125272 [details]
Patch

thanks! i thought about doing the set() but opted for skipping another local variable in this case.
Comment 9 WebKit Review Bot 2012-02-03 08:28:15 PST
Comment on attachment 125272 [details]
Patch

Clearing flags on attachment: 125272

Committed r106663: <http://trac.webkit.org/changeset/106663>
Comment 10 WebKit Review Bot 2012-02-03 08:28:22 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Antoine Labour 2012-02-03 13:32:08 PST
Merged to branch 1025: http://trac.webkit.org/changeset/106691