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
View All Add an attachment (proposed patch, testcase, etc.)

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].