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

[bugzilla-daemon at webkit.org]

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





--- Comment #43 from Tien-Ren Chen <trchen at chromium.org>  2012-06-08 13:26:36 PST ---
(From update of attachment 144645)
View in context: https://bugs.webkit.org/attachment.cgi?id=144645&action=review

>>> 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.
> 
> is not it what Source/WebCore/page/GestureTapHighlighter.cpp is responsible for, among others? (see also https://bugs.webkit.org/show_bug.cgi?id=84989)

GestureTapHighlighter has some major flaws that I don't think are easily fixable.
1. The support for transformation is terribly wrong. It gets the absoluteRects then apply layer transformation again?
2. It doesn't separate highlights for different graphicsLayer. Which is essential to support threaded-scrolling/CSS animation.

>> Source/WebCore/rendering/RenderObject.cpp:2869
>> +    size_t oldSize = quads.size();
> 
> This is useless as quads is always empty when the function is called.

True for our current code, but there may be a chance we will batch multiple calls to the same vector. The overhead is negligible anyway.

>> 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)

Hmm I'm just not sure if blk->containingBlock would ever jump over a repainContainer. If not I guess we can simply use blk && blk != repaintContainer.

>> 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.

Sounds good

>> 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.

I think you're right. Clip loosely is better than not clipping at all in this case.
We can allow some false positive. Manipulating polygons is simply too expensive.

>> Source/WebKit/chromium/src/WebHighlight.cpp:63
>> +            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.

I agree it is too much leak of internals. But in this case the caller must know something about graphicsLayer in order to support threaded-scrolling/CSS animation.

>> Source/WebKit/chromium/tests/LinkHighlightTest.cpp:176
>> +#if 0
> 
> Why are you disabling this test? I see no mention of it anywhere.

Ah, actually the whole LinkHighlightTest needs to be reworked for the new code. I'm still figuring out how to keep these tests.

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