Bug 225578

Summary:

[WebXR] Allow WebXR to be tested on PLATFORM(COCOA)

Product:

WebKit

Reporter:

Dean Jackson <dino>

Component:

WebXR

Assignee:

Dean Jackson <dino>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

adachan, benjamin, cdumez, clopez, cmarcelo, ews-watchlist, lmoura, sam, tsavell, webkit-bug-importer, youennf

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=168623

Attachments:

Description	Flags
Patch	none
Patch	ews-feeder: commit-queue-
Patch	none
EWS test	none
Patch	none
Patch	sam: review+, ews-feeder: commit-queue-
EWS test	ews-feeder: commit-queue-
EWS test 2	ews-feeder: commit-queue-
EWS test 3	ews-feeder: commit-queue-
EWS test 4	none

Description Dean Jackson 2021-05-09 01:58:16 PDT

[WebXR] Allow WebXR to be tested on PLATFORM(COCOA)

Comment 1 Radar WebKit Bug Importer 2021-05-09 01:58:48 PDT

<rdar://problem/77707283>

Comment 2 Dean Jackson 2021-05-09 02:08:42 PDT Comment hidden (obsolete)

Created attachment 428118 [details]
Patch

Comment 3 Dean Jackson 2021-05-09 11:42:59 PDT Comment hidden (obsolete)

Created attachment 428131 [details]
Patch

Comment 4 Dean Jackson 2021-05-09 12:05:49 PDT

Created attachment 428132 [details]
Patch

Comment 5 Dean Jackson 2021-05-09 12:21:30 PDT

Created attachment 428133 [details]
EWS test

Comment 6 Sam Weinig 2021-05-09 14:36:42 PDT

Comment on attachment 428133 [details]
EWS test

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

> Source/WebCore/platform/xr/cocoa/PlatformXRCocoa.mm:29
>  #if ENABLE(WEBXR) && PLATFORM(COCOA)

We should probably add a new ENABLE() for the dummy implementation. Doesn't have to be cocoa specific. 

For cocoa platforms it can be defined if HAVE(WEBXR_INTERNALS) is false.

Comment 7 Dean Jackson 2021-05-09 18:29:38 PDT

Created attachment 428148 [details]
Patch

Comment 8 Dean Jackson 2021-05-09 18:31:42 PDT

(In reply to Sam Weinig from comment #6)
> Comment on attachment 428133 [details]
> EWS test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428133&action=review
> 
> > Source/WebCore/platform/xr/cocoa/PlatformXRCocoa.mm:29
> >  #if ENABLE(WEBXR) && PLATFORM(COCOA)
> 
> We should probably add a new ENABLE() for the dummy implementation. Doesn't
> have to be cocoa specific. 
> 
> For cocoa platforms it can be defined if HAVE(WEBXR_INTERNALS) is false.

What do you suggest? 

I didn't want to put extra tests everywhere.

Comment 9 Sam Weinig 2021-05-09 18:38:38 PDT

Comment on attachment 428148 [details]
Patch

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

> Source/WTF/wtf/PlatformEnableCocoa.h:602
> +#if !defined(ENABLE_WEBXR) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

If possible, I'd prefer we enabled this everywhere, with the dummy implementation so we don't require more per-platform tests or skips.

Comment 10 Sam Weinig 2021-05-09 18:39:27 PDT

(In reply to Dean Jackson from comment #8)
> (In reply to Sam Weinig from comment #6)
> > Comment on attachment 428133 [details]
> > EWS test
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=428133&action=review
> > 
> > > Source/WebCore/platform/xr/cocoa/PlatformXRCocoa.mm:29
> > >  #if ENABLE(WEBXR) && PLATFORM(COCOA)
> > 
> > We should probably add a new ENABLE() for the dummy implementation. Doesn't
> > have to be cocoa specific. 
> > 
> > For cocoa platforms it can be defined if HAVE(WEBXR_INTERNALS) is false.
> 
> What do you suggest? 

I'm not sure what you are asking here, sorry. What do I suggest for what?

Comment 11 Dean Jackson 2021-05-09 19:09:11 PDT

(In reply to Sam Weinig from comment #10)
> (In reply to Dean Jackson from comment #8)
> > (In reply to Sam Weinig from comment #6)
> > > Comment on attachment 428133 [details]
> > > EWS test
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=428133&action=review
> > > 
> > > > Source/WebCore/platform/xr/cocoa/PlatformXRCocoa.mm:29
> > > >  #if ENABLE(WEBXR) && PLATFORM(COCOA)
> > > 
> > > We should probably add a new ENABLE() for the dummy implementation. Doesn't
> > > have to be cocoa specific. 
> > > 
> > > For cocoa platforms it can be defined if HAVE(WEBXR_INTERNALS) is false.
> > 
> > What do you suggest? 
> 
> I'm not sure what you are asking here, sorry. What do I suggest for what?

What do you suggest for ENABLE_DUMMY_WEBXR?

If it was something like that, wouldn't that mean we'd have to change a lot of

#if ENABLE(WEBXR)

to

#if ENABLE(WEBXR) || ENABLE(DUMMY_WEBXR)
?

Or, do you think we'd keep ENABLE_WEBXR
and have a USE_DUMMY_WEBXR to represent the platform bit that is missing?

Comment 12 Dean Jackson 2021-05-09 19:09:28 PDT

(In reply to Sam Weinig from comment #9)
> Comment on attachment 428148 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428148&action=review
> 
> > Source/WTF/wtf/PlatformEnableCocoa.h:602
> > +#if !defined(ENABLE_WEBXR) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)
> 
> If possible, I'd prefer we enabled this everywhere, with the dummy
> implementation so we don't require more per-platform tests or skips.

👍

Comment 13 Sam Weinig 2021-05-09 20:13:16 PDT

(In reply to Dean Jackson from comment #11)
> (In reply to Sam Weinig from comment #10)
> > (In reply to Dean Jackson from comment #8)
> > > (In reply to Sam Weinig from comment #6)
> > > > Comment on attachment 428133 [details]
> > > > EWS test
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=428133&action=review
> > > > 
> > > > > Source/WebCore/platform/xr/cocoa/PlatformXRCocoa.mm:29
> > > > >  #if ENABLE(WEBXR) && PLATFORM(COCOA)
> > > > 
> > > > We should probably add a new ENABLE() for the dummy implementation. Doesn't
> > > > have to be cocoa specific. 
> > > > 
> > > > For cocoa platforms it can be defined if HAVE(WEBXR_INTERNALS) is false.
> > > 
> > > What do you suggest? 
> > 
> > I'm not sure what you are asking here, sorry. What do I suggest for what?
> 
> What do you suggest for ENABLE_DUMMY_WEBXR?
> 
> If it was something like that, wouldn't that mean we'd have to change a lot
> of
> 
> #if ENABLE(WEBXR)
> 
> to
> 
> #if ENABLE(WEBXR) || ENABLE(DUMMY_WEBXR)
> ?
> 
> Or, do you think we'd keep ENABLE_WEBXR
> and have a USE_DUMMY_WEBXR to represent the platform bit that is missing?

The latter.

Comment 14 Dean Jackson 2021-05-10 19:52:41 PDT

Created attachment 428239 [details]
Patch

Comment 15 EWS Watchlist 2021-05-10 19:53:49 PDT

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

Comment 16 Sam Weinig 2021-05-10 20:10:49 PDT

Comment on attachment 428239 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/webxr/xrViewport_valid.https.html:21
> +        console.log(pose.views);

Probably don’t want to land this.

Comment 17 Ada Chan 2021-05-10 20:51:25 PDT

Comment on attachment 428239 [details]
Patch

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

> Source/WTF/wtf/PlatformUse.h:325
> +#if !defined(HAVE_WEBXR_INTERNALS) && !HAVE(WEBXR_INTERNALS)

Would #if !HAVE(WEBXR_INTERNALS) be enough?

Comment 18 Dean Jackson 2021-05-11 15:31:35 PDT

Created attachment 428317 [details]
EWS test

Comment 19 Dean Jackson 2021-05-12 12:51:45 PDT

Created attachment 428404 [details]
EWS test 2

Comment 20 Dean Jackson 2021-05-12 16:11:22 PDT

Created attachment 428429 [details]
EWS test 3

Comment 21 Dean Jackson 2021-05-12 17:37:36 PDT

Clearly something strange happens with the tests. I'll need to debug it before this can be enabled.

Comment 22 Dean Jackson 2021-05-13 12:44:34 PDT

Created attachment 428547 [details]
EWS test 4

Comment 23 Dean Jackson 2021-05-13 16:02:01 PDT

By grabthar's hammer, it looks like this latest patch is passing. I wonder if it will be flakey.

Comment 24 Dean Jackson 2021-05-13 17:54:26 PDT

Committed r277468 (237709@main): <https://commits.webkit.org/237709@main>

Comment 25 Lauro Moura 2021-05-13 20:33:41 PDT

Tests skipped in GTK in r277478 (Only WPE has it enabled).

Comment 26 Truitt Savell 2021-05-14 16:44:29 PDT

This test imported/w3c/web-platform-tests/webxr/xrViewerPose_views_sameObject.https.html

is flaky after being enabled 

history:
https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fwebxr%2FxrViewerPose_views_sameObject.https.html