Bug 174202

Summary: [WebVR] Add IDLs and stubs
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: New BugsAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, buildbot, dino, jiewen_tan, jonlee, mitz, rniwa, sam, thorton, webkit-bug-importer, zan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
WIP patch
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews113 for mac-elcapitan
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Patch dino: review+

Description Sergio Villar Senin 2017-07-06 08:37:32 PDT
[WebVR] Add IDLs and stubs
Comment 1 Sergio Villar Senin 2017-07-06 09:07:58 PDT
Created attachment 314723 [details]
Patch
Comment 2 Sergio Villar Senin 2017-07-06 09:09:21 PDT
It's missing the Xcode stuff, but perhaps could be added later by someone else.
Comment 3 Alex Christensen 2017-07-06 09:30:01 PDT
Comment on attachment 314723 [details]
Patch

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

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:223
> +ENABLE_WEBVR = ENABLE_WEBVR;
> +ENABLE_OFFSCREEN_CANVAS = ENABLE_OFFSCREEN_CANVAS;

I'm not sure if we'll want to have these on by default during development.

> Source/WebCore/Modules/webvr/NavigatorWebVR.cpp:39
> +const Vector<RefPtr<VRDisplay>>& NavigatorWebVR::activeVRDisplays(Navigator&)

Could this return a const Vector<Ref<VRDisplay>>& ?

> Source/WebCore/Modules/webvr/VRStageParameters.idl:33
> +] interface VRStageParameters {
> +    readonly attribute Float32Array sittingToStandingTransform;
> +
> +    readonly attribute float sizeX;
> +    readonly attribute float sizeZ;
> +};

I guess this is more of a spec issue, but I've never been convinced that a stage is universally part of VR.  I understand the need for having two displays with two projection matrices and I think that's necessary for VR, but I'm not sure all VR applications want to define a 2D stage where the person is.  Was it really worth standardization?
Comment 4 Sam Weinig 2017-07-06 11:54:30 PDT
This should probably be discussed on webkit-dev before moving forward.
Comment 5 Sam Weinig 2017-07-06 11:58:03 PDT
Some notes:

- IDL 'unsigned long' maps to WebCore 'unsigned'.
- It would be better to split Offscreen Canvas and WebVR into separate changes.
Comment 6 Sergio Villar Senin 2017-07-07 01:38:16 PDT
(In reply to Sam Weinig from comment #4)
> This should probably be discussed on webkit-dev before moving forward.

Oh right. I had the impression that it was already discussed but it was in the context of some private emails with some Apple folks.
Comment 7 Sergio Villar Senin 2017-07-12 08:56:13 PDT
Created attachment 315247 [details]
Patch

Moved unsigned long to unsigned. Removed OffscreenCanvas stuff
Comment 8 Sergio Villar Senin 2017-08-02 16:19:30 PDT
Maciej asked in the list to replace the build flag by a runtime flag. Makes total sense, I'll do it.
Comment 9 Sergio Villar Senin 2017-08-10 04:13:58 PDT
Created attachment 317801 [details]
Patch
Comment 10 Sergio Villar Senin 2017-08-10 04:15:55 PDT
Recent changes include
* Moved from build flag -> runtime flag
* Added a couple of basic test cases
* Fixed build without GAMEPAD support
* Added temptative Xcode stuff. Cannot make it build as some DerivedSources are not generated (will need some help from an Appler).
Comment 11 Sergio Villar Senin 2017-08-10 04:22:19 PDT
Created attachment 317802 [details]
Patch
Comment 12 Sergio Villar Senin 2017-08-29 05:01:51 PDT
I know we're in summer time, any reviewer?

Also could someone help with the Xcode stuff?
Comment 13 Zan Dobersek 2017-08-30 13:00:56 PDT
Created attachment 319389 [details]
WIP patch

Attachment #317802 [details] with necessary XCode changes.
Comment 14 Zan Dobersek 2017-08-30 13:48:23 PDT
(In reply to Zan Dobersek from comment #13)
> Created attachment 319389 [details]
> WIP patch
> 
> Attachment #317802 [details] with necessary XCode changes.

This builds now, but there's test failures for the two added tests.

Also, please don't forget to update the ChangeLog entries, a few of them are redundant now that the ENABLE_WEBVR flag isn't introduced anymore.
Comment 15 Build Bot 2017-08-30 14:27:44 PDT
Comment on attachment 319389 [details]
WIP patch

Attachment 319389 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4412049

New failing tests:
webvr/webvr-enabled.html
webvr/webvr-disabled.html
Comment 16 Build Bot 2017-08-30 14:27:45 PDT
Created attachment 319401 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 17 Build Bot 2017-08-30 14:31:43 PDT
Comment on attachment 319389 [details]
WIP patch

Attachment 319389 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4412055

New failing tests:
webvr/webvr-enabled.html
webvr/webvr-disabled.html
Comment 18 Build Bot 2017-08-30 14:31:45 PDT
Created attachment 319403 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 19 Build Bot 2017-08-30 14:46:48 PDT
Comment on attachment 319389 [details]
WIP patch

Attachment 319389 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4412089

New failing tests:
webvr/webvr-enabled.html
webvr/webvr-disabled.html
Comment 20 Build Bot 2017-08-30 14:46:50 PDT
Created attachment 319408 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 21 Build Bot 2017-08-30 14:55:33 PDT
Comment on attachment 319389 [details]
WIP patch

Attachment 319389 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4412099

New failing tests:
webvr/webvr-enabled.html
webvr/webvr-disabled.html
Comment 22 Build Bot 2017-08-30 14:55:34 PDT
Created attachment 319411 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 23 Sergio Villar Senin 2017-09-05 03:05:57 PDT
Created attachment 319890 [details]
Patch

Tests working fine. Mac should build now too
Comment 24 Sergio Villar Senin 2017-09-11 01:35:14 PDT
Ping reviewers. The patch is getting rotten :)
Comment 25 Dean Jackson 2017-09-12 16:24:58 PDT
Comment on attachment 319890 [details]
Patch

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

It's unclear to me if we should implement this version (1.1) or the next version (2.0). Version 1.1 says it is already deprecated. Version 2.0 says it isn't ready to implement yet. I guess we should start with 1.1.

> Source/WebKit/UIProcess/API/C/WKPreferences.cpp:1881
> +
> +void WKPreferencesSetWebVREnabled(WKPreferencesRef preferencesRef, bool flag)
> +{
> +    toImpl(preferencesRef)->setWebVREnabled(flag);
> +}
> +
> +bool WKPreferencesGetWebVREnabled(WKPreferencesRef preferencesRef)
> +{
> +    return toImpl(preferencesRef)->webVREnabled();
> +}

I don't think we need the C API entry points.

> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:531
> +// Defaults to false.
> +WK_EXPORT void WKPreferencesSetWebVREnabled(WKPreferencesRef, bool flag);
> +WK_EXPORT bool WKPreferencesGetWebVREnabled(WKPreferencesRef);
> +

Ditto.
Comment 26 Sergio Villar Senin 2017-09-13 01:50:45 PDT
(In reply to Dean Jackson from comment #25)
> Comment on attachment 319890 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319890&action=review
> 
> It's unclear to me if we should implement this version (1.1) or the next
> version (2.0). Version 1.1 says it is already deprecated. Version 2.0 says
> it isn't ready to implement yet. I guess we should start with 1.1.

I just followed the "do what other browsers do" "rule". Everybody else is shipping 1.1. It's true that the specs are tagged as deprecated but 2.0 is not a thing yet. Also the amount of API changes between 1.1 and 2.0 is not that big, I expect the transition not to be overwhelmingly difficult.
Comment 27 Sergio Villar Senin 2017-09-13 04:17:37 PDT
Committed r221966: <http://trac.webkit.org/changeset/221966>
Comment 28 Radar WebKit Bug Importer 2017-09-27 13:01:51 PDT
<rdar://problem/34694508>