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

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

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

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	WebXR (show other bugs)
Version:	WebKit Nightly Build
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Dean Jackson

URL:
Keywords:	InRadar

Depends on:
Blocks:

Reported:	2021-05-09 01:58 PDT by Dean Jackson
Modified:	2023-10-27 00:36 PDT (History)
CC List:	11 users (show)

See Also:	168623

Attachments
Patch (17.51 KB, patch) 2021-05-09 02:08 PDT, Dean Jackson	no flags	Details \| Formatted Diff \| Diff
Patch (17.51 KB, patch) 2021-05-09 11:42 PDT, Dean Jackson	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
Patch (18.48 KB, patch) 2021-05-09 12:05 PDT, Dean Jackson	no flags	Details \| Formatted Diff \| Diff
EWS test (19.20 KB, patch) 2021-05-09 12:21 PDT, Dean Jackson	no flags	Details \| Formatted Diff \| Diff
Patch (19.11 KB, patch) 2021-05-09 18:29 PDT, Dean Jackson	no flags	Details \| Formatted Diff \| Diff
Patch (24.83 KB, patch) 2021-05-10 19:52 PDT, Dean Jackson	sam: review+ ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
EWS test (24.17 KB, patch) 2021-05-11 15:31 PDT, Dean Jackson	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
EWS test 2 (26.52 KB, patch) 2021-05-12 12:51 PDT, Dean Jackson	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
EWS test 3 (27.47 KB, patch) 2021-05-12 16:11 PDT, Dean Jackson	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
EWS test 4 (33.09 KB, patch) 2021-05-13 12:44 PDT, Dean Jackson	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (8) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

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