[webkit-reviews] review denied: [Bug 12571] <clipPath> doesn't correctly handle <text> elements : [Attachment 52510] patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 12571: <clipPath> doesn't correctly handle <text> elements
https://bugs.webkit.org/show_bug.cgi?id=12571

Attachment 52510: patch
https://bugs.webkit.org/attachment.cgi?id=52510&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Moin Dirk,

I still have some questions, sorry for the number of iteration with this patch,
but it's quite involved :-)

> +    // ClipPath gets clipped.
"If a clipPath gets clipped, we need to use image-based clipping" would be
easier to understand.

Ok I see the approach below, pathOnlyClipping() should check wheter we can use
path only clipping, or return false if not.
So you optimized for the fast-case, where we only have paths to clip. If there
are more complex objects to clip, you'd waste time calling toClipPath() on the
shape objects though. I think it's still reasonable to do that. If we ever see
this is a bottleneck, we may want to introduce a seperated function which
checks wheter path-only clipping is possible _in advance_. (I just remember
Beth identifying toPathData() as a bottle-neck in a certain situation...) Hm,
feel free to keep this approach for now, or introduce a new function
"isPathOnlyClippingPossible".


> +    Path clipPath = Path();
> +    for (Node* childNode = node()->firstChild(); childNode; childNode =
childNode->nextSibling()) {
...
> +	   if (!svgStyle->clipPath().isEmpty())
> +	       return false;
How about adding a comment that clip-path on children inside a <clipPath>
disallow path-only clipping?

> +	   if (clipPath.isEmpty()) {
> +	       clipPath = styled->toClipPath();
> +	       clipRule = svgStyle->clipRule();
> +	   } else
> +	       return false;

Can you explain that approach? You're initially setting clipPath to Path(),
then assign a toClipPath() of the first path-only clipping child.
So when there are two <rect> as children of a <clipPath> you will fall-back to
image-based clipping? Even when both share the same clip-rule?
Am I overlooking something?


> +    }
> +    if (static_cast<SVGClipPathElement*>(node())->clipPathUnits() ==
SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {
> +	   AffineTransform obbTransform;
> +	   obbTransform.translate(objectBoundingBox.x(),
objectBoundingBox.y());
> +	   obbTransform.scaleNonUniform(objectBoundingBox.width(),
objectBoundingBox.height());
> +	   clipPath.transform(obbTransform);
Please use "transform" as name, obbTransform looks awkward, and we shouldn't
use abbrevations, and "objectBoundingBoxTransform" is way too long ;-)

> +    if (clipPath.isEmpty())
> +	   clipPath.addRect(FloatRect());
I know why you need this, but a comment would help for others reading the code.


> +bool RenderSVGResourceClipper::applyClippingToContext(RenderObject* object,
const FloatRect& objectBoundingBox,
> +						   const FloatRect&
repaintRect, GraphicsContext* context)
Can you align the "const' with the "RenderObject", looks slightly better.


...
> +	   // Save the old RenderStyle of the current object for restoring
after drawing
> +	   // it to the MaskImage. The new intermediate RenderStyle needs to
inherit from
> +	   // the old one.
> +	   RefPtr<RenderStyle> oldRenderStyle =
RenderStyle::clone(renderer->style());
> +	   RefPtr<RenderStyle> newRenderStyle =
RenderStyle::clone(renderer->style());
That puzzles me. Why do you need two clones? Can't you just grab the old
renderer->style() and stash it into a RefPtr<RenderStyle>? I guess it's a
copy/paste error.
Can you check wheter a cloned RenderStyle is automatically "unique" (There's a
setUnique() method on RenderStyle, preventing this style to be shared across
different renderers, we have to be sure that happens, otherwhise we'll get in
trouble)

> +
> +	   if (isUseElement)
> +	       renderer = childNode->renderer();
A comment regarding the special <use> handling would help here as well.

Other than that, looks great to me. Hope we can resolve the remaining issues
quickly...

Cheers,
Niko