Bug 115483

Summary: [WK2][Mac] Pass plug-in bundle ID to LaunchServices
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: WebKit2Assignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, darin
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch darin: review+

Description Alexey Proskuryakov 2013-05-01 11:39:02 PDT
<rdar://problem/13600073>
Comment 1 Alexey Proskuryakov 2013-05-01 11:43:22 PDT
Created attachment 200230 [details]
proposed patch
Comment 2 Darin Adler 2013-05-01 13:34:36 PDT
Comment on attachment 200230 [details]
proposed patch

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

> Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm:47
> +const CFStringRef _kLSPluginBundleIdentifierKey = CFSTR("LSPluginBundleIdentifierKey");

Why the underscore? Don’t we need “static” on this to prevent it from having external linkage?

> Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm:285
> +static String loadSandboxProfileForDirectory(String bundleIdentifier, NSString *sandboxProfileDirectoryPath)

Why the change from const String& to String for the argument type? Please use const String&.
Comment 3 Alexey Proskuryakov 2013-05-01 14:20:02 PDT
Comment on attachment 200230 [details]
proposed patch

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

>> Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm:47
>> +const CFStringRef _kLSPluginBundleIdentifierKey = CFSTR("LSPluginBundleIdentifierKey");
> 
> Why the underscore? Don’t we need “static” on this to prevent it from having external linkage?

I guess I was just thinking of matching other similar keys. No need to do that here indeed.

In C++, constants do not have external linkage, so we do not need "static" here.

>> Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm:285
>> +static String loadSandboxProfileForDirectory(String bundleIdentifier, NSString *sandboxProfileDirectoryPath)
> 
> Why the change from const String& to String for the argument type? Please use const String&.

I'm modifying the argument in the function.

I think that asking the caller to do the copy is a fairly common and desirable pattern, as this gives the compiler more opportunities for optimization.
Comment 4 Alexey Proskuryakov 2013-05-06 11:39:50 PDT
Committed <http://trac.webkit.org/r149619>.
Comment 5 Alexey Proskuryakov 2013-05-06 11:40:44 PDT
Went with const String&. While I think that a String is just fine, we should probably start using this idiom in some place where it demonstrably improves performance, not here.