Bug 234306 - Simplify accesses to SystemVersion.plist
Summary: Simplify accesses to SystemVersion.plist
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Keywords: InRadar
Depends on:
Reported: 2021-12-14 12:38 PST by David Quesada
Modified: 2022-01-24 10:01 PST (History)
7 users (show)

See Also:

Patch (7.97 KB, patch)
2021-12-14 13:09 PST, David Quesada
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Quesada 2021-12-14 12:38:57 PST
WebCore::createSystemMarketingVersion() and WebKit::systemVersionPlist() have convoluted logic to build the path to SystemVersion.plist and load its contents. Those call sites should be replaced with usage of SPI such as _CFCopySystemVersionDictionary() (which is already being used in PluginBlocklist and WebGLBlocklist)
Comment 1 David Quesada 2021-12-14 13:09:35 PST
Created attachment 447149 [details]
Comment 2 Radar WebKit Bug Importer 2021-12-21 12:39:17 PST
Comment 3 EWS 2022-01-24 09:45:25 PST
Committed r288451 (246340@main): <https://commits.webkit.org/246340@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447149 [details].
Comment 4 Darin Adler 2022-01-24 10:01:37 PST
Comment on attachment 447149 [details]

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

> Source/WebCore/platform/cocoa/SystemVersion.mm:27
>  #import "SystemVersion.h"
> +#import <pal/spi/cf/CFUtilitiesSPI.h>

WebKit coding style says there should be a blank line after the file’s own header before the other includes.

> Source/WebCore/platform/cocoa/SystemVersion.mm:35
> +    CFStringRef productVersion = static_cast<CFStringRef>(CFDictionaryGetValue(systemVersionDictionary.get(), _kCFSystemVersionProductVersionKey));

Typically in newly-written WebKit code we would use auto for the type rather than repeating the same type in both on the left and in the static_cast, and either checked_cf_cast or dynamic_cf_cast instead of static_cast.

> Source/WebCore/platform/cocoa/SystemVersion.mm:36
> +    return adoptNS([(__bridge NSString *)productVersion copy]);

WebKit has bridge_cast for this:

    return adoptNS([bridge_cast(productVersion) copy]);

> Source/WebKit/UIProcess/Inspector/mac/WebInspectorUIProxyMac.mm:793
> +    NSDictionary *plist = adoptCF(_CFCopySystemVersionDictionary()).bridgingAutorelease();

We’d like to avoid introducing any autorelease, and there’s no need for it here, we can have a RetainPtr local variable.

    auto plistCF = adoptCF(_CFCopySystemVersionDictionary());
    auto plist = bridge_cast(plistCF.get());

> Source/WebKit/UIProcess/Inspector/mac/WebInspectorUIProxyMac.mm:799
> +    result.targetBuildVersion = plist[static_cast<NSString *>(_kCFSystemVersionBuildVersionKey)];
> +    result.targetProductVersion = plist[static_cast<NSString *>(_kCFSystemVersionProductUserVisibleVersionKey)];