Bug 223635 – Implement cachedPropertyValue for WebXR [SameObject] attributes

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

[Imanol Fernandez]

[SameObject] is not currently implemented in WebKit. We need to implement custom getters.

[Imanol Fernandez]

Created attachment 424019 [details]
Patch

[Chris Dumez]

Comment on attachment 424019 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRRigidTransform.idl:38
> +    [CustomGetter] readonly attribute Float32Array matrix;

Same issue as below.

> Source/WebCore/Modules/webxr/WebXRViewerPose.idl:35
> +    [CustomGetter, SameObject] readonly attribute FrozenArray<WebXRView> views;

You also need [JSCustomMarkFunction] on the interface and custom bindings code for visitAdditionalChildren() which visits cachedViews() on the impl object.

[Chris Dumez]

Comment on attachment 424019 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRRigidTransform.idl:36
>      [SameObject] readonly attribute DOMPointReadOnly position;

Note that SameObject does nothing besides documenting. You don't have to do it in this change since it does not seem needed for WPT tests. That said, I suggest you add a new test that does:
transfer.position.foo = 1;
gc();
setTimeout(() => {
  gc();
  shouldBe("transfer.position.foo", "1");
}, 0);

I suspect this will fail because we don't guarantee we are returning the same object after GC. You can see how JSDOMQuad::visitAdditionalChildren() visits its internal DOMPoint attributes to keep them alive. You'll likely need to do the exact same thing as JSDOMQuad::visitAdditionalChildren() for position & orientation attributes here. The inverse attribute will likely be an issue too.

[Imanol Fernandez]

Created attachment 424025 [details]
Patch

Add JSCustomMarkFunction

[Imanol Fernandez]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 424019 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424019&action=review
> 
> > Source/WebCore/Modules/webxr/WebXRRigidTransform.idl:36
> >      [SameObject] readonly attribute DOMPointReadOnly position;
> 
> Note that SameObject does nothing besides documenting. You don't have to do
> it in this change since it does not seem needed for WPT tests. That said, I
> suggest you add a new test that does:
> transfer.position.foo = 1;
> gc();
> setTimeout(() => {
>   gc();
>   shouldBe("transfer.position.foo", "1");
> }, 0);
> 
> I suspect this will fail because we don't guarantee we are returning the
> same object after GC. You can see how JSDOMQuad::visitAdditionalChildren()
> visits its internal DOMPoint attributes to keep them alive. You'll likely
> need to do the exact same thing as JSDOMQuad::visitAdditionalChildren() for
> position & orientation attributes here. The inverse attribute will likely be
> an issue too.

Thanks for the info. I'll test that and add the additional visitAdditionalChildren and test if required

[Sam Weinig]

(In reply to Chris Dumez from comment #2)
> Comment on attachment 424019 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424019&action=review
> 
> > Source/WebCore/Modules/webxr/WebXRRigidTransform.idl:38
> > +    [CustomGetter] readonly attribute Float32Array matrix;
> 
> Same issue as below.
> 
> > Source/WebCore/Modules/webxr/WebXRViewerPose.idl:35
> > +    [CustomGetter, SameObject] readonly attribute FrozenArray<WebXRView> views;
> 
> You also need [JSCustomMarkFunction] on the interface and custom bindings
> code for visitAdditionalChildren() which visits cachedViews() on the impl
> object.

Could something like:

[CachedAttribute, SameObject] readonly attribute FrozenArray<WebXRView> views;

be used instead for a case like this (it's been a while since I looked into this, so I can never quire remember).

[Chris Dumez]

Comment on attachment 424025 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRViewerPose.idl:36
> +    [CustomGetter, SameObject] readonly attribute FrozenArray<WebXRView> views;

I suggest you try [CachedAttribute, SameObject] for this FrozenArray, as Sam suggested. If it works, then you won't need custom bindings for this one, which would be nice.

[Imanol Fernandez]

Comment on attachment 424025 [details]
Patch

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

>> Source/WebCore/Modules/webxr/WebXRViewerPose.idl:36
>> +    [CustomGetter, SameObject] readonly attribute FrozenArray<WebXRView> views;
> 
> I suggest you try [CachedAttribute, SameObject] for this FrozenArray, as Sam suggested. If it works, then you won't need custom bindings for this one, which would be nice.

It works, thanks for the suggestion

[Imanol Fernandez]

Created attachment 424363 [details]
Patch for landing

[EWS]

Committed r275102: <https://commits.webkit.org/r275102>

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

[Radar WebKit Bug Importer]

<rdar://problem/75889305>