Bug 212470

Created attachment 400466 [details] Patch

Created attachment 400468 [details] Patch

Comment on attachment 400468 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400468&action=review > Source/WebCore/Modules/webxr/WebXRSystem.cpp:60 > + return this == &other ? true : false; Should be able to avoid the ternary operator. > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:83 > + RETURN_IF_FAILED(result, "xrEnumerateApiLayerProperties()", m_instance); How is m_instance supposed to be used here when it's not yet initialized? It's passed to xrResultToString(), but the docs for that entrypoint suggest it must be a valid handle. > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:149 > +Instance::Impl::~Impl() > +{ > +} Can be `= default;`-ed, unless it should destroy the XrInstance object. > Source/WebCore/testing/WebFakeXRDevice.h:73 > + bool operator==(const Device& other) const final { return this == &other ? true : false; } As before, the ?: operator can be omitted.

Comment on attachment 400468 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400468&action=review Thanks for the review! >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:83 >> + RETURN_IF_FAILED(result, "xrEnumerateApiLayerProperties()", m_instance); > > How is m_instance supposed to be used here when it's not yet initialized? It's passed to xrResultToString(), but the docs for that entrypoint suggest it must be a valid handle. It's initialized to XR_NULL_HANDLE but that's indeed an invalid handle. The Monado runtime early returns with invalid handles but I agree we're not respecting the API contract. I'll add a check in resultToString() before landing. >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:149 >> +} > > Can be `= default;`-ed, unless it should destroy the XrInstance object. Right, we should call xrDestroyInstance here.

Comment on attachment 400468 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400468&action=review > Source/WebCore/platform/xr/PlatformXR.h:55 > + virtual bool operator==(const Device& other) const = 0; In theory, we could have A == B be true and B == A be false. Maybe we should have a function operator==(const Device&, const Device&) that would return false if the two device types are not the same derived type.

(In reply to youenn fablet from comment #5) > Comment on attachment 400468 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=400468&action=review > > > Source/WebCore/platform/xr/PlatformXR.h:55 > > + virtual bool operator==(const Device& other) const = 0; > > In theory, we could have A == B be true and B == A be false. > Maybe we should have a function operator==(const Device&, const Device&) > that would return false if the two device types are not the same derived > type. I am not sure whether I understand this proposal. Looks like you are suggesting to replace the virtual member operator by a function operator to check that the derived types are the same. My question is, where would you then define the operator== for the OpenXRDevice subclasses which compares the ids? Because all the code outside of platform (and WebCore/testing) should have to only deal with Device classes and not any of their subclasses.

Comment on attachment 400468 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400468&action=review >>> Source/WebCore/platform/xr/PlatformXR.h:55 >>> + virtual bool operator==(const Device& other) const = 0; >> >> In theory, we could have A == B be true and B == A be false. >> Maybe we should have a function operator==(const Device&, const Device&) that would return false if the two device types are not the same derived type. > > I am not sure whether I understand this proposal. Looks like you are suggesting to replace the virtual member operator by a function operator to check that the derived types are the same. > > My question is, where would you then define the operator== for the OpenXRDevice subclasses which compares the ids? Because all the code outside of platform (and WebCore/testing) should have to only deal with Device classes and not any of their subclasses. It’s true, virtual operators are typically not a good pattern. It’s easy to get them wrong. I suggest we name this operation rather than overloading ==. Just call it isSameDevice. If it’s an error to call this with a derived class pointer, then at the very least the derived classes should make their overloads private.

(In reply to Darin Adler from comment #7) > Comment on attachment 400468 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=400468&action=review > > >>> Source/WebCore/platform/xr/PlatformXR.h:55 > >>> + virtual bool operator==(const Device& other) const = 0; > >> > >> In theory, we could have A == B be true and B == A be false. > >> Maybe we should have a function operator==(const Device&, const Device&) that would return false if the two device types are not the same derived type. > > > > I am not sure whether I understand this proposal. Looks like you are suggesting to replace the virtual member operator by a function operator to check that the derived types are the same. > > > > My question is, where would you then define the operator== for the OpenXRDevice subclasses which compares the ids? Because all the code outside of platform (and WebCore/testing) should have to only deal with Device classes and not any of their subclasses. > > It’s true, virtual operators are typically not a good pattern. It’s easy to > get them wrong. > > I suggest we name this operation rather than overloading ==. Just call it > isSameDevice. > If it’s an error to call this with a derived class pointer, then at the very > least the derived classes should make their overloads private. Well I was overloading == because I want all the operations in containers to just work without any extra code. What would you suggest? In the end we might now strictly need it because those containers will typically store a Ref<>, RefPtr<> of those objects. Another option could be to just keep the current code, i.e., we generate consecutive integers to identify devices and that's it.

(In reply to Sergio Villar Senin from comment #8) > In the end we might now strictly need it because those containers ^^^ not

Comment on attachment 400468 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400468&action=review >>>>> Source/WebCore/platform/xr/PlatformXR.h:55 >>>>> + virtual bool operator==(const Device& other) const = 0; >>>> >>>> In theory, we could have A == B be true and B == A be false. >>>> Maybe we should have a function operator==(const Device&, const Device&) that would return false if the two device types are not the same derived type. >>> >>> I am not sure whether I understand this proposal. Looks like you are suggesting to replace the virtual member operator by a function operator to check that the derived types are the same. >>> >>> My question is, where would you then define the operator== for the OpenXRDevice subclasses which compares the ids? Because all the code outside of platform (and WebCore/testing) should have to only deal with Device classes and not any of their subclasses. >> >> It’s true, virtual operators are typically not a good pattern. It’s easy to get them wrong. >> >> I suggest we name this operation rather than overloading ==. Just call it isSameDevice. >> If it’s an error to call this with a derived class pointer, then at the very least the derived classes should make their overloads private. > > Well I was overloading == because I want all the operations in containers to just work without any extra code. What would you suggest? > > In the end we might now strictly need it because those containers will typically store a Ref<>, RefPtr<> of those objects. > > Another option could be to just keep the current code, i.e., we generate consecutive integers to identify devices and that's it. OK. Please give me one or more specific examples of a useful container operation so I can consider the value and comment.

Comment on attachment 400468 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400468&action=review >>>>>> Source/WebCore/platform/xr/PlatformXR.h:55 >>>>>> + virtual bool operator==(const Device& other) const = 0; >>>>> >>>>> In theory, we could have A == B be true and B == A be false. >>>>> Maybe we should have a function operator==(const Device&, const Device&) that would return false if the two device types are not the same derived type. >>>> >>>> I am not sure whether I understand this proposal. Looks like you are suggesting to replace the virtual member operator by a function operator to check that the derived types are the same. >>>> >>>> My question is, where would you then define the operator== for the OpenXRDevice subclasses which compares the ids? Because all the code outside of platform (and WebCore/testing) should have to only deal with Device classes and not any of their subclasses. >>> >>> It’s true, virtual operators are typically not a good pattern. It’s easy to get them wrong. >>> >>> I suggest we name this operation rather than overloading ==. Just call it isSameDevice. >>> If it’s an error to call this with a derived class pointer, then at the very least the derived classes should make their overloads private. >> >> Well I was overloading == because I want all the operations in containers to just work without any extra code. What would you suggest? >> >> In the end we might now strictly need it because those containers will typically store a Ref<>, RefPtr<> of those objects. >> >> Another option could be to just keep the current code, i.e., we generate consecutive integers to identify devices and that's it. > > OK. Please give me one or more specific examples of a useful container operation so I can consider the value and comment. Typically you can’t store a polymorphic object in a container: As you said you’ll be storing pointers in the containers.

Created attachment 401135 [details] Patch

Darin, Youenn PTAL I finally removed the equality operator overloading. So far there is no need for them.

Comment on attachment 401135 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401135&action=review > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:95 > + }(); Why do we need these lambdas? It seems its purpose is to not do an if/else. Can we remove them, maybe by using functions with a name that will tell what it does? > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:157 > + XrResult result = xrEnumerateViewConfigurations(m_instance, device.xrSystemId(), 0, &viewConfigurationCount, nullptr); auto > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:167 > + WTFLogAlways("xrEnumerateViewConfigurations(): error %s\n", resultToString(result, m_instance).utf8().data()); No need for these two WTFLogAlways. Maybe we should use RELEASE_LOG(XR, ...) as well instead. > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:182 > + supportedModes.append(SessionMode::ImmersiveVr); Should we use a switch here?

Comment on attachment 401135 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401135&action=review Thanks for the review! >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:95 >> + }(); > > Why do we need these lambdas? It seems its purpose is to not do an if/else. > Can we remove them, maybe by using functions with a name that will tell what it does? Also creating different scopes for variables with the same name. But yeah we don't really need them. >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:157 >> + XrResult result = xrEnumerateViewConfigurations(m_instance, device.xrSystemId(), 0, &viewConfigurationCount, nullptr); > > auto OK. >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:167 >> + WTFLogAlways("xrEnumerateViewConfigurations(): error %s\n", resultToString(result, m_instance).utf8().data()); > > No need for these two WTFLogAlways. > Maybe we should use RELEASE_LOG(XR, ...) as well instead. The duplication is likely a merge issue. I'll replace them by the RELEASE_LOG. >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:182 >> + supportedModes.append(SessionMode::ImmersiveVr); > > Should we use a switch here? OK

Committed r262838: <https://trac.webkit.org/changeset/262838>

<rdar://problem/64206806>

Re-opened since this is blocked by bug 213045

Sorry I managed to fix that one. Closing.