Bug 231123

Summary: [WebXR] Stubs for WebXR Hand Input Module
Product: WebKit Reporter: Dean Jackson <dino>
Component: New BugsAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, benjamin, calvaris, cdumez, cmarcelo, esprehn+autocc, ews-watchlist, gyuyoung.kim, kondapallykalyan, ryuan.choi, sam, sergio, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch sam: review+, ews-feeder: commit-queue-

Description Dean Jackson 2021-10-02 15:04:32 PDT
[WebXR] Stubs for WebXR Hands Input Module
Comment 1 Radar WebKit Bug Importer 2021-10-02 15:05:29 PDT
<rdar://problem/83802135>
Comment 2 Dean Jackson 2021-10-02 15:12:59 PDT
Created attachment 439974 [details]
Patch
Comment 3 Sam Weinig 2021-10-02 20:26:07 PDT
Probably not ready for review, so removing the r? flag given all the red.
Comment 4 Dean Jackson 2021-10-03 09:57:11 PDT
Created attachment 440005 [details]
Patch
Comment 5 Dean Jackson 2021-10-03 09:57:39 PDT
I think it was a missing header file (not sure how it built locally).
Comment 6 Dean Jackson 2021-10-03 10:04:56 PDT
Comment on attachment 440005 [details]
Patch

ugh
Comment 7 Dean Jackson 2021-10-03 10:08:50 PDT Comment hidden (obsolete)
Comment 8 Dean Jackson 2021-10-03 10:12:06 PDT Comment hidden (obsolete)
Comment 9 Dean Jackson 2021-10-03 10:37:43 PDT
Created attachment 440006 [details]
Patch
Comment 10 Dean Jackson 2021-10-03 11:04:00 PDT
Created attachment 440007 [details]
Patch
Comment 11 Sam Weinig 2021-10-03 13:28:48 PDT
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.
Comment 12 Dean Jackson 2021-10-04 08:21:29 PDT
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.
Comment 13 Dean Jackson 2021-10-04 10:16:33 PDT
Created attachment 440072 [details]
Patch
Comment 14 Sam Weinig 2021-10-04 10:32:46 PDT
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.
Comment 15 Dean Jackson 2021-10-07 15:48:10 PDT
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.
Comment 16 Dean Jackson 2021-10-17 09:48:09 PDT
Created attachment 441539 [details]
Patch
Comment 17 Dean Jackson 2021-10-17 10:43:11 PDT
Created attachment 441540 [details]
Patch
Comment 18 Dean Jackson 2021-10-17 11:46:57 PDT
Created attachment 441541 [details]
Patch
Comment 19 Dean Jackson 2021-10-17 12:40:05 PDT
Created attachment 441544 [details]
Patch
Comment 20 Dean Jackson 2021-10-17 13:56:38 PDT
Created attachment 441548 [details]
Patch
Comment 21 Dean Jackson 2021-10-17 13:58:25 PDT
Hopefully the GTK/Win failures were because ENABLE(WEBXR) is false there. I forgot to wrap JSWebXRSessionCustom with it.
Comment 22 Sam Weinig 2021-10-17 14:11:26 PDT
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.
Comment 23 Dean Jackson 2021-10-18 01:05:34 PDT
Committed r284347 (243139@main): <https://commits.webkit.org/243139@main>