Bug 194362

Created attachment 361333 [details] Patch

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.

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.

(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!

Created attachment 361450 [details] Patch

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

> > 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.

(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.

<rdar://problem/47920880>