[webkit-reviews] review denied: [Bug 103914] Add support for tracking hit test rectangles to enable fast event rejection in the compositor : [Attachment 177615] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied  review:
Bug 103914: Add support for tracking hit test rectangles to enable fast event
rejection in the compositor
https://bugs.webkit.org/show_bug.cgi?id=103914

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=177615&action=review


> Source/WebCore/rendering/RenderBox.cpp:483
> +void
RenderBox::addRepaintContainerHitTestRects(RepaintContainerHitTestRects&
repaintContainerRects, const RenderLayerModelObject* currentRepaintContainer,
const LayoutPoint& repaintOffset) const

What does "current" add to repaintContainer?

> Source/WebCore/rendering/RenderBox.cpp:485
> +    LayoutPoint adjustedRepaintOffset = repaintOffset;

It's odd that this is called a repaintOffset when it has nothing to do with
(re)painting. It should just be offsetFromContainer.

> Source/WebCore/rendering/RenderBox.cpp:493
> +    if (width() && height()) {
> +	   IntRect result(pixelSnappedIntRect(adjustedRepaintOffset, size()));

I'd prefer this to be a call to something like borderBoxRect(). Just using
size() doesn't indicate that you've thought about which rect makes sense for
hit testing.

> Source/WebCore/rendering/RenderObject.cpp:622
> +    // Self-painting layers subtract the renderer's location. Otherwise, we
need to map our location to container space.

self-painting layer means something different in RL speak.

> Source/WebCore/rendering/RenderObject.cpp:629
> +	   TransformState
transformState(TransformState::ApplyTransformDirection, FloatPoint());
> +	   mapLocalToContainer(repaintContainer, transformState,
ApplyContainerFlip | UseTransforms | SnapOffsetForTransforms);
> +	   transformState.flatten();
> +	   offset = LayoutPoint(transformState.lastPlanarPoint());

See comment below.

> Source/WebCore/rendering/RenderObject.h:965
> +    void repaintContainerAndOffset(const RenderLayerModelObject*&,
LayoutPoint&) const;

r- for this method. This encourages incorrect usage, where this is a
software-rendered transform between you and your repaint container. Just
offsetting a rect by that offset will do the wrong thing there.