Bug 231123 – [WebXR] Stubs for WebXR Hand Input Module

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

[Dean Jackson]

[WebXR] Stubs for WebXR Hands Input Module

[Radar WebKit Bug Importer]

<rdar://problem/83802135>

[Dean Jackson]

Created attachment 439974 [details]
Patch

[Sam Weinig]

Probably not ready for review, so removing the r? flag given all the red.

[Dean Jackson]

Created attachment 440005 [details]
Patch

[Dean Jackson]

I think it was a missing header file (not sure how it built locally).

[Dean Jackson]

Comment on attachment 440005 [details]
Patch

ugh

[Dean Jackson]

GTK doesn't seem to be generating one of the derived sources:

CMake Error at Source/cmake/WebKitMacros.cmake:125 (add_library):
  Cannot find source file:
    /app/webkit/WebKitBuild/Release/WebCore/DerivedSources/JSWebXRHand.cpp

[Dean Jackson]

/Volumes/Data/worker/macOS-Catalina-Release-Build-EWS/build/WebKitBuild/Release/DerivedSources/WebCore/JSDOMWindow.cpp:6172:188: error: no member named 'webXR' in 'WebCore::Settings::Values'
    if ((jsCast<JSDOMGlobalObject*>(globalObject())->scriptExecutionContext()->isSecureContext() && jsCast<JSDOMGlobalObject*>(globalObject())->scriptExecutionContext()->settingsValues().webXR))
                                                                                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^

Hmm... that should be webXREnabled, not just webXR.

How is that being generated differently for Release and Debug?

[Dean Jackson]

Created attachment 440006 [details]
Patch

[Dean Jackson]

Created attachment 440007 [details]
Patch

[Sam Weinig]

Comment on attachment 440007 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +

Given all the issues we have had with GC in this part of the project, can you write out what the ownership semantics are for the new interfaces in this module?

> Source/WTF/wtf/PlatformEnableCocoa.h:632
> +#if !defined(ENABLE_WEBXR_HANDS)
> +#define ENABLE_WEBXR_HANDS 1
> +#endif

You should probably also make this require ENABLE_WEBXR. 

#if !defined(ENABLE_WEBXR_HANDS) && ENABLE(WEBXR)


(or alternatively, add a macro assertion somewhere that WEBXR_HANDS requires WEBXR. See the bottom of PlatformEnable.h for examples of the macro assertions).


Please also add ENABLE_WEBXR_HANDS to PlatformEnable.h, which has a section for default values. In this case, it should be off by default.

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:249
> +#if ENABLE(WEBXR_HANDS)
> +// https://immersive-web.github.io/webxr-hand-input/#dom-xrframe-getjointpose
> +ExceptionOr<RefPtr<WebXRJointPose>> WebXRFrame::getJointPose(const Document&, const WebXRJointSpace& joint, const WebXRSpace& baseSpace)

Missing newline after #if ENABLE(WEBXR_HANDS)

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:270
> +#endif
> +
> +

Extra newlines.

> Source/WebCore/Modules/webxr/WebXRFrame.idl:41
> +    [Conditional=WEBXR_HANDS, CallWith=Document] WebXRJointPose? getJointPose(WebXRJointSpace joint, WebXRSpace baseSpace);
> +    [Conditional=WEBXR_HANDS, CallWith=Document] boolean fillJointRadii(sequence<WebXRJointSpace> jointSpaces, Float32Array radii);
> +    [Conditional=WEBXR_HANDS, CallWith=Document] boolean fillPoses(sequence<WebXRSpace> spaces, WebXRSpace baseSpace, Float32Array transforms);

What is the document for? 

These should be in their own file, using a partial interface like in the spec. The naming pattern we use would make it called WebXRFrame+Hand.idl (the +Hand could be something like +HandInput, as long as we are consistent).

These also need EnabledBySetting=WebXRHandInputEnabled. (once you move them to their own partial interface, the whole partial interface can have that).

> Source/WebCore/Modules/webxr/WebXRHand.idl:27
> +    EnabledBySetting=WebXREnabled,

This should probably be EnabledBySetting=WebXREnabled&WebXRHandInputEnabled

> Source/WebCore/Modules/webxr/WebXRInputSource.idl:45
> +    // https://immersive-web.github.io/webxr-hand-input/#xrinputsource-interface
> +    [Conditional=WEBXR_HANDS, SameObject] readonly attribute WebXRHand? hand;

This also should be in its own IDL file and needs to have EnabledBySetting=WebXRHandInputEnabled

> Source/WebCore/Modules/webxr/WebXRJointPose.idl:27
> +    EnabledBySetting=WebXREnabled,

This should probably be EnabledBySetting=WebXREnabled&WebXRHandInputEnabled

> Source/WebCore/Modules/webxr/WebXRJointPose.idl:33
> +] interface WebXRJointPose {

The spec says this should inherit from XRPose.

> Source/WebCore/Modules/webxr/WebXRJointSpace.idl:27
> +    EnabledBySetting=WebXREnabled,

This should probably be EnabledBySetting=WebXREnabled&WebXRHandInputEnabled

> Source/WebCore/Modules/webxr/WebXRJointSpace.idl:33
> +] interface WebXRJointSpace: WebXRSpace {

Missing space after WebXRJointSpace.

> Source/WebCore/Modules/webxr/WebXRJointSpace.idl:34
> +  readonly attribute XRHandJoint jointName;

Please use 4 spaces here.

> Source/WebCore/Modules/webxr/XRHandJoint.idl:27
> +    EnabledBySetting=WebXREnabled,

Enums don't really need EnabledBySetting (it doesn't do anything). I should look into making this an error. But, if you want to keep it, this should probably be EnabledBySetting=WebXREnabled&WebXRHandInputEnabled.

> Source/WebCore/Modules/webxr/XRHandJoint.idl:31
> +  "wrist",

Please use 4 spaces.

[Dean Jackson]

Comment on attachment 440007 [details]
Patch

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

>> Source/WTF/wtf/PlatformEnableCocoa.h:632
>> +#endif
> 
> You should probably also make this require ENABLE_WEBXR. 
> 
> #if !defined(ENABLE_WEBXR_HANDS) && ENABLE(WEBXR)
> 
> 
> (or alternatively, add a macro assertion somewhere that WEBXR_HANDS requires WEBXR. See the bottom of PlatformEnable.h for examples of the macro assertions).
> 
> 
> Please also add ENABLE_WEBXR_HANDS to PlatformEnable.h, which has a section for default values. In this case, it should be off by default.

Done

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:249
>> +ExceptionOr<RefPtr<WebXRJointPose>> WebXRFrame::getJointPose(const Document&, const WebXRJointSpace& joint, const WebXRSpace& baseSpace)
> 
> Missing newline after #if ENABLE(WEBXR_HANDS)

👍

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:270
>> +
> 
> Extra newlines.

👍

>> Source/WebCore/Modules/webxr/WebXRFrame.idl:41
>> +    [Conditional=WEBXR_HANDS, CallWith=Document] boolean fillPoses(sequence<WebXRSpace> spaces, WebXRSpace baseSpace, Float32Array transforms);
> 
> What is the document for? 
> 
> These should be in their own file, using a partial interface like in the spec. The naming pattern we use would make it called WebXRFrame+Hand.idl (the +Hand could be something like +HandInput, as long as we are consistent).
> 
> These also need EnabledBySetting=WebXRHandInputEnabled. (once you move them to their own partial interface, the whole partial interface can have that).

Removed document.

Moved into WebXRFrame+HandInput.idl (and put the EnabledBy onto the partial)

>> Source/WebCore/Modules/webxr/WebXRHand.idl:27
>> +    EnabledBySetting=WebXREnabled,
> 
> This should probably be EnabledBySetting=WebXREnabled&WebXRHandInputEnabled

Done

>> Source/WebCore/Modules/webxr/WebXRInputSource.idl:45
>> +    [Conditional=WEBXR_HANDS, SameObject] readonly attribute WebXRHand? hand;
> 
> This also should be in its own IDL file and needs to have EnabledBySetting=WebXRHandInputEnabled

Done. WebXRInputSource+HandInput.idl

>> Source/WebCore/Modules/webxr/WebXRJointPose.idl:27
>> +    EnabledBySetting=WebXREnabled,
> 
> This should probably be EnabledBySetting=WebXREnabled&WebXRHandInputEnabled

Done.

>> Source/WebCore/Modules/webxr/WebXRJointPose.idl:33
>> +] interface WebXRJointPose {
> 
> The spec says this should inherit from XRPose.

Done

>> Source/WebCore/Modules/webxr/WebXRJointSpace.idl:27
>> +    EnabledBySetting=WebXREnabled,
> 
> This should probably be EnabledBySetting=WebXREnabled&WebXRHandInputEnabled

Done.

>> Source/WebCore/Modules/webxr/WebXRJointSpace.idl:33
>> +] interface WebXRJointSpace: WebXRSpace {
> 
> Missing space after WebXRJointSpace.

Done.

>> Source/WebCore/Modules/webxr/WebXRJointSpace.idl:34
>> +  readonly attribute XRHandJoint jointName;
> 
> Please use 4 spaces here.

Done.

>> Source/WebCore/Modules/webxr/XRHandJoint.idl:27
>> +    EnabledBySetting=WebXREnabled,
> 
> Enums don't really need EnabledBySetting (it doesn't do anything). I should look into making this an error. But, if you want to keep it, this should probably be EnabledBySetting=WebXREnabled&WebXRHandInputEnabled.

I removed the Enabled. I also moved the enum values in the .h into Platform assuming we'll want it at that level.

>> Source/WebCore/Modules/webxr/XRHandJoint.idl:31
>> +  "wrist",
> 
> Please use 4 spaces.

Done.

[Dean Jackson]

Created attachment 440072 [details]
Patch

[Sam Weinig]

Comment on attachment 440072 [details]
Patch

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

> Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:1591
> +WebXRHandInputEnabled:

In https://bugs.webkit.org/show_bug.cgi?id=231149, I established the convention for submodules of WebXR*ModuleEnabled. Can you rename this to WebXRHandInputModuleEnabled?

> Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:1598
> +      default: WebKit::defaultWebXREnabled() # Follows WebXREnabled

I don't think these comments are needed.l

> Source/WebCore/DerivedSources-input.xcfilelist:592
> +$(PROJECT_DIR)/Modules/webxr/WebXRHand.idl

This isn't new, but what determines if a class is prefixed with WebXR or just XR?

> Source/WebCore/Modules/webxr/WebXRHand.idl:33
> +] interface WebXRHand {

Does this wrapper get properly kept alive by the owning input source? I would guess it would need some marking / GenerateIsReachable or something.  Since this is observable, you really should have a test for this.

> Source/WebCore/Modules/webxr/WebXRJointSpace.h:64
> +    Ref<WebXRSession> m_session;

I think this is a circular reference / leak, since:

WebXRSession owns WebXRInputSourceArray, which own WebXRInputSource, which owns WebXRHand, which owns WebXRJointSpace.

[Dean Jackson]

Comment on attachment 440072 [details]
Patch

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

>> Source/WebCore/DerivedSources-input.xcfilelist:592
>> +$(PROJECT_DIR)/Modules/webxr/WebXRHand.idl
> 
> This isn't new, but what determines if a class is prefixed with WebXR or just XR?

I'm not sure but it seems to be the classes are prefixed if there will be a platform version of them.

>> Source/WebCore/Modules/webxr/WebXRHand.idl:33
>> +] interface WebXRHand {
> 
> Does this wrapper get properly kept alive by the owning input source? I would guess it would need some marking / GenerateIsReachable or something.  Since this is observable, you really should have a test for this.

I've added a GenerateIsReaachable, but I haven't worked out how to write a test yet.

>> Source/WebCore/Modules/webxr/WebXRJointSpace.h:64
>> +    Ref<WebXRSession> m_session;
> 
> I think this is a circular reference / leak, since:
> 
> WebXRSession owns WebXRInputSourceArray, which own WebXRInputSource, which owns WebXRHand, which owns WebXRJointSpace.

This is very annoying because WebXRSpace requires implementation of a virtual method to retrieve the WebXRSession, so all the subclasses keep a Ref.

[Dean Jackson]

Created attachment 441539 [details]
Patch

[Dean Jackson]

Created attachment 441540 [details]
Patch

[Dean Jackson]

Created attachment 441541 [details]
Patch

[Dean Jackson]

Created attachment 441544 [details]
Patch

[Dean Jackson]

Created attachment 441548 [details]
Patch

[Dean Jackson]

Hopefully the GTK/Win failures were because ENABLE(WEBXR) is false there. I forgot to wrap JSWebXRSessionCustom with it.

[Sam Weinig]

Comment on attachment 441548 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRFrame+HandInput.idl:26
> +[

Please add a spec link matching the other idl files.

> Source/WebCore/Modules/webxr/WebXRHand.h:49
> +    unsigned long size() { return m_size; }

This should just be an 'unsigned'. The name 'unsigned long' in WebIDL maps to our 'unsigned'.

> Source/WebCore/Modules/webxr/WebXRHand.h:70
> +    unsigned long m_size { 0 };

Same as above. 'unsigned'.

> Source/WebCore/Modules/webxr/WebXRInputSource+HandInput.idl:30
> +    // https://immersive-web.github.io/webxr-hand-input/#xrinputsource-interface

Please put the spec link above the extended attributes matching the other IDLs.

> Source/WebCore/Modules/webxr/XRHandJoint.idl:26
> +[

Please add spec link.

> LayoutTests/ChangeLog:9
> +        New test that checks an XRHand object can survive garbage collection.

Be good to have tests for WebXRJointSpace as well, but that can happen in time.

[Dean Jackson]

Committed r284347 (243139@main): <https://commits.webkit.org/243139@main>