Bug 173721 - REGRESSION (r218419): 7 leaks in PluginLoadClientPolicies.mm
Summary: REGRESSION (r218419): 7 leaks in PluginLoadClientPolicies.mm
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords:
Depends on: 173472
Blocks:
  Show dependency treegraph
 
Reported: 2017-06-22 09:41 PDT by David Kilzer (:ddkilzer)
Modified: 2017-06-25 16:41 PDT (History)
5 users (show)

See Also:


Attachments
Patch v1 (3.65 KB, patch)
2017-06-22 09:42 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (4.14 KB, patch)
2017-06-22 11:37 PDT, David Kilzer (:ddkilzer)
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2017-06-22 09:41:02 PDT
Use autoreleased methods to create NSMutableDictionary and
NSNumber objects instead of +1 retained methods.

* TestWebKitAPI/Tests/WebKit2Cocoa/PluginLoadClientPolicies.mm:
(TEST):
Comment 1 David Kilzer (:ddkilzer) 2017-06-22 09:42:03 PDT
Created attachment 313638 [details]
Patch v1
Comment 2 David Kilzer (:ddkilzer) 2017-06-22 11:37:55 PDT
Created attachment 313646 [details]
Patch v2
Comment 3 Joseph Pecoraro 2017-06-22 11:56:51 PDT
Comment on attachment 313646 [details]
Patch v2

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

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/PluginLoadClientPolicies.mm:71
> +    RetainPtr<NSMutableDictionary> newPolicies = adoptNS([policies mutableCopy]);

r=me, but I find this way harder to read.

How about switching to ObjC literal syntax? You'd end up with something like:

    RetainPtr<NSMutableDictionary> newPolicies = adoptNS([policies mutableCopy]);
    [newPolicies removeObjectForKey:@"apple.com"];
    newPolicies.get()[@"google.com"] = @{
        @"com.macromedia.Flash Player.plugin": @{
            @"26.0.0.126": @(kWKPluginLoadClientPolicyAllowAlways),
        },
        @"com.apple.QuickTime.plugin": @{
            @"1.0": @(kWKPluginLoadClientPolicyBlock),
            @"1.1": @(kWKPluginLoadClientPolicyAllow),
        },
    };
Comment 4 David Kilzer (:ddkilzer) 2017-06-22 14:03:52 PDT
Comment on attachment 313646 [details]
Patch v2

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

>> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/PluginLoadClientPolicies.mm:71
>> +    RetainPtr<NSMutableDictionary> newPolicies = adoptNS([policies mutableCopy]);
> 
> r=me, but I find this way harder to read.
> 
> How about switching to ObjC literal syntax? You'd end up with something like:
> 
>     RetainPtr<NSMutableDictionary> newPolicies = adoptNS([policies mutableCopy]);
>     [newPolicies removeObjectForKey:@"apple.com"];
>     newPolicies.get()[@"google.com"] = @{
>         @"com.macromedia.Flash Player.plugin": @{
>             @"26.0.0.126": @(kWKPluginLoadClientPolicyAllowAlways),
>         },
>         @"com.apple.QuickTime.plugin": @{
>             @"1.0": @(kWKPluginLoadClientPolicyBlock),
>             @"1.1": @(kWKPluginLoadClientPolicyAllow),
>         },
>     };

I like this better, but it has the (slight) disadvantage of putting all the objects in an autoreleasepool.

I'll switch to adoptNS().get() instead, as we talked about in person.
Comment 5 David Kilzer (:ddkilzer) 2017-06-22 14:36:20 PDT
Committed r218719: <http://trac.webkit.org/changeset/218719>
Comment 6 Darin Adler 2017-06-25 16:41:51 PDT
Comment on attachment 313646 [details]
Patch v2

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

>>> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/PluginLoadClientPolicies.mm:71
>>> +    RetainPtr<NSMutableDictionary> newPolicies = adoptNS([policies mutableCopy]);
>> 
>> r=me, but I find this way harder to read.
>> 
>> How about switching to ObjC literal syntax? You'd end up with something like:
>> 
>>     RetainPtr<NSMutableDictionary> newPolicies = adoptNS([policies mutableCopy]);
>>     [newPolicies removeObjectForKey:@"apple.com"];
>>     newPolicies.get()[@"google.com"] = @{
>>         @"com.macromedia.Flash Player.plugin": @{
>>             @"26.0.0.126": @(kWKPluginLoadClientPolicyAllowAlways),
>>         },
>>         @"com.apple.QuickTime.plugin": @{
>>             @"1.0": @(kWKPluginLoadClientPolicyBlock),
>>             @"1.1": @(kWKPluginLoadClientPolicyAllow),
>>         },
>>     };
> 
> I like this better, but it has the (slight) disadvantage of putting all the objects in an autoreleasepool.
> 
> I'll switch to adoptNS().get() instead, as we talked about in person.

Readability seems *so* much more important than putting a few objects into the autorelease pool.