Bug 230085 – [iOS] Ensure the CA transaction has been synchronously committed before snapshotting it

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

Committing the CA transaction may be an asynchronous operation.

[Said Abou-Hallawa]

<rdar://81800118>

[Said Abou-Hallawa]

Created attachment 437714 [details]
Patch

[Tim Horton]

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)

[Tim Horton]

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)

[Said Abou-Hallawa]

Created attachment 437838 [details]
Patch

[Alexey Proskuryakov]

Is this likely to fix any flaky tests? :)

[Wenson Hsieh]

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)

[Simon Fraser (smfr)]

Comment on attachment 437838 [details]
Patch

r- because it changes SPI

[Said Abou-Hallawa]

Created attachment 438096 [details]
Patch

[Said Abou-Hallawa]

Created attachment 441666 [details]
Patch

[Tim Horton]

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"

[Said Abou-Hallawa]

Created attachment 441832 [details]
Patch

[EWS]

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