[webkit-reviews] review granted: [Bug 175562] Web Automation: use automation session configurations to propagate per-session settings : [Attachment 318931] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 175562: Web Automation: use automation session configurations to propagate
per-session settings
https://bugs.webkit.org/show_bug.cgi?id=175562

Attachment 318931: Patch

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




--- Comment #3 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 318931
  --> https://bugs.webkit.org/attachment.cgi?id=318931
Patch

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

r=me, needs a nod from an OWNER

> Source/WebKit/UIProcess/API/Cocoa/_WKAutomationSession.mm:40
> +    _WKAutomationSessionConfiguration *_configuration;

This is auto-synthesized from the @property declaration so is unnecessary.
Unless you want to change the type, for example to
`RetainPtr<_WKAutomationSessionConfiguration *> _configuration`.

> Source/WebKit/UIProcess/API/Cocoa/_WKAutomationSession.mm:56
> +    _configuration = [configuration copy];

This needs to be released in -dealloc. Or assigned into a RetainPtr member
`_configuration = adoptNS([configuration copy]);` which would automatically
handle releasing in destruction.

> Source/WebKit/UIProcess/API/Cocoa/_WKAutomationSession.mm:93
> +- (_WKAutomationSessionConfiguration *)configuration
> +{
> +    return [_configuration copy];
> +}

I don't think you need to implement this. I believe the autosynthesized
property should do the right thing.

In the event that you do need something here (like if you used a RetainPtr
member) WebKit is not ARC this would need an autorelease.

> Source/WebKit/UIProcess/API/Cocoa/_WKAutomationSessionConfiguration.mm:47
> +- (NSString *)description
> +{
> +    return [NSString stringWithFormat:@"<%@: %p>",
NSStringFromClass(self.class), self];
> +}

Is this different from the default -description?