[webkit-reviews] review granted: [Bug 132417] [iOS WebKit2] Swipe snapshots should be taken asynchronously : [Attachment 230552] patch

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has granted Tim Horton
<thorton at apple.com>'s request for review:
Bug 132417: [iOS WebKit2] Swipe snapshots should be taken asynchronously
https://bugs.webkit.org/show_bug.cgi?id=132417

Attachment 230552: patch
https://bugs.webkit.org/attachment.cgi?id=230552&action=review

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=230552&action=review


> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:495
> +- (void)_takeViewSnapshot:(WebKit::ViewSnapshot&)snapshot

I think this should return a ViewSnapshot instead.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:3040
> +- (void)_takeViewSnapshot:(ViewSnapshot&)snapshot

I think this should return a ViewSnapshot.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:3074
> +    RetainPtr<CGImageRef> croppedSnapshotImage =
adoptCF(CGImageCreateWithImageInRect(windowSnapshotImage.get(),
NSRectToCGRect([window convertRectToBacking:croppedImageRect])));

auto?

> Source/WebKit2/UIProcess/API/mac/WKViewInternal.h:41
> +struct KeypressCommand;

I think structs should come after the classes.

> Source/WebKit2/UIProcess/WebMemoryPressureHandler.cpp:40
> +    NeverDestroyed<WebMemoryPressureHandler> memoryPressureHandler;

This should be static.

> Source/WebKit2/UIProcess/WebMemoryPressureHandler.cpp:48
> +WebMemoryPressureHandler::WebMemoryPressureHandler()
> +{
> +    memoryPressureHandler().setLowMemoryHandler(handleLowMemory);
> +    memoryPressureHandler().install();
> +}

How does this interact with WebKit1 and coexistence on iOS?

> Source/WebKit2/UIProcess/WebMemoryPressureHandler.h:33
> +    static WebMemoryPressureHandler& ensureSharedHandler();

I think this should just be called shared().

> Source/WebKit2/UIProcess/WebMemoryPressureHandler.h:37
> +    static void handleLowMemory(bool);

This can probably be a lambda.

> Source/WebKit2/UIProcess/mac/ViewSnapshotStore.h:48
> +    uint32_t slot;

I think this should be called slotID. If you want you can just assign = 0 here
and not bother with the constructor.