Bug 230085

Summary: [iOS] Ensure the CA transaction has been synchronously committed before snapshotting it
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: ImagesAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, ggaren, sabouhallawa, sihui_liu, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=227318
https://bugs.webkit.org/show_bug.cgi?id=226257
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
thorton: review+
Patch none

Description Said Abou-Hallawa 2021-09-08 23:52:01 PDT 
Committing the CA transaction may be an asynchronous operation.
Comment 1 Said Abou-Hallawa 2021-09-08 23:58:23 PDT 
<rdar://81800118>
Comment 2 Said Abou-Hallawa 2021-09-09 00:00:51 PDT 
Created attachment 437714 [details]
Patch
Comment 3 Tim Horton 2021-09-09 00:11:08 PDT 
Comment on attachment 437714 [details]
Patch

This seems like wild overkill for 99.9% of snapshotting use (which is snapshotting the currently displayed content). Is it really ok to -[CATransaction synchronize] every navigation, for example? (When we take back/forward snapshots)
Comment 4 Tim Horton 2021-09-09 00:19:18 PDT 
Comment on attachment 437714 [details]
Patch

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

> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:3208
> +        [CATransaction synchronize];

At a /minimum/, it seems like you need to not do this if WKSnapshotConfiguration.afterScreenUpdates=NO (which currently isn't plumbed in here but could be, and is one way to avoid it in the swipe snapshot case)
Comment 5 Said Abou-Hallawa 2021-09-09 23:16:50 PDT 
Created attachment 437838 [details]
Patch
Comment 6 Alexey Proskuryakov 2021-09-12 14:07:26 PDT 
Is this likely to fix any flaky tests? :)
Comment 7 Wenson Hsieh 2021-09-13 13:22:42 PDT 
Comment on attachment 437838 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:493
> +- (void)_snapshotRectWithConfiguration:(WKSnapshotConfiguration *)snapshotConfiguration rectInViewCoordinates:(CGRect)rectInViewCoordinates intoImageOfWidth:(CGFloat)imageWidth completionHandler:(void(^)(CGImageRef))completionHandler;

Note that Mail on iOS still uses this method, so we'll need to keep the old SPI around for source/binary compatibility.

(The new method should include a `WK_API_AVAILABLE(…)`, as well)
Comment 8 Simon Fraser (smfr) 2021-09-13 13:28:56 PDT 
Comment on attachment 437838 [details]
Patch

r- because it changes SPI
Comment 9 Said Abou-Hallawa 2021-09-13 18:33:33 PDT 
Created attachment 438096 [details]
Patch
Comment 10 Said Abou-Hallawa 2021-10-18 18:03:48 PDT 
Created attachment 441666 [details]
Patch
Comment 11 Tim Horton 2021-10-19 15:23:03 PDT 
Comment on attachment 441666 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:493
> +- (void)_snapshotRectWithAfterScreenUpdates:(BOOL)afterScreenUpdates rectInViewCoordinates:(CGRect)rectInViewCoordinates intoImageOfWidth:(CGFloat)imageWidth completionHandler:(void(^)(CGImageRef))completionHandler WK_API_AVAILABLE(ios(WK_IOS_TBA));

drop the "with"
Comment 12 Said Abou-Hallawa 2021-10-19 18:37:06 PDT 
Created attachment 441832 [details]
Patch
Comment 13 EWS 2021-10-20 10:30:38 PDT 
Committed r284547 (243289@main): <https://commits.webkit.org/243289@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441832 [details].