[Webkit-unassigned] [Bug 32738] Speed-up SVG Masking

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=32738


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #45333|review?                     |review+
               Flag|                            |




--- Comment #23 from Darin Adler <darin at apple.com>  2009-12-21 10:45:07 PST ---
(From update of attachment 45333)
> +    // 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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.