Bug 31856 – Make WebKit build on Windows with ACCELERATED_COMPOSITING turned on.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Chris Marrin]

This is needed not only to allow the Windows build to work without the DXSDK when ACCELERATED_COMPOSITING is turned off, but to allow implementations that don't have the DX9 runtime to work. When the library is not found we would turn off accelerated compositing.

[Chris Marrin]

There is more to the fix than just soft linking. I need to get rid of dependencies to make soft linking simpler and move a couple of things around to avoid hard links in header files. This bug is now the general "get accelerated compositing on Windows to build.

[Chris Marrin]

Created attachment 44094 [details]
Patch

soft links with d3d9, d3dx9 and QuartzCore using #pragma comment(). Also does runtime checks for presence of DLLs and turns off accelerated compositing if not found.

Now you can turn accerated compositing simply by going into wtf/Platform.h and changing USE_ACCELERATED_COMPOSITING from 0 to 1 in the windows ifdef, and going into WebCore/WebCore.vcproj/WebCoreCommon.vsprops and changing DISABLE_3D_RENDERING to ENABLED_3D_RENDERING.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/win/WKCACFLayer.h:60:  Missing spaces around =  [whitespace/operators] [4]
WebCore/platform/graphics/win/GraphicsLayerCACF.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/win/WKCACFLayer.cpp:112:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/win/WKCACFLayer.cpp:113:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/win/WKCACFLayer.cpp:114:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/win/WKCACFLayer.cpp:115:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/win/WKCACFLayer.cpp:122:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/win/WKCACFLayer.cpp:129:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/win/WKCACFLayer.cpp:130:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/win/WKCACFLayer.cpp:541:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/platform/graphics/win/WKCACFLayer.cpp:540:  Missing space before ( in switch(  [whitespace/parens] [5]
WebCore/platform/graphics/win/WKCACFLayer.cpp:567:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/platform/graphics/win/WKCACFLayer.cpp:566:  Missing space before ( in switch(  [whitespace/parens] [5]
Total errors found: 13

[Adam Barth]

Comment on attachment 44094 [details]
Patch

Please address the style issues.

[Vangelis Kokkevis]

Still including headers from the QuartzCore directory, eg in WebCore/platform/graphics/win/GraphicsLayerCACF.cpp :

...
#include <QuartzCore/CATransform3D.h>
...

As far as I can tell those are not available in the WebKit tree and the code won't build without them.

[Chris Marrin]

This fix is not intended to make it possible to build without QuartzCore when ACCELERATED_COMPOSITING is turned on. You still need the headers to build. And you're right they are not yet available (patience, my young Padawan). What this fix will do is to make it possible build without adding anything to the VC proj files that would generate errors without the libs. It will also make it possible to have WebKit run (with accelerated compositing disabled) if build with ACCELERATED_COMPOSITING and the dlls are missing.

This is just one step on the path.

[Adam Roben (:aroben)]

Comment on attachment 44094 [details]
Patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 51553)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,31 @@
> [phone]  Chris Marrin  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Delay load DLLs for accelerated compositing
> +        https://bugs.webkit.org/show_bug.cgi?id=31856
> +        
> +        This also reduces the number of files requiring external linkage to two
> +        and adds checks for the DLLs (d3d9 and QuartzCore). If not present it
> +        turns off accelerated compositing and avoids calling any of the functions
> +        in the DLLs.
> +
> +        * platform/graphics/win/GraphicsLayerCACF.cpp:
> +        (WebCore::GraphicsLayerCACF::GraphicsLayerCACF):
> +        (WebCore::GraphicsLayerCACF::updateLayerPreserves3D):
> +        (WebCore::GraphicsLayerCACF::updateContentsImage):
> +        * platform/graphics/win/WKCACFLayer.cpp:
> +        * platform/graphics/win/WKCACFLayer.h:
> +        (WebCore::WKCACFLayer::):
> +        * platform/graphics/win/WKCACFLayerRenderer.cpp:
> +        (WebCore::WKCACFLayerRenderer::setScrollFrame):
> +        (WebCore::WKCACFLayerRenderer::setRootContents):
> +        (WebCore::WKCACFLayerRenderer::setRootChildLayer):
> +        (WebCore::WKCACFLayerRenderer::setNeedsDisplay):
> +        (WebCore::WKCACFLayerRenderer::createRenderer):
> +        (WebCore::WKCACFLayerRenderer::render):
> +        * rendering/RenderLayerBacking.cpp:
> +
>  2009-12-01  Joseph Pecoraro  <[e-mail]>
>  
>          Reviewed by Timothy Hatcher.
> Index: WebCore/platform/graphics/win/GraphicsLayerCACF.cpp
> ===================================================================
> --- WebCore/platform/graphics/win/GraphicsLayerCACF.cpp	(revision 51545)
> +++ WebCore/platform/graphics/win/GraphicsLayerCACF.cpp	(working copy)
> @@ -38,6 +38,7 @@
>  #include "WKCACFLayer.h"
>  #include <wtf/CurrentTime.h>
>  #include <wtf/StringExtras.h>
> +#include <QuartzCore/CATransform3D.h>
>  
>  using namespace std;
>  
> @@ -123,7 +124,7 @@ GraphicsLayerCACF::GraphicsLayerCACF(Gra
>      , m_contentsLayerPurpose(NoContentsLayer)
>      , m_contentsLayerHasBackgroundColor(false)
>  {
> -    m_layer = WKCACFLayer::create(kCACFLayer, this);
> +    m_layer = WKCACFLayer::create(WKCACFLayer::Layer, this);
>      
>      updateDebugIndicators();
>  }
> @@ -536,7 +537,7 @@ void GraphicsLayerCACF::updateLayerPrese
>  {
>      if (m_preserves3D && !m_transformLayer) {
>          // Create the transform layer.
> -        m_transformLayer = WKCACFLayer::create(kCACFTransformLayer, this);
> +        m_transformLayer = WKCACFLayer::create(WKCACFLayer::TransformLayer, this);
>  
>  #ifndef NDEBUG
>          m_transformLayer->setName(String().format("Transform Layer CATransformLayer(%p) GraphicsLayer(%p)", m_transformLayer.get(), this));
> @@ -609,7 +610,7 @@ void GraphicsLayerCACF::updateContentsIm
>  {
>      if (m_pendingContentsImage) {
>          if (!m_contentsLayer.get()) {
> -            RefPtr<WKCACFLayer> imageLayer = WKCACFLayer::create(kCACFLayer, this);
> +            RefPtr<WKCACFLayer> imageLayer = WKCACFLayer::create(WKCACFLayer::Layer, this);
>  #ifndef NDEBUG
>              imageLayer->setName("Image Layer");
>  #endif
> @@ -620,7 +621,7 @@ void GraphicsLayerCACF::updateContentsIm
>  
>          // FIXME: maybe only do trilinear if the image is being scaled down,
>          // but then what if the layer size changes?
> -        m_contentsLayer->setMinificationFilter(kCACFFilterTrilinear);
> +        m_contentsLayer->setMinificationFilter(WKCACFLayer::Trilinear);
>          m_contentsLayer->setContents(m_pendingContentsImage.get());
>          m_pendingContentsImage = 0;
>          
> Index: WebCore/platform/graphics/win/WKCACFLayer.cpp
> ===================================================================
> --- WebCore/platform/graphics/win/WKCACFLayer.cpp	(revision 51545)
> +++ WebCore/platform/graphics/win/WKCACFLayer.cpp	(working copy)
> @@ -33,12 +33,39 @@
>  
>  #include <stdio.h>
>  #include <QuartzCore/CACFContext.h>
> +#include <QuartzCore/CACFLayer.h>
> +#include <QuartzCore/CACFVector.h>
>  #include <QuartzCore/CARender.h>
>  
> +#pragma comment(lib, "d3d9")
> +#pragma comment(lib, "d3dx9")
> +#pragma comment(lib, "QuartzCore")
> +
>  namespace WebCore {
>  
>  using namespace std;
>  
> +bool WKCACFLayer::acceleratedCompositingAvailable()
> +{
> +    static bool avail = false;
> +    static bool tested = false;

We normally omit initialization of statics to 0/false, since this happens automatically.

I'd rename "avail" to "available". I don't think the abbreviation buys you much here.

> +    if (!tested) {
> +        tested = true;
> +        HMODULE library = LoadLibrary(TEXT("d3d9.dll"));
> +        if (library) {
> +            FreeLibrary(library);
> +            library = LoadLibrary(TEXT("QuartzCore.dll"));
> +            if (library) {
> +                FreeLibrary(library);
> +                avail = true;
> +            }
> +        }
> +    }
> +
> +    return avail;

You could rewrite this to use early returns:

if (tested)
    return avail;
tested = true;
HMODULE library = ...;
if (!library)
    return false;
...
avail = true;
return avail;

> -WKCACFLayer::WKCACFLayer(CFStringRef className, GraphicsLayerCACF* owner)
> -    : m_layer(AdoptCF, CACFLayerCreate(className))
> +WKCACFLayer::WKCACFLayer(LayerType layerType, GraphicsLayerCACF* owner)
> +: m_layer(AdoptCF, CACFLayerCreate((layerType == TransformLayer) ? kCACFTransformLayer : kCACFLayer))
>      , m_needsDisplayOnBoundsChange(false)
>      , m_owner(owner)
>  {

You undid the correct indentation of the initializer. I'm surprised the style-queue didn't flag this.

> +    static PassRefPtr<WKCACFLayer> create(LayerType layerType, GraphicsLayerCACF* owner=0);
> +    static WKCACFLayer* layer(CACFLayerRef layer);

No need for the "layerType" and "layer" parameter names. You should add spaces around the = in the create() declaration.

> +    static RetainPtr<CFTypeRef> cfValue(const TransformationMatrix& value);
> +    static RetainPtr<CFTypeRef> cfValue(const FloatPoint& value);
> +    static RetainPtr<CFTypeRef> cfValue(const FloatRect& rect);
> +    static RetainPtr<CFTypeRef> cfValue(const Color& color);

No need for the parameter names here, or in most/all of the now-non-inline setters.

Why did all these functions need to become non-inline? The ChangeLog says "This also reduces the number of files requiring external linkage to two", but I don't see why that matters.

>  void WKCACFLayerRenderer::setScrollFrame(int width, int height, int scrollX, int scrollY)
>  {
> +    if (!WKCACFLayer::acceleratedCompositingAvailable())
> +        return;
> +

Can we prevent WKCACFLayerRenderers from being allocated when acceleratedCompositingAvailable() returns false (just like you do for WKCACFLayer)? That would reduce the number of checks you need to make, and reduce the possibility of people forgetting to check in the future.


> @@ -361,7 +373,7 @@ void WKCACFLayerRenderer::render(const V
>  
>          if (err == D3DERR_DEVICELOST) {
>              // Lost device situation.
> -            CARenderOGLPurge(m_renderer);
> +            //CARenderOGLPurge(m_renderer);
>              resetDevice();
>              CARenderUpdateAddRect(u, &bounds);
>          }

This looks like a mistake.

[Chris Marrin]

Created attachment 44197 [details]
Replacement patch

This reverts the change to reduce the dependencies (we agreed that this should be done separately, if at all) and deals with all style issues and comments from Adam

[Chris Marrin]

Created attachment 44198 [details]
Another replacement patch

The last patch was messed up, showing every line in WKCACFLayer.cpp having changed. I think MSDev torched the line endings of something. All better now.

[Chris Marrin]

Created attachment 44199 [details]
Same replacement patch with a better changelog message

I forgot to add a message to the Changelog explaining why I changed the WndProc in WebView to be a member function.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/win/WebView.cpp:1997:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebKit/win/WebView.cpp:2017:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebKit/win/WebView.cpp:2101:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 3

[Adam Roben (:aroben)]

Comment on attachment 44199 [details]
Same replacement patch with a better changelog message

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 51618)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,47 @@
> [phone]  Chris Marrin  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Delay load DLLs for accelerated compositing
> +        https://bugs.webkit.org/show_bug.cgi?id=31856
> +        
> +        If the DLLs (d3d9 and QuartzCore). are not present it
> +        turns off accelerated compositing and avoids calling 
> +        any of the functions in the DLLs.
> +
> +        * platform/graphics/win/WKCACFLayer.cpp:
> +        (WebCore::WKCACFLayer::acceleratedCompositingAvailable):
> +        (WebCore::displayInContext):
> +        (WebCore::WKCACFLayer::WKCACFLayer):
> +        (WebCore::WKCACFLayer::~WKCACFLayer):
> +        (WebCore::WKCACFLayer::display):
> +        (WebCore::WKCACFLayer::becomeRootLayerForContext):
> +        (WebCore::WKCACFLayer::setNeedsCommit):
> +        (WebCore::WKCACFLayer::addSublayer):
> +        (WebCore::WKCACFLayer::insertSublayer):
> +        (WebCore::WKCACFLayer::insertSublayerAboveLayer):
> +        (WebCore::WKCACFLayer::insertSublayerBelowLayer):
> +        (WebCore::WKCACFLayer::replaceSublayer):
> +        (WebCore::WKCACFLayer::removeFromSuperlayer):
> +        (WebCore::WKCACFLayer::removeSublayer):
> +        (WebCore::WKCACFLayer::indexOfSublayer):
> +        (WebCore::WKCACFLayer::ancestorOrSelfWithSuperlayer):
> +        (WebCore::WKCACFLayer::setBounds):
> +        (WebCore::WKCACFLayer::setFrame):
> +        (WebCore::WKCACFLayer::rootLayer):
> +        (WebCore::WKCACFLayer::removeAllSublayers):
> +        (WebCore::WKCACFLayer::setSublayers):
> +        (WebCore::WKCACFLayer::moveSublayers):
> +        (WebCore::WKCACFLayer::superlayer):
> +        (WebCore::WKCACFLayer::setNeedsDisplay):
> +        * platform/graphics/win/WKCACFLayer.h:
> +        (WebCore::WKCACFLayer::create):
> +        (WebCore::WKCACFLayer::cfValue):
> +        * platform/graphics/win/WKCACFLayerRenderer.cpp:
> +        (WebCore::WKCACFLayerRenderer::create):
> +        * platform/graphics/win/WKCACFLayerRenderer.h:
> +        * rendering/RenderLayerBacking.cpp:

I think many of these functions are no longer affected by this patch. Maybe you should regenerate the ChangeLog.

> +++ WebCore/platform/graphics/win/WKCACFLayer.cpp	(working copy)
> @@ -35,10 +35,37 @@
>  #include <QuartzCore/CACFContext.h>
>  #include <QuartzCore/CARender.h>
>  
> +#pragma comment(lib, "d3d9")
> +#pragma comment(lib, "d3dx9")
> +#pragma comment(lib, "QuartzCore")

It would be slightly more correct to only link against QuartzCore in this file, since nothing in this file explicitly requires D3D. Then you would like against D3D in WKCACFLayerRenderer.cpp. (Maybe someday in the Star Trek Future there will be a CACF implementation that doesn't use D3D, and this source file had better be ready for it!)

> +bool WKCACFLayer::acceleratedCompositingAvailable()
> +{
> +    static bool available;
> +    static bool tested;
> +
> +    if (tested)
> +        return available;
> +
> +    tested = true;
> +    HMODULE library = LoadLibrary(TEXT("d3d9.dll"));
> +    if (!library)
> +        return false;
> +
> +    FreeLibrary(library);
> +    library = LoadLibrary(TEXT("QuartzCore.dll"));
> +    if (!library)
> +        return false;
> +
> +    FreeLibrary(library);
> +    available = true;
> +    return available;
> +}

I think this might be more appropriate in WKCACFLayerRenderer.

> +WKCACFLayerRenderer* WKCACFLayerRenderer::create()
> +{
> +    if (!WKCACFLayer::acceleratedCompositingAvailable())
> +        return 0;
> +    return new WKCACFLayerRenderer;
> +}

This should return a PassOwnPtr<WKCACFLayerRenderer>.

> +        This patch also changes the WKCACFLayerRenderer to be a pointer.
> +        This allows me to have a create() method which will not create it when
> +        accelerated compositing is disabled because of missing DLLs. It 
> +        avoids having to do so many checks. I also made WebViewWndProc 
> +        a member function to allow several methods to be made protected, which
> +        allows me to avoid doing availability checks there as well.

I think you could get the same effect with many fewer changes by just making WebViewWndProc a static member function and not adding the wndProc non-static member function. Or you could keep WebViewWndProc a non-member and declare it as a friend. I think it's good to keep this patch as small and focused as possible. And I really like that you've minimized the number of acceleratedCompositingAvailable() checks!

> +    , m_layerRenderer(0)

You won't need this initializer if you make m_layerRenderer an OwnPtr.

>  WebView::~WebView()
>  {
> +#if USE(ACCELERATED_COMPOSITING)
> +    setAcceleratedCompositing(false);
> +#endif

I don't think you'll need to do this if you make m_layerRenderer and OwnPtr.

> -            if (SUCCEEDED(webView->uiDelegate(&uiDelegate)) && uiDelegate &&
> +            if (SUCCEEDED(WebView::uiDelegate(&uiDelegate)) && uiDelegate &&

We normally write this as "this->uiDelegate(&uiDelegate)", but hopefully you'll take my advice to revert these changes.

> +    if (accelerated) {
> +        if (m_layerRenderer)
> +            delete m_layerRenderer;

It's OK to try to delete 0. But if you make m_layerRenderer an OwnPtr you won't have to do this at all.

> -    void setRootLayerNeedsDisplay() { m_layerRenderer.setNeedsDisplay(); }
> +    void setRootLayerNeedsDisplay() { if (m_layerRenderer) m_layerRenderer->setNeedsDisplay(); }

Is it an error for this function to be called if m_layerRenderer is 0?

> -    WebCore::WKCACFLayerRenderer m_layerRenderer;
> +    WebCore::WKCACFLayerRenderer* m_layerRenderer;

You should make this an OwnPtr (if that wasn't clear by now :-)).

r- so we can reduce the number of changes to WebView.cpp (which I think will also get the style-queue to stop complaining).

[Chris Marrin]

Created attachment 44280 [details]
Replacement patch with all issues addressed.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44280 [details] without any errors.

[Adam Roben (:aroben)]

Comment on attachment 44280 [details]
Replacement patch with all issues addressed.

Yay, so much smaller!

But you never call WKCACFLayerRenderer::create()!

I also think we need to make sure the D3D/QuartzCore DLLs are delay-loaded so that WebKit can launch/run without them present. We can probably do this right in the vcproj (I don't think it's harmful to specify /delayload for a DLL that you don't end up linking against (in non-ACCELERATED_COMPOSITING builds)), or we could do it via more #pragma comment lines.

So close!

[Chris Marrin]

Created attachment 44320 [details]
Patch which fixes the line endings on WebView.cpp

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44320 [details] without any errors.

[Adam Roben (:aroben)]

Comment on attachment 44320 [details]
Patch which fixes the line endings on WebView.cpp

(I'll bet that the random style fixes scattered throughout this patch could be removed without angering the style-queue, now that those functions aren't otherwise being touched. But it's good to fix style issues!)

This all looks good except for the delay-load issue. r- so we can get that fixed.

[Adam Roben (:aroben)]

Comment on attachment 44320 [details]
Patch which fixes the line endings on WebView.cpp

Chris is going to implement delay-loading in a separate patch.

r=me on this one.

[Chris Marrin]

Patch landed in http://trac.webkit.org/changeset/51906

[Chris Marrin]

Created attachment 44562 [details]
Final Patch to enable ACCELERATED_COMPOSITING on Windows

This patch will build with accelerated compositing turned on, but only if you have the internal SDK. So OpenSource builds will still have it turned off.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitLibraries/win/include/QuartzCoreInterface/QuartzCoreInterface.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1

[Adam Roben (:aroben)]

Comment on attachment 44562 [details]
Final Patch to enable ACCELERATED_COMPOSITING on Windows

It might be worth adding a FIXME in Platform.h saying that defining ENABLE_3D_RENDERING in this file isn't really appropriate.

r=me

[Chris Marrin]

Created attachment 44625 [details]
Patch with some fixes from Adam

[Chris Marrin]

Created attachment 44636 [details]
One more try of final patch

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/win/WKCACFLayer.cpp:84:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/platform/graphics/win/WKCACFLayer.cpp:94:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/platform/graphics/win/WKCACFLayer.cpp:152:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 3

[Adam Barth]

> Attachment 44636 [details] did not pass style-queue:

Those style issues look like true positives, FWIW.

[Adam Roben (:aroben)]

Comment on attachment 44636 [details]
One more try of final patch

> +#define STATIC_CACF_STRING(name, qcName) \
> +    static CFStringRef name() \
> +    { \
> +        static RetainPtr<CFStringRef> name; \
> +        if (!name) \
> +            name = RetainPtr<CFStringRef>(AdoptCF, wkqcCFStringRef(wkqc##qcName)); \
> +        return name.get(); \
> +    }

It would be better to store a bare pointer so that an exit-time destructor isn't generated:

static CFStringRef name = wkqcCFStringRef(wkcq##qcName);
return name;

> +STATIC_CACF_STRING(ltTransformLayer, kCACFTransformLayer)

No need for the "lt" prefix here.

> +STATIC_CACF_STRING(Center, kCACFGravityCenter)
> +STATIC_CACF_STRING(Top, kCACFGravityTop)
> +STATIC_CACF_STRING(Bottom, kCACFGravityBottom)
> +STATIC_CACF_STRING(Left, kCACFGravityLeft)
> +STATIC_CACF_STRING(Right, kCACFGravityRight)
> +STATIC_CACF_STRING(TopLeft, kCACFGravityTopLeft)
> +STATIC_CACF_STRING(TopRight, kCACFGravityTopRight)
> +STATIC_CACF_STRING(BottomLeft, kCACFGravityBottomLeft)
> +STATIC_CACF_STRING(BottomRight, kCACFGravityBottomRight)
> +STATIC_CACF_STRING(Resize, kCACFGravityResize)
> +STATIC_CACF_STRING(ResizeAspect, kCACFGravityResizeAspect)
> +STATIC_CACF_STRING(ResizeAspectFill, kCACFGravityResizeAspectFill)
> +STATIC_CACF_STRING(Linear, kCACFFilterLinear)
> +STATIC_CACF_STRING(Nearest, kCACFFilterNearest)
> +STATIC_CACF_STRING(Trilinear, kCACFFilterTrilinear)
> +STATIC_CACF_STRING(Lanczos, kCACFFilterLanczos)

It's strange for all these functions to start with an uppercase letter.

> +static CFStringRef toCACFLayerType(WKCACFLayer::LayerType type)
> +{
> +    switch (type)
> +    {

I think the brace is supposed to go on the same line as the switch keyword. (The same thing occurs elsewhere in this patch.)

> +        case WKCACFLayer::Layer: return wkqcCFStringRef(wkqckCACFLayer);
> +        case WKCACFLayer::TransformLayer: return wkqcCFStringRef(wkqckCACFTransformLayer);

Why not use your Layer() and TransformLayer() functions here?

> +static WKCACFLayer::ContentsGravityType fromCACFContentsGravityType(CFStringRef string)
> +{
> +    if (CFStringCompare(string, Top(), 0) == kCFCompareEqualTo)
> +        return WKCACFLayer::Top;
> +
> +    if (CFStringCompare(string, Bottom(), 0) == kCFCompareEqualTo)
> +        return WKCACFLayer::Bottom;
> +
> +    if (CFStringCompare(string, Left(), 0) == kCFCompareEqualTo)
> +        return WKCACFLayer::Left;
> +
> +    if (CFStringCompare(string, Right(), 0) == kCFCompareEqualTo)
> +        return WKCACFLayer::Right;
> +
> +    if (CFStringCompare(string, TopLeft(), 0) == kCFCompareEqualTo)
> +        return WKCACFLayer::TopLeft;
> +
> +    if (CFStringCompare(string, TopRight(), 0) == kCFCompareEqualTo)
> +        return WKCACFLayer::TopRight;
> +
> +    if (CFStringCompare(string, BottomLeft(), 0) == kCFCompareEqualTo)
> +        return WKCACFLayer::BottomLeft;
> +
> +    if (CFStringCompare(string, BottomRight(), 0) == kCFCompareEqualTo)
> +        return WKCACFLayer::BottomRight;
> +
> +    if (CFStringCompare(string, Resize(), 0) == kCFCompareEqualTo)
> +        return WKCACFLayer::Resize;
> +
> +    if (CFStringCompare(string, ResizeAspect(), 0) == kCFCompareEqualTo)
> +        return WKCACFLayer::ResizeAspect;
> +
> +    if (CFStringCompare(string, ResizeAspectFill(), 0) == kCFCompareEqualTo)
> +        return WKCACFLayer::ResizeAspectFill;

I think you can use CFEqual here and elsewhere in this patch.

r=me, but do consider fixing the above issues.

[Chris Marrin]

Landed in http://trac.webkit.org/changeset/52006