Bug 224931

Created attachment 426814 [details] Patch

Created attachment 426904 [details] Patch Fix compilation error in debug builds

<rdar://problem/77321283>

Comment on attachment 426904 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426904&action=review Looking great. > Source/WebCore/ChangeLog:42 > + (PlatformXR::XrPoseIdentity): Creates a identify pose. identify->identity > Source/WebCore/Sources.txt:2200 > +platform/xr/openxr/OpenXRInputSource.cpp @no-unify Not for this patch, but as the feature matures we should think about removing the @no-unify at some point and get the benefit of unified builds. > Source/WebCore/platform/xr/openxr/OpenXRInput.cpp:52 > + m_handleIndex++; Does 0 have any special meaning or can it be freely used as handle index. > Source/WebCore/platform/xr/openxr/OpenXRInput.cpp:54 > + if (inputSource) You can instead do if (auto inputSource = OpenXRInputSource::create(m_instance, m_session, handeness, m_handleIndex)) > Source/WebCore/platform/xr/openxr/OpenXRInput.h:39 > + OpenXRInput(XrInstance, XrSession, XrSpace); I guess the constructor should be private. > Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:98 > + const OpenXRProfileId* profileIds { nullptr }; Why not using a Vector?. Same for the other fields > Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:110 > + It's unfortunate to have all this specific stuff for one device, i.e., it isn't scalable to include here every single device profile. Isn't there any library providing those (or any other generic mechanism)? > Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:34 > + return input; We can even use a ternary operator here > Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:39 > + if (m_actionSet != XR_NULL_HANDLE) We can do it in a followup patch but we should definitely add a isXrNullHandle() (or similar) method to the OpenXRUtils header and replace all those comparisons. > Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:52 > + m_handle = handle; I prefer to have these initializations in the constructor. > Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:59 > + String prefix = "input_" + handenessName; Doesn't those special prefixes/suffixes have a name defined in OpenXR? If not, maybe it does make sense to define some constexpr for them. > Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:96 > + RETURN_RESULT_IF_FAILED(createBinding(profile.path, m_pointerAction, m_subactionPathName + "/input/aim/pose", bindings), m_instance); Ditto, using constexpr for the strings > Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:158 > + return WTF::nullopt; // Trigger is mandatory in xr-standard mapping Move the comment above the if. BTW comments must have a full stop at the end. > Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:211 > + for (size_t i = 0; i < profile.profileIdsSize; ++i) for (auto profileId : profile.profileIds) > Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:223 > + ASSERT(space); Knowing that it cannot be null, we better pass a reference here. > Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:235 > + ASSERT(action); Ditto reference. > Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:249 > + ASSERT(actions); Ditto. > Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:280 > + ASSERT(pose); Ditto > Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:337 > + ASSERT(value); Ditto > Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:352 > + ASSERT(value); Ditto > Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:367 > + ASSERT(value); Ditto. > Source/WebCore/platform/xr/openxr/OpenXRUtils.h:130 > + case XRHandedness::Right: return "right"; Nit: I think the returns should be in a different line > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:586 > + if (!didNotifyInputInitialization) { Better invert the condition and early return. That way we could save an indentation level > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:590 > + fprintf(stderr, "OpenXRDevice::updateInteractionProfile6 \n"); Remove debug code or use LOG() instead

Created attachment 429155 [details] Patch Address review feedback

Comment on attachment 426904 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426904&action=review >> Source/WebCore/ChangeLog:42 >> + (PlatformXR::XrPoseIdentity): Creates a identify pose. > > identify->identity done >> Source/WebCore/Sources.txt:2200 >> +platform/xr/openxr/OpenXRInputSource.cpp @no-unify > > Not for this patch, but as the feature matures we should think about removing the @no-unify at some point and get the benefit of unified builds. makes sense, I'll address it as a follow-up >> Source/WebCore/platform/xr/openxr/OpenXRInput.cpp:54 >> + if (inputSource) > > You can instead do > > if (auto inputSource = OpenXRInputSource::create(m_instance, m_session, handeness, m_handleIndex)) done >> Source/WebCore/platform/xr/openxr/OpenXRInput.h:39 >> + OpenXRInput(XrInstance, XrSession, XrSpace); > > I guess the constructor should be private. done >> Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:98 >> + const OpenXRProfileId* profileIds { nullptr }; > > Why not using a Vector?. Same for the other fields Vector doesn't support constexpr. I'm trying to define a constexpr with all the available bindings in a declarative way. >> Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:110 >> + > > It's unfortunate to have all this specific stuff for one device, i.e., it isn't scalable to include here every single device profile. Isn't there any library providing those (or any other generic mechanism)? AFAIK there isn't any library or generic way, just the khr/simple_controller fallback, but only supports a limited subset of controller buttons. Chromium is also definining bindings for each controller they want to have full support of. >> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:34 >> + return input; > > We can even use a ternary operator here unique_ptr complains when using the ternary operator because of the deleted copy constructor will nullptr. I also tried with {} but didn't work either. >> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:39 >> + if (m_actionSet != XR_NULL_HANDLE) > > We can do it in a followup patch but we should definitely add a isXrNullHandle() (or similar) method to the OpenXRUtils header and replace all those comparisons. Ok, I'll address that as a follow-up >> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:52 >> + m_handle = handle; > > I prefer to have these initializations in the constructor. Done >> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:59 >> + String prefix = "input_" + handenessName; > > Doesn't those special prefixes/suffixes have a name defined in OpenXR? If not, maybe it does make sense to define some constexpr for them. I have created a constexpr as they are not defined in OpenXR constant. >> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:96 >> + RETURN_RESULT_IF_FAILED(createBinding(profile.path, m_pointerAction, m_subactionPathName + "/input/aim/pose", bindings), m_instance); > > Ditto, using constexpr for the strings done >> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:158 >> + return WTF::nullopt; // Trigger is mandatory in xr-standard mapping > > Move the comment above the if. > > BTW comments must have a full stop at the end. done >> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:223 >> + ASSERT(space); > > Knowing that it cannot be null, we better pass a reference here. Done >> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:235 >> + ASSERT(action); > > Ditto reference. Done >> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:280 >> + ASSERT(pose); > > Ditto Done >> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:337 >> + ASSERT(value); > > Ditto Done. >> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:352 >> + ASSERT(value); > > Ditto Done. >> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:367 >> + ASSERT(value); > > Ditto. Done. >> Source/WebCore/platform/xr/openxr/OpenXRUtils.h:130 >> + case XRHandedness::Right: return "right"; > > Nit: I think the returns should be in a different line Done >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:590 >> + fprintf(stderr, "OpenXRDevice::updateInteractionProfile6 \n"); > > Remove debug code or use LOG() instead Done

Comment on attachment 429155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429155&action=review Pretty happy with the patch. I have just a few comments on some stuff I didn't spot on the first review. > Source/WebCore/platform/xr/openxr/OpenXRInput.cpp:82 > + Vector<Device::FrameData::InputSource> result; Supernit for landing: move the declaration down to the place we first use it. > Source/WebCore/platform/xr/openxr/OpenXRInput.h:40 > + Vector<Device::FrameData::InputSource> getInputSources(const XrFrameState&) const; Didn't spot this in this first pass but we don't usually use the "get" prefix for methods. This is not exactly a getter anyway as we're constructing it, so I'd use another prefix like collect/construct/... > Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:42 > +constexpr std::array<OpenXRButtonType, 7> OpenXRButtonTypes { This should be openXRButtonTypes. Alternatively if you want to preserve the capitalized "O" then prepend something like availableOpenXR..., validOpenXR..., supportedOpenXR... what > Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:76 > +constexpr std::array<OpenXRAxisType, 2> OpenXRAxisTypes { Ditto openXRAxisTypes > Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:133 > +// Default fallback when thre isn't a specific controller binding. Nit: thre -> there > Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:152 > +constexpr std::array<OpenXRInputProfile, 2> OpenXRInputProfiles { HTCViveInputProfile, KHRSimpleInputProfile }; Ditto openXRInputProfiles > Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:65 > + // Initialize Action Set Nit: missing dot at the end. Same for the other comments. > Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:218 > + if (!strcmp(profile.path, buffer)) { Can we use strncmp with writtenCount ? > Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:244 > + createInfo.subactionPaths = &m_subactionPath; Why are you getting a reference here? Shouldn't be just m_subactionPath ? > Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:273 > + if (it != bindings.end()) I guess we can do if (auto it = bindings.find(profilePath); it != bindings.end()) since "it" is only used in this branch > Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:303 > + if (actions.press != XR_NULL_HANDLE && XR_SUCCEEDED(getActionState(actions.press, &result.pressed))) Looks like we can convert these tree if conditions into a lambda as they're basically doing the same with different arguments. Also the hasValue=true assignment could be moved into the lambda as well. We could call it getActionStateIfNotNull() for example. > Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:321 > + return result; return hasValue ? makeOptional(result) : WTF::nullopt; > Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:334 > + return WTF::nullopt; I know it's a matter of taste but I think I prefer to do the failure return in the if block. Otherwise it looks like the "normal" flow is to return a null axis.

Created attachment 429521 [details] Patch Address review feedback

Created attachment 429522 [details] Patch Fix nits in OpenXRInput.cpp

Comment on attachment 429155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429155&action=review >> Source/WebCore/platform/xr/openxr/OpenXRInput.cpp:82 >> + Vector<Device::FrameData::InputSource> result; > > Supernit for landing: move the declaration down to the place we first use it. done >> Source/WebCore/platform/xr/openxr/OpenXRInput.h:40 >> + Vector<Device::FrameData::InputSource> getInputSources(const XrFrameState&) const; > > Didn't spot this in this first pass but we don't usually use the "get" prefix for methods. This is not exactly a getter anyway as we're constructing it, so I'd use another prefix like collect/construct/... done >> Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:42 >> +constexpr std::array<OpenXRButtonType, 7> OpenXRButtonTypes { > > This should be openXRButtonTypes. Alternatively if you want to preserve the capitalized "O" then prepend something like availableOpenXR..., validOpenXR..., supportedOpenXR... what done >> Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:76 >> +constexpr std::array<OpenXRAxisType, 2> OpenXRAxisTypes { > > Ditto openXRAxisTypes done >> Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:133 >> +// Default fallback when thre isn't a specific controller binding. > > Nit: thre -> there done >> Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:152 >> +constexpr std::array<OpenXRInputProfile, 2> OpenXRInputProfiles { HTCViveInputProfile, KHRSimpleInputProfile }; > > Ditto openXRInputProfiles done >> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:65 >> + // Initialize Action Set > > Nit: missing dot at the end. Same for the other comments. done >> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:218 >> + if (!strcmp(profile.path, buffer)) { > > Can we use strncmp with writtenCount ? done >> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:244 >> + createInfo.subactionPaths = &m_subactionPath; > > Why are you getting a reference here? Shouldn't be just m_subactionPath ? It's because createInfo.subactionPaths expects a pointer to an array, and we are treating a single variable as a one sized array, >> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:273 >> + if (it != bindings.end()) > > I guess we can do > if (auto it = bindings.find(profilePath); it != bindings.end()) > > since "it" is only used in this branch done >> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:303 >> + if (actions.press != XR_NULL_HANDLE && XR_SUCCEEDED(getActionState(actions.press, &result.pressed))) > > Looks like we can convert these tree if conditions into a lambda as they're basically doing the same with different arguments. Also the hasValue=true assignment could be moved into the lambda as well. We could call it getActionStateIfNotNull() for example. good idea, done. >> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:321 >> + return result; > > return hasValue ? makeOptional(result) : WTF::nullopt; done >> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:334 >> + return WTF::nullopt; > > I know it's a matter of taste but I think I prefer to do the failure return in the if block. Otherwise it looks like the "normal" flow is to return a null axis. Makes sense, done.

Comment on attachment 429522 [details] Patch Nice! Looking forward to trying it locally

Committed r277954 (238081@main): <https://commits.webkit.org/238081@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 429522 [details].