Bug 144214 – Expand compositing coverage rect when scrolling and animating

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Fraser (smfr)]

Expand compositing coverage rect when scrolling and animating

[Simon Fraser (smfr)]

Created attachment 251670 [details]
Patch

[Simon Fraser (smfr)]

rdar://problem/20695926

[Simon Fraser (smfr)]

Created attachment 251671 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 251673 [details]
Patch

Comment on attachment 251673 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251673&action=review

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1100
> +    return layer.preserves3D() || (layer.parent() ? layer.parent()->preserves3D() : false);

layer.preserves3D() || (layer.parent() && layer.parent()->preserves3D())?

[Darin Adler]

Comment on attachment 251673 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251673&action=review

>> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1100
>> +    return layer.preserves3D() || (layer.parent() ? layer.parent()->preserves3D() : false);
> 
> layer.preserves3D() || (layer.parent() && layer.parent()->preserves3D())?

I like the && form.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1181
> +GraphicsLayerCA::VisibleAndCoverageRect GraphicsLayerCA::computeVisibleAndCoverageRect(TransformState& state, bool preserves3D, ComputeVisibleRectFlags flags) const

I almost never use this syntax, but note there is a modern way to write this without having to repeat the GraphicsLayerCA prefix. I think it’s like this:

auto GraphicsLayerCA::computeVisibleAndCoverageRect(TransformState& state, bool preserves3D, ComputeVisibleRectFlags flags) const -> VisibleAndCoverageRect

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1223
> +    std::unique_ptr<FloatQuad> quad = state.mappedSecondaryQuad(&mapWasClamped);

Returning a pointer is a peculiar way to implement an optional return value. We should change this to return Optional<FloatQuad> instead of std::unique_ptr<FloatQuad> for better efficiency and possibly even better clarity. Except that I also see other call sites using FloatQuad* for an optional argument. I suppose that would have to change too.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1245
> +    default:
> +        break;

What other types exist besides the two above? Do we really need the default: break?

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1253
> +    if (rects.second != coverageRect) {
> +        rects.second = coverageRect;
> +        return true;
> +    }
> +
> +    return false;

I would have written this the other way, with early return for the case where there is not a change. That’s because I like the “main” flow to be on the left and the “simpler early outs” to be nested in if statements.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:279
> +    typedef std::pair<FloatRect, FloatRect> VisibleAndCoverageRect;

Should we use a tuple instead of a pair? Not sure about which is best for a modern API. Should we use a struct so that they are called visibleRect and coverageRect instead of first and second?

> Source/WebCore/platform/graphics/transforms/TransformState.h:125
> +    TransformDirection inverseDirection() const
> +    {
> +        return m_direction == ApplyTransformDirection ? UnapplyInverseTransformDirection : ApplyTransformDirection;
> +    }

When functions start to get bigger like this, I often prefer to put the body of the function outside the class definition to keep it readable. Putting it at the bottom of the header with the inline keyword is more wordy, but makes its easier to get an overview of what’s in the class.

[Simon Fraser (smfr)]

Comments addressed. Landed in https://trac.webkit.org/r183354