[webkit-reviews] review requested: [Bug 94355] [chromium] Enhance support for transforms in LinkHighlight. : [Attachment 163237] Patch

[bugzilla-daemon at webkit.org]

W. James MacLean <wjmaclean at chromium.org> has asked  for review:
Bug 94355: [chromium] Enhance support for transforms in LinkHighlight.
https://bugs.webkit.org/show_bug.cgi?id=94355

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

------- Additional Comments from W. James MacLean <wjmaclean at chromium.org>
(In reply to comment #6)
> (From update of attachment 160783 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=160783&action=review
> 
> Close - I think it needs a bit more polish, though.
> 
> > Source/WebKit/chromium/src/LinkHighlight.cpp:144
> > +static void convertPointsTargetToCompositedLayer(const Vector<IntPoint>&
vector, RenderObject* targetRenderer, RenderObject* compositedRenderer,
Vector<FloatPoint>& outVector)
> 
> vector / outVector aren't great variable names - can you name them for the
data they represent?

Fixed.

> > Source/WebKit/chromium/src/LinkHighlight.cpp:161
> > +static void addQuadToPath(const Vector<FloatPoint>&quad, Path& path)
> 
> space between & and quad

Fixed.

> > Source/WebKit/chromium/src/LinkHighlight.cpp:163
> > +	 // FIXME: Make this create rounded rectangles, just like the
axis-aligned case.
> 
> Path has an addPathForRoundedRect, if it's useful

Hmmm, I don't think it helps ... it takes a rect as input, and not a quad, so
it seems it only helps with axis-aligned quads.

> > Source/WebKit/chromium/src/LinkHighlight.cpp:176
> > +	 // FIXME: find way to compare highlight paths, or get rid of the
return value for this function.
> 
> SkPath has an operator==. I think you should resolve this fixme one way or
the other in this patch - there's no real value in leaving this in such a weird
state when all changes will be internal to this file.

Had to wait for a Skia patch to land before I could follow up with this patch.
The == operator is used within skia, and based on initial testing seems to do
what we want.

> > Source/WebKit/chromium/src/LinkHighlight.cpp:192
> > +	     boundingQuad.append(roundedIntPoint(quads[quadIndex].p1()));
> 
> rounded points aren't necessarily bounding, so either the code or the
variable name is wrong here. Do you want FloatQuad::enclosingBoundingBox(),
perhaps?

It's not critical that it be a bounding box, and it keeps the code simpler to
just use rounded points, so for now I'm just using the rounded points and have
renamed the variable.

I've also remove the ZIndex test, as it seems that applying a 2-D rotation
kills the auto zIndex property of the target, thus rendering rotated divs
non-highlightable.