Bug 115483 – [WK2][Mac] Pass plug-in bundle ID to LaunchServices

• Component: WebKit2
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

<rdar://problem/13600073>

[Alexey Proskuryakov]

Created attachment 200230 [details]
proposed patch

[Darin Adler]

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

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/r149619>.

[Alexey Proskuryakov]

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.