Bug 220732 – Complete WebXRRigidTransform implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebXR

[Imanol Fernandez]

- Implement inverse()
- Expose raw matrix to be used in XRRigitTransform and XRPose math operations
- Correctly reuse the Float32Array matrixData

[Imanol Fernandez]

Created attachment 418139 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 418139 [details]
Patch

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

Looking pretty good, just one question...

> Source/WebCore/Modules/webxr/WebXRRigidTransform.cpp:153
> +    m_inverse->m_inverse = this;

Aren't we creating a reference loop here?

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:585
> +// in TransformationMatrix::recompose4().

Nit: you can put everything in a single line

[Imanol Fernandez]

Created attachment 418143 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 418143 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRRigidTransform.cpp:131
> +    // If the operation IsDetachedBuffer on internal matrix is false, return transformâs internal matrix.

Nit: I think the code is self explanatory here, maybe we don't need the comment.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:584
> +// TODO: Once https://bugs.webkit.org/show_bug.cgi?id=220856 is addressed we can reuse this function in TransformationMatrix::recompose4().

Nit: in WebKit we normally use FIXME instead of TODO

[Imanol Fernandez]

Created attachment 418267 [details]
Patch

[Imanol Fernandez]

Thanks for the review, I fixed the nits in the last patch

[EWS]

ChangeLog entry in Source/WebCore/ChangeLog is not at the top of the file.

[Imanol Fernandez]

Created attachment 418273 [details]
Patch

[Imanol Fernandez]

Created attachment 418277 [details]
Patch

[Imanol Fernandez]

Created attachment 418379 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/73617302>

[EWS]

Downloading mechanize-0.4.5...
Installing mechanize-0.4.5...
Installed mechanize-0.4.5!
Sergio Villar found in /Volumes/Data/worker/Commit-Queue/build/LayoutTests/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/worker/Commit-Queue/build/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Imanol Fernandez]

Created attachment 418519 [details]
Patch for landing

[EWS]

Committed r271941: <https://trac.webkit.org/changeset/271941>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418519 [details].