[webkit-reviews] review granted: [Bug 87794] Noticeable delay taking an HTML5 trailer fullscreen. : [Attachment 144643] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 87794: Noticeable delay taking an HTML5 trailer fullscreen.
https://bugs.webkit.org/show_bug.cgi?id=87794

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=144643&action=review


> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:186
> +static RetainPtr<CGImageRef> CGImageDeepCopy(CGImageRef sourceImage)

Our own functions should not have CG prefixes on their names. Those names are
reserved for use in Core Graphics itself.

If we were trying to name this the way it would have been named in CG, then
we’d want to include the word Create as in the CGImageCreateCopy function.

I think a better name might be CGImageCreateWithCopiedData as the CG name, or
in our code createImageWithCopiedData.

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:193
> +    CGColorSpaceRef colorspace = CGImageGetColorSpace(sourceImage);

Should be named colorSpace.

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:196
> +    RetainPtr<CFDataRef> data(AdoptCF,
CGDataProviderCopyData(CGImageGetDataProvider(sourceImage)));
> +    RetainPtr<CGDataProviderRef> provider(AdoptCF,
CGDataProviderCreateWithCFData(data.get()));

These might read better with the adoptCF function rather than the AdoptCF
constructor.

This two-line sequence might be more readable as a separate
createDataProviderWithCopiedData.

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:200
> +    return RetainPtr<CGImageRef>(AdoptCF, CGImageCreate(width, height,
bitsPerComponent, bitsPerPixel, bytesPerRow, colorspace, bitmapInfo,
provider.get(), 0, shouldInterpolate, intent));

Would read better with the adoptCF function.

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:224
> +    // Copy the image data into our own process from the WindowServer:

The use of a colon here is strange. Normally we’d use a period.

A better comment would explain why we are copying the data.