Bug 209859 - [WebXR] Test IDLs and stubs
Summary: [WebXR] Test IDLs and stubs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords: InRadar
Depends on: 210738 210788
Blocks: 208988
  Show dependency treegraph
 
Reported: 2020-04-01 08:21 PDT by Sergio Villar Senin
Modified: 2020-08-18 20:42 PDT (History)
14 users (show)

See Also:


Attachments
Patch (83.13 KB, patch)
2020-04-01 09:54 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (84.85 KB, patch)
2020-04-09 08:25 PDT, Sergio Villar Senin
dino: review+
Details | Formatted Diff | Diff
Patch (82.77 KB, patch)
2020-04-16 07:12 PDT, Sergio Villar Senin
youennf: review+
Details | Formatted Diff | Diff
Patch (83.33 KB, patch)
2020-04-16 12:40 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch for landing (82.30 KB, patch)
2020-04-17 02:10 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch for landing (82.68 KB, patch)
2020-04-21 07:07 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch for landing v2 (81.48 KB, patch)
2020-04-21 13:17 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2020-04-01 08:21:25 PDT
[WebXR] Test IDLs and stubs
Comment 1 Sergio Villar Senin 2020-04-01 09:54:32 PDT
Created attachment 395179 [details]
Patch
Comment 2 Sergio Villar Senin 2020-04-03 01:34:41 PDT
Ping reviewers
Comment 3 youenn fablet 2020-04-03 01:56:04 PDT
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.
Comment 4 Sergio Villar Senin 2020-04-06 11:14:37 PDT
(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.
Comment 5 Sergio Villar Senin 2020-04-08 17:39:23 PDT
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
Comment 6 Sergio Villar Senin 2020-04-09 08:25:51 PDT
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
Comment 7 youenn fablet 2020-04-14 07:26:43 PDT
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?
Comment 8 Sergio Villar Senin 2020-04-14 12:22:20 PDT
(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.
Comment 9 Dean Jackson 2020-04-14 18:45:16 PDT
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.
Comment 10 Dean Jackson 2020-04-14 18:47:26 PDT
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 ?
Comment 11 Dean Jackson 2020-04-14 18:49:15 PDT
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!
Comment 12 Sergio Villar Senin 2020-04-15 03:02:43 PDT
(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.
Comment 13 youenn fablet 2020-04-15 03:37:24 PDT
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.
Comment 14 Sergio Villar Senin 2020-04-15 05:33:07 PDT
(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.
Comment 15 youenn fablet 2020-04-15 05:38:30 PDT
(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.
Comment 16 Sergio Villar Senin 2020-04-15 06:31:47 PDT
(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.
Comment 17 Sergio Villar Senin 2020-04-15 10:11:21 PDT
(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
Comment 18 Sergio Villar Senin 2020-04-16 07:12:20 PDT
Created attachment 396646 [details]
Patch
Comment 19 Sergio Villar Senin 2020-04-16 07:33:25 PDT
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.
Comment 20 youenn fablet 2020-04-16 07:59:25 PDT
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...
Comment 21 Sergio Villar Senin 2020-04-16 12:40:26 PDT
Created attachment 396688 [details]
Patch
Comment 22 Sergio Villar Senin 2020-04-16 12:56:33 PDT
The PR for the wpt code in https://github.com/web-platform-tests/wpt/pull/23031
Comment 23 Sergio Villar Senin 2020-04-17 02:10:47 PDT
Created attachment 396748 [details]
Patch for landing
Comment 24 Sergio Villar Senin 2020-04-20 13:59:13 PDT
(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.
Comment 25 Sergio Villar Senin 2020-04-21 07:07:22 PDT
Created attachment 397078 [details]
Patch for landing
Comment 26 Sergio Villar Senin 2020-04-21 09:30:41 PDT
Committed r260432: <https://trac.webkit.org/changeset/260432>
Comment 27 Radar WebKit Bug Importer 2020-04-21 09:31:14 PDT
<rdar://problem/62113053>
Comment 28 Sergio Villar Senin 2020-04-21 10:20:33 PDT
Reverted r260432 for reason:

Broke WPE build

Committed r260436: <https://trac.webkit.org/changeset/260436>
Comment 29 Sergio Villar Senin 2020-04-21 13:17:38 PDT
Created attachment 397108 [details]
Patch for landing v2
Comment 30 Sergio Villar Senin 2020-04-22 03:19:40 PDT
Committed r260505: <https://trac.webkit.org/changeset/260505>