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

• Status: RESOLVED
• Resolution: FIXED
• Component: WebXR

[Dean Jackson]

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

[Radar WebKit Bug Importer]

<rdar://problem/77707283>

[Dean Jackson]

Created attachment 428118 [details]
Patch

[Dean Jackson]

Created attachment 428131 [details]
Patch

[Dean Jackson]

Created attachment 428132 [details]
Patch

[Dean Jackson]

Created attachment 428133 [details]
EWS test

[Sam Weinig]

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.

[Dean Jackson]

Created attachment 428148 [details]
Patch

[Dean Jackson]

(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.

[Sam Weinig]

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.

[Sam Weinig]

(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?

[Dean Jackson]

(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?

[Dean Jackson]

(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.

👍

[Sam Weinig]

(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.

[Dean Jackson]

Created attachment 428239 [details]
Patch

[EWS Watchlist]

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

[Sam Weinig]

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.

[Ada Chan]

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?

[Dean Jackson]

Created attachment 428317 [details]
EWS test

[Dean Jackson]

Created attachment 428404 [details]
EWS test 2

[Dean Jackson]

Created attachment 428429 [details]
EWS test 3

[Dean Jackson]

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

[Dean Jackson]

Created attachment 428547 [details]
EWS test 4

[Dean Jackson]

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

[Dean Jackson]

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

[Lauro Moura]

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

[Truitt Savell]

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