Bug 77582 – [Chromium] Use the current clip when marking paints as opaque

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[Chromium] Use the current clip when marking paints as opaque

[Dana Jansens]

Created attachment 125040 [details]
Patch

[Dana Jansens]

Fixes http://code.google.com/p/chromium/issues/detail?id=112356

[Dana Jansens]

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.

[Stephen White]

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.

[Dana Jansens]

Yep that should be fine. I like the idea too.

[Dana Jansens]

Created attachment 125272 [details]
Patch

- const the canvas pointer with a 2nd accessor

@senorblanco can you be reviewer for this?

[Stephen White]

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.

[Dana Jansens]

Comment on attachment 125272 [details]
Patch

thanks! i thought about doing the set() but opted for skipping another local variable in this case.

[WebKit Review Bot]

Comment on attachment 125272 [details]
Patch

Clearing flags on attachment: 125272

Committed r106663: <http://trac.webkit.org/changeset/106663>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antoine Labour]

Merged to branch 1025: http://trac.webkit.org/changeset/106691