[webkit-reviews] review granted: [Bug 186204] [Cocoa] Web Automation: include browser name and version in listing for automation targets : [Attachment 341803] Proposed Fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 186204: [Cocoa] Web Automation: include browser name and version in listing
for automation targets
https://bugs.webkit.org/show_bug.cgi?id=186204

Attachment 341803: Proposed Fix

https://bugs.webkit.org/attachment.cgi?id=341803&action=review




--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 341803
  --> https://bugs.webkit.org/attachment.cgi?id=341803
Proposed Fix

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

> Source/WebKit/UIProcess/Cocoa/AutomationClient.h:58
> +    String browserName() const override;
> +    String browserVersion() const override;

final instead of override?

> Source/WebKit/UIProcess/Cocoa/AutomationClient.mm:88
> +	   return [m_delegate.get()
_processPoolBrowserNameForAutomation:m_processPool];

No need for the ".get()" here.

> Source/WebKit/UIProcess/Cocoa/AutomationClient.mm:94
> +    NSString *displayName = appBundle.infoDictionary[(__bridge NSString
*)_kCFBundleDisplayNameKey];
> +    NSString *readableName = appBundle.infoDictionary[(__bridge NSString
*)kCFBundleNameKey];
> +    return displayName ?: readableName;

Seems a bit of a shame to compute the readableName when the displayName is
non-null. Maybe omit the local variables?

> Source/WebKit/UIProcess/Cocoa/AutomationClient.mm:100
> +	   return [m_delegate.get()
_processPoolBrowserVersionForAutomation:m_processPool];

No need for the ".get()" here.