Bug 209859 – [WebXR] Test IDLs and stubs

• Component: New Bugs
• Status: RESOLVED FIXED

[Sergio Villar Senin]

[WebXR] Test IDLs and stubs

[Sergio Villar Senin]

Created attachment 395179 [details]
Patch

[Sergio Villar Senin]

Ping reviewers

[youenn fablet]

Comment on attachment 395179 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        navigator.xr.test.

Is navigator.xr.test something we want to expose to the web? Should we add a dedicated flag for this fake test API?
Another approach would be to use WebDriver: When WebDriver is enabled, we use fake devices, accessed using the regular navigator.xr API.
This would allow using real XR APIs fuelled by fake devices.

If the goal is passing WPT tests as part of WTR, another option is to move these to WebCoreTestSupport.

[Sergio Villar Senin]

(In reply to youenn fablet from comment #3)
> Comment on attachment 395179 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395179&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        navigator.xr.test.
> 
> Is navigator.xr.test something we want to expose to the web? Should we add a
> dedicated flag for this fake test API?
> Another approach would be to use WebDriver: When WebDriver is enabled, we
> use fake devices, accessed using the regular navigator.xr API.
> This would allow using real XR APIs fuelled by fake devices.

That's a good point. Actually xr.test should not be exposed to the web at all, it's just there for testing. The thing about using WebDriver is that all the WPT tests are using this testing API, so we wouldn't be able to use them. It isn't indeed mandatory, but as they're already there I think we should use them, wouln't we?

It seems a good idea anyway to try that at some point. I'll eventually ask you more details about those fake devices when using WebDriver.

> If the goal is passing WPT tests as part of WTR, another option is to move
> these to WebCoreTestSupport.

OK, I'll check that path.

[Sergio Villar Senin]

Youenn, I've tried moving the new testing interfaces to WebCore/testing. I moved them all but WebXRSystemWebXRTest.idl which is the one adding a partial interface to WebXRSystem. That's why I think that cannot be moved, at least I was not able to do that.

In any case I think we still have to use a dedicated flag for that testing API, otherwise I see no way not to expose it at runtime when WebXR is enabled. BTW is is possible to add multiple flags in "EnabledAtRuntime"? I know in "Conditional" it's possible but haven't seen the same for EnabledAtRuntime. Another possibility could be to have dependencies between flags but not sure that is supported. Something like WebXREnabled -> WebXREnabledTestAPI

[Sergio Villar Senin]

Created attachment 395954 [details]
Patch

Moved most of the API to WebCore/testing. I also added a new runtime flag in order not to expose the WebXR testing API at will even if WebXR is enabled

[youenn fablet]

I am unsure of the goal of this .test accessor.

If the goal is to run WTR on WPT imported tests, I think we could have an internals.xrTest getter and we could set xr.test = internals.xrTest in testharnessreport.js for instance, or use WPT platform-specific files to populate xr.test.

If the goal is to actually run WPT tests in Safari, then I guess it might be fine to add such a property behind its own flag but I am not sure this is worth it.
Are other browsers on board with this approach? Are they/Do they plan shipping xr.test?

[Sergio Villar Senin]

(In reply to youenn fablet from comment #7)
> I am unsure of the goal of this .test accessor.
> 
> If the goal is to run WTR on WPT imported tests, I think we could have an
> internals.xrTest getter and we could set xr.test = internals.xrTest in
> testharnessreport.js for instance, or use WPT platform-specific files to
> populate xr.test.
> 
> If the goal is to actually run WPT tests in Safari, then I guess it might be
> fine to add such a property behind its own flag but I am not sure this is
> worth it.
> Are other browsers on board with this approach? Are they/Do they plan
> shipping xr.test?

Currently there are two implementations, Chrome's and Servo's. In the case of Chrome they use platform specific files in WPT to populate xr.test and then use mojo interfaces to talk to the xr services.

In the case of Servo they implement the test API IDLs as any other web standard similar to what I've done here.

WRT the goal, from the WebXR development POV it should be enough to be able to run WTR on WPT imported tests, after all what I want those tests for is to ensure that I'm implementing the API correctly and that I don't regress. However I think it's good PR for WebKit browsers (like Safari) to have good figures in wpt.fyi. That said I'm fine with any solution, I'd like to land any of the two soon, so that it does not block further developments.

[Dean Jackson]

Seems weird that they (FF) expose xr.test to the Web!

How about for now we go with Youenn's suggestion to put it in Internals and map navigator.xr.test = internals.xrtest in the global script? Is that ok with you?

Then we can decide whether or not to expose it publicly in Safari and other WebKit browsers. It would be kind of easy to put it behind a flag for Safari.

[Dean Jackson]

Comment on attachment 395954 [details]
Patch

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

> Source/WebCore/CMakeLists.txt:162
> +    "${WEBCORE_DIR}/testing"

I don't have a strong opinion, but i wonder if this should be under Modules/webxr/testing ?

[Dean Jackson]

Hopefully Youenn will give a thumbs up in Europe time. But it looks ok to me!

Since you have a Runtime flag, adding an "internal" webkit feature to expose xr.test publicly is easy. I'd still start with checking that we can do it via Internals first (even though that would require modifying our WPT wrapper).

Thanks Sergio!

[Sergio Villar Senin]

(In reply to Dean Jackson from comment #9)
> Seems weird that they (FF) expose xr.test to the Web!

Note that I don't have enough knowledge about Servo to know whether they finally expose that API to the web. I guess they most likely have a similar mechanism to our runtime flags to do that.

> How about for now we go with Youenn's suggestion to put it in Internals and
> map navigator.xr.test = internals.xrtest in the global script? Is that ok
> with you?

OK I can try that.
 
> Then we can decide whether or not to expose it publicly in Safari and other
> WebKit browsers. It would be kind of easy to put it behind a flag for Safari.

Fair enough let's do that.

[youenn fablet]

LGTM too with that approach.

Elsewhere in the code, we use mock instead of fake, see Source/WebCore/platform/mock/ and Source/WebCore/platform/testing for instance.

If this is not raised already, we might want to file an issue to the XR WG to propose an approach similar to fake camera/microphone and getUserMedia.
Sergio, let me know if you want to do it.

For getUserMedia, Safari (and Chrome through command line option) is using a mock camera instead of the real one based on a global switch (WebDriver, Developer menu, preference). From the point of view of the test, navigator.mediaDevices.getUserMedia remains the same.
To control what cameras/microphones can do, the idea is to add some WebDriver APIs (add camera, remove camera...), that would probably be similar to what the XR test spec is defining.

WTR could provide the same set of controls through testRunner or internals.

[Sergio Villar Senin]

(In reply to youenn fablet from comment #13)
> LGTM too with that approach.
> 
> Elsewhere in the code, we use mock instead of fake, see
> Source/WebCore/platform/mock/ and Source/WebCore/platform/testing for
> instance.
> 
> If this is not raised already, we might want to file an issue to the XR WG
> to propose an approach similar to fake camera/microphone and getUserMedia.
> Sergio, let me know if you want to do it.

Not sure what you want to propose to the WG. Is it just about naming (Fake->Mock) or are you talking about completely change the approach and use WebDriver APIs to do testing?

If it's just about naming then it might be doable (bikeshedding risks as usual but shrug) but if we're talking about completely changing the approach I don't think it's very practical because there are already 2 working implementations.

[youenn fablet]

(In reply to Sergio Villar Senin from comment #14)
> (In reply to youenn fablet from comment #13)
> > LGTM too with that approach.
> > 
> > Elsewhere in the code, we use mock instead of fake, see
> > Source/WebCore/platform/mock/ and Source/WebCore/platform/testing for
> > instance.
> > 
> > If this is not raised already, we might want to file an issue to the XR WG
> > to propose an approach similar to fake camera/microphone and getUserMedia.
> > Sergio, let me know if you want to do it.
> 
> Not sure what you want to propose to the WG. Is it just about naming
> (Fake->Mock) or are you talking about completely change the approach and use
> WebDriver APIs to do testing?

I am talking about something like a WebDriver API.
If that has been considered, I would be interested to know why this was not deemed appropriate.

About Fake -> Mock, given the spec talks about Fake, agreed we should stick with this in the implementation.

[Sergio Villar Senin]

(In reply to youenn fablet from comment #15)
> (In reply to Sergio Villar Senin from comment #14)
> > (In reply to youenn fablet from comment #13)
> > > LGTM too with that approach.
> > > 
> > > Elsewhere in the code, we use mock instead of fake, see
> > > Source/WebCore/platform/mock/ and Source/WebCore/platform/testing for
> > > instance.
> > > 
> > > If this is not raised already, we might want to file an issue to the XR WG
> > > to propose an approach similar to fake camera/microphone and getUserMedia.
> > > Sergio, let me know if you want to do it.
> > 
> > Not sure what you want to propose to the WG. Is it just about naming
> > (Fake->Mock) or are you talking about completely change the approach and use
> > WebDriver APIs to do testing?
> 
> I am talking about something like a WebDriver API.
> If that has been considered, I would be interested to know why this was not
> deemed appropriate.

Ah in this case I think you'd better to it, mainly because I completely lack the context that lead to using WebDriver to mock media devices. You could very likely give insightful details I am not aware off. New issues can be filled in here: https://github.com/immersive-web/webxr-test-api/issues/
 
> About Fake -> Mock, given the spec talks about Fake, agreed we should stick
> with this in the implementation.

[Sergio Villar Senin]

(In reply to youenn fablet from comment #15)
> (In reply to Sergio Villar Senin from comment #14)
> > (In reply to youenn fablet from comment #13)
> > > LGTM too with that approach.
> > > 
> > > Elsewhere in the code, we use mock instead of fake, see
> > > Source/WebCore/platform/mock/ and Source/WebCore/platform/testing for
> > > instance.
> > > 
> > > If this is not raised already, we might want to file an issue to the XR WG
> > > to propose an approach similar to fake camera/microphone and getUserMedia.
> > > Sergio, let me know if you want to do it.
> > 
> > Not sure what you want to propose to the WG. Is it just about naming
> > (Fake->Mock) or are you talking about completely change the approach and use
> > WebDriver APIs to do testing?
> 
> I am talking about something like a WebDriver API.
> If that has been considered, I would be interested to know why this was not
> deemed appropriate.

Youenn I've just found out this https://github.com/immersive-web/webxr-test-api/issues/9

[Sergio Villar Senin]

Created attachment 396646 [details]
Patch

[Sergio Villar Senin]

Summary of the changes:

1- Moved WebXRTest to Internals
2- Map xr.test to internals.xrtest in webxr_utils.js in web-platform-tests/webxr/resources/webxr_utils.js
3- Remove the WebXRSystemWebXRTest (no longer needed as we don't need a partial interface anymore)
4- Remove the WebXRTest runtime flag (enough with the current WebXR one as internals is only available for testing)

I'd have to land the wpt change but it shouldn't be controversial.

[youenn fablet]

Comment on attachment 396646 [details]
Patch

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

> Source/WebCore/testing/FakeXRButtonStateInit.idl:26
> +// Bcause the primary button is always guaranteed to be present, and other buttons

s/Bcause/Because/

> Source/WebCore/testing/WebFakeXRDevice.cpp:36
> +void WebFakeXRDevice::setViews(Vector<FakeXRViewInit>) { }

add some CRLF here and below. in other files as well.

> Source/WebCore/testing/WebFakeXRDevice.h:44
> +void setViews(Vector<FakeXRViewInit>);

Indentation.
Ditto below.

> Source/WebCore/testing/WebXRTest.idl:44
> +  Promise<void> disconnectAllDevices();

two spaces vs four spaces below.

> LayoutTests/imported/w3c/web-platform-tests/webxr/resources/webxr_util.js:21
> +    }

Maybe add a comment this is dedicated to WebKit/WebKit test runner.
Ideally, we should isolate that code in a vendor specific file but since there is already Chromium specific code...

[Sergio Villar Senin]

Created attachment 396688 [details]
Patch

[Sergio Villar Senin]

The PR for the wpt code in https://github.com/web-platform-tests/wpt/pull/23031

[Sergio Villar Senin]

Created attachment 396748 [details]
Patch for landing

[Sergio Villar Senin]

(In reply to Sergio Villar Senin from comment #22)
> The PR for the wpt code in
> https://github.com/web-platform-tests/wpt/pull/23031

This landed today, so I'll import it from WPT repo into trunk and then remove it from this patch. After this, the patch will be ready to land.

[Sergio Villar Senin]

Created attachment 397078 [details]
Patch for landing

[Sergio Villar Senin]

Committed r260432: <https://trac.webkit.org/changeset/260432>

[Radar WebKit Bug Importer]

<rdar://problem/62113053>

[Sergio Villar Senin]

Reverted r260432 for reason:

Broke WPE build

Committed r260436: <https://trac.webkit.org/changeset/260436>

[Sergio Villar Senin]

Created attachment 397108 [details]
Patch for landing v2

[Sergio Villar Senin]

Committed r260505: <https://trac.webkit.org/changeset/260505>