Bug 211457 - Add the feature flag plist file parser
Summary: Add the feature flag plist file parser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-05 11:22 PDT by Peng Liu
Modified: 2020-05-12 22:35 PDT (History)
4 users (show)

See Also:


Attachments
Patch (11.48 KB, patch)
2020-05-05 11:28 PDT, Peng Liu
thorton: review+
Details | Formatted Diff | Diff
Revised patch based on Tim's comments (14.81 KB, patch)
2020-05-12 19:42 PDT, Peng Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peng Liu 2020-05-05 11:22:38 PDT
Add the feature flag plist file parser
Comment 1 Peng Liu 2020-05-05 11:23:17 PDT
<rdar://problem/60562564>
Comment 2 Peng Liu 2020-05-05 11:28:18 PDT
Created attachment 398531 [details]
Patch
Comment 3 Tim Horton 2020-05-12 17:50:28 PDT
Comment on attachment 398531 [details]
Patch

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

> Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:41
> +bool featureFlagEnabled(const String& featureName)

maybe a leading is-? I'm not sure

> Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:43
> +    BOOL isWebKitBundleFromStageFramework = [[[NSBundle mainBundle] bundlePath] hasPrefix:@"/Library/Apple/System/Library/StagedFrameworks/WebKit"];

"staged", not "stage"

> Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:46
> +        return _os_feature_enabled_impl("WebKit", (const char*)featureName.characters8());

I think you want `.utf8().data()`, no?

> Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:50
> +    if ([[dictionary objectForKey:featureName] objectForKey:@"Enabled"] == nil)

normally we use if (!x) instead of if (x == nil)

> Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:53
> +    return [[[dictionary objectForKey:featureName] objectForKey:@"Enabled"] boolValue];

Do you want to make sure it's an NSNumber before calling -boolValue?
Comment 4 Peng Liu 2020-05-12 19:42:37 PDT
Created attachment 399233 [details]
Revised patch based on Tim's comments
Comment 5 Peng Liu 2020-05-12 22:21:00 PDT
Comment on attachment 398531 [details]
Patch

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

>> Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:41
>> +bool featureFlagEnabled(const String& featureName)
> 
> maybe a leading is-? I'm not sure

Renamed to isFeatureFlagEnabled().

>> Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:43
>> +    BOOL isWebKitBundleFromStageFramework = [[[NSBundle mainBundle] bundlePath] hasPrefix:@"/Library/Apple/System/Library/StagedFrameworks/WebKit"];
> 
> "staged", not "stage"

Fixed.

>> Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:46
>> +        return _os_feature_enabled_impl("WebKit", (const char*)featureName.characters8());
> 
> I think you want `.utf8().data()`, no?

Right, fixed.

>> Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:50
>> +    if ([[dictionary objectForKey:featureName] objectForKey:@"Enabled"] == nil)
> 
> normally we use if (!x) instead of if (x == nil)

Fixed.

>> Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:53
>> +    return [[[dictionary objectForKey:featureName] objectForKey:@"Enabled"] boolValue];
> 
> Do you want to make sure it's an NSNumber before calling -boolValue?

Fixed.
Comment 6 EWS 2020-05-12 22:35:44 PDT
Committed r261599: <https://trac.webkit.org/changeset/261599>

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