[Webkit-unassigned] [Bug 161450] No reliable way to get a snapshot of WKWebView (macOS)

Wed Feb 8 16:11:44 PST 2017

https://bugs.webkit.org/show_bug.cgi?id=161450

--- Comment #47 from Beth Dakin <bdakin at apple.com> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170209/289b4621/attachment-0001.html>