AXNotification enum ought to be an X macro #25805

rkirsling · 2024-03-13T03:30:18Z

`129ce14`

AXNotification enum ought to be an X macro
https://bugs.webkit.org/show_bug.cgi?id=270893

Reviewed by Darin Adler.

AXNotification is a giant and frequently updated enum.
Every time it is updated, exhaustive switch statements must be updated accordingly.

This is a very appropriate place for an X macro --
selfishly, it will help PlayStation port avoid build breaks (since we handle this enum in WKAPICast),
but irrespective of platform, it also means that AXLogger won't need to be updated each time AXNotification is updated.

* Source/WebCore/accessibility/AXLogger.cpp:
(WebCore::operator<<):
* Source/WebCore/accessibility/AXObjectCache.h:

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

f65af2a

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	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim		✅ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-03-13T03:30:34Z

EWS run on previous version of this PR (hash 78460d1)

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	❌ 🛠 wpe-skia
	❌ 🧪 api-ios	❌ 🧪 mac-wk2	❌ 🛠 gtk
	❌ 🛠 tv	❌ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	❌ 🛠 tv-sim	❌ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
	❌ 🛠 watch
	❌ 🛠 watch-sim

webkit-early-warning-system · 2024-03-13T04:47:40Z

EWS run on previous version of this PR (hash 1ebd6fc)

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	❌ 🛠 wpe-skia
	❌ 🧪 api-ios	❌ 🧪 mac-wk2	~~🛠 gtk~~
	❌ 🛠 tv	❌ 🧪 mac-AS-debug-wk2	~~🧪 gtk-wk2~~
	❌ 🛠 tv-sim	❌ 🧪 mac-wk2-stress	~~🧪 api-gtk~~
	❌ 🛠 watch
	❌ 🛠 watch-sim

webkit-early-warning-system · 2024-03-13T04:58:28Z

EWS run on previous version of this PR (hash b0f75c2)

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	✅ 🛠 wpe-skia
	~~🧪 api-ios~~	💥 🧪 mac-wk2	💥 🛠 gtk
	✅ 🛠 tv	❌ 🧪 mac-AS-debug-wk2	💥 🧪 gtk-wk2
	✅ 🛠 tv-sim	💥 🧪 mac-wk2-stress	💥 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

rkirsling · 2024-03-13T05:12:26Z

Background: #23035 (comment)

Basically, it may or may not actually be beneficial for us to upstream our current AX impl, since it differs so much from other platforms, but if we could X-macro-ify AXNotification and AccessibilityRole, it would save us from a lot of build breaks. And since exhaustive switches exist upstream for each of these, it's not just PS that benefits.

webkit-early-warning-system · 2024-03-13T05:28:22Z

EWS run on previous version of this PR (hash 4da9cac)

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	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim		✅ 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

rniwa · 2024-03-13T05:47:13Z

Source/WebCore/accessibility/AXObjectCache.h

@@ -139,6 +139,110 @@ class AccessibilityReplacedText {
    VisiblePositionIndexRange m_replacedRange;
 };

+#define WEBCORE_AXNOTIFICATION_KEYS_DEFAULT(macro) \
+    macro(AccessKeyChanged) \


This looks strictly worse. I don't understand why we want to adopt this change.

I linked the explanation above but I can link it again here: #23035 (comment)

Introducing WebKit/UIProcess/API/C/WKAXTypes.h with enums like...

enum AXNotification { // must be same as WebCore::AXObjectCache::AXNotification kWKAXAccessKeyChanged, ... }; typedef uint32_t WKAXNotification;

...and extending WebKit/UIProcess/API/C/WKAPICast.h with casts like...

inline WKAXNotification toAPI(WebCore::AXObjectCache::AXNotification notification) { switch (notification) { case WebCore::AXObjectCache::AXNotification::AXAccessKeyChanged: return kWKAXAccessKeyChanged; ... } }

...feels rather hard to justify if only one platform is planning to use these casts.

Since the idea is just to ensure that the API casts get updated whenever the WebCore enums get updated, the ideal situation would be to have this happen automatically, without requiring developers to be conscious of it. This automation is achieved by X macros, which allow us to do an exhaustive switch on a enum without being concerned with the specific values of that enum changing. Indeed, forgetting about any platform-specific needs, in AXLogger, we're able to reduce a switch from 286 lines to just 8, and those 8 lines needn't be touched the next time AXNotification changes.

I understand that while this is commonplace in JSC (looks like we may have almost 200 of them), it surely feels quite foreign in WebCore. But I'm not sure of a different tool that fits this problem so well.

Seems like I've overestimated how foreign this actually is outside of JSC. We have X macros defined in...

Source/WebCore/bindings/js/WebCoreBuiltinNames.h

Source/WebCore/cssjit/SelectorCompiler.cpp

Source/WebCore/html/parser/HTMLDocumentParserFastPath.cpp

Source/WebCore/inspector/InspectorCanvasCallTracer.h

Source/WebCore/inspector/InstrumentingAgents.h

Source/WebCore/page/ResourceUsageData.h

Source/WebCore/platform/CommonAtomStrings.h

Source/WebCore/platform/Logging.h

Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h

Source/WebCore/platform/graphics/texmap/TextureMapperShaderProgram.h

Source/WebCore/PAL/pal/Logging.h

Source/WebKit/Platform/Logging.h

Source/WebKit/UIProcess/ios/WKContentViewInteraction.h

Source/WTF/wtf/Logging.h

Source/WTF/wtf/SystemTracing.h

Tools/TestRunnerShared/EventSerialization/mac/EventSerializerMac.mm

Tools/TestWebKitAPI/Tests/WebCore/DisplayListRecorderTests.cpp

...and a few more files which are GTK/WPE-specific.

So I revoke my hedging; this is not only the right tool for the job, it's also a perfectly normal thing for us to do.

webkit-early-warning-system · 2024-03-19T05:57:34Z

EWS run on previous version of this PR (hash 0bc8bf0)

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~~	~~🛠 wpe-skia~~
⏳ 🛠 🧪 jsc	❌ 🧪 api-ios	~~🧪 mac-wk2~~	~~🛠 gtk~~
	❌ 🛠 tv	❌ 🧪 mac-AS-debug-wk2	~~🧪 gtk-wk2~~
	~~🛠 tv-sim~~	~~🧪 mac-wk2-stress~~	~~🧪 api-gtk~~
	~~🛠 watch~~
	❌ 🛠 watch-sim

rkirsling · 2024-03-19T06:01:59Z

Per @darinadler's suggestion on Slack, I've removed the enum class change such that this patch is solely focused on making AXNotification an X macro. (I'm still happy to do the other part in a separate patch if desired.)

webkit-early-warning-system · 2024-03-19T06:05:04Z

EWS run on previous version of this PR (hash bb5fd9b)

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	✅ 🛠 wpe-skia
⏳ 🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim		✅ 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

darinadler · 2024-03-19T14:48:36Z

Source/WebCore/accessibility/AXLogger.cpp

-        stream << "AXTextCompositionEnded";
+#define WEBCORE_LOG_AXNOTIFICATION(name) \
+    case AXObjectCache::AXNotification::name: \
+        stream << #name; \


If we put “AX” #name here then we could leave all the repeated AX out of the macro arguments.

Great idea!

darinadler · 2024-03-19T14:49:49Z

Source/WebCore/accessibility/AXObjectCache.h

-        AXDraggingEnteredDropZone,
-        AXDraggingDropped,
-        AXDraggingExitedDropZone
+#define WEBCORE_DEFINE_AXNOTIFICATION_ENUM(name) name,


And here AX##name.

webkit-early-warning-system · 2024-03-20T03:48:14Z

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

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	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim		✅ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-03-20T03:48:14Z

EWS run on previous version of this PR (hash e9b308b)

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
⏳ 🧪 webkitpy	⏳ 🧪 ios-wk2-wpt	⏳ 🧪 mac-wk1	⏳ 🛠 wpe-skia
⏳ 🛠 🧪 jsc	⏳ 🧪 api-ios	⏳ 🧪 mac-wk2	⏳ 🛠 gtk
⏳ 🛠 🧪 jsc-arm64	⏳ 🛠 tv	⏳ 🧪 mac-AS-debug-wk2	⏳ 🧪 gtk-wk2
⏳ 🧪 services	⏳ 🛠 tv-sim	⏳ 🧪 mac-wk2-stress	⏳ 🧪 api-gtk
❌ 🛠 🧪 merge	⏳ 🛠 watch		⏳ 🛠 jsc-armv7
	⏳ 🛠 watch-sim		⏳ 🧪 jsc-armv7-tests

https://bugs.webkit.org/show_bug.cgi?id=270893 Reviewed by Darin Adler. AXNotification is a giant and frequently updated enum. Every time it is updated, exhaustive switch statements must be updated accordingly. This is a very appropriate place for an X macro -- selfishly, it will help PlayStation port avoid build breaks (since we handle this enum in WKAPICast), but irrespective of platform, it also means that AXLogger won't need to be updated each time AXNotification is updated. * Source/WebCore/accessibility/AXLogger.cpp: (WebCore::operator<<): * Source/WebCore/accessibility/AXObjectCache.h: Canonical link: https://commits.webkit.org/276393@main

webkit-commit-queue · 2024-03-20T07:42:02Z

Committed 276393@main (129ce14): https://commits.webkit.org/276393@main

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

rkirsling requested review from a team, cdumez and rniwa as code owners March 13, 2024 03:30

rkirsling self-assigned this Mar 13, 2024

rkirsling added the Accessibility For bugs related to accessibility. label Mar 13, 2024

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 13, 2024

rkirsling removed the merging-blocked Applied to prevent a change from being merged label Mar 13, 2024

rkirsling force-pushed the eng/AXNotification-ought-to-be-an-X-macroed-enum-class branch from 78460d1 to 1ebd6fc Compare March 13, 2024 04:47

rkirsling force-pushed the eng/AXNotification-ought-to-be-an-X-macroed-enum-class branch from 1ebd6fc to b0f75c2 Compare March 13, 2024 04:58

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 13, 2024

rkirsling removed the merging-blocked Applied to prevent a change from being merged label Mar 13, 2024

rkirsling force-pushed the eng/AXNotification-ought-to-be-an-X-macroed-enum-class branch from b0f75c2 to 4da9cac Compare March 13, 2024 05:28

rniwa reviewed Mar 13, 2024

View reviewed changes

rkirsling requested a review from darinadler March 18, 2024 02:23

rkirsling changed the title ~~AXNotification ought to be an X-macroed enum class~~ AXNotification enum ought to be a X macro Mar 19, 2024

rkirsling force-pushed the eng/AXNotification-ought-to-be-an-X-macroed-enum-class branch from 4da9cac to 0bc8bf0 Compare March 19, 2024 05:57

rkirsling force-pushed the eng/AXNotification-ought-to-be-an-X-macroed-enum-class branch from 0bc8bf0 to bb5fd9b Compare March 19, 2024 06:04

darinadler approved these changes Mar 19, 2024

View reviewed changes

rkirsling changed the title ~~AXNotification enum ought to be a X macro~~ AXNotification enum ought to be an X macro Mar 19, 2024

rkirsling force-pushed the eng/AXNotification-ought-to-be-an-X-macroed-enum-class branch from bb5fd9b to e9b308b Compare March 19, 2024 15:06

rkirsling added safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks merge-queue Applied to send a pull request to merge-queue labels Mar 19, 2024

rkirsling removed the merge-queue Applied to send a pull request to merge-queue label Mar 20, 2024

rkirsling force-pushed the eng/AXNotification-ought-to-be-an-X-macroed-enum-class branch from e9b308b to f65af2a Compare March 20, 2024 03:48

rkirsling added merge-queue Applied to send a pull request to merge-queue and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Mar 20, 2024

webkit-commit-queue force-pushed the eng/AXNotification-ought-to-be-an-X-macroed-enum-class branch from f65af2a to 129ce14 Compare March 20, 2024 07:41

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 20, 2024

webkit-commit-queue merged commit 129ce14 into WebKit:main Mar 20, 2024

rkirsling deleted the eng/AXNotification-ought-to-be-an-X-macroed-enum-class branch March 20, 2024 07:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AXNotification enum ought to be an X macro #25805

AXNotification enum ought to be an X macro #25805

rkirsling commented Mar 13, 2024 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Mar 13, 2024 •

edited

Loading

webkit-early-warning-system commented Mar 13, 2024 •

edited

Loading

webkit-early-warning-system commented Mar 13, 2024 •

edited

Loading

rkirsling commented Mar 13, 2024

webkit-early-warning-system commented Mar 13, 2024 •

edited

Loading

rniwa Mar 13, 2024

rkirsling Mar 13, 2024 •

edited

Loading

rkirsling Mar 18, 2024 •

edited

Loading

webkit-early-warning-system commented Mar 19, 2024 •

edited

Loading

rkirsling commented Mar 19, 2024

webkit-early-warning-system commented Mar 19, 2024 •

edited

Loading

darinadler Mar 19, 2024

rkirsling Mar 19, 2024

darinadler Mar 19, 2024

webkit-early-warning-system commented Mar 20, 2024 •

edited

Loading

webkit-early-warning-system commented Mar 20, 2024 •

edited

Loading

webkit-commit-queue commented Mar 20, 2024

AXNotification enum ought to be an X macro #25805

AXNotification enum ought to be an X macro #25805

Conversation

rkirsling commented Mar 13, 2024 • edited by webkit-early-warning-system Loading

129ce14

webkit-early-warning-system commented Mar 13, 2024 • edited Loading

webkit-early-warning-system commented Mar 13, 2024 • edited Loading

webkit-early-warning-system commented Mar 13, 2024 • edited Loading

rkirsling commented Mar 13, 2024

webkit-early-warning-system commented Mar 13, 2024 • edited Loading

rniwa Mar 13, 2024

Choose a reason for hiding this comment

rkirsling Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

rkirsling Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

webkit-early-warning-system commented Mar 19, 2024 • edited Loading

rkirsling commented Mar 19, 2024

webkit-early-warning-system commented Mar 19, 2024 • edited Loading

darinadler Mar 19, 2024

Choose a reason for hiding this comment

rkirsling Mar 19, 2024

Choose a reason for hiding this comment

darinadler Mar 19, 2024

Choose a reason for hiding this comment

webkit-early-warning-system commented Mar 20, 2024 • edited Loading

webkit-early-warning-system commented Mar 20, 2024 • edited Loading

webkit-commit-queue commented Mar 20, 2024

rkirsling commented Mar 13, 2024 •

edited by webkit-early-warning-system

Loading

`129ce14`

webkit-early-warning-system commented Mar 13, 2024 •

edited

Loading

webkit-early-warning-system commented Mar 13, 2024 •

edited

Loading

webkit-early-warning-system commented Mar 13, 2024 •

edited

Loading

webkit-early-warning-system commented Mar 13, 2024 •

edited

Loading

rkirsling Mar 13, 2024 •

edited

Loading

rkirsling Mar 18, 2024 •

edited

Loading

webkit-early-warning-system commented Mar 19, 2024 •

edited

Loading

webkit-early-warning-system commented Mar 19, 2024 •

edited

Loading

webkit-early-warning-system commented Mar 20, 2024 •

edited

Loading

webkit-early-warning-system commented Mar 20, 2024 •

edited

Loading