[WebXR] Port enum classes to new serialization format #21161

csaavedra · 2023-12-01T15:46:20Z

`3fb2a42`

[WebXR] Port enum classes to new serialization format
https://bugs.webkit.org/show_bug.cgi?id=265665

Reviewed by Dan Glastonbury.

Move the XR enums to the new serialization format to be able to get
rid of the enum traits for them.

 * Source/WebCore/platform/xr/PlatformXR.h:
 * Source/WebKit/Shared/XR/XRSystem.serialization.in:

Canonical link: https://commits.webkit.org/271466@main

6463484

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-12-01T15:46:26Z

EWS run on previous version of this PR (hash 6fd1d1c)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	❌ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	~~🧪 api-mac~~	❌ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

csaavedra · 2023-12-01T15:48:26Z

This is untested as I don't have the means to build a XR platform. Someone who is actually working on WebXR should try it first, it's possible that it won't work out of the box. Marking as a draft for that reason, but hopefully someone can give me feedback on it.

webkit-early-warning-system · 2023-12-01T17:15:34Z

EWS run on previous version of this PR (hash 42ecf56)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	❌ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-12-01T20:50:09Z

EWS run on current version of this PR (hash 6463484)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

djg · 2023-12-03T22:51:37Z

Source/WebKit/Shared/XR/XRSystem.serialization.in

+    ImmersiveAr,
+};
+
+[Nested] enum class PlatformXR::ReferenceSpaceType : uint8_t {


Q: For my own understanding, why does this one need the [Nested] attribute?

I compiled without [Nested] and it works, so I'd suggest removing it.

My understanding is that you can use Nested if the class is only serialized as part of another class, simplifying a bit the declaration. Removing [Nested] adds the following forward declarations to the GeneratedSerializers.h header, which is not really necessary in this case:

@@ -73,6 +73,9 @@ enum class SessionFeature : uint8_t; enum class SessionMode : uint8_t; #endif #if ENABLE(WEBXR) +enum class ReferenceSpaceType : uint8_t; +#endif +#if ENABLE(WEBXR) enum class VisibilityState : uint8_t; #endif #if ENABLE(WEBXR) @@ -7688,6 +7691,9 @@ template<> bool isValidEnum<PlatformXR:: template<> bool isValidEnum<PlatformXR::SessionMode, void>(uint8_t); #endif #if ENABLE(WEBXR) +template<> bool isValidEnum<PlatformXR::ReferenceSpaceType, void>(uint8_t); +#endif +#if ENABLE(WEBXR) template<> bool isValidEnum<PlatformXR::VisibilityState, void>(uint8_t); #endif #if ENABLE(WEBXR)

@djg Based on the explanation above I am going to merge-as-is, please let me know if you disagree.

djg

djg · 2023-12-03T22:53:20Z

@csaavedra I'll test it for you.

djg

djg · 2023-12-04T03:20:54Z

Source/WebKit/Shared/XR/XRSystem.serialization.in

+    ImmersiveAr,
+};
+
+[Nested] enum class PlatformXR::ReferenceSpaceType : uint8_t {


I compiled without [Nested] and it works, so I'd suggest removing it.

csaavedra · 2023-12-04T09:48:37Z

@csaavedra I'll test it for you.

Thanks btw! :)

https://bugs.webkit.org/show_bug.cgi?id=265665 Reviewed by Dan Glastonbury. Move the XR enums to the new serialization format to be able to get rid of the enum traits for them. * Source/WebCore/platform/xr/PlatformXR.h: * Source/WebKit/Shared/XR/XRSystem.serialization.in: Canonical link: https://commits.webkit.org/271466@main

webkit-commit-queue · 2023-12-04T10:42:26Z

Committed 271466@main (3fb2a42): https://commits.webkit.org/271466@main

Reviewed commits have been landed. Closing PR #21161 and removing active labels.

csaavedra self-assigned this Dec 1, 2023

csaavedra added the WebXR For bugs in WebXR label Dec 1, 2023

csaavedra marked this pull request as draft December 1, 2023 15:47

csaavedra requested a review from djg December 1, 2023 15:49

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 1, 2023

csaavedra removed the merging-blocked Applied to prevent a change from being merged label Dec 1, 2023

csaavedra force-pushed the eng/WebXR-Port-enum-classes-to-new-serialization-format branch from 6fd1d1c to 42ecf56 Compare December 1, 2023 17:15

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 1, 2023

csaavedra removed the merging-blocked Applied to prevent a change from being merged label Dec 1, 2023

csaavedra force-pushed the eng/WebXR-Port-enum-classes-to-new-serialization-format branch from 42ecf56 to 6463484 Compare December 1, 2023 20:49

djg reviewed Dec 3, 2023

View reviewed changes

djg approved these changes Dec 3, 2023

View reviewed changes

djg self-requested a review December 3, 2023 22:53

djg approved these changes Dec 4, 2023

View reviewed changes

csaavedra added the merge-queue Applied to send a pull request to merge-queue label Dec 4, 2023

csaavedra marked this pull request as ready for review December 4, 2023 09:49

webkit-commit-queue force-pushed the eng/WebXR-Port-enum-classes-to-new-serialization-format branch from 6463484 to 3fb2a42 Compare December 4, 2023 10:42

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Dec 4, 2023

webkit-commit-queue merged commit 3fb2a42 into WebKit:main Dec 4, 2023

csaavedra deleted the eng/WebXR-Port-enum-classes-to-new-serialization-format branch December 12, 2023 09:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WebXR] Port enum classes to new serialization format #21161

[WebXR] Port enum classes to new serialization format #21161

csaavedra commented Dec 1, 2023 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Dec 1, 2023 •

edited

Loading

csaavedra commented Dec 1, 2023

webkit-early-warning-system commented Dec 1, 2023 •

edited

Loading

webkit-early-warning-system commented Dec 1, 2023 •

edited

Loading

djg Dec 3, 2023

djg Dec 4, 2023

csaavedra Dec 4, 2023

csaavedra Dec 4, 2023

djg left a comment

djg commented Dec 3, 2023

djg left a comment

djg Dec 4, 2023

csaavedra commented Dec 4, 2023

webkit-commit-queue commented Dec 4, 2023

[WebXR] Port enum classes to new serialization format #21161

[WebXR] Port enum classes to new serialization format #21161

Conversation

csaavedra commented Dec 1, 2023 • edited by webkit-early-warning-system Loading

3fb2a42

webkit-early-warning-system commented Dec 1, 2023 • edited Loading

csaavedra commented Dec 1, 2023

webkit-early-warning-system commented Dec 1, 2023 • edited Loading

webkit-early-warning-system commented Dec 1, 2023 • edited Loading

djg Dec 3, 2023

Choose a reason for hiding this comment

djg Dec 4, 2023

Choose a reason for hiding this comment

csaavedra Dec 4, 2023

Choose a reason for hiding this comment

csaavedra Dec 4, 2023

Choose a reason for hiding this comment

djg left a comment

Choose a reason for hiding this comment

djg commented Dec 3, 2023

djg left a comment

Choose a reason for hiding this comment

djg Dec 4, 2023

Choose a reason for hiding this comment

csaavedra commented Dec 4, 2023

webkit-commit-queue commented Dec 4, 2023

csaavedra commented Dec 1, 2023 •

edited by webkit-early-warning-system

Loading

`3fb2a42`

webkit-early-warning-system commented Dec 1, 2023 •

edited

Loading

webkit-early-warning-system commented Dec 1, 2023 •

edited

Loading

webkit-early-warning-system commented Dec 1, 2023 •

edited

Loading