[webkit-reviews] review granted: [Bug 244564] TextureMapper: Calculate zFar and zNear for the projection matrix : [Attachment 462019] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Fujii Hironori
<Hironori.Fujii at sony.com>'s request for review:
Bug 244564: TextureMapper: Calculate zFar and zNear for the projection matrix
https://bugs.webkit.org/show_bug.cgi?id=244564

Attachment 462019: Patch

https://bugs.webkit.org/attachment.cgi?id=462019&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 462019
  --> https://bugs.webkit.org/attachment.cgi?id=462019
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:86
> +    virtual void setDepthRange(float, float) { }

Need argument names here. It’s not obvious what the two floats are.

This class is mixing float with double. TransformationMatrix uses double, but
we also use FloatRect and FloatPoint. It’s not obvious to me what would be
best.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:872
> +static inline TransformationMatrix createProjectionMatrix(const IntSize&
size, bool mirrored, float zNear, float zFar)

Here we are converting floats to doubles when we put the results of our
expression into a TransformationMatrix object. Maybe they should just be
doubles from the start? I am not sure this mix of doubles and floats in the
existing code is OK.

I’d like to see tests that cover this somehow in the future.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:38
> +struct TextureMapperComputeTransformData;

I think this struct should be a member of the TextureMapperLayer class. It’s an
implementation detail. I suggest naming it
TextureMapperLayer::ComputeTransformsData.