Bug 77582 - [Chromium] Use the current clip when marking paints as opaque
Summary: [Chromium] Use the current clip when marking paints as opaque
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dana Jansens
Depends on:
Reported: 2012-02-01 16:26 PST by Dana Jansens
Modified: 2012-02-03 13:32 PST (History)
10 users (show)

See Also:

Patch (8.56 KB, patch)
2012-02-01 16:30 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (8.52 KB, patch)
2012-02-02 23:28 PST, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
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]

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]

- 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]

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;
SkRect deviceClip;
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]

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]

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