[webkit-reviews] review denied: [Bug 84936] [chromium] Compute highlight for touch targets : [Attachment 144645] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Tien-Ren Chen
<trchen at chromium.org>'s request for review:
Bug 84936: [chromium] Compute highlight for touch targets
https://bugs.webkit.org/show_bug.cgi?id=84936

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=144645&action=review


Not good yet. The patch is too massive to be properly reviewed, please find
some ways to split it as it would help to get it landed: I couldn't look at
everything due to the size.

> Source/WebCore/ChangeLog:9
> +	   to generate the clickable region for link highlighting.

If it's only for link, it should be more explicit in the naming.

> Source/WebCore/rendering/RenderObject.cpp:2867
> +void RenderObject::hitQuadsForContainerWithClipping(Vector<FloatQuad>&
quads, RenderBoxModelObject* repaintContainer) const

The naming could really use a *verb* and be more precise. I don't understand
what the 'with clipping' means, if you mean clipped then please use 'clipped'
instead (which would match the rest of the code, e.g.
clippedOverflowRectForRepaint (which may do something similar to what you are
doing btw, I don't have the strength of looking into that)).

> Source/WebCore/rendering/RenderObject.cpp:2869
> +    size_t oldSize = quads.size();

This is useless as quads is always empty when the function is called.

> Source/WebCore/rendering/RenderObject.cpp:2880
> +	   for (RenderBlock* blk = containingBlock(); blk &&
blk->isDescendantOf(repaintContainer); blk = blk->containingBlock()) {

I hope you understand this is a quadratic operation thanks to the
isDescendantOf call and because it's in another loop it's horribly slow. 2 ways
to make it less sucky:
* look-ahead and you cache the top-most containing block to avoid the
isDescendantOf() call.
* cache the whole containing block chain (likely faster as containingBlock() is
a non-trivial function)

> Source/WebCore/rendering/RenderObject.cpp:2881
> +	       if (!blk->hasOverflowClip())

please don't abbreviate, especially since it should be containingBlock for
preciseness.

> Source/WebCore/rendering/RenderObject.cpp:2883
> +	       FloatQuad boundQuad =
blk->localToContainerQuad(FloatRect(blk->visualOverflowRect()),
repaintContainer);

visualOverflowRect is more or less the border-box for elements with a clip: it
would include only a couple of operations that bleed outside the box (like
box-shadow) but would miss some like outline.

I am not sure why you don't just use the border-box.

> Source/WebCore/rendering/RenderObject.cpp:2884
> +	       if (!boundQuad.isRectilinear()) // don't know how to clip this.
:(

You can always do the intersection with the quad's bounding box but it will be
inaccurate - as in the resulting FloatRect will cover a bigger surface - if
it's not rectilinear.

If you need accuracy, you would need to manipulate Path and not rects.

> Source/WebKit/chromium/src/WebHighlight.cpp:63
> +	   GraphicsLayer* graphicsLayer = 0;
> +	   if (repaintContainer && repaintContainer->layer() &&
repaintContainer->layer()->backing())
> +	       graphicsLayer =
repaintContainer->layer()->backing()->graphicsLayer();

It makes me cry that you need to know so much about how rendering works just to
collect some simple hit quads.

> Source/WebKit/chromium/src/WebHighlight.h:48
> +class WebHighlight {

This refactoring is somehow tangential to the change (I guess it's to be able
to test more easily). Please move it to a blocking bug as it would shrink your
already massive patch.

> Source/WebKit/chromium/tests/LinkHighlightTest.cpp:176
> +#if 0

Why are you disabling this test? I see no mention of it anywhere.

> Source/WebKit/chromium/tests/data/webhighlight_test.html:4
> +<a href="" id="link1"
style="position:absolute;left:50px;top:50px;background-color:#ccffcc">
> +    <div
style="display:inline-block;width:500px;height:20px;background-color:#ccffcc"><
/div>

Please factor the style into some common classes and some id selectors. It's
difficult to read the test as-is.