[webkit-reviews] review denied: [Bug 84936] [chromium] Compute highlight for touch targets : [Attachment 143183] 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 143183: Patch
https://bugs.webkit.org/attachment.cgi?id=143183&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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.

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

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

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

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

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

> Source/WebCore/rendering/RenderInline.cpp:1465
> +

This is inconsistent: you don't call your parent class' method here.

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

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

Style violation

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

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

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

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

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