Bug 236135 – Update preference location used for CaptivePortalMode

• Component: WebKit2
• Status: RESOLVED FIXED

[Gavin]

Change the preference location used to determine whether to enable CaptivePortalMode

[Gavin]

<rdar://problem/88486544>

[Gavin]

Created attachment 450887 [details]
Patch

[Darin Adler]

Comment on attachment 450887 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450887&action=review

Looks fine except for the storage leaks.

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:134
> +static const char* const WebKitCaptivePortalModeChangedNotification_Legacy = "WebKitCaptivePortalModeEnabled";
> +static const char* const WebKitCaptivePortalModeChangedNotification = "WKCaptivePortalModeEnabled";
> +static const char* const WebKitCaptivePortalModeIgnoredNotification = "WKCaptivePortalModeIgnored";

In new code it’s better to use constexpr. Instead of "static const char* const" I would write "constexpr auto".

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:956
> +    CFBooleanRef systemValue = dynamic_cf_cast<CFBooleanRef>(CFPreferencesCopyValue(CFStringCreateWithCString(kCFAllocatorDefault, WebKitCaptivePortalModeChangedNotification, kCFStringEncodingUTF8), kCFPreferencesAnyApplication, kCFPreferencesAnyUser, kCFPreferencesCurrentHost));
> +    if (!systemValue || !CFBooleanGetValue(systemValue))
> +        return false;

This has two storage leaks, leaks a string every time and theoretically leaks a preferences value each time, although that is not a problem when the value is a CFBoolean. We must use adoptCF any time we call a CF Copy or Create function. But also, if we want to check for boolean true and only that value, we can do it without converting to CFBoolean. I would write the code like this.

    auto changedNotificationName = adoptCF(CFStringCreateWithCString(kCFAllocatorDefault, WebKitCaptivePortalModeChangedNotification, kCFStringEncodingUTF8));
    if (adoptCF(CFPreferencesCopyValue(changedNotificationName(), kCFPreferencesAnyApplication, kCFPreferencesAnyUser, kCFPreferencesCurrentHost)).get() != kCFBooleanTrue)
        return false;

Since the code is a little bit repetitive we could add a helper function, but I also think the above is OK.

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:960
> +    CFBooleanRef configIgnoreValue = dynamic_cf_cast<CFBooleanRef>(CFPreferencesCopyValue(CFStringCreateWithCString(kCFAllocatorDefault, WebKitCaptivePortalModeIgnoredNotification, kCFStringEncodingUTF8), CFSTR("com.apple.WebKit.cpmconfig"), kCFPreferencesCurrentUser, kCFPreferencesAnyHost));
> +    if (configIgnoreValue && CFBooleanGetValue(configIgnoreValue))
> +        return false;

Ditto.

[Gavin]

Created attachment 451410 [details]
Patch

[Darin Adler]

Comment on attachment 451410 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451410&action=review

Some comments on the WIP patch.

> Source/WebKit/UIProcess/API/C/mac/WKCaptivePortalModePrivateMac.mm:39
> +    auto pref_val = adoptCF(CFPreferencesCopyValue(CFSTR("WKCaptivePortalModeEnabled"), kCFPreferencesAnyApplication, kCFPreferencesAnyUser, kCFPreferencesCurrentHost));
> +    return pref_val.get() == kCFBooleanTrue;

WebKit coding style says local variables would be named value or preferenceValue, not pref_val. But you can also just use the one line style without local variable as below.

> Source/WebKit/UIProcess/API/C/mac/WKCaptivePortalModePrivateMac.mm:41
> +    return false

Need a semicolon. Not sure about ignoring this entirely without the APPLE_INTERNAL_SDK, that’s not our usual approach.

> Source/WebKit/UIProcess/API/C/mac/WKCaptivePortalModePrivateMac.mm:51
> +    return false

Semicolon.

> Source/WebKit/UIProcess/API/C/mac/WKCaptivePortalModePrivateMac.mm:55
> +bool WKCaptivePortalModeGetIgnoredForContainer(NSString* containerPath)

WebKit coding style says write this as "NSString *containerPath".

> Source/WebKit/UIProcess/API/C/mac/WKCaptivePortalModePrivateMac.mm:60
> +    return false

Semicolon.

> Source/WebKit/UIProcess/API/C/mac/WKCaptivePortalModePrivateMac.mm:64
> +bool WKCaptivePortalModeSetIgnoredForContainer(NSString* containerPath, bool ignored)

WebKit coding style says write this as "NSString *containerPath".

> Source/WebKit/UIProcess/API/C/mac/WKCaptivePortalModePrivateMac.mm:70
> +    return false

Semicolon.

[Gavin]

Created attachment 451570 [details]
Patch

[Chris Dumez]

Comment on attachment 451570 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451570&action=review

Looks like there are some build issues on iOS.

> Source/WebKit/UIProcess/API/Cocoa/_WKSystemPreferences.h:32
> ++ (BOOL) _isCaptivePortalModeEnabledGlobally;

Looking at our other headers, looks like we usually don't have a space between (BOOL) and the method name.

> Source/WebKit/UIProcess/API/Cocoa/_WKSystemPreferences.h:35
> ++ (BOOL) _setCaptivePortalModeIgnoredForContainer:(NSString*)containerPath :(BOOL)ignore;

Somebody more familiar with our APIs and Objective C will have to comment on this. I have never seen an unnamed parameter in ObjC :)

> Source/WebKit/UIProcess/API/Cocoa/_WKSystemPreferences.mm:31
> +#if USE(APPLE_INTERNAL_SDK)

Need a blank line before this.

But also, why do we need this guard, what API is not part of the public SDK? We use CFPreferences API elsewhere in WebKit without guards.

Also, I assume the NSUserDefaults ObjC API is insufficient here for some reason and that's why you have to use CFPreferences?

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:630
> +    auto pool = extractWebProcessPool(observer);

I think this would look nicer like so:
```
if (auto pool = extractWebProcessPool(observer))
    pool->captivePortalModeStateChanged();
```

[Darin Adler]

Comment on attachment 451570 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451570&action=review

> Source/WebKit/UIProcess/API/Cocoa/_WKSystemPreferences.h:37
> +WK_CLASS_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA))
> +@interface _WKSystemPreferences : NSObject
> +
> ++ (BOOL) _isCaptivePortalModeEnabledGlobally;
> ++ (BOOL) _setCaptivePortalModeEnabledGlobally:(BOOL)enabled;
> ++ (BOOL) _isCaptivePortalModeIgnoredForContainer:(NSString*)containerPath;
> ++ (BOOL) _setCaptivePortalModeIgnoredForContainer:(NSString*)containerPath :(BOOL)ignore;
> +
> +@end

Coding style issue:

Not sure we are adding value by including all these underscores. If the class is internal and only intended for use inside WebKit, then OK, use an underscore. If it’s intended for outside WebKit, then no underscore is needed. Please discuss with the people who think about WebKit public, private, and internal things.

Within a class that is itself not API, no reason to make the methods have a leading underscore.

Also, unnamed "ignore" argument for setCaptivePortalModeIgnoredForContainer is not right. Should follow Cocoa method naming guidelines for such things.

Finally, "NSString*" -> "NSString *" in WebKit code and especially in public APIs.

Non-obvious that these setters can fail and the BOOL they return indicates whether they succeeded or not. May need a comment. Also, will any callers really take advantage of this? If not, then it’s a bit innovative and confusing, but for no benefit.

> Source/WebKit/UIProcess/API/Cocoa/_WKSystemPreferences.mm:62
> +    return [[NSFileManager defaultManager] fileExistsAtPath:cpmconfigIgnoreFilePath] == TRUE;

Should not write "== TRUE" if we are returning a bool.

[Gavin]

Created attachment 451590 [details]
Patch

[Gavin]

Created attachment 451594 [details]
Patch

[Geoffrey Garen]

Comment on attachment 451594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451594&action=review

The nice thing about putting the strings in constants is that it's a compile time guarantee against typo. A secondary nicety is that the strings themselves are a little non-obvious.

> Source/WebKit/UIProcess/API/Cocoa/_WKSystemPreferences.mm:37
> +    auto preferenceValue = adoptCF(CFPreferencesCopyValue(CFSTR("WKCaptivePortalModeEnabled"), kCFPreferencesAnyApplication, kCFPreferencesAnyUser, kCFPreferencesCurrentHost));

Would be nice to put "WKCaptivePortalModeEnabled" in a constant in the header.

> Source/WebKit/UIProcess/API/Cocoa/_WKSystemPreferences.mm:55
> +    NSString *cpmconfigIgnoreFilePath = [containerPath stringByAppendingPathComponent:@"Library/Preferences/com.apple.WebKit.cpmconfig_ignore"];

Would be nice to put @"Library/Preferences/com.apple.WebKit.cpmconfig_ignore" in a constant at the top of the file.

> Source/WebKit/UIProcess/API/Cocoa/_WKSystemPreferences.mm:75
> +    CFNotificationCenterPostNotification(CFNotificationCenterGetDarwinNotifyCenter(), CFSTR("com.apple.WebKit.cpmconfig"), nullptr, nullptr, true);

Would be nice to put "com.apple.WebKit.cpmconfig" in a constant in the header.

[Gavin]

Created attachment 451602 [details]
Patch

[Gavin]

Created attachment 451604 [details]
Patch

[Gavin]

Created attachment 451606 [details]
Patch

[Geoffrey Garen]

Comment on attachment 451606 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451606&action=review

> Source/WebKit/UIProcess/API/Cocoa/_WKSystemPreferences.mm:38
> +#if USE(APPLE_INTERNAL_SDK)

Looks pretty good to me.

I think this is the one piece of feedback not yet addressed. Chris had asked:

But also, why do we need this guard, what API is not part of the public SDK? We use CFPreferences API elsewhere in WebKit without guards.

> Source/WebKit/UIProcess/API/Cocoa/_WKSystemPreferences.mm:59
> +    NSString *cpmconfigIgnoreFilePath = [containerPath stringByAppendingPathComponent:CaptivePortalConfigurationIgnoreFilePath];

I think you could do pathWithComponents:@{ containerPath, @"Library/Preferences/", WebKitCaptivePortalModeContainerConfigurationChangedNotification }. (And then no need for CaptivePortalConfigurationIgnoreFilePath.)

FWIW, I like how that makes explicit how we put together the path.

> Source/WebKit/UIProcess/API/Cocoa/_WKSystemPreferencesInternal.h:29
> +constexpr auto WebKitCaptivePortalModeGlobalConfigurationChangedNotification = "WKCaptivePortalModeEnabled";
> +constexpr auto WebKitCaptivePortalModeContainerConfigurationChangedNotification = @"com.apple.WebKit.cpmconfig";

Apologies, I didn't fully grasp before that these were change notifications. We usually try to make those strings more symbolically similar to the constant names. (See, e.g., AXObjectCacheMac.mm.)

Would this work, or does it conflict with something else?

constexpr auto WKCaptivePortalModeGlobalConfigurationChangedNotification = "WKCaptivePortalModeGlobalConfigurationChangedNotification";
constexpr auto WKCaptivePortalModeContainerConfigurationChangedNotification = @"WKCaptivePortalModeContainerConfigurationChangedNotification";

(I subbed in WK because that's WebKit's public prefix.)

[Gavin]

Comment on attachment 451606 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451606&action=review

>> Source/WebKit/UIProcess/API/Cocoa/_WKSystemPreferences.mm:38
>> +#if USE(APPLE_INTERNAL_SDK)
> 
> Looks pretty good to me.
> 
> I think this is the one piece of feedback not yet addressed. Chris had asked:
> 
> But also, why do we need this guard, what API is not part of the public SDK? We use CFPreferences API elsewhere in WebKit without guards.

I agree, I think this can now be removed. I'll address this.

>> Source/WebKit/UIProcess/API/Cocoa/_WKSystemPreferences.mm:59
>> +    NSString *cpmconfigIgnoreFilePath = [containerPath stringByAppendingPathComponent:CaptivePortalConfigurationIgnoreFilePath];
> 
> I think you could do pathWithComponents:@{ containerPath, @"Library/Preferences/", WebKitCaptivePortalModeContainerConfigurationChangedNotification }. (And then no need for CaptivePortalConfigurationIgnoreFilePath.)
> 
> FWIW, I like how that makes explicit how we put together the path.

We want the file name to have a somewhat fixed format which doesn't align with your suggestion for the notification name. That said, I think pathWithComponents is much clearer - so I'll address this this too but with a new FileName variable if that's OK (This filename and notification name don't have to match).

[Geoffrey Garen]

> We want the file name to have a somewhat fixed format which doesn't align
> with your suggestion for the notification name. That said, I think
> pathWithComponents is much clearer - so I'll address this this too but with
> a new FileName variable if that's OK (This filename and notification name
> don't have to match).

Sure, sounds good!

[Gavin]

Created attachment 451617 [details]
Patch

[Gavin]

Created attachment 451620 [details]
Patch

[Gavin]

Created attachment 451746 [details]
Patch

[Geoffrey Garen]

Comment on attachment 451746 [details]
Patch

r=me

Looks good!

[Geoffrey Garen]

Follow-up thought: probably good to write some basic "API tests". That would mean adding a couple cases to Tools/TestWebKitAPI that set and get these preferences in basic ways.

[Geoffrey Garen]

...Not super sure at first glance if that's possible without accidentally changing the behavior of the surrounding device.

[Brent Fulgham]

Comment on attachment 451746 [details]
Patch

Looks great!

[EWS]

Committed r289702 (247187@main): <https://commits.webkit.org/247187@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 451746 [details].