REGRESSION (299884@main): [macOS] Safari AutoFill filling unit tests are hanging

whsieh · whsieh · commit 1a86d2229137 · 2025-09-23T12:23:45.000-07:00
https://bugs.webkit.org/show_bug.cgi?id=299379 rdar://160980482 Reviewed by Richard Robinson and Abrar Rahman Protyasha. Partially revert logic introduced in 299884@main, which observes changes to all user defaults in order to update smart lists enablement. This was only necessary for the scenario where the user default is modified by the user or app, through the command line or `NSUserDefaults` API. Importantly, these user defaults still persist across app launches, and also update correctly as the user uses the context menu or other UI that's integrated with `-[WKWebView _toggleSmartLists:]`. Test: Tools/TestWebKitAPI/Tests/WebKitCocoa/SmartLists.mm * Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm: (WebKit::WebProcessPool::registerNotificationObservers): (WebKit::WebProcessPool::unregisterNotificationObservers): * Tools/TestWebKitAPI/Tests/WebKitCocoa/SmartLists.mm: (TEST(SmartLists, EnablementIsLogicallyConsistentWhenInterfacedThroughResponder)): Adjust an API test by moving the `user default => nil, preference => true` test case up to the start, when the user default is (initially) set to `nil`. Canonical link: https://commits.webkit.org/300414@main
diff --git a/Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm b/Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm
@@ -854,17 +854,6 @@ static void logProcessPoolState(const WebProcessPool& pool)
         textCheckerStateChanged();
     }];
 
-    // FIXME: This isn't that ideal since it listens to every user default change and not just the smart lists default.
-    m_smartListsNotificationObserver = [[NSNotificationCenter defaultCenter] addObserverForName:NSUserDefaultsDidChangeNotification object:nil queue:[NSOperationQueue currentQueue] usingBlock:^(NSNotification *notification) {
-        TextChecker::didChangeSmartListsEnabled();
-
-        auto newValue = TextChecker::state().contains(TextCheckerState::SmartListsEnabled);
-        if (std::exchange(m_smartListsEnabled, newValue) == newValue)
-            return;
-
-        textCheckerStateChanged();
-    }];
-
     m_accessibilityDisplayOptionsNotificationObserver = [[NSWorkspace.sharedWorkspace notificationCenter] addObserverForName:NSWorkspaceAccessibilityDisplayOptionsDidChangeNotification object:nil queue:[NSOperationQueue currentQueue] usingBlock:^(NSNotification *notification) {
         screenPropertiesChanged();
     }];
@@ -1023,7 +1012,6 @@ static void logProcessPoolState(const WebProcessPool& pool)
     [[NSNotificationCenter defaultCenter] removeObserver:m_automaticSpellingCorrectionNotificationObserver.get()];
     [[NSNotificationCenter defaultCenter] removeObserver:m_automaticQuoteSubstitutionNotificationObserver.get()];
     [[NSNotificationCenter defaultCenter] removeObserver:m_automaticDashSubstitutionNotificationObserver.get()];
-    [[NSNotificationCenter defaultCenter] removeObserver:m_smartListsNotificationObserver.get()];
     [[NSWorkspace.sharedWorkspace notificationCenter] removeObserver:m_accessibilityDisplayOptionsNotificationObserver.get()];
     [[NSNotificationCenter defaultCenter] removeObserver:m_scrollerStyleNotificationObserver.get()];
     [[NSNotificationCenter defaultCenter] removeObserver:m_deactivationObserver.get()];
diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/SmartLists.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/SmartLists.mm
@@ -126,6 +126,8 @@ static void runTest(NSString *input, NSString *expectedHTML, NSString *expectedS
 
 TEST(SmartLists, EnablementIsLogicallyConsistentWhenInterfacedThroughResponder)
 {
+    resetUserDefaults();
+
     RetainPtr configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
     RetainPtr webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectZero configuration:configuration.get()]);
     [webView synchronouslyLoadHTMLString:@"<div>hi</div>"];
@@ -134,7 +136,6 @@ static void runTest(NSString *input, NSString *expectedHTML, NSString *expectedS
     // Case 1: user default => nil, preference => false
 
     setSmartListsPreference(configuration.get(), NO);
-    resetUserDefaults();
 
     EXPECT_FALSE([webView _isSmartListsEnabled]);
     EXPECT_NULL(userDefaultsValue());
@@ -143,22 +144,9 @@ static void runTest(NSString *input, NSString *expectedHTML, NSString *expectedS
     EXPECT_FALSE([webView _isSmartListsEnabled]);
     EXPECT_NULL(userDefaultsValue());
 
-    // Case 2: user default => true, preference => false
-
-    setSmartListsPreference(configuration.get(), NO);
-    setUserDefaultsValue(YES);
-
-    EXPECT_FALSE([webView _isSmartListsEnabled]);
-    EXPECT_TRUE([userDefaultsValue() boolValue]);
-
-    [webView _setSmartListsEnabled:YES];
-    EXPECT_FALSE([webView _isSmartListsEnabled]);
-    EXPECT_TRUE([userDefaultsValue() boolValue]);
-
-    // Case 3: user default => nil, preference => true
+    // Case 2: user default => nil, preference => true
 
     setSmartListsPreference(configuration.get(), YES);
-    resetUserDefaults();
 
     EXPECT_TRUE([webView _isSmartListsEnabled]);
     EXPECT_NULL(userDefaultsValue());
@@ -171,6 +159,18 @@ static void runTest(NSString *input, NSString *expectedHTML, NSString *expectedS
     EXPECT_TRUE([webView _isSmartListsEnabled]);
     EXPECT_TRUE([userDefaultsValue() boolValue]);
 
+    // Case 3: user default => true, preference => false
+
+    setSmartListsPreference(configuration.get(), NO);
+    setUserDefaultsValue(YES);
+
+    EXPECT_FALSE([webView _isSmartListsEnabled]);
+    EXPECT_TRUE([userDefaultsValue() boolValue]);
+
+    [webView _setSmartListsEnabled:YES];
+    EXPECT_FALSE([webView _isSmartListsEnabled]);
+    EXPECT_TRUE([userDefaultsValue() boolValue]);
+
     // Case 4: user default => true, preference => true
 
     setSmartListsPreference(configuration.get(), YES);
