[webkit-reviews] review granted: [Bug 32738] Speed-up SVG Masking : [Attachment 45333] Speed-up of SVG Mask

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Dirk Schulze <krit at webkit.org>'s
request for review:
Bug 32738: Speed-up SVG Masking
https://bugs.webkit.org/show_bug.cgi?id=32738

Attachment 45333: Speed-up of SVG Mask
https://bugs.webkit.org/attachment.cgi?id=45333&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    // calculate the smallest rect for the mask ImageBuffer.

Should capitalize this as well as having a period at the end to use the
"sentence style".

> +    for (Node* n = firstChild(); n; n = n->nextSibling()) {

We normally prefer short words over single letters for variable names like this
one.

> +	   if (!n->isSVGElement() || !static_cast<SVGElement*>(n)->isStyled()
|| !n->renderer())
> +	       continue;
> +	   // FIXME: repaintRectInLocalCoordinates() is not correctly
implemented.
> +	   // The current implementation makes it possible to use it here.
> +	   // We need more detailed boundingBoxes.
> +	   // See bug: https://bugs.webkit.org/show_bug.cgi?id=32815

I don't understand the phrase "makes it possible to use it here" in this
comment.

To me it's completely non obvious that we should union all "non-styled" SVG
elements' repaint rects here. I think this needs a comment to make clear why
this is the correct algorithm. I know this code was just moved, but I still
think the clarity improvement would be simple.

> +    if (maskContentUnits() == SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX)
{

Here, I think the most useful type of comment would be a brief one that note
that the following is an optimization and explains why the optimization can
only be applied when the mask content units have this type.

r=me