Bug 194362 – Add afterScreenUpdates to WKSnapshotConfiguration

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Beth Dakin]

<rdar://problem/40655528> Please add an "after screen updates" property to WKSnapshotConfiguration (to solve blank snapshots)

[Beth Dakin]

Created attachment 361333 [details]
Patch

[EWS Watchlist]

Attachment 361333 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1159:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 361333 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKSnapshotConfiguration.h:51
> +@property (readwrite) BOOL afterScreenUpdates WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

nonatomic, and you don't need the "readwrite"

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1159
> +- (void)_callCompletionHandler:(BlockPtr<void (UIImage *, NSError *)>)completionHandler withSnapshotImage:(CGImageRef)snapshotImage atDeviceScale:(CGFloat)deviceScale

_callCompletionHandler:withSnapshotImage:atDeviceScale: is a pretty odd name for a WKWebView method (mostly because _callCompletionHandler at the beginning is super vague)

Maybe this is _didTakeSnapshot:atDeviceScale:completionHandler:? But also maybe it doesn't need to be a WKWebView method at all? You could just shove this in a local block inside takeSnapshotWithConfiguration and call it from the two call sites... that would be a bit less weird.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1193
> +            [strongSelf _snapshotRect:rectInViewCoordinates intoImageOfWidth:(snapshotWidth * deviceScale) completionHandler:[strongSelf, handler, deviceScale](CGImageRef snapshotImage) {
> +                if (!strongSelf)
> +                    return;
> +                [strongSelf _callCompletionHandler:handler withSnapshotImage:snapshotImage atDeviceScale:deviceScale];
> +            }];

Aren't all five of these lines shared between the two cases? We should share more! I think the lambda thing above is probably the way to go, but it can encapsulate all of these lines too.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewSnapshot.mm:301
> +    // or without afterScreeUpdates set on the configuration. On device, afterScreeUpdates will be required to

scree! twice.

[Beth Dakin]

(In reply to Tim Horton from comment #3)
> Comment on attachment 361333 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361333&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKSnapshotConfiguration.h:51
> > +@property (readwrite) BOOL afterScreenUpdates WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> nonatomic, and you don't need the "readwrite"

Thanks, fixed!

> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1159
> > +- (void)_callCompletionHandler:(BlockPtr<void (UIImage *, NSError *)>)completionHandler withSnapshotImage:(CGImageRef)snapshotImage atDeviceScale:(CGFloat)deviceScale
> 
> _callCompletionHandler:withSnapshotImage:atDeviceScale: is a pretty odd name
> for a WKWebView method (mostly because _callCompletionHandler at the
> beginning is super vague)
> 
> Maybe this is _didTakeSnapshot:atDeviceScale:completionHandler:? But also
> maybe it doesn't need to be a WKWebView method at all? You could just shove
> this in a local block inside takeSnapshotWithConfiguration and call it from
> the two call sites... that would be a bit less weird.
> 

Yeah, I know. Should be a local block, I was just having a hard time writing it. 🙃

> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1193
> > +            [strongSelf _snapshotRect:rectInViewCoordinates intoImageOfWidth:(snapshotWidth * deviceScale) completionHandler:[strongSelf, handler, deviceScale](CGImageRef snapshotImage) {
> > +                if (!strongSelf)
> > +                    return;
> > +                [strongSelf _callCompletionHandler:handler withSnapshotImage:snapshotImage atDeviceScale:deviceScale];
> > +            }];
> 
> Aren't all five of these lines shared between the two cases? We should share
> more! I think the lambda thing above is probably the way to go, but it can
> encapsulate all of these lines too.
> 

Fixed! With your help!

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewSnapshot.mm:301
> > +    // or without afterScreeUpdates set on the configuration. On device, afterScreeUpdates will be required to
> 
> scree! twice.

Fixed!

[Beth Dakin]

Created attachment 361450 [details]
Patch

[Tim Horton]

Comment on attachment 361450 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKSnapshotConfiguration.h:51
> +@property (nonatomic) BOOL afterScreenUpdates WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

Obviously need to review this name.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1188
> +    if (snapshotConfiguration && snapshotConfiguration.afterScreenUpdates) {

This could be inverted and be an early return (after calling callSnapshotRect), and then the code below could be outdented one line 🤷‍♂️ don't care

[Geoffrey Garen]

> > Source/WebKit/UIProcess/API/Cocoa/WKSnapshotConfiguration.h:51
> > +@property (nonatomic) BOOL afterScreenUpdates WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> Obviously need to review this name.

Hush. It is perfect.

[Beth Dakin]

(In reply to Tim Horton from comment #6)
> Comment on attachment 361450 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361450&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKSnapshotConfiguration.h:51
> > +@property (nonatomic) BOOL afterScreenUpdates WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> Obviously need to review this name.
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1188
> > +    if (snapshotConfiguration && snapshotConfiguration.afterScreenUpdates) {
> 
> This could be inverted and be an early return (after calling
> callSnapshotRect), and then the code below could be outdented one line 🤷‍♂️
> don't care

Yes, actually! Not only can this be done, but it should be so that a nil snapshotConfiguration has the same result as the default configuration.

[Beth Dakin]

https://trac.webkit.org/changeset/241199/webkit

[Radar WebKit Bug Importer]

<rdar://problem/47920880>