Bug 175562 - Web Automation: use automation session configurations to propagate per-session settings
Summary: Web Automation: use automation session configurations to propagate per-sessio...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks: 175563
  Show dependency treegraph
 
Reported: 2017-08-14 21:46 PDT by BJ Burg
Modified: 2017-08-25 14:08 PDT (History)
11 users (show)

See Also:


Attachments
Patch (33.20 KB, application/mbox)
2017-08-23 14:51 PDT, BJ Burg
no flags Details
Patch (23.20 KB, patch)
2017-08-23 15:24 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
For Landing (23.13 KB, patch)
2017-08-25 10:06 PDT, BJ Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2017-08-14 21:46:45 PDT
<rdar://problem/30853362>
Comment 1 BJ Burg 2017-08-23 14:51:57 PDT
Created attachment 318925 [details]
Patch
Comment 2 BJ Burg 2017-08-23 15:24:15 PDT
Created attachment 318931 [details]
Patch
Comment 3 Joseph Pecoraro 2017-08-23 15:56:50 PDT
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?
Comment 4 Tim Horton 2017-08-24 17:16:26 PDT
👍 as long as you fix joe's complains about the configuration, those were my only comments
Comment 5 BJ Burg 2017-08-24 17:30:03 PDT
(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.
Comment 6 BJ Burg 2017-08-25 09:44:28 PDT
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.
Comment 7 BJ Burg 2017-08-25 10:06:09 PDT
Created attachment 319086 [details]
For Landing
Comment 8 Joseph Pecoraro 2017-08-25 11:02:29 PDT
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.
Comment 9 BJ Burg 2017-08-25 14:08:19 PDT
Committed r221204: <http://trac.webkit.org/changeset/221204>