[webkit-reviews] review granted: [Bug 133522] [New Multicolumn] Hit testing of composited content is broken : [Attachment 232550] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Dave Hyatt
<hyatt at apple.com>'s request for review:
Bug 133522: [New Multicolumn] Hit testing of composited content is broken
https://bugs.webkit.org/show_bug.cgi?id=133522

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

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


>>> Source/WebCore/rendering/RenderLayer.cpp:955
>>> +	     m_enclosingLayerIsPaginatedAndComposited = isComposited();
>> 
>> Do you actually need to cache m_enclosingLayerIsPaginatedAndComposited? Is
m_enclosingLayerIsPaginatedAndComposited ever different from
m_enclosingPaginationLayer->isComposited()?
> 
> Yes. An intermediate layer can be composited that sits in between the
enclosing pagination layer and this layer.

The name of m_enclosingLayerIsPaginatedAndComposited is a bit confusing then.

Is it worth caching this or could you just detect it on the fly by walking up
the layer tree?

> Source/WebCore/rendering/RenderLayer.cpp:966
> +	   m_enclosingPaginationLayer =
parent()->enclosingPaginationLayer(IncludeCompositedPaginatedLayers);
> +	   m_enclosingLayerIsPaginatedAndComposited = isComposited() ? true :
parent()->enclosingLayerIsPaginatedAndComposited();
> +	   if (parent()->hasTransform()) {
>	       m_enclosingPaginationLayer = 0;

Why bother computing enclosingPaginationLayer() if you're just going to null it
out? Also nullptr.

> Source/WebCore/rendering/RenderLayer.cpp:984
> +	       m_enclosingPaginationLayer =
containingBlock->layer()->enclosingPaginationLayer(IncludeCompositedPaginatedLa
yers);
> +	       m_enclosingLayerIsPaginatedAndComposited = isComposited() ? true
: containingBlock->layer()->enclosingLayerIsPaginatedAndComposited();
> +	       if (containingBlock->layer()->hasTransform()) {
>		   m_enclosingPaginationLayer = 0;

Ditto.

> Source/WebCore/rendering/RenderLayer.cpp:5587
> +    const RenderLayer* paginationLayer = 0;

nullptr.

> Source/WebCore/rendering/RenderLayer.h:522
> +	       return 0;

nullptr.