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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue May 22 14:28:14 PDT 2012


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





--- Comment #34 from Tien-Ren Chen <trchen at chromium.org>  2012-05-22 14:27:18 PST ---
(In reply to comment #33)
> (From update of attachment 143183 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143183&action=review
> 
> I will only comment on the general bits in the rendering which are clunky. Added the compositor people to look at what are the implication for the compositor's and what you need to do.

Thanks for the valuable comments!

> > Source/WebCore/ChangeLog:14
> > +        One important difference from RenderObject::absoluteQuads is that
> > +        the returned quads will be in the coordinates of a repaint container
> > +        specified by the caller. This will allow threaded-scrolling/animation
> > +        without having to regenerate the highlights.
> 
> What happens if your repaintContainer change? (layer removed or added for example or promotion / demotion from being hardware accelerated)

Yes that would be a problem. The current implementation generates the highlight at the time a tap gesture happened, then it stays the same no matter the nodes are moved / transformed / zapped / whatever. Ideally we should store a reference to the node, then regenerate the highlight on every compositor commit. I doubt if it'd be an overkill though.

> > Source/WebCore/ChangeLog:17
> > +        No new tests. Tests will be added with later patches that actually
> > +        add highlight to layers.
> 
> You are exposing the new function to Chromium. Will it be tested there?
> 
> What do you mean by layers: RenderLayers or GraphicsLayers?

Sorry the comment no longer applies. I'll add tests in revised patch.

> > Source/WebCore/rendering/RenderBlock.h:450
> > +    virtual void repaintContainerHighlightQuads(Vector<FloatQuad>&, RenderBoxModelObject* repaintContainer) const;
> 
> Not a huge fan of this function name: I thought it was a repaint* function (which obviously it's not). How about collectHighlightQuadsForContainer(...) or something similar?

Totally agree... Another thing aelias@ is suggesting that what the function actually does is to mimic the hit-test area, and later we might use it to generate the non-fast touch events region for threaded input event handling. So maybe collectHittestQuadsForContainer would be a better name?

> > Source/WebCore/rendering/RenderBox.cpp:596
> > +    if (!size().isEmpty())
> 
> This should be an early return. I also find this check inconsistent with the rest of the code (you would not check the other classes for empty quads?)

Empty quad probably doesn't matter, but I'll add check for all of them for the sake of consistency.

> > Source/WebCore/rendering/RenderBox.cpp:597
> > +        quads.append(localToContainerQuad(FloatRect(0, 0, width(), height()), repaintContainer, false, 0));
> 
> You don't need the 2 last parameters here. They make the code less readable and more confusing as you wonder why you need them here. Same comment later at the other call sites.
> 
> > Source/WebCore/rendering/RenderBox.h:155
> > +    virtual void repaintContainerHighlightQuads(Vector<FloatQuad>&, RenderBoxModelObject* repaintContainer) const;
> 
> We annotate those functions with OVERRIDE (not repeated).

Done

> > Source/WebCore/rendering/RenderInline.cpp:1465
> > +
> 
> This is inconsistent: you don't call your parent class' method here.

I don't understand this comment. Could you explain in detail?

> > Source/WebCore/rendering/RenderInline.cpp:1467
> > +    if (continuation())
> > +        continuation()->repaintContainerHighlightQuads(quads, repaintContainer);
> 
> continuation is RenderBoxModelObject concept which makes me dubious of the duplication between RenderInline and RenderBlock: there should be a RenderBoxModelObject function that handles the continuation for you.

I don't think there is one, as absoluteRects and absoluteQuads use the same practice. continuation() is declared as protected instead of private, I guess that means subclasses can be aware of it?

> > Source/WebKit/chromium/src/WebHighlight.cpp:54
> > +    for (Node *node = topNode; node; node = node->traverseNextNode(topNode)) {
> 
> Style violation

Done

> > Source/WebKit/chromium/src/WebHighlight.cpp:57
> > +        if (!renderer)
> > +            continue;
> 
> My question here: have you considered SVG? Do you want to support it or not?
> 
> If you don't care about SVG, you don't care about anything that is not a RenderBoxModelObject anyway. That would simplify the code above.

Currently I don't care about SVG as I almost never see them used on the internet... But I'll say we'd like to support at a later time when we have more time budget.

> > Source/WebKit/chromium/src/WebHighlight.cpp:59
> > +        RenderLayer* repaintLayer = renderer->enclosingLayer()->enclosingCompositingLayerForRepaint();
> 
> NO! Using RenderLayer in the WebKit/ directory is a show-stopper. It means you need access to something you shouldn't be knowing about.
> 
> RenderBoxModelObject* repaintContainer = renderer->containerForRepaint();
> 
> Will give you the renderer you need. If you need more of the hidden bits, it looks like you should rethink your architecture.

Ah ha, this is exactly what I need. Thanks!

> > Source/WebKit/chromium/src/WebHighlight.cpp:65
> > +        GraphicsLayer* graphicsLayer = repaintLayer->backing()->graphicsLayer();
> 
> What proofs do you have that this RenderLayer is hardware accelerated? If it is not, this is a NULL crasher.

Because it's returned from enclosingCompositingLayerForRepaint(), either it is NULL or it has a backing?

> > Source/WebKit/chromium/src/WebHighlight.cpp:68
> > +        Path* path = m_quadMap.get(graphicsLayer);
> > +        if (!path)
> > +            m_quadMap.set(graphicsLayer, adoptPtr(path = new Path()));
> 
> This is super confusing and inefficient as you are doing 2 hash lookups if you have no entry:
> 
> Path* path = m_quadMap.add(graphicsLayer, adoptPtr(new Path())).iterator->second;

This one allocates a new Path even when there is existing entry. :(

How about this?
HashMap<...>::iterator i = m_quadMap.add(graphicsLayer, adoptPtr(new Path())).iterator;
if (!i->second)
    i->second = new Path();
Path* path = i->second;

> > Source/WebKit/chromium/src/WebHighlight.cpp:73
> > +        for (size_t i = 0; i < newquads.size(); i++) {
> 
> I don't understand the purpose of this. If you need a Path can't you get one from WebCore directly instead of this hocus pocus code?

What I'm thinking is that the WebCore function should only return the exact hit test area, while the WebKit function adds decorations (like round smooth corners), and try to do some basic clipping.

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



More information about the webkit-unassigned mailing list