[webkit-reviews] review denied: [Bug 236135] Update preference location used for CaptivePortalMode : [Attachment 450887] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Gavin <gavin.p at apple.com>'s request
for review:
Bug 236135: Update preference location used for CaptivePortalMode
https://bugs.webkit.org/show_bug.cgi?id=236135

Attachment 450887: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 450887
  --> https://bugs.webkit.org/attachment.cgi?id=450887
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.