[Webkit-unassigned] [Bug 84936] [chromium] Compute highlight for touch targets

[bugzilla-daemon at webkit.org]

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


Julien Chaffraix <jchaffraix at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #144645|review?                     |review-
               Flag|                            |




--- Comment #41 from Julien Chaffraix <jchaffraix at webkit.org>  2012-06-07 10:23:51 PST ---
(From update of attachment 144645)
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.

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