[webkit-reviews] review denied: [Bug 21789] -webkit-border-radius clipping : [Attachment 28896] Patch to make replaced elements work properly with border-radius clipping

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Dave Hyatt
<hyatt at apple.com>'s request for review:
Bug 21789: -webkit-border-radius clipping
https://bugs.webkit.org/show_bug.cgi?id=21789

Attachment 28896: Patch to make replaced elements work properly with
border-radius clipping
https://bugs.webkit.org/attachment.cgi?id=28896&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/rendering/RenderReplaced.cpp
> ===================================================================

> +    if (style()->overflowX() != OVISIBLE && style()->hasBorderRadius()) {
> +	   // Push a clip if we have a border radius, since we want to round
the foreground content that gets painted.
> +	   paintInfo.context->save();
> +	   paintInfo.context->addRoundedRectClip(IntRect(tx, ty, width(),
height()),
> +						
style()->borderTopLeftRadius(),
> +						
style()->borderTopRightRadius(), 
> +						
style()->borderBottomLeftRadius(),
> +						
style()->borderBottomRightRadius());
> +    }
> +
>      paintReplaced(paintInfo, tx, ty);
> -    
> +
> +    if (style()->overflowX() != OVISIBLE && style()->hasBorderRadius())
> +	   paintInfo.context->restore();

I know you minused already, but a brief comment. In general code like the above
seems
prone to mismatched push/pop, and you're doing an complex test twice. Maybe
either
stash the result in a boolean:

bool haveRoundedClip = style()->overflowX() != OVISIBLE &&
style()->hasBorderRadius();
if (haveRoundedClip) {
  paintInfo.context->save();

...
if (haveRoundedClip) {
  paintInfo.context->restore();

or we should make a little stack-based state maintainer.