Bug 233015 – [Cocoa] Web Inspector: fix bundle identifier lookup for enabling remote inspection

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

.

[Blaze Burg]

<rdar://85277115>

[Blaze Burg]

Created attachment 444001 [details]
Patch v1.0

[Blaze Burg]

Created attachment 444008 [details]
Patch v1.1

[Darin Adler]

Comment on attachment 444008 [details]
Patch v1.1

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

> Source/WebKit/UIProcess/Inspector/WebInspectorUtilities.h:48
> +String bundleIdentifierForSandboxBroker();

I think this should return a CFStringRef. Also, could return ASCIILiteral if you really want to stick with WTF instead of platform-specific.

[Patrick Angle]

Comment on attachment 444008 [details]
Patch v1.1

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

> Source/WebKit/UIProcess/Inspector/WebInspectorUtilities.cpp:119
> +    auto sandboxBrokerBundleIdentifier = "com.apple.Safari.SandboxBroker"_s;

Nit: Now that this logic is in a standalone function, can we just return the correct string inside the conditionals instead of setting a local variable?

[Blaze Burg]

Created attachment 444296 [details]
Patch v1.2

[Darin Adler]

Comment on attachment 444296 [details]
Patch v1.2

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

> Source/WebKit/UIProcess/Cocoa/WebInspectorPreferenceObserver.mm:53
> +    String sandboxBrokerBundleIdentifier = WebKit::bundleIdentifierForSandboxBroker();

I suggest not involving WTF::String at all:

    auto sandboxBrokerBundleIdentifier = WebKit::bundleIdentifierForSandboxBroker();
    m_userDefaults = adoptNS([[NSUserDefaults alloc] initWithSuiteName:bridge_cast(sandboxBrokerBundleIdentifier)]);
    if (!m_userDefaults) {
        WTFLogAlways("Could not init user defaults instance for domain %@.", sandboxBrokerBundleIdentifier);

[Darin Adler]

Comment on attachment 444296 [details]
Patch v1.2

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

> Source/WebKit/UIProcess/Inspector/WebInspectorUtilities.cpp:122
> +    if (WebCore::applicationBundleIdentifier() == "com.apple.SafariTechnologyPreview"_s)
> +        return CFSTR("com.apple.SafariTechnologyPreview.SandboxBroker");
> +    if (WebCore::applicationBundleIdentifier() == "com.apple.Safari.automation"_s)
> +        return CFSTR("com.apple.Safari.automation.SandboxBroker");

One other thought: Since bundle identifiers are not case sensitive, it might be safer to call equalLettersIgnoringASCIICase instead of using ==.

[Blaze Burg]

Created attachment 445440 [details]
Patch for Landing

[Joseph Pecoraro]

Comment on attachment 445440 [details]
Patch for Landing

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

> Source/WebKit/UIProcess/Inspector/WebInspectorUtilities.cpp:119
> +    if (WebCore::applicationBundleIdentifier() == "com.apple.SafariTechnologyPreview"_s)

Perhaps avoid calling `WebCore::applicationBundleIdentifier` twice with a local?

Is the result always `applicationBundleIdentifier() + ".SandboxBroker"?

[EWS]

Committed r286314 (244673@main): <https://commits.webkit.org/244673@main>

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