| Summary: | [Cocoa] Web Inspector: fix bundle identifier lookup for enabling remote inspection | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | BJ Burg <bburg> | ||||||||||
| Component: | Web Inspector | Assignee: | BJ Burg <bburg> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | bburg, darin, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, pangle, webkit-bug-importer | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
BJ Burg
2021-11-11 14:21:10 PST
Created attachment 444001 [details]
Patch v1.0
Created attachment 444008 [details]
Patch v1.1
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. 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? Created attachment 444296 [details]
Patch v1.2
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); 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 ==. Created attachment 445440 [details]
Patch for Landing
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"? 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]. |