87794 2012-05-29 17:00:45 -0700 Noticeable delay taking an HTML5 trailer fullscreen. 2012-05-30 00:14:25 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Mac OS X 10.7 RESOLVED FIXED InRadar, PlatformOnly P2 Normal --- 1 jer.noble jer.noble oldest_to_newest 636574 0 jer.noble 2012-05-29 17:00:45 -0700 Noticeable delay taking an HTML5 trailer fullscreen. 636577 1 144643 jer.noble 2012-05-29 17:05:52 -0700 Created attachment 144643 Patch 636582 2 jer.noble 2012-05-29 17:06:59 -0700 <rdar://problem/11505987> 636595 3 144643 darin 2012-05-29 17:12:34 -0700 Comment on attachment 144643 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. 636916 4 jer.noble 2012-05-29 23:34:07 -0700 Committed r118896: <http://trac.webkit.org/changeset/118896> 636948 5 144643 jer.noble 2012-05-30 00:14:25 -0700 Comment on attachment 144643 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. 144643 2012-05-29 17:05:52 -0700 2012-05-30 00:14:25 -0700 Patch bug-87794-20120529170551.patch text/plain 3559 jer.noble U3VidmVyc2lvbiBSZXZpc2lvbjogMTE4NDExCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggMzhiMjFkM2JhYWQzZTMx YTEwZTkxOTBjMzY4ZGFhMTEzNWQzNjc2Ni4uNDUxZDQyOWIwZDNjYTAyOTk3OWYzYTY5YjkzMDFj MjI0YjVjZGI1OSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIwIEBACisyMDEyLTA1LTI5ICBKZXIg Tm9ibGUgIDxqZXIubm9ibGVAYXBwbGUuY29tPgorCisgICAgICAgIE5vdGljZWFibGUgZGVsYXkg dGFraW5nIGFuIEhUTUw1IHRyYWlsZXIgZnVsbHNjcmVlbi4KKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTg3Nzk0CisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgV29yayBhcm91bmQgYSBub3QtZW50aXJlbHkgdW5k ZXJzdG9vZCBkZWxheSB3aGVuIHVzaW5nIHRoZSByZXN1bHRzIG9mIGEgQ0dXaW5kb3dMaXN0Q3Jl YXRlSW1hZ2UoKQorICAgICAgICBjYWxsIGFzIHRoZSBjb250ZW50cyBvZiBhIHBsYWNlaG9sZGVy IHZpZXcuICBUaGUgYmFja2luZyBvZiB0aGUgcmVzdWx0aW5nIENHSW1hZ2VSZWYgcmVzaWRlcyBv bgorICAgICAgICB0aGUgV2luZG93U2VydmVyLCBhbmQgZHJhd2luZyB0aGF0IGltYWdlIHJlcXVp cmVzIHN5bmNocm9ub3VzIGNhbGxzIHRvIHRoZSBXaW5kb3dTZXJ2ZXIgcHJvY2Vzcy4KKyAgICAg ICAgQnkgY29weWluZyB0aGUgaW1hZ2UgZGF0YSwgdXAgZnJvbnQsIGludG8gb3VyIG93biBwcm9j ZXNzLCB3ZSBjYW4gYXZvaWQgdGhvc2Ugc3luY2hyb25vdXMgY2FsbHMKKyAgICAgICAgbGF0ZXIg YW5kIGF2b2lkIHRoZSBkZWxheS1jYXVzaW5nIGRyYXdpbmcgYmVoYXZpb3IuCisKKyAgICAgICAg KiBVSVByb2Nlc3MvbWFjL1dLRnVsbFNjcmVlbldpbmRvd0NvbnRyb2xsZXIubW06CisgICAgICAg IChDR0ltYWdlRGVlcENvcHkpOiBBZGRlZCBhIGhlbHBlciBmdW5jdGlvbiB3aGljaCBjb3BpZXMg dGhlIGltYWdlIGRhdGEgaW50byBhIG5ldyBDR0ltYWdlLgorICAgICAgICAoLVtXS0Z1bGxTY3Jl ZW5XaW5kb3dDb250cm9sbGVyIGVudGVyRnVsbFNjcmVlbjpdKTogVXNlIHRoZSBhYm92ZS4KKwog MjAxMi0wNS0yNCAgSm9obiBNZWxsb3IgIDxqb2hubWVAY2hyb21pdW0ub3JnPgogCiAgICAgICAg IEZvbnQgQm9vc3Rpbmc6IEFkZCBjb21waWxlIGZsYWcgYW5kIHJ1bnRpbWUgc2V0dGluZwpkaWZm IC0tZ2l0IGEvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL21hYy9XS0Z1bGxTY3JlZW5XaW5kb3dD b250cm9sbGVyLm1tIGIvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL21hYy9XS0Z1bGxTY3JlZW5X aW5kb3dDb250cm9sbGVyLm1tCmluZGV4IDY0OTE3NjIxNTc1MzJiZWY4YjI2NzkxZjY5MTY0ZWNh NTgxZmY1OTIuLjg0MWIyYTg4Y2VkZjk1MWE5ZTg4OWQzNmVlOGVlYzRmY2UxNzAwNWQgMTAwNjQ0 Ci0tLSBhL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9tYWMvV0tGdWxsU2NyZWVuV2luZG93Q29u dHJvbGxlci5tbQorKysgYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvbWFjL1dLRnVsbFNjcmVl bldpbmRvd0NvbnRyb2xsZXIubW0KQEAgLTE4Myw2ICsxODMsMjMgQEAgc3RhdGljIGNvbnN0IE5T VGltZUludGVydmFsIERlZmF1bHRXYXRjaGRvZ1RpbWVySW50ZXJ2YWwgPSAxOwogI3ByYWdtYSBt YXJrIC0KICNwcmFnbWEgbWFyayBFeHBvc2VkIEludGVyZmFjZQogCitzdGF0aWMgUmV0YWluUHRy PENHSW1hZ2VSZWY+IENHSW1hZ2VEZWVwQ29weShDR0ltYWdlUmVmIHNvdXJjZUltYWdlKQorewor ICAgIHNpemVfdCB3aWR0aCA9IENHSW1hZ2VHZXRXaWR0aChzb3VyY2VJbWFnZSk7CisgICAgc2l6 ZV90IGhlaWdodCA9IENHSW1hZ2VHZXRIZWlnaHQoc291cmNlSW1hZ2UpOworICAgIHNpemVfdCBi aXRzUGVyQ29tcG9uZW50ID0gQ0dJbWFnZUdldEJpdHNQZXJDb21wb25lbnQoc291cmNlSW1hZ2Up OworICAgIHNpemVfdCBiaXRzUGVyUGl4ZWwgPSBDR0ltYWdlR2V0Qml0c1BlclBpeGVsKHNvdXJj ZUltYWdlKTsKKyAgICBzaXplX3QgYnl0ZXNQZXJSb3cgPSBDR0ltYWdlR2V0Qnl0ZXNQZXJSb3co c291cmNlSW1hZ2UpOworICAgIENHQ29sb3JTcGFjZVJlZiBjb2xvcnNwYWNlID0gQ0dJbWFnZUdl dENvbG9yU3BhY2Uoc291cmNlSW1hZ2UpOworICAgIENHQml0bWFwSW5mbyBiaXRtYXBJbmZvID0g Q0dJbWFnZUdldEJpdG1hcEluZm8oc291cmNlSW1hZ2UpOworICAgIFJldGFpblB0cjxDRkRhdGFS ZWY+IGRhdGEoQWRvcHRDRiwgQ0dEYXRhUHJvdmlkZXJDb3B5RGF0YShDR0ltYWdlR2V0RGF0YVBy b3ZpZGVyKHNvdXJjZUltYWdlKSkpOworICAgIFJldGFpblB0cjxDR0RhdGFQcm92aWRlclJlZj4g cHJvdmlkZXIoQWRvcHRDRiwgQ0dEYXRhUHJvdmlkZXJDcmVhdGVXaXRoQ0ZEYXRhKGRhdGEuZ2V0 KCkpKTsKKyAgICBib29sIHNob3VsZEludGVycG9sYXRlID0gQ0dJbWFnZUdldFNob3VsZEludGVy cG9sYXRlKHNvdXJjZUltYWdlKTsKKyAgICBDR0NvbG9yUmVuZGVyaW5nSW50ZW50IGludGVudCA9 IENHSW1hZ2VHZXRSZW5kZXJpbmdJbnRlbnQoc291cmNlSW1hZ2UpOworCisgICAgcmV0dXJuIFJl dGFpblB0cjxDR0ltYWdlUmVmPihBZG9wdENGLCBDR0ltYWdlQ3JlYXRlKHdpZHRoLCBoZWlnaHQs IGJpdHNQZXJDb21wb25lbnQsIGJpdHNQZXJQaXhlbCwgYnl0ZXNQZXJSb3csIGNvbG9yc3BhY2Us IGJpdG1hcEluZm8sIHByb3ZpZGVyLmdldCgpLCAwLCBzaG91bGRJbnRlcnBvbGF0ZSwgaW50ZW50 KSk7Cit9CisKIC0gKHZvaWQpZW50ZXJGdWxsU2NyZWVuOihOU1NjcmVlbiAqKXNjcmVlbgogewog ICAgIGlmIChfaXNGdWxsU2NyZWVuKQpAQCAtMjA0LDYgKzIyMSw5IEBAIHN0YXRpYyBjb25zdCBO U1RpbWVJbnRlcnZhbCBEZWZhdWx0V2F0Y2hkb2dUaW1lckludGVydmFsID0gMTsKICAgICBDR1dp bmRvd0lEIHdpbmRvd0lEID0gW1tfd2ViVmlldyB3aW5kb3ddIHdpbmRvd051bWJlcl07CiAgICAg UmV0YWluUHRyPENHSW1hZ2VSZWY+IHdlYlZpZXdDb250ZW50cyhBZG9wdENGLCBDR1dpbmRvd0xp c3RDcmVhdGVJbWFnZShOU1JlY3RUb0NHUmVjdCh3ZWJWaWV3RnJhbWUpLCBrQ0dXaW5kb3dMaXN0 T3B0aW9uSW5jbHVkaW5nV2luZG93LCB3aW5kb3dJRCwga0NHV2luZG93SW1hZ2VTaG91bGRCZU9w YXF1ZSkpOwogCisgICAgLy8gQ29weSB0aGUgaW1hZ2UgZGF0YSBpbnRvIG91ciBvd24gcHJvY2Vz cyBmcm9tIHRoZSBXaW5kb3dTZXJ2ZXI6CisgICAgd2ViVmlld0NvbnRlbnRzID0gQ0dJbWFnZURl ZXBDb3B5KHdlYlZpZXdDb250ZW50cy5nZXQoKSk7CisKICAgICAvLyBTY3JlZW4gdXBkYXRlcyB0 byBiZSByZS1lbmFibGVkIGluIGJlZ2FuRW50ZXJGdWxsU2NyZWVuV2l0aEluaXRpYWxGcmFtZTpm aW5hbEZyYW1lOgogICAgIE5TRGlzYWJsZVNjcmVlblVwZGF0ZXMoKTsKICAgICBbW3NlbGYgd2lu ZG93XSBzZXRBdXRvZGlzcGxheTpOT107Cg==