Bug 87794 – Noticeable delay taking an HTML5 trailer fullscreen.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Noticeable delay taking an HTML5 trailer fullscreen.

[Jer Noble]

Created attachment 144643 [details]
Patch

[Jer Noble]

<rdar://problem/11505987>

[Darin Adler]

Comment on attachment 144643 [details]
Patch

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.

[Jer Noble]

Committed r118896: <http://trac.webkit.org/changeset/118896>

[Jer Noble]

Comment on attachment 144643 [details]
Patch

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.

"createImageWithCopiedData" certainly beats out "copyImageDeeply" or "createDeeplyCopiedImage". :)

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

Changed.

>> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:196
>> +    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.

Sure thing.