[WebXR] Refactor OpenXR platform code

• Component: New Bugs
• Status: RESOLVED FIXED

[Sergio Villar Senin]

[WebXR] Refactor OpenXR platform code

[Sergio Villar Senin]

Created attachment 400466 [details]
Patch

[Sergio Villar Senin]

Created attachment 400468 [details]
Patch

[Zan Dobersek]

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.

[Sergio Villar Senin]

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.

[youenn fablet]

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.

[Sergio Villar Senin]

(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.

[Darin Adler]

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.

[Sergio Villar Senin]

(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.

[Sergio Villar Senin]

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

[Darin Adler]

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.

[Darin Adler]

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.

[Sergio Villar Senin]

Created attachment 401135 [details]
Patch

[Sergio Villar Senin]

Darin, Youenn PTAL

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

[youenn fablet]

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?

[Sergio Villar Senin]

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

[Sergio Villar Senin]

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

[Radar WebKit Bug Importer]

<rdar://problem/64206806>

[Yusuke Suzuki]

Re-opened since this is blocked by bug 213045

[Philippe Normand]

Sorry I managed to fix that one. Closing.