Bug 175562 – Web Automation: use automation session configurations to propagate per-session settings

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

<rdar://problem/30853362>

[Blaze Burg]

Created attachment 318925 [details]
Patch

[Blaze Burg]

Created attachment 318931 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 318931 [details]
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?

[Tim Horton]

👍 as long as you fix joe's complains about the configuration, those were my only comments

[Blaze Burg]

(In reply to Tim Horton from comment #4)
> 👍 as long as you fix joe's complains about the configuration, those were my
> only comments

I'll change to use RetainPtr and autorelease as we do this most places it seems.

[Blaze Burg]

Comment on attachment 318931 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/_WKAutomationSessionConfiguration.mm:47
>> +}
> 
> Is this different from the default -description?

Oops, you are right. I'll just omit this.

[Blaze Burg]

Created attachment 319086 [details]
For Landing

[Joseph Pecoraro]

Comment on attachment 319086 [details]
For Landing

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

> Source/WebKit/UIProcess/Cocoa/AutomationClient.mm:74
> +    NSString *retainedIdentifier = sessionIdentifier;
> +    requestAutomationSessionWithCapabilities(retainedIdentifier, nil);

I think this extra line (73) ends up being unnecessary.

[Blaze Burg]

Committed r221204: <http://trac.webkit.org/changeset/221204>