Bug 161450 – [Cocoa] No reliable way to get a snapshot of WKWebView

• Component: WebKit API
• Status: RESOLVED FIXED

[Dan Saunders]

The cocoa API -[NSView cacheDisplayInRect:(NSRect)rect toBitmapImageRep:(NSBitmapImageRep *)bitmapImageRep] does not work for WKWebView, it only returns an empty image. We need a way to get the visible contents of the web page on macOS. The UI is rendered in a separate process, there should be an API to grab the visible bitmap from the UI process and expose it as a method on WKWebView that the embedding application can consume. We need to be able to grab the web page contents regardless if the WKWebView is hidden or overlapped by other views, CGWindowListCreateImage will not work for that scenario.

For iOS, you can use the -[UIView drawViewHierarchyInRect:(CGRect)rect afterScreenUpdates:(BOOL)afterUpdates] API to get the web page contents.

[Dan Saunders]

Created attachment 288553 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:44:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:65:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:103:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:118:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 6 files


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

[Tim Horton]

Comment on attachment 288553 [details]
Patch

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

Thanks for the patch! Since it adds new public API, the process for getting it landed is a little... different than usual. I'll CC the appropriate people.

> Source/WebKit2/ChangeLog:1
> [phone]  DAN SAUNDERS  <[e-mail]>

You probably want to repair the capitalization of your name.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:228
> +- (void)snapshotViewportWithScaleFactor:(CGFloat)scaleFactor completionHandler:(void(^)(CGImageRef _Nullable snapshotImage, NSError * _Nullable error))completionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

I think I prefer something more like the API that we have for iOS WebKit:

- (void)_snapshotRect:(CGRect)rectInViewCoordinates intoImageOfWidth:(CGFloat)imageWidth completionHandler:(void(^)(CGImageRef))completionHandler;

A rect and a width instead of a scale. This has worked out well for its clients, too.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:885
> +    _page->takeSnapshot(snapshotRect, bitmapSize, WebKit::SnapshotOptionsInViewCoordinates, [handler](const WebKit::ShareableBitmap::Handle& imageHandle, WebKit::CallbackBase::Error errorCode) {

This is a software painted snapshot, meaning that 3D transforms will be flattened and ugly, and video/WebGL may-or-may-not work. So, it's not great.

We have code (WebViewImpl::takeViewSnapshot) that shows how to do a window-server snapshot, which captures all of those things, but it has a downside: if the web view is obscured, the thing it's obscured will will end up in the snapshot as well.

_snapshotRect:intoImageOfWidth:completionHandler: gets around this, but uses functions that are only available/possible on iOS. We should definitely make use of that to implement this method on iOS, though, since it is much, much better than a software snapshot.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:44
> +    [webView snapshotViewportWithScaleFactor:1.0 completionHandler:^(CGImageRef snapshotImage, NSError * error) {

No space between the * and 'error'

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:65
> +    [webView snapshotViewportWithScaleFactor:1.0 completionHandler:^(CGImageRef snapshotImage, NSError * error) {

Ditto.

[Tim Horton]

Comment on attachment 288553 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:223
> +/*! @abstract Get the snapshot for the visible viewport of WKWebView control asynchronously.

The word "control" is definitely not something I would associate with WKWebView.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:226
> + @discussion The completionHandler is passed the image from the UI process or an error.

I don't think this should say "from the UI process" here.

[Dan Saunders]

Comment on attachment 288553 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:885
>> +    _page->takeSnapshot(snapshotRect, bitmapSize, WebKit::SnapshotOptionsInViewCoordinates, [handler](const WebKit::ShareableBitmap::Handle& imageHandle, WebKit::CallbackBase::Error errorCode) {
> 
> This is a software painted snapshot, meaning that 3D transforms will be flattened and ugly, and video/WebGL may-or-may-not work. So, it's not great.
> 
> We have code (WebViewImpl::takeViewSnapshot) that shows how to do a window-server snapshot, which captures all of those things, but it has a downside: if the web view is obscured, the thing it's obscured will will end up in the snapshot as well.
> 
> _snapshotRect:intoImageOfWidth:completionHandler: gets around this, but uses functions that are only available/possible on iOS. We should definitely make use of that to implement this method on iOS, though, since it is much, much better than a software snapshot.

In our scenario, the WKWebView is sometimes overlapped by other views, clipped by a scroll view, or not parented to a window at all but we still need to get a snapshot of the contents. We cannot use CGWindowListCreateImage and crop out the WKWebView because of this limitation. Preferably the snapshot would include video and WebGL, but it is better to have a snapshot resembling the web page in most cases than nothing at all. I will look into changing the implementation for iOS.

[Dan Saunders]

Created attachment 288770 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:913:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:82:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 6 files


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

[Dan Saunders]

I have a few questions while I was coding this patch. I see that -[_snapshotRect:intoImageOfWidth:completionHandler:] passes in WebKit::SnapshotOptionsExcludeDeviceScaleFactor when taking the software snapshot. -[_WKThumbnailView requestSnapshot] is another caller of this method that does not pass this parameter. While testing on OSX with Retina display I did not notice a difference in the output image. In what scenarios will this affect the snapshot?

I am now calling -[_snapshotRect:intoImageOfWidth:completionHandler:] for iOS. When I tried calling this private API on my iOS 10 iPhone device, a WebGL sample page and HTML5 video were not being captured. The HTML overlays and surrounding DOM elements were all that was included in the snapshot. Does iOS 10 webkit.framework include the change to take a hardware snapshot? I see this method was modified a few times in the past year.

I believe the check-webkit-style script is outputting incorrect results for the anonymous callbacks. I see this style being used other locations in the project.
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]

Thanks,
Dan

[Tim Horton]

(In reply to comment #8)
> I have a few questions while I was coding this patch. I see that
> -[_snapshotRect:intoImageOfWidth:completionHandler:] passes in
> WebKit::SnapshotOptionsExcludeDeviceScaleFactor when taking the software
> snapshot. -[_WKThumbnailView requestSnapshot] is another caller of this
> method that does not pass this parameter. While testing on OSX with Retina
> display I did not notice a difference in the output image. In what scenarios
> will this affect the snapshot?

You should get a 1x image if you pass that option, and I'm not sure why _snapshotRect would do that. But it definitely looks like WebPage::snapshotAtSize/scaledSnapshotWithOptions respect that option and don't apply the device scale.

> I am now calling -[_snapshotRect:intoImageOfWidth:completionHandler:] for
> iOS. When I tried calling this private API on my iOS 10 iPhone device, a
> WebGL sample page and HTML5 video were not being captured. The HTML overlays
> and surrounding DOM elements were all that was included in the snapshot.
> Does iOS 10 webkit.framework include the change to take a hardware snapshot?
> I see this method was modified a few times in the past year.

It does. Is the view in-window at the time? That really should work.

> I believe the check-webkit-style script is outputting incorrect results for
> the anonymous callbacks. I see this style being used other locations in the
> project.

Yep, this is a known problem.

> ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:46: 
> Place brace on its own line for function definitions.  [whitespace/braces]
> [4]
> 
> Thanks,
> Dan

[Dan Saunders]

Created attachment 289304 [details]
Snapshot API test case outputs

I did some testing to make it easier for reviewers to see the behavior of this API for different cases, on iOS/Mac. Please refer to snapshot_testing_output.zip for the output images. There are some potential bugs with SnapshotOptionsExcludeDeviceScaleFactor described below, but I believe these are bug level fixes in WebKit and should not impact the design as long as we can agree on the input/outputs expected from this API.

Test application:
All snapshots were taken with a WKWebView that has a width of 700 and a height of 600. I pass the entire viewport size into the snapshotRect API (0, 0, 700, 600) in device independent points. I pass in different bitmap widths and make it clear in the image name the dimensions used. If not specified in the file name, the device had a Retina display.

Test devices:
MacBook Pro with Retina display (El Capitan) calling new API snapshotRect:intoImageOfWidth:completionHandler:
iPad Air 2 with Retina display (iOS 10) calling private API _snapshotRect:intoImageOfWidth:completionHandler:
iPhone 5s simulator (iOS 9.3) calling new API snapshotRect:intoImageOfWidth:completionHandler:
Mac Pro with non-Retina display (El Capitan) calling new API snapshotRect:intoImageOfWidth:completionHandler:

Bitmap size tests:
https://www.google.com
- It seems that through testing and analyzing the code, SnapshotOptionsExcludeDeviceScaleFactor does change the behavior of the snapshot but it is not intuitive. The logic in snapshotAtSize will call graphicsContext->applyDeviceScaleFactor(scaleFactor) and then immediately call the inverse graphicsContext->scale(1/scaleFactor). This means that that whatever is passed as the imageWidth parameter will be the width of the CGImage (in pixels) returned by this method regardless of device scale. The only difference is that CGContextSetBaseCTM will get called with device scale which affects some CoreGraphics features. Because scale is actually computed and not always equal to the device scale, this will cause CG features such as focus rectangle to appear at incorrect sizes unless imageWidth / viewportWidth == deviceScaleFactor. This will be the usual case for our intended purposes of the API, but I think this is a bug in WebPage::snapshotAtSize that could be looked into. I believe we want CG features such as focus rectangle to be scaled to the computed scale factor, not always relative to the device while taking a snapshot.
- Software snapshot will cause text to still look crisp as higher resolutions than the viewport, could be useful in some cases.
- 700_600_device_scale the focus rectangle looks too large because of the device scale factor issue above.
- iOS device private API starts looking blurry when imageWidth / viewportWidth != deviceScaleFactor because it is using a hardware snapshot.
- A common case will be to pass output imageWidth that factors in the device scale, in this case the snapshot looks good. It should be clearly documented that the imageWidth is in pixels (not scaled for device scale factor), while rectInViewCoordinates are in device independent points inside WKWebView bounds. This is the behavior of _snapshotRect private API, if a different design is better please let me know.

CSS 3D transform tests:
1400_1200_css_3d_transform.png - https://desandro.github.io/3dtransforms/examples/carousel-02-dynamic.html
- Software snapshot looks distorted for CSS 3d transform. Not ideal, but I am able to understand what the thumbnail or placeholder represents. Not a better option available now for Mac that allows snapshots of obscured views which will occur.
- iOS device snapshot looks good in this case.

HTML5 video
1400_1200_html_video.png - http://www.w3schools.com/html/html5_video.asp
- iOS 10 device hardware snapshot is not working as assumed. It is just showing a white rectangle as a placeholder for HTML5 video content when the snapshot is taken while video is playing. Could this be improved in the future, or are there technical limitations?
- Software snapshot is actually performing better for HTML5 video than hardware snapshot, it is able to capture an image for this sample video on my Mac that the iOS 10 device could not.
- Mac software snapshot is getting a blank rectangle for youtube video HTML5 case. Have not investigated the reason why it is able to capture one example, but not another (1400_1200_youtube.png - https://www.youtube.com/watch?v=CQY3KUR3VzM).

WebGL tests:
1400_1200_webgl.png - http://globe.chromeexperiments.com
- Neither iOS device or Mac software snapshot could capture this WebGL content.
- I waited until the WebGL content was fully loaded before I called the snapshot API. Regardless of this, I am only getting the HTML content that is not using hardware acceleration such as loading div, texts, and tabs.


Code relevant to SnapshotOptionsExcludeDeviceScaleFactor for this new API. I describe how it works above.
PassRefPtr<WebImage> WebPage::snapshotAtSize(const IntRect& rect, const IntSize& bitmapSize, SnapshotOptions options)
{
    Frame* coreFrame = m_mainFrame->coreFrame();
    if (!coreFrame)
        return nullptr;

    FrameView* frameView = coreFrame->view();
    if (!frameView)
        return nullptr;

    IntRect snapshotRect = rect;
    float horizontalScaleFactor = static_cast<float>(bitmapSize.width()) / rect.width();
    float verticalScaleFactor = static_cast<float>(bitmapSize.height()) / rect.height();
    float scaleFactor = std::max(horizontalScaleFactor, verticalScaleFactor);

    auto snapshot = WebImage::create(bitmapSize, snapshotOptionsToImageOptions(options));
    if (!snapshot->bitmap())
        return nullptr;

    auto graphicsContext = snapshot->bitmap()->createGraphicsContext();

    if (options & SnapshotOptionsPrinting) {
        PrintContext::spoolAllPagesWithBoundaries(*coreFrame, *graphicsContext, snapshotRect.size());
        return snapshot;
    }

    Color documentBackgroundColor = frameView->documentBackgroundColor();
    Color backgroundColor = (coreFrame->settings().backgroundShouldExtendBeyondPage() && documentBackgroundColor.isValid()) ? documentBackgroundColor : frameView->baseBackgroundColor();
    graphicsContext->fillRect(IntRect(IntPoint(), bitmapSize), backgroundColor);

    if (!(options & SnapshotOptionsExcludeDeviceScaleFactor)) {
        double deviceScaleFactor = corePage()->deviceScaleFactor();
        graphicsContext->applyDeviceScaleFactor(deviceScaleFactor);
        scaleFactor /= deviceScaleFactor;
    }

    graphicsContext->scale(FloatSize(scaleFactor, scaleFactor));
    graphicsContext->translate(-snapshotRect.x(), -snapshotRect.y());

    FrameView::SelectionInSnapshot shouldPaintSelection = FrameView::IncludeSelection;
    if (options & SnapshotOptionsExcludeSelectionHighlighting)
        shouldPaintSelection = FrameView::ExcludeSelection;

    FrameView::CoordinateSpaceForSnapshot coordinateSpace = FrameView::DocumentCoordinates;
    if (options & SnapshotOptionsInViewCoordinates)
        coordinateSpace = FrameView::ViewCoordinates;

    frameView->paintContentsForSnapshot(*graphicsContext, snapshotRect, shouldPaintSelection, coordinateSpace);

    if (options & SnapshotOptionsPaintSelectionRectangle) {
        FloatRect selectionRectangle = m_mainFrame->coreFrame()->selection().selectionBounds();
        graphicsContext->setStrokeColor(Color(0xFF, 0, 0));
        graphicsContext->strokeRect(selectionRectangle, 1);
    }
    
    return snapshot;
}

void GraphicsContext::applyDeviceScaleFactor(float deviceScaleFactor)
{
    scale(FloatSize(deviceScaleFactor, deviceScaleFactor));

    if (isRecording()) {
        m_displayListRecorder->applyDeviceScaleFactor(deviceScaleFactor);
        return;
    }

    platformApplyDeviceScaleFactor(deviceScaleFactor);
}

void GraphicsContext::platformApplyDeviceScaleFactor(float deviceScaleFactor)
{
    if (paintingDisabled())
        return;

    ASSERT(!isRecording());

    // CoreGraphics expects the base CTM of a HiDPI context to have the scale factor applied to it.
    // Failing to change the base level CTM will cause certain CG features, such as focus rings,
    // to draw with a scale factor of 1 rather than the actual scale factor.
    CGContextSetBaseCTM(platformContext(), CGAffineTransformScale(CGContextGetBaseCTM(platformContext()), deviceScaleFactor, deviceScaleFactor));
}

[Harrison Shapley]

Hi Tim or others,

It seems that there are some bugs in other APIs, but that should not block this snapshot API from being approved/checked in. We believe the code is good as-is, can you take a final pass through it and approve? We need this API as soon as possible to get unblocked with our transition to WKWebView. 

We can file the other bugs we found (mentioned in previous posts) if you want. However, our top priority is to get this change approved.

Thanks,
Harrison

[Dan Saunders]

Hey,

Could we get an update on this bug? I haven't heard new comments for a couple weeks, is there anything I should be changing with the code, or test cases to try? I understand the process to check-in an API is different than bug fixes, could we get more information about the process and anything we can do to help with landing this API? This is currently blocking our migration to WKWebView which gives a lot of benefits over WebView. Any ETA would be helpful.

Thanks,
Dan

[Darin Adler]

Comment on attachment 288770 [details]
Patch

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

I know you are in a hurry, but we need to be cautious about creating API that we will have to live with for a long time.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:886
> +    // Software snapshot will not capture elements rendered with hardware acceleration (WebGL, video, etc).

If true, I am not sure it is acceptable to create API that will fail in this manner for so much content?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:895
> +                    auto rawHandler = (void (^)(CGImageRef, NSError *))handler.get();

I believe there is a way to do this without all these typecast casts to the block type, perhaps by using the equivalence of blocks and lambdas.

[Darin Adler]

I think we need significantly more test coverage. The single test is better than nothing, but I’d like to see tests covering different types of content.

[Darin Adler]

Is there a way to instead make existing AppKit or UIKit snapshot API and techniques work successfully on WKWebView, rather than adding a new WKWebView feature?

[Dan Saunders]

(In reply to comment #15)
> Is there a way to instead make existing AppKit or UIKit snapshot API and
> techniques work successfully on WKWebView, rather than adding a new
> WKWebView feature?

Thanks for taking a look Darin. I have searched for solutions using AppKit API, and it looks like other developers are having the same issue where none of the AppKit API will capture the contents of WKWebView. Many people refer to a private API solution, which is no good for store apps. With iOS UIKit, we are able to get the contents with (- (BOOL)drawViewHierarchyInRect:(CGRect)rect afterScreenUpdates:(BOOL)afterUpdates), but a solution does not seem to be available for Mac. The reason I was adding the API was for Mac, but to be consistent and allow for developer code reuse I am making it available for both Mac and iOS. If you know of a different method that I did not try, please let me know. Below are some examples I have tried and verified they cannot get the WKWebView snapshot on OSX, while most of them work on the legacy WebVew.

- (NSImage *)snapshotView1:(NSView *)view
{
    // WebView returns the correct content, WKWebView returns white snapshot.
    NSImage *image = [[NSImage alloc] initWithSize:view.bounds.size];
    NSBitmapImageRep *imageRep = [view bitmapImageRepForCachingDisplayInRect:view.bounds];
    [view cacheDisplayInRect:view.bounds toBitmapImageRep:imageRep];
    [image addRepresentation:imageRep];

    return image;
}


- (NSImage *)snapshotView2:(NSView *)view
{
    // WebView returns the correct content, WKWebView returns white snapshot.
    NSImage *image = [[NSImage alloc] initWithSize:view.bounds.size];
    [image lockFocus];
    CGContextRef ctx = [NSGraphicsContext currentContext].graphicsPort;
    [view.layer renderInContext:ctx];
    [image unlockFocus];
    
    return image;

}

- (NSImage *)snapshotView3:(NSView *)view
{
    // Returns an empty image with both WKWebView and WebView.
    [view lockFocus];
    NSImage *image = [[NSImage alloc] initWithData:[view dataWithPDFInsideRect:view.bounds]];
    [view unlockFocus];
    
    return image;
    
}

- (CGImageRef)snapshotView4:(NSView *)view
{
    // CGWindowListCreateImage can get WKWebView content, but only the visible portions in the parented window. If the view is inside a scrollview, clipped by the window, or overlapped with another view, it cannot access the full WKWebView viewport.
    CGImageRef image = CGWindowListCreateImage(CGRectNull, kCGWindowListOptionIncludingWindow, (CGWindowID)self.view.window.windowNumber, kCGWindowImageDefault);

    return image;
}

[Tim Horton]

I think Darin was asking whether your patch could make one of the existing AppKit snapshotting mechanisms work. I think the answer is no, because they're all synchronous and we'd really prefer not to add another sync wait from the UI process, but perhaps there is a way (or perhaps there is an asynchronous one? I haven't seen it.)

[Dan Saunders]

(In reply to comment #17)
> I think Darin was asking whether your patch could make one of the existing
> AppKit snapshotting mechanisms work. I think the answer is no, because
> they're all synchronous and we'd really prefer not to add another sync wait
> from the UI process, but perhaps there is a way (or perhaps there is an
> asynchronous one? I haven't seen it.)

Sorry I misunderstood the question. Making WKWebView respond to the existing AppKit API would work well for us, I am not sure how to do it without a synchronous IPC call. I think I need to better understand the underlying problem, hopefully you may be able to help. From looking at the code, it looks like the Mac WKWebView to use a remote CALayer to do the rendering from the WebContent process to UI process. The corresponding API seem to be WKCAContextMakeRemoteForWindowServer in the WebContent process which sends a context id to the UI process and creates the CALayer with WKMakeRenderLayer(contextId). I don't have the source code to these two API, so I am not sure their internal implementation. The CALayerHost remote layer is added as a sublayer of WKFlippedView's layer. Using renderInContext on the CALayerHost, or the WKFlippedView layer is not producing any image. Is there any private API we could use to get the content from the layer?

I see some of the private API are being used to capture WKWebView snapshot content in iOS, but it is not linking when I try to use them on Mac. Is there any equivalent API that would work for Mac:
void CARenderServerCaptureLayerWithTransform(mach_port_t, uint32_t clientId, uint64_t layerId, uint32_t slotId, int32_t ox, int32_t oy, const CATransform3D*);
void CARenderServerRenderLayerWithTransform(mach_port_t server_port, uint32_t client_id, uint64_t layer_id, IOSurfaceRef, int32_t ox, int32_t oy, const CATransform3D*);
void CARenderServerRenderDisplayLayerWithTransformAndTimeOffset(mach_port_t, CFStringRef display_name, uint32_t client_id, uint64_t layer_id, IOSurfaceRef, int32_t ox, int32_t oy, const CATransform3D*, CFTimeInterval);

Given the implementation of WebViewImpl::takeViewSnapshot() that is using a window snapshot instead of a view snapshot for a similar case, I am thinking there may not currently be an AppKit API available that could let us render the layer to an image. It is possible a better implementation was overlooked for takeViewSnapshot. Could you please confirm.

Thanks,
Dan

[Tim Horton]

(In reply to comment #18)
> (In reply to comment #17)
> > I think Darin was asking whether your patch could make one of the existing
> > AppKit snapshotting mechanisms work. I think the answer is no, because
> > they're all synchronous and we'd really prefer not to add another sync wait
> > from the UI process, but perhaps there is a way (or perhaps there is an
> > asynchronous one? I haven't seen it.)
> 
> Sorry I misunderstood the question. Making WKWebView respond to the existing
> AppKit API would work well for us, I am not sure how to do it without a
> synchronous IPC call. I think I need to better understand the underlying
> problem, hopefully you may be able to help. From looking at the code, it
> looks like the Mac WKWebView to use a remote CALayer to do the rendering
> from the WebContent process to UI process. The corresponding API seem to be
> WKCAContextMakeRemoteForWindowServer in the WebContent process which sends a
> context id to the UI process and creates the CALayer with
> WKMakeRenderLayer(contextId). I don't have the source code to these two API,
> so I am not sure their internal implementation. The CALayerHost remote layer
> is added as a sublayer of WKFlippedView's layer. Using renderInContext on
> the CALayerHost, or the WKFlippedView layer is not producing any image. Is
> there any private API we could use to get the content from the layer?
> 
> I see some of the private API are being used to capture WKWebView snapshot
> content in iOS, but it is not linking when I try to use them on Mac. Is
> there any equivalent API that would work for Mac:

No, there aren't.

> void CARenderServerCaptureLayerWithTransform(mach_port_t, uint32_t clientId,
> uint64_t layerId, uint32_t slotId, int32_t ox, int32_t oy, const
> CATransform3D*);
> void CARenderServerRenderLayerWithTransform(mach_port_t server_port,
> uint32_t client_id, uint64_t layer_id, IOSurfaceRef, int32_t ox, int32_t oy,
> const CATransform3D*);
> void CARenderServerRenderDisplayLayerWithTransformAndTimeOffset(mach_port_t,
> CFStringRef display_name, uint32_t client_id, uint64_t layer_id,
> IOSurfaceRef, int32_t ox, int32_t oy, const CATransform3D*, CFTimeInterval);
> 
> Given the implementation of WebViewImpl::takeViewSnapshot() that is using a
> window snapshot instead of a view snapshot for a similar case, I am thinking
> there may not currently be an AppKit API available that could let us render
> the layer to an image.

Right.

> It is possible a better implementation was overlooked
> for takeViewSnapshot.

I don't think so.

> Could you please confirm.
> 
> Thanks,
> Dan

[Darin Adler]

You should let Tim continue to guide you on resolving this. My question wasn’t a deeply informed one.

[Dan Saunders]

Thanks for the responses. It seems like there is not another option that is currently available other than to use the asynchronous software snapshot on Mac. Tim, are you ok with adding this new API that uses the software snapshot. In the future if AppKit adds an API that is able to get the full contents of the layer including hardware accelerated elements, we can change the implementation to use that instead without changing the signature.

[Tim Horton]

(In reply to comment #21)
> Thanks for the responses. It seems like there is not another option that is
> currently available other than to use the asynchronous software snapshot on
> Mac. Tim, are you ok with adding this new API that uses the software
> snapshot.

I think it's fine, for now. I still need to talk to Anders about how to make progress with adding new API, but the software-paint mechanism is probably good enough for now, and we can improve as things get better, as you say; the API doesn't tie us to a particular implementation.

In the meantime, can you address Darin's other comments?

> I believe there is a way to do this without all these typecast casts to the block type, perhaps by using the equivalence of blocks and lambdas.

> I think we need significantly more test coverage. The single test is better than nothing, but I’d like to see tests covering different types of content.

Also, I wonder if it is more appropriate for a modern ObjC API to return ObjC types (NSImage and UIImage) instead of CGImageRef.

[Tim Horton]

(In reply to comment #22)
> (In reply to comment #21)

> Also, I wonder if it is more appropriate for a modern ObjC API to return
> ObjC types (NSImage and UIImage) instead of CGImageRef.

Dan and Anders agree with this part.

[Dan Saunders]

Comment on attachment 288770 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:886
>> +    // Software snapshot will not capture elements rendered with hardware acceleration (WebGL, video, etc).
> 
> If true, I am not sure it is acceptable to create API that will fail in this manner for so much content?

It is not ideal, but we haven't been able to find a better option to capture the remote layer contents reliably. If a new AppKit API becomes available that will allow us to capture this content, we can change this implementation to use it instead. Should I open a tracking bug and reference it here?

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:895
>> +                    auto rawHandler = (void (^)(CGImageRef, NSError *))handler.get();
> 
> I believe there is a way to do this without all these typecast casts to the block type, perhaps by using the equivalence of blocks and lambdas.

All of the asynchronous callbacks in WebViewImpl.mm and WKWebView.mm are using typecasts, so I was following the convention. I run into compiler errors with adoptNS constructor if I try to give it the correct type, and it is important to use the RetainPtr to make sure the callback does not get released outside of the lambda. I could do a manual release and avoid the RetainPtr, but this approach does not seem to be used other places. I can remove 1 of the typecasts so I will go ahead and do that in the next iteration.

[Dan Saunders]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> 
> > Also, I wonder if it is more appropriate for a modern ObjC API to return
> > ObjC types (NSImage and UIImage) instead of CGImageRef.
> 
> Dan and Anders agree with this part.

I will return the NSImage or UIImage types.

[Dan Saunders]

(In reply to comment #14)
> I think we need significantly more test coverage. The single test is better
> than nothing, but I’d like to see tests covering different types of content.

I can look into adding additional tests. The problem with any test that is testing WebKit pixel contents is it is likely to unreliable when the rendering changes slightly. I see TestInvocation::dumpPixelsAndCompareWithExpected is already used in the test, perhaps this has been unreliable in the past. I don't want to make maintaining the test to be too much overhead. For Mac we are just calling into WebPage::takeSnapshot which is used to take a snapshot in other places such as thumbnails. The thumbnail tests do not compare pixel content and are more basic to be reliable. My unit tests are testing the code I added is functioning, but not testing all the edge cases of WebPage::takeSnapshot specifically. I am testing that I can retrieve a pixel that should deterministically be a certain color. WebPage::takeSnapshot should be reliable because it is used for core functionality of WebKit.

[Dan Saunders]

Created attachment 292310 [details]
Patch

[Felix Deimel]

I'm also desperately awaiting this feature. Was previously using private API (_WKThumbnailView via WKView) which broke in 10.12.1 or 10.12 already, not sure because WKView is not exposed as a subview of WKWebView anymore. See https://github.com/lemonmojo/WKWebView-Screenshot for more details.

When can we expect the new snapshotRect:intoImageOfWidth:completionHandler: API to end up in a macOS/Safari release?

[Michael Catanzaro]

I just want to add a quick warning. We have a similar API in WebKitGTK+, but it is not reliable because there is no way to know when the page has actually been rendered, so no way to know when it's safe to call the API. If you try to take a snapshot when LOAD_FINISHED is received, you could wind up with a blank snapshot since the page is not guaranteed to actually be rendered when LOAD_FINISHED is called. The only way to use the API is to take the snapshot later using some arbitrary timeout, which works in practice but is really terrible. You'll want to be careful to ensure this isn't a problem for the WKWebView API.

[Michael Catanzaro]

(In reply to comment #29)
> If you try to take a snapshot when LOAD_FINISHED is received, you could wind up
> with a blank snapshot since the page is not guaranteed to actually be
> rendered when LOAD_FINISHED is called.

And of course it's a race condition, so you could be affected by this bug even if you're not able to reproduce it on your hardware. Hopefully the Mac ports have a more reliable rendering architecture.

[Dan Saunders]

(In reply to comment #30)
> (In reply to comment #29)
> > If you try to take a snapshot when LOAD_FINISHED is received, you could wind up
> > with a blank snapshot since the page is not guaranteed to actually be
> > rendered when LOAD_FINISHED is called.
> 
> And of course it's a race condition, so you could be affected by this bug
> even if you're not able to reproduce it on your hardware. Hopefully the Mac
> ports have a more reliable rendering architecture.

Thanks for the warning. I have not noticed this being an issue on Mac during my testing, I am always getting a non-blank snapshot on from my machines. I can do some stress testing to confirm. Since Mac is using a software snapshot, it should not be as impacted by hardware race conditions. I see there was an issues a few years ago on the iOS API I am calling but it was fixed with https://bugs.webkit.org/show_bug.cgi?id=146476. Tim mentions some other edge cases in that bug, but I do not have the specific cases. The iOS API has been available for a few years, so I am assuming it works reliably.

[Darin Adler]

Dan, I’m not sure what you are referring to when you cite “hardware race conditions”. What Michael is talking about is a race between the WebKit page loading process and the snapshot. And it affects the iOS API too.

[Dan Saunders]

(In reply to comment #32)
> Dan, I’m not sure what you are referring to when you cite “hardware race
> conditions”. What Michael is talking about is a race between the WebKit page
> loading process and the snapshot. And it affects the iOS API too.

Sorry for using confusing wording. Michael mentioned that the race condition may not repro on all hardware. I tried a few hundred navigations on my two Mac machines, and never ran into an issue with an incomplete snapshot due to race condition as long as I waited for (- (void)webView:(WKWebView *)webView didFinishNavigation:(null_unspecified WKNavigation *)navigation) delegate. The only time I could get a blank snapshot after initial page load is when I start to navigate to a new page, and the new page has not called didFinishNavigation yet. The caller can be aware of this condition though, it is not guaranteed that the snapshot will be available until navigation is finished and they get the notification. The performance on the API is not bad for Mac, in most cases it takes 10-20 milliseconds for me to retrieve the async snapshot. If I call the API immediately after navigation finishes on a complex page, it could take 1 second although this seems likely due to the web content process being busy executing JavaScript/other tasks and the snapshot request queued behind it. If I delay the snapshot by 1 second after page navigation completes, the snapshot request only takes 20 milliseconds.

[Darin Adler]

(In reply to comment #33)
> I tried a few hundred navigations on my two Mac machines

What webpages did you test with?

[Dan Saunders]

(In reply to comment #34)
> (In reply to comment #33)
> > I tried a few hundred navigations on my two Mac machines
> 
> What webpages did you test with?

For Mac, I could capture the initial page load of all the websites I tried without issues:
https://www.stackoverflow.com
https://www.google.com
https://www.facebook.com
https://www.apple.com
https://www.bing.com
https://www.youtube.com (just home page, did not click on video which does not get captured with software snapshot)
https://www.wikipedia.org
https://www.amazon.com
https://www.twitter.com
https://www.yahoo.com

[Dan Saunders]

Any update on this bug?

[Beth Dakin]

Comment on attachment 292310 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:230
> +- (void)snapshotRect:(CGRect)rectInViewCoordinates intoImageOfWidth:(CGFloat)imageWidth completionHandler:(void(^)(UIImage * _Nullable snapshotImage, NSError * _Nullable error))completionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

Dan, thanks so much for this patch. We have been discussing internally, and we think this signature needs to change. Instead of taking both a rect and a width, you should define a new configuration object. That will let us more easily extend this API in the future if we find that other clients have other things they want to specify about the snapshot. And we'll be able to add those features without changing the API. It should be something like:

- (void)snapshotWithConfiguration:(WKSnapshotConfiguration)snapshotConfiguration completionHandler:(void(^)(UIImage * _Nullable snapshotImage, NSError * _Nullable error))completionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

You can take a look at WKWebViewConfiguration if you want to see a similar object and API pattern in the existing API.

[Beth Dakin]

Created attachment 299290 [details]
Patch

Here's a new cut.

[Tim Horton]

Comment on attachment 299290 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKSnapshotConfiguration.mm:35
> +    return [self retain];

That's no copy :) Why is this OK?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:224
> +/*! @abstract Get the snapshot for the visible viewport of WKWebView asynchronously.

s/the snapshot/a snapshot/, I think?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:226
> + @param imageWidth The width in points of the image to return.

Points! Interesting choice. I guess it's reasonable?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:918
> +    void(^handler)(NSImage *, NSError *) = [completionHandler copy];

BlockPtr?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:939
> +        RetainPtr<NSImage> nsImage = adoptNS( [[NSImage alloc] initWithCGImage:cgImage.get() size:NSMakeSize(snapshotWidth, imageHeight)]);

weird space between adoptNS( and the [

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:948
> +    void(^handler)(UIImage *, NSError *) = [completionHandler copy];

BlockPtr?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:955
> +        RetainPtr<UIImage>  uiImage;

weird space before uiImage

[Dan Saunders]

(In reply to comment #38)
> Created attachment 299290 [details]
> Patch
> 
> Here's a new cut.

Thanks a lot for the updated patch Beth. If this gets approved, do you have an estimate of when can we expect the API to be available from the released framework? Do you only ship new public API on major OS updates?

[Beth Dakin]

Created attachment 300287 [details]
Patch

[Beth Dakin]

(In reply to comment #39)
> Comment on attachment 299290 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299290&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKSnapshotConfiguration.mm:35
> > +    return [self retain];
> 
> That's no copy :) Why is this OK?
> 

Not okay. Fixed!

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:224
> > +/*! @abstract Get the snapshot for the visible viewport of WKWebView asynchronously.
> 
> s/the snapshot/a snapshot/, I think?
> 

Oops, forgot to fix that in this patch, I'll upload a new one shortly.

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:226
> > + @param imageWidth The width in points of the image to return.
> 
> Points! Interesting choice. I guess it's reasonable?
> 

I'm open to alternatives.

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:918
> > +    void(^handler)(NSImage *, NSError *) = [completionHandler copy];
> 
> BlockPtr?

Fixed.

> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:939
> > +        RetainPtr<NSImage> nsImage = adoptNS( [[NSImage alloc] initWithCGImage:cgImage.get() size:NSMakeSize(snapshotWidth, imageHeight)]);
> 
> weird space between adoptNS( and the [

Fixed.

> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:948
> > +    void(^handler)(UIImage *, NSError *) = [completionHandler copy];
> 
> BlockPtr?

Fixed.

> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:955
> > +        RetainPtr<UIImage>  uiImage;
> 
> weird space before uiImage

Fixed.

[Beth Dakin]

Created attachment 300288 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 300288 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:227
> + @param rectInViewCoordinates The rect to use for snapshot source in view coordinates.
> + @param imageWidth The width in points of the image to return.
> + @discussion rectInViewCoordinates parameter should be contained within WKWebView's bounds.

These comments need to move to the configuration object header.

[Tim Horton]

Comment on attachment 300288 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:28
> +#if WK_API_ENABLED

Do these tests work with different display colorspaces? We should test with a few.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:44
> +    RetainPtr<CGImageSourceRef> source = adoptCF(CGImageSourceCreateWithData((CFDataRef)[image TIFFRepresentation], NULL));
> +    return adoptCF(CGImageSourceCreateImageAtIndex(source.get(), 0, NULL));

Is there any reason this doesn't just use CGImageForProposedRect?

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:54
> +    CGImageRef cgImage = [image CGImage];
> +    CFRetain(cgImage);
> +    return adoptCF(cgImage);

No need for this dance, just 'return image.CGImage;' will do the right thing (it is autoreleased, RetainPtr constructor will retain it).

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:73
> +    WKSnapshotConfiguration *snapshotConfiguration = [[WKSnapshotConfiguration alloc] init];

Pretty sure this leaks.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:94
> +    [webView loadHTMLString:@"<body style='background-color: red;'><div style='background-color: blue;position: absolute;width: 100px; height: 100px; top: 50px; left: 50px'></div></body>" baseURL:nil];

Spaces after semicolons? Kind of inconsistent at the moment.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:97
> +    WKSnapshotConfiguration *snapshotConfiguration = [[WKSnapshotConfiguration alloc] init];

Pretty sure this leaks.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:99
> +    snapshotConfiguration.snapshotWidth = viewWidth;

Here, viewWidth is being used as points...

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:108
> +        CGColorSpaceRef colorSpace = CGColorSpaceCreateDeviceRGB();

Why not a RetainPtr?

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:111
> +        CGContextRef context = CGBitmapContextCreate(rgba, viewWidth, viewHeight, 8, 4 * viewWidth, colorSpace, kCGImageAlphaPremultipliedLast | kCGBitmapByteOrder32Big);

... and here as pixels. Does this matter? Does the test succeed on hardware of various deviceScaleFactors?

Also, why not a RetainPtr?

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:112
> +        CGContextDrawImage(context, CGRectMake(0, 0, viewWidth, viewHeight), cgImage.get());

Or maybe it's OK because it gets scaled into place here?

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:148
> +    WKSnapshotConfiguration *snapshotConfiguration = [[WKSnapshotConfiguration alloc] init];

Pretty sure this leaks.

[Beth Dakin]

(In reply to comment #44)
> Comment on attachment 300288 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=300288&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:227
> > + @param rectInViewCoordinates The rect to use for snapshot source in view coordinates.
> > + @param imageWidth The width in points of the image to return.
> > + @discussion rectInViewCoordinates parameter should be contained within WKWebView's bounds.
> 
> These comments need to move to the configuration object header.

Good catch! Moved.

[Beth Dakin]

(In reply to comment #45)
> Comment on attachment 300288 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=300288&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:28
> > +#if WK_API_ENABLED
> 
> Do these tests work with different display colorspaces? We should test with
> a few.
> 

They do not. We should. My next patch won't do this yet.

> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:44
> > +    RetainPtr<CGImageSourceRef> source = adoptCF(CGImageSourceCreateWithData((CFDataRef)[image TIFFRepresentation], NULL));
> > +    return adoptCF(CGImageSourceCreateImageAtIndex(source.get(), 0, NULL));
> 
> Is there any reason this doesn't just use CGImageForProposedRect?

Switched to CGImageForProposedRect.

> 
> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:54
> > +    CGImageRef cgImage = [image CGImage];
> > +    CFRetain(cgImage);
> > +    return adoptCF(cgImage);
> 
> No need for this dance, just 'return image.CGImage;' will do the right thing
> (it is autoreleased, RetainPtr constructor will retain it).

Fixed.

> 
> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:73
> > +    WKSnapshotConfiguration *snapshotConfiguration = [[WKSnapshotConfiguration alloc] init];
> 
> Pretty sure this leaks.

RetainPtr'd.

> 
> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:94
> > +    [webView loadHTMLString:@"<body style='background-color: red;'><div style='background-color: blue;position: absolute;width: 100px; height: 100px; top: 50px; left: 50px'></div></body>" baseURL:nil];
> 
> Spaces after semicolons? Kind of inconsistent at the moment.

Fixed.

> 
> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:97
> > +    WKSnapshotConfiguration *snapshotConfiguration = [[WKSnapshotConfiguration alloc] init];
> 
> Pretty sure this leaks.
> 

RetainPtr'd.

> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:99
> > +    snapshotConfiguration.snapshotWidth = viewWidth;
> 
> Here, viewWidth is being used as points...

Yes.

> 
> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:108
> > +        CGColorSpaceRef colorSpace = CGColorSpaceCreateDeviceRGB();
> 
> Why not a RetainPtr?
> 

RetainPtr'd.

> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:111
> > +        CGContextRef context = CGBitmapContextCreate(rgba, viewWidth, viewHeight, 8, 4 * viewWidth, colorSpace, kCGImageAlphaPremultipliedLast | kCGBitmapByteOrder32Big);
> 
> ... and here as pixels. Does this matter? Does the test succeed on hardware
> of various deviceScaleFactors?
> 

Fixed to use real pixels.

> Also, why not a RetainPtr?
> 

RetainPtr'd.

> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:112
> > +        CGContextDrawImage(context, CGRectMake(0, 0, viewWidth, viewHeight), cgImage.get());
> 
> Or maybe it's OK because it gets scaled into place here?

Fixed, I think.

> 
> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:148
> > +    WKSnapshotConfiguration *snapshotConfiguration = [[WKSnapshotConfiguration alloc] init];
> 
> Pretty sure this leaks.

RetainPtr'd.

[Beth Dakin]

Created attachment 300982 [details]
Patch

[Beth Dakin]

Created attachment 301058 [details]
Patch

[Beth Dakin]

Created attachment 301068 [details]
Patch

[Tim Horton]

Comment on attachment 301068 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        No reliable way to get a snapshot of WKWebView (macOS)

No need for the macOS in the title because you're adding cross-platform API

> Source/WebKit2/ChangeLog:13
> +       It also adds a new API object WKSnapshotConfiguration.

This is indented wrong.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:233
> +#if TARGET_OS_IPHONE
> +- (void)takeSnapshotWithConfiguration:(WKSnapshotConfiguration *)snapshotConfiguration completionHandler:(void(^)(UIImage * _Nullable snapshotImage, NSError * _Nullable error))completionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> +#else
> +- (void)takeSnapshotWithConfiguration:(WKSnapshotConfiguration *)snapshotConfiguration completionHandler:(void(^)(NSImage * _Nullable snapshotImage, NSError * _Nullable error))completionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> +#endif

I wonder if we should accept a null snapshotConfiguration and set rectInViewCoordinates to the bounds of the WKWebView and snapshotWidth to the width of the bounds of the WKWebView.

[Beth Dakin]

(In reply to comment #51)
> Comment on attachment 301068 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301068&action=review
> 
> > Source/WebKit2/ChangeLog:3
> > +        No reliable way to get a snapshot of WKWebView (macOS)
> 
> No need for the macOS in the title because you're adding cross-platform API
> 

Fixed.

> > Source/WebKit2/ChangeLog:13
> > +       It also adds a new API object WKSnapshotConfiguration.
> 
> This is indented wrong.
> 

Fixed.

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:233
> > +#if TARGET_OS_IPHONE
> > +- (void)takeSnapshotWithConfiguration:(WKSnapshotConfiguration *)snapshotConfiguration completionHandler:(void(^)(UIImage * _Nullable snapshotImage, NSError * _Nullable error))completionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> > +#else
> > +- (void)takeSnapshotWithConfiguration:(WKSnapshotConfiguration *)snapshotConfiguration completionHandler:(void(^)(NSImage * _Nullable snapshotImage, NSError * _Nullable error))completionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> > +#endif
> 
> I wonder if we should accept a null snapshotConfiguration and set
> rectInViewCoordinates to the bounds of the WKWebView and snapshotWidth to
> the width of the bounds of the WKWebView.

I like this! New patch incoming.

[Beth Dakin]

Created attachment 301081 [details]
Patch

[Beth Dakin]

Created attachment 301828 [details]
Patch

[Beth Dakin]

Created attachment 301839 [details]
Patch

[Build Bot]

Comment on attachment 301839 [details]
Patch

Attachment 301839 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3135714

New failing tests:
js/dom/gc-slot-visitor-parallel-drain-pings-runloop-when-done.html

[Build Bot]

Created attachment 301863 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Beth Dakin]

Created attachment 301971 [details]
Patch

Comment on attachment 301971 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKSnapshotConfiguration.h:39
> + @discussion This rect should be contained within WKWebView's bounds. If the rect is not set, 
> + the WKWebView's bounds will be used.

I think the comment can be more explicit (and also make it clear how to reset the property). Namely, it can say that if set to the null rect, the view’s bounds will be used, and that the initial value is the null rect.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:232
> +- (void)takeSnapshotWithConfiguration:(nullable WKSnapshotConfiguration *)snapshotConfiguration completionHandler:(void(^)(UIImage * _Nullable snapshotImage, NSError * _Nullable error))completionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

Omit the macosx entry here.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:234
> +- (void)takeSnapshotWithConfiguration:(nullable WKSnapshotConfiguration *)snapshotConfiguration completionHandler:(void(^)(NSImage * _Nullable snapshotImage, NSError * _Nullable error))completionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

Omit the ios entry here!

[Beth Dakin]

Created attachment 302422 [details]
Patch

[Beth Dakin]

Created attachment 302450 [details]
Patch

[Tim Horton]

Comment on attachment 302450 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKSnapshotConfiguration.h:46
> + rectâs width will be used.

I'm guessing it's this apostrophe that is the problem.

[Beth Dakin]

Created attachment 302563 [details]
Patch

[Tim Horton]

Comment on attachment 302563 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKSnapshotConfiguration.h:45
> + @discussion snapshotWidth represents the width in points. If the snapshotWidth is not set, 

"not set", or maybe "nil" (since you can set it to nil)

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:984
> +    // For iOS perform a hardware snapshot of the view for better quality.
> +    // Need to scale by device scale factor or the image will be distorted.

I would almost just remove these comments, they're kind of vague and unhelpful.

> Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:454
> +		93F56DA31E564938003EDE84 /* libicucore.dylib in Frameworks */ = {isa = PBXBuildFile; fileRef = 7C83E0331D0A5F2700FEBCF3 /* libicucore.dylib */; };

‽‽‽

> Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:-1423
> -				7C83E03F1D0A61A000FEBCF3 /* libicucore.dylib in Frameworks */,

‽‽‽

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:74
> +    [snapshotConfiguration setSnapshotWidth:[NSNumber numberWithFloat:viewWidth]];

@(viewWidth)

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:79
> +        EXPECT_WK_STREQ(@"WKErrorDomain", [error domain]);

Dots :)

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKWebViewSnapshot.mm:99
> +    backingScaleFactor = [[webView window] backingScaleFactor];

Isn't this just window? no need for [webView window]

[Beth Dakin]

I addressed all of Tim's comments.

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

[aditya]

400_1200_html_video.png - https://www.welookups.com/html/html5_video.html


- iOS 10 device hardware snapshot is not working as assumed. It is just showing a white rectangle as a placeholder for HTML5 video content when the snapshot is taken while video is playing. Could this be improved in the future, or are there technical limitations?
- Software snapshot is actually performing better for HTML5 video than hardware snapshot, it is able to capture an image for this sample video on my Mac that the iOS 10 device could not.

[Darin Adler]

Comments here aren’t the best way to report additional bugs now that the API has been added. Please file new bug reports.