Bug 27314 – Implement Accelerated Compositing on Windows (3D CSS Transforms)

• Component: CSS
• Status: RESOLVED FIXED

[Chris Foresman]

3D CSS Transforms, currently a draft specification with the W3C ( ), are enabled in builds of WebKit on iPhone OS, Safari 4 on Mac OS X 10.6, and nightly builds of WebKit for Mac OS X 10.5.x. Request that support be enabled for the Windows port of WebKit/Safari 4.

[Mark Rowe (bdash)]

*** Bug 27350 has been marked as a duplicate of this bug. ***

[Chris Foresman]

Adding comments from Michael Davy from duplicate bug 27350, as I feel they offer a succinct justification for enabling support on Windows:

*  The features were demonstrated at WWDC as being features of Safari 4, yet
the Windows version of Safari is shipped without the feature.
*  Providing the feature only on some platforms causes a disparity which,
subject to the provision of detailed technical information to the contrary,
would seem to be arbitrary
*  As I understand it, Apple intend for these feature to become part of the
CSS3 specification.  Demonstrating the ability to implement the features cross
platform would seem to be an important step to achieving this.

[Chris Marrin]

Created attachment 41071 [details]
First patch (new files)

[Chris Marrin]

Created attachment 41072 [details]
Second patch (hooking up accelerated compositing, but not building)

This second patch contains all the changes to existing logic, all behind the USE_ACCELERATED_COMPOSITING flag

[Chris Marrin]

Created attachment 41073 [details]
Third patch (adding files to build - off by default)

[Simon Fraser (smfr)]

Comment on attachment 41071 [details]
First patch (new files)

In general, I wonder if we should try to share more code between the Mac and Windows implementations.
I feel like we're going to be fixing all the bugs twice if the code is in both.

> Index: WebCore/platform/graphics/win/GraphicsLayerCACF.cpp
> ===================================================================

> +namespace WebCore {
> +
> +// The threshold width or height above which a tiled layer will be used. This should be
> +// large enough to avoid tiled layers for most GraphicsLayers, but less than the OpenGL
> +// texture size limit on all supported hardware.
> +static const int cMaxPixelDimension = 2000;
> +
> +// The width and height of a single tile in a tiled layer. Should be large enough to
> +// avoid lots of small tiles (and therefore lots of drawing callbacks), but small enough
> +// to keep the overall tile cost low.
> +static const int cTiledLayerTileSize = 512;

I think you should just remove all the tiled layer-related code.

> +} // namespace WebCore
> +
> +namespace WebCore {

Why close and re-open the namespace?

> +#ifndef NDEBUG
> +bool GraphicsLayer::showDebugBorders()
> +{
> +    static bool showDebugBorders = false; //[[NSUserDefaults standardUserDefaults] boolForKey:@"WebCoreLayerBorders"];
> +    return showDebugBorders;
> +}
> +
> +bool GraphicsLayer::showRepaintCounter()
> +{
> +    static bool showRepaintCounter = false; //[[NSUserDefaults standardUserDefaults] boolForKey:@"WebCoreLayerRepaintCounter"];
> +    return showRepaintCounter;
> +}
> +#endif

You shouldn't check in commented out code. We should figure out how to store preferences on Windows, or remove these methods.

> +void GraphicsLayerCACF::addChildBelow(GraphicsLayer* childLayer, GraphicsLayer* sibling)
> +{
> +    // FIXME share code
> +    ASSERT(!childLayer->parent());
> +    ASSERT(childLayer != this);

This is what used to be in GraphicsLayerCA.mm before I implemented batching. I wonder if it makes sense to share the same code on Windows but just commit the batch more eagerly? In addition, the change to use batching also fixed a number of bugs; you're copying older code with bugs in here.

> +void GraphicsLayerCACF::removeFromParent()
> +{
> +    GraphicsLayer::removeFromParent();
> +
> +    
> +    layerForSuperlayer()->removeFromSuperlayer();            
> +}

Extra space here.

> +
> +void GraphicsLayerCACF::setPosition(const FloatPoint& point)
> +{
> +    // don't short-circuit here, because position and anchor point
> +    // are inter-related
> +    GraphicsLayer::setPosition(point);
> +
> +    // position is offset on the layer by the layer anchor point
> +    CGPoint posPoint = CGPointMake(m_position.x() + m_anchorPoint.x() * m_size.width(),
> +                                   m_position.y() + m_anchorPoint.y() * m_size.height());
> +    
> +    primaryLayer()->setPosition(posPoint);
> +}
> +
> +void GraphicsLayerCACF::setAnchorPoint(const FloatPoint3D& point)
> +{
> +    // Don't short-circuit here, because position and anchor point are inter-dependent.

Use this same comment formatting in setPosition().

> +void GraphicsLayerCACF::setOpacity(float opacity)
> +{
> +    float clampedOpacity = std::max(std::min(opacity, 1.0f), 0.0f);

You did 'using namespace std' at the top so this should obmit the std::

> +void GraphicsLayerCACF::setNeedsDisplayInRect(const FloatRect& rect)
> +{
> +    if (drawsContent()) {
> +		CGRect r;
> +		r.origin.x = rect.x(); r.origin.y = rect.y();
> +		r.size.width = rect.width(); r.size.height = rect.height();

Tabs here!

There's an operator CGRect on FloatRect; no need to convert here.

> +void GraphicsLayerCACF::setContentsToImage(Image* image)
> +{
> +    if (image) {
> +        if (!m_contentsLayer.get()) {
> +
> +            RefPtr<WKCACFLayer> imageLayer = WKCACFLayer::create(kCACFLayer, this);
> +#ifndef NDEBUG
> +            imageLayer->setName("Image Layer");
> +#endif
> +            setupContentsLayer(imageLayer.get());
> +        }
> +
> +        // FIXME: maybe only do trilinear if the image is being scaled down,
> +        // but then what if the layer size changes?
> +        m_contentsLayer->setMinificationFilter(kCACFFilterTrilinear);
> +        CGImageRef theImage = image->nativeImageForCurrentFrame();
> +        m_contentsLayer->setContents(theImage);

This is missing the bug fix related to image color profiles.

> +void GraphicsLayerCACF::setupContentsLayer(WKCACFLayer* contentsLayer)
> +{
> +    if (contentsLayer == m_contentsLayer)
> +        return;
> +
> +    if (m_contentsLayer) {
> +        m_contentsLayer->removeFromSuperlayer();
> +        m_contentsLayer = 0;
> +    }
> +    
> +    if (contentsLayer) {
> +        // Turn off implicit animations on the inner layer.
> +        m_contentsLayer = contentsLayer;

The comment is not relevant.

> +#endif // USE(ACCELERATED_COMPOSITI
> \ No newline at end of file

Looks like you truncated the file here.

> Property changes on: WebCore/platform/graphics/win/GraphicsLayerCACF.cpp
> ___________________________________________________________________
> Added: svn:executable

Really? Other files have this too.

> Index: WebCore/platform/graphics/win/GraphicsLayerCACF.h
> ===================================================================

> +    // return true if we started an animation
> +    virtual void setOpacity(float opacity);

Comment is bogus.

> Index: WebCore/platform/graphics/win/WKCACFContextFlusher.cpp
> ===================================================================

> +/*
> + *  ContextFlusher.cpp
> + *  WebCore
> + *
> + *  Created by Adam Roben on 6/24/08.
> + *  Adapted from ContextFlusher by Chris Marrin on 10/21/08.
> + *  Copyright 2008 Apple Inc. All rights reserved.
> + *
> + */

Needs a real license.

Does the whole file need #if USE(ACCELERATED_COMPOSITING) ?

> +#include <QuartzCore/CACFContext.h>

How will people building WebKit standalone get that header file?

> +void WKCACFContextFlusher::flushAllContexts()
> +{
> +    // addContext might get called beneath CACFContextFlush, and we don't want m_contexts to change while
> +    // we're iterating over it, so we move the contexts into a local ContextSet and iterate over that instead.
> +    ContextSet contextsToFlush;
> +    contextsToFlush.swap(m_contexts);
> +
> +    ContextSet::const_iterator end = contextsToFlush.end();
> +    for (ContextSet::const_iterator it = contextsToFlush.begin(); it != end; ++it) {
> +        CACFContextRef context = *it;
> +        CACFContextFlush(context);
> +        didFlushContext(context);
> +        CFRelease(context);

Why is this CFRelease here? Did *it implicitly CFRetain?

> Index: WebCore/platform/graphics/win/WKCACFContextFlusher.h
> ===================================================================

> +/*
> + *  ContextFlusher.h
> + *  WebCore
> + *
> + *  Created by Adam Roben on 6/24/08.
> + *  Adapted from ContextFlusher by Chris Marrin on 10/21/08.
> + *  Copyright 2008 Apple Inc. All rights reserved.
> + *
> + */

Needs a real license.

Does the whole file need #if USE(ACCELERATED_COMPOSITING) ?

> +
> +#ifndef WKCACFContextFlusher_h
> +#define WKCACFContextFlusher_h
> +
> +#include "config.h"

config.h should not be included in header files.

> Index: WebCore/platform/graphics/win/WKCACFLayer.cpp
> ===================================================================
> +/*
> + *  WKCACFLayer.cpp
> + *  WebCore
> + *
> + *  Created by Adam Roben on 6/24/08.
> + *  Adapted from CoreAnimationLayer by Chris Marrin on 10/21/08.
> + *  Copyright 2008 Apple Inc. All rights reserved.
> + *
> + */

Needs license.

Does the whole file need #if USE(ACCELERATED_COMPOSITING) ?

> +
> +#include "WKCACFLayer.h"
> +
> +#include "WKCACFContextFlusher.h"
> +
> +#include <stdio.h>
> +#include <QuartzCore/CACFContext.h>
> +#include <QuartzCore/CARender.h>
> +#include <QuartzCore/CATransform3DPrivate.h>

Can non-Apple people build this?

> +    // These macros were copied from QuartzCore/LayerKit/utilities/CACGUtil.h

I don't think you should say this here.

> +void WKCACFLayer::display(PlatformGraphicsContext* context)
> +{
> +    if (!m_owner)
> +        return;
> +
> +    CGContextSaveGState(context);
> +
> +    CGRect layerBounds = bounds();
> +    if (m_owner->contentsOrientation() == WebCore::GraphicsLayer::CompositingCoordinatesBottomUp) {
> +        CGContextScaleCTM(context, 1, -1);
> +        CGContextTranslateCTM(context, 0, -layerBounds.size.height);
> +    }
> +
> +    if (m_owner->client()) {
> +        GraphicsContext graphicsContext(context);
> +
> +        // It's important to get the clip from the context, because it may be significantly
> +        // smaller than the layer bounds (e.g. tiled layers)
> +        CGRect clipBounds = CGContextGetClipBoundingBox(context);
> +        IntRect clip(enclosingIntRect(clipBounds));
> +        m_owner->paintGraphicsLayerContents(graphicsContext, clip);
> +    }
> +#ifndef NDEBUG
> +    else {
> +        ASSERT_NOT_REACHED();
> +
> +        // FIXME: ideally we'd avoid calling -setNeedsDisplay on a layer that is a plain color,
> +        // so CA never makes backing store for it (which is what -setNeedsDisplay will do above).
> +        CGContextSetRGBFillColor(context, 0.0f, 1.0f, 0.0f, 1.0f);
> +        CGContextFillRect(context, layerBounds);
> +    }
> +#endif
> +
> +#ifndef NDEBUG
> +    if (m_owner->showRepaintCounter()) {
> +        char text[16]; // that's a lot of repaints
> +        _snprintf(text, sizeof(text), "%d", m_owner->incrementRepaintCount());
> +
> +        CGContextSaveGState(context);
> +        CGContextSetRGBFillColor(context, 1.0f, 0.0f, 0.0f, 0.8f);
> +        
> +        CGRect aBounds = layerBounds;
> +
> +        aBounds.size.width = 10 + 12 * strlen(text);
> +        aBounds.size.height = 25;
> +        CGContextFillRect(context, aBounds);
> +        
> +        CGContextSetRGBFillColor(context, 0.0f, 0.0f, 0.0f, 1.0f);
> +
> +        CGContextSetTextMatrix(context, CGAffineTransformMakeScale(1.0f, -1.0f));
> +        CGContextSelectFont(context, "Helvetica", 25, kCGEncodingMacRoman);
> +        CGContextShowTextAtPoint(context, aBounds.origin.x + 3.0f, aBounds.origin.y + 20.0f, text, strlen(text));
> +        
> +        CGContextRestoreGState(context);        
> +    }
> +#endif
> +
> +    CGContextRestoreGState(context);
> +}

I'm not sure this code belongs here. WKCACFLayer is really just a C++ wrapper around CACFLayer, and, and such, it should not have anything WebKit-specific. I think you need a subclass which knows about GraphicsContexts and painting.

> +void WKCACFLayer::layout()
> +{
> +	if (m_owner)
> +		m_owner->layout();
> +}

Tabs here.

> +void WKCACFLayer::becomeLayerForContext(CACFContextRef context)
> +{
> +    CACFContextSetLayer(context, layer());
> +    setNeedsCommit();
> +}
> +
> +void WKCACFLayer::setNeedsCommit()
> +{
> +    WKCACFContextFlusher::shared().addContext(CACFLayerGetContext(rootLayer()->layer()));
> +    if (m_owner)
> +        m_owner->notifySyncRequired();
> +}

notifySyncRequired() is only required if you do layer property batching.

> +void WKCACFLayer::insertSublayer(PassRefPtr<WKCACFLayer> prpSublayer, size_t index)

prpSublayer? No Hungarian notation please. Why is it a PassRefPtr when the code immediately makes a RefPtr out of it?

> +void WKCACFLayer::setSublayersByAdoptingVector(Vector<RefPtr<WKCACFLayer> >& sublayers)
> +{
> +    checkSublayersConsistency();
> +
> +    if (sublayers.isEmpty()) {
> +        CACFLayerSetSublayers(layer(), 0);
> +        m_sublayers.clear();
> +    } else {
> +        // FIXME: This call to CACFLayerSetSublayers is a workaround for
> +        // <rdar://6199008>. We can remove it once that bug is fixed.
> +        CACFLayerSetSublayers(layer(), 0);
> +
> +        // Create a vector of CACFLayers.
> +        Vector<const void*> layers;
> +        for (size_t i = 0; i < sublayers.size(); i++)
> +            layers.append(sublayers[i]->layer());
> +    
> +        RetainPtr<CFArrayRef> layersArray(AdoptCF, CFArrayCreate(0, layers.data(), layers.size(), 0));
> +    
> +        CACFLayerSetSublayers(layer(), layersArray.get());
> +    
> +        m_sublayers.clear();
> +        m_sublayers.swap(sublayers);
> +    }
> +    
> +    setNeedsCommit();
> +
> +    checkSublayersConsistency();
> +}

I don't really like how both WKCACFLayer and CACFLayer both store an array of sublayers. That means that both have to be kept in sync, and is yet another paralell tree that we have to maintain (GraphicsLayer also has one). If you can always get from a CACFLayer to a WKCACFLayer, then no extra sublayer storage is required.


> +#endif // CHECK_SUBLAYERS_CONSISTENCY
> +
> +}
> \ No newline at end of file

Fix this.

> Index: WebCore/platform/graphics/win/WKCACFLayer.h
> ===================================================================
> --- WebCore/platform/graphics/win/WKCACFLayer.h	(revision 0)
> +++ WebCore/platform/graphics/win/WKCACFLayer.h	(revision 0)
> @@ -0,0 +1,255 @@
> +/*
> + *  WKCACFLayer.h
> + *  WebKit
> + *
> + *  Created by Adam Roben on 6/11/08.
> + *  Adapted from CoreAnimationLayer by Chris Marrin on 10/21/08.
> + *  Copyright 2008 Apple Inc. All rights reserved.
> + *
> + */

Needs license.


> +#pragma mark Conversion to CFType
> +    static RetainPtr<CFTypeRef> cfValue(float value) { return RetainPtr<CFTypeRef>(AdoptCF, CFNumberCreate(0, kCFNumberFloat32Type, &value)); }
> +    static RetainPtr<CFTypeRef> cfValue(const TransformationMatrix& value)
> +	{
> +		CATransform3D t;
> +		t.m11 = value.m11(); t.m12 = value.m12(); t.m13 = value.m13(); t.m14 = value.m14(); 
> +		t.m21 = value.m21(); t.m22 = value.m22(); t.m23 = value.m23(); t.m24 = value.m24(); 
> +		t.m31 = value.m31(); t.m32 = value.m32(); t.m33 = value.m33(); t.m34 = value.m34(); 
> +		t.m41 = value.m41(); t.m42 = value.m42(); t.m43 = value.m43(); t.m44 = value.m44(); 
> +		return RetainPtr<CFTypeRef>(AdoptCF, CACFVectorCreateTransform(t));
> +	}
> +    static RetainPtr<CFTypeRef> cfValue(const FloatPoint& value)
> +	{
> +		CGPoint p;
> +		p.x = value.x(); p.y = value.y();
> +		return RetainPtr<CFTypeRef>(AdoptCF, CACFVectorCreatePoint(p));
> +	}
> +    static RetainPtr<CFTypeRef> cfValue(const FloatRect& rect)
> +    {
> +		CGRect r;
> +		r.origin.x = rect.x(); r.origin.y = rect.y();
> +		r.size.width = rect.width(); r.size.height = rect.height();
> +		CGFloat v[4] = { CGRectGetMinX(r), CGRectGetMinY(r), CGRectGetMaxX(r), CGRectGetMaxY(r) };

Lots of tabs here.

> +#pragma mark Coordinate Mapping

I don't think you should use #pragma mark in WebCore code.

> +    // FIXME: Make this private once <rdar://6200213> is fixed and the
> +    // workaround in CylindricalGridLayer.cpp has been removed.
> +    void setNeedsCommit();

Comment is not relevant here.

> Index: WebCore/platform/graphics/win/WKCACFLayerWindow.cpp
> ===================================================================
> +/*
> + *  WKCACFLayerWindow.cpp
> + *  WebCore
> + *
> + *  Created by Adam Roben on 6/24/08.
> + *  Adapted from CALayerWindow by Chris Marrin on 10/21/08.
> + *  Copyright 2008 Apple Inc. All rights reserved.
> + *
> + */

License needed.

> +#include "WKCACFLayerWindow.h"
> +
> +#include "WKCACFContextFlusher.h"
> +#include "WKCACFLayer.h"
> +//#include "DrawingUtilities.h"

Remove it if it's not needed.

> +// pulled from DrawingUtilities.h

Not relevant here.

> +static D3DPRESENT_PARAMETERS initialPresentationParameters()
> +{
> +    D3DPRESENT_PARAMETERS parameters = {0};
> +    parameters.Windowed = TRUE;
> +    parameters.SwapEffect = D3DSWAPEFFECT_COPY;
> +    parameters.BackBufferCount = 1;
> +    parameters.BackBufferFormat = D3DFMT_A8R8G8B8;
> +    parameters.MultiSampleType = D3DMULTISAMPLE_NONE;
> +
> +	return parameters;

Tab here.

> +void WKCACFLayerWindow::setScrollFrame(int width, int height, int scrollX, int scrollY)
> +{
> +    CGRect contentsRect = bounds();
> +    contentsRect.size.width = width;
> +    contentsRect.size.height = height;
> +    contentsRect.origin.x = scrollX;
> +    contentsRect.origin.y = scrollY;

Why init the rect to bounds() and then replace every member?

> +void WKCACFLayerWindow::setRootContents(CGImageRef image)
> +{
> +    ASSERT(m_rootLayer);
> +    m_rootLayer->setContents(image);
> +	renderSoon();

Tab here.

> +void WKCACFLayerWindow::setRootChildLayer(WebCore::PlatformLayer* layer)
> +{
> +    if (!m_scrollLayer)
> +        return;
> +
> +    m_scrollLayer->removeAllSublayers();
> +    if (layer)
> +	    m_scrollLayer->addSublayer(layer);

Tab here.

> +	// D3D doesn't like to make back buffers for 0 size windows. We skirt this problem if we make the
> +	// passed backbuffer width and height non-zero. The window will necessarily get set to a non-zero
> +	// size eventually, and then the backbuffer size will get reset.
> +    RECT rect;
> +    GetClientRect(m_hostWindow, &rect);
> +
> +	if (rect.left-rect.right == 0 || rect.bottom-rect.top == 0) {
> +		parameters.BackBufferWidth = 1;
> +		parameters.BackBufferHeight = 1;
> +	}

Lots of tabs.

> +    // Create the root hierarchy
> +	m_rootLayer = WKCACFLayer::create(kCACFLayer);
> +	m_scrollLayer = WKCACFLayer::create(kCACFLayer);

Tabs.

> +void WKCACFLayerWindow::resize()
> +{
> +    if (!m_d3dDevice)
> +        return;
> +
> +    resetDevice();
> +
> +    if (m_rootLayer) {
> +        m_rootLayer->setFrame(bounds());
> +
> +        // FIXME: This should be the bounds inside the scrollbars
> +        m_scrollLayer->setFrame(bounds());

It worries me that there's code down here that has to know about scrollbars.

> +void WKCACFLayerWindow::render(const Vector<CGRect>& dirtyRects)
> +{
> +    ASSERT(m_d3dDevice);
> +
> +    // Flush the root layer to the render tree.
> +    WKCACFContextFlusher::shared().flushAllContexts();
> +
> +    CGRect bounds = this->bounds();
> +
> +    CFTimeInterval t = CACurrentMediaTime();
> +
> +    char space[4096];

This is very myserious. What is it for?

> +    CARenderUpdate* u = CARenderUpdateBegin(space, sizeof(space), t, 0, 0, &bounds);

> +        /* FIXME: don't need to clear dirty region if layer tree is opaque. */

The FIXMEs need bugs filed on them. Use C++ comments, not C-style comments.

> +        if (err == D3DERR_DEVICELOST) {
> +            /* Lost device situation. */

Use C++ comments.

> +    CARenderUpdateFinish (u);

No space before (

> +void WKCACFLayerWindow::renderSoon()
> +{
> +    m_renderTimer.startOneShot(0);

Should this make sure the timer isn't already active?
> +}
> \ No newline at end of file

Fix this.

> Index: WebCore/platform/graphics/win/WKCACFLayerWindow.h
> ===================================================================
> +/*
> + *  WKCACFLayerWindow.h
> + *  WebCore
> + *
> + *  Created by Adam Roben on 6/24/08.
> + *  Adapted from CALayerWindow by Chris Marrin on 10/21/08.
> + *  Copyright 2008 Apple Inc. All rights reserved.
> + *
> + */

Needs license.


So my main concerns here are:
1. Is it buildable by someone outside of Apple?
2. GraphicsLayerCACF should be closer to GraphicsLayerCA, and maybe share some code.
3. The extra sublayer storage by WKCACFLayer
4. WKCACFLayer knowing about WebCore stuff

[Simon Fraser (smfr)]

Comment on attachment 41072 [details]
Second patch (hooking up accelerated compositing, but not building)

> Index: WebKit/win/WebView.cpp
> ===================================================================
> --- WebKit/win/WebView.cpp	(revision 49310)
> +++ WebKit/win/WebView.cpp	(working copy)
> @@ -321,6 +321,7 @@ WebView::WebView()
>  , m_lastPanY(0)
>  , m_xOverpan(0)
>  , m_yOverpan(0)
> +, m_isAcceleratedCompositing(false)

So accelerated compositing is a property of the entire WebView, rather than a single WebDocumentView? Does it work in framesets and iframes?

>  void WebView::repaint(const WebCore::IntRect& windowRect, bool contentChanged, bool immediate, bool repaintContentOnly)
>  {
> +#if USE(ACCELERATED_COMPOSITING)
> +    if (isAcceleratedCompositing())
> +	    setRootLayerNeedsDisplay();
> +#endif

Tab here.

> +#if USE(ACCELERATED_COMPOSITING)
> +    if (!isAcceleratedCompositing()) {
> +#endif

I don't like hiding logic flow inside of #ifdefs. Can the common code be moved into a new method?

> +#if USE(ACCELERATED_COMPOSITING)
> +    } else {
> +        // Get the backing store into a CGImage
> +        BITMAP bitmap;
> +        GetObject(m_backingStoreBitmap.get(), sizeof(bitmap), &bitmap);
> +        int bmSize = bitmap.bmWidthBytes * bitmap.bmHeight;
> +        RetainPtr<CFDataRef> data(AdoptCF, 
> +                                    CFDataCreateWithBytesNoCopy(
> +                                            0, static_cast<UInt8*>(bitmap.bmBits),
> +                                            bmSize, kCFAllocatorNull));
> +        CGDataProviderRef cgData = CGDataProviderCreateWithCFData(data.get());
> +        CGColorSpaceRef space = CGColorSpaceCreateDeviceRGB();
> +        m_backingStoreImage.adoptCF(CGImageCreate(bitmap.bmWidth, bitmap.bmHeight,
> +                                         8, bitmap.bmBitsPixel, 
> +                                         bitmap.bmWidthBytes, space, 
> +                                         kCGBitmapByteOrder32Little | kCGImageAlphaNoneSkipFirst,
> +                                         cgData, 0, false, 
> +                                         kCGRenderingIntentDefault));
> +
> +        // Hand the CGImage to CACF for compositing
> +        m_layerWindow.setRootContents(m_backingStoreImage.get());
> +        m_layerWindow.setScrollFrame(frameView->layoutWidth(), frameView->layoutHeight(), 
> +                                     frameView->scrollX(), frameView->scrollY());
> +        CGColorSpaceRelease(space);
> +        CGDataProviderRelease(cgData);
> +    }

Does this blow away any repaint rect optimizations that the non-accelerated path uses?

> +#if USE(ACCELERATED_COMPOSITING)
> +                if (webView->isAcceleratedCompositing())
> +				    webView->setRootLayerNeedsDisplay();
> +#endif

Tabs here.

> +#if USE(ACCELERATED_COMPOSITING)
> +void WebView::setRootChildLayer(WebCore::PlatformLayer* layer)
> +{
> +    setAcceleratedCompositing(layer ? true : false);
> +    m_layerWindow.setRootChildLayer(layer);
> +}
> +
> +void WebView::setAcceleratedCompositing(bool accelerated)
> +{
> +    if (m_isAcceleratedCompositing == accelerated)
> +        return;
> +
> +    m_isAcceleratedCompositing = accelerated;
> +    if (m_isAcceleratedCompositing) {
> +	    // Create the root layer
> +        ASSERT(m_viewWindow);
> +	    m_layerWindow.setHostWindow(m_viewWindow);
> +    } else {
> +        // Tear down the root layer
> +        m_layerWindow.destroyRenderer();
> +    }
> +}

Tabs here.

> Index: WebKit/win/WebView.h
> ===================================================================

> +#if USE(ACCELERATED_COMPOSITING)
> +    void setRootLayerNeedsDisplay()
> +    {
> +        //m_layerWindow.setRootContents(m_backingStoreImage.get());
> +        m_layerWindow.setNeedsDisplay();
> +    }

Remove commented code if it's not needed.

> +    void resizeLayerWindow() { m_layerWindow.resize(); }
> +    void showLayerWindow() { m_layerWindow.createRenderer(); }

showLayerWindow() sounds like an imperative. It's more layerWindowBecameVisible or something.


> +#if USE(ACCELERATED_COMPOSITING)
> +    void setAcceleratedCompositing(bool);
> +	WebCore::WKCACFLayerWindow m_layerWindow;

Tab here.

> Index: WebKit/win/WebCoreSupport/WebChromeClient.cpp
> ===================================================================
> +#if USE(ACCELERATED_COMPOSITING)
> +void WebChromeClient::attachRootGraphicsLayer(Frame* frame, GraphicsLayer* graphicsLayer)
> +{
> +	m_webView->setRootChildLayer(graphicsLayer ? graphicsLayer->platformLayer() : 0);

Tab here.

[Brent Fulgham]

Given the need to also implement WebGL (e.g., Bug 28018), I'm curious if the use of Direct3D means there will be replicated codepaths to set up OpenGL scenes and Direct3D scenes under Windows.  I'm not sure how often both 3D CSS and WebGL will be used together, but is there any concern about contention between the 3D stacks over the hardware resources?

[Chris Marrin]

Both OpenGL and Direct3D can handle multiple contexts without any trouble. There is always the issue of running out of resources, but that's the case in general for WebKit or any app on Windows. I don't think this implementation exacerbates that issue.

[Yong Li]

Comment on attachment 41073 [details]
Third patch (adding files to build - off by default)

Let commit bot land it

[Chris Marrin]

The third patch cannot be committed until the other two are. Please do not submit to the commit queue

[Adam Barth]

Comment on attachment 41072 [details]
Second patch (hooking up accelerated compositing, but not building)

Instructed not to commit with queue.

[Chris Marrin]

Issues below addressed by upcoming patch:

Simon Fraser said:

> So my main concerns here are:
> 1. Is it buildable by someone outside of Apple?

I got rid of the private headers. And QuartzCore will be shipped as a DLL and includes

> 2. GraphicsLayerCACF should be closer to GraphicsLayerCA, and maybe share some code.

We discussed this. I think we should hold off on this for a while.

> 3. The extra sublayer storage by WKCACFLayer

Gone.

> 4. WKCACFLayer knowing about WebCore stuff

WKCACFLayer is more like a combination of a wrapper around CACFLayer and WebLayer. In fact the code you cite is from WebLayer. I think it's overkill to split these at this point. Other than that this code is all platform legal. It does reach outside platform for anything.

[Chris Marrin]

Looking at Simon's WebView comments:

1) The iframe and frameset cases might not be handled correctly. I will do more testing after checkin

2) I tried some ways of getting rid of the "if (!accelerated...)" flow control inside the ifdef. But it would require function calls and passing params which would make the rendering more expensive, which I want to avoid.

3) My technique of passing the entire bitmap to CACF does not take advantage of any paint rectangle optimizations. There is currently no way to do partial updates of CACF contents. But I plan to talk to John about this (also affects video rendering)

Other than that, all comments are integrated in the upcoming patch

[Chris Marrin]

Created attachment 43743 [details]
Replacement patch for all 3 previous patches

This is a patch for the complete implementation. Since two of the three patches were r+, I just included everything in the replacement. It makes it much easier. I've addressed all the issues with the exception of those called out in previous posts.

[Adam Roben (:aroben)]

I think it would be good to set things up so that the DirectX SDK is only required if USE(ACCELERATED_COMPOSITING) is enabled. This means all DirectX #includes should be guarded by the USE() flag, and we should soft-link to d3d9.lib. (See WebCore/platform/win/SoftLinking.h for some macros to make that easier.)

[Simon Fraser (smfr)]

Comment on attachment 43743 [details]
Replacement patch for all 3 previous patches

> Index: WebCore/WebCore.vcproj/WebCore.vcproj
> ===================================================================

>  				<File
> +					RelativePath="..\platform\graphics\TextRenderingMode.h"
> +					>
> +				</File>
> +				<File
>  					RelativePath="..\platform\win\WCDataObject.cpp"
>  					>
>  					<FileConfiguration
> @@ -22110,6 +22120,68 @@
>  					>
>  				</File>
>  				<File
> +					RelativePath="..\WebCorePrefix.cpp"
> +					>

Not sure why this moved around.

> Index: WebCore/platform/graphics/GraphicsLayer.cpp
> ===================================================================
> --- WebCore/platform/graphics/GraphicsLayer.cpp	(revision 51122)
> +++ WebCore/platform/graphics/GraphicsLayer.cpp	(working copy)
> @@ -69,7 +69,7 @@ GraphicsLayer::GraphicsLayer(GraphicsLay
>      , m_drawsContent(false)
>      , m_paintingPhase(GraphicsLayerPaintAll)
>      , m_geometryOrientation(CompositingCoordinatesTopDown)
> -    , m_contentsOrientation(CompositingCoordinatesTopDown)
> +    , m_contentsOrientation(CompositingCoordinatesBottomUp)

Why this change?

> Index: WebCore/platform/graphics/win/GraphicsLayerCACF.cpp
> ===================================================================

> +GraphicsLayerCACF::GraphicsLayerCACF(GraphicsLayerClient* client)
> +: GraphicsLayer(client)
> +, m_contentsLayerPurpose(NoContentsLayer)

These should be indented.

> +void GraphicsLayerCACF::removeFromParent()
> +{
> +    GraphicsLayer::removeFromParent();
> +    layerForSuperlayer()->removeFromSuperlayer();            
> +}

Why isn't this updating the parent's sublayer list?

> +void GraphicsLayerCACF::setAnchorPoint(const FloatPoint3D& point)
> +{
> +    GraphicsLayer::setAnchorPoint(point);
> +
> +    // set the value on the layer to the new transform

Comment makes no sense.

> +    primaryLayer()->setAnchorPoint(FloatPoint(point.x(), point.y()));
> +    primaryLayer()->setAnchorPointZ(m_anchorPoint.z());
> +
> +    // position depends on anchor point
> +    setPosition(m_position);
> +}
> +
> +void GraphicsLayerCACF::setSize(const FloatSize& size)
> +{
> +    GraphicsLayer::setSize(size);
> +
> +    CGRect rect = CGRectMake(0.0f, 0.0f, m_size.width(), m_size.height());
> +    
> +    if (m_transformLayer) {
> +        m_transformLayer->setBounds(rect);
> +    
> +        // the anchor of the contents layer is always at 0.5, 0.5, so the position
> +        // is center-relative

Sentence-case, please.

> +    bool needTiledLayer = requiresTiledLayer(m_size);
> +    if (needTiledLayer != m_usingTiledLayer) {
> +        swapFromOrToTiledLayer(needTiledLayer);

I think you should just remove all the tiled layer code. It's better ot just make large layers
than have layers stop working when they get big.

> +    // if we've changed the bounds, we need to recalculate the position
> +    // of the layer, taking anchor point into account

Sentence-case.

> +void GraphicsLayerCACF::setTransform(const TransformationMatrix& t)
> +{
> +    GraphicsLayer::setTransform(t);
> +    
> +    CATransform3D transform;
> +    copyTransform(transform, t);
> +    primaryLayer()->setTransform(transform);
> +}

This should bail if t == m_transform.

> +void GraphicsLayerCACF::setChildrenTransform(const TransformationMatrix& t)
> +{

This should bail if t == m_childrenTransform.


> +void GraphicsLayerCACF::setPreserves3D(bool preserves3D)
> +{
> +    GraphicsLayer::setPreserves3D(preserves3D);

Do an early return if it didn't change.

> +    CGPoint point = CGPointMake(m_size.width() / 2.0f, m_size.height() / 2.0f);
> +    CGPoint centerPoint = CGPointMake(0.5f, 0.5f);
> +    
> +    if (preserves3D && !m_transformLayer) {

Rather than do all this work here, I think it would be better for future refactoring
if you stayed closer to GraphicsLayerCA, and kept the update...() functions, so here
you'd call updateLayerPreserves3D().

Also, you have some tabs in this code.

> +void GraphicsLayerCACF::setDrawsContent(bool inDrawsContent)
> +{
> +    if (inDrawsContent != m_drawsContent) {

Early return is easier to read.

> +void GraphicsLayerCACF::setBackfaceVisibility(bool visible)
> +{
> +    if (m_backfaceVisibility == visible)
> +        return;
> +    
> +    GraphicsLayer::setBackfaceVisibility(visible);
> +    
> +    m_layer->setDoubleSided(visible);
> +}
> +    

Extra whitespace

> +void GraphicsLayerCACF::setOpacity(float opacity)
> +{
> +    float clampedOpacity = max(min(opacity, 1.0f), 0.0f);
> +    
> +    bool opacitiesDiffer = (m_opacity != clampedOpacity);

opacitiesDiffer is unused. Use the early return form.

> +void GraphicsLayerCACF::updateContentsRect()

So here is one update function. Why this and not the others?

> +PlatformLayer* GraphicsLayerCACF::hostLayerForSublayers() const
> +{
> +    return m_transformLayer ? m_transformLayer.get() : m_layer.get();
> +}
> +
> +PlatformLayer* GraphicsLayerCACF::layerForSuperlayer() const
> +{
> +    if (m_transformLayer)
> +        return m_transformLayer.get();
> +
> +    return m_layer.get();
> +}

Not sure why those methods don't both use the conditional operator.

> +#ifndef NDEBUG
> +void GraphicsLayerCACF::setDebugBackgroundColor(const Color& color)

These are no longer debug-only.

> +void GraphicsLayerCACF::updateContentsTransform()
> +{
> +    ASSERT(contentsOrientation() == CompositingCoordinatesTopDown);

Since updateContentsTransform() is an internal method, I'm not sure why this is useful.

> +void GraphicsLayerCACF::setupContentsLayer(WKCACFLayer* contentsLayer)
> +#ifndef NDEBUG
> +        if (showDebugBorders()) {
> +            setLayerBorderColor(m_contentsLayer.get(), Color(0, 0, 128, 180));
> +            m_contentsLayer->setBorderWidth(1.0f);
> +        }
> +#endif
> +    }
> +#ifndef NDEBUG
> +    updateDebugIndicators();
> +#endif

No longer debug-only.

> Index: WebCore/platform/graphics/win/GraphicsLayerCACF.h
> ===================================================================

> +class GraphicsLayerCACF : public GraphicsLayer {
> +public:
> +
> +    GraphicsLayerCACF(GraphicsLayerClient*);
> +    virtual ~GraphicsLayerCACF();
> +
> +    virtual void layout() { }

Where did layout() come from?

> +#ifndef NDEBUG
> +    virtual void setDebugBackgroundColor(const Color&);
> +    virtual void setDebugBorder(const Color&, float borderWidth);
> +#endif

No longer debug-only.

> +    CompositingCoordinatesOrientation defaultContentsOrientation() const;
> +    void updateContentsTransform();
> +	void updateSublayerList();

You have tabs here.

> Index: WebCore/platform/graphics/win/WKCACFContextFlusher.cpp
> ===================================================================

> +WKCACFContextFlusher& WKCACFContextFlusher::shared()
> +{
> +    static WKCACFContextFlusher& flusher = *new WKCACFContextFlusher;

This should use the DEFINE_STATIC_LOCAL macro.

> +void WKCACFContextFlusher::addContext(CACFContextRef context)
> +{
> +    if (!context)
> +        return;

Or ASSERT, perhaps?

> +
> +    if (m_contexts.add(context).second)
> +        CFRetain(context);

This is pretty confusing. What is add().second testing?

> +void WKCACFContextFlusher::removeContext(CACFContextRef context)
> +{
> +    if (!context)
> +        return;
> +
> +    ContextSet::iterator found = m_contexts.find(context);
> +    if (found == m_contexts.end())
> +        return;
> +
> +    CFRelease(*found);
> +    m_contexts.remove(found);
> +}

Do calls to addContext() and removeContext() with the same context properly nest?
This seems like fragile code.

> Index: WebCore/platform/graphics/win/WKCACFContextFlusherWin.cpp
> ===================================================================

> +void WKCACFContextFlusher::didFlushContext(CACFContextRef context)
> +{
> +    WKCACFLayerWindow::didFlushContext(context);
> +}

Since WKCACFContextFlusher will probably only ever be used on Windows,
I don't see the benefit of slicing this part into a separate file.

> Index: WebCore/platform/graphics/win/WKCACFLayer.cpp
> ===================================================================

> +void WKCACFLayer::display(PlatformGraphicsContext* context)

> +#ifndef NDEBUG

No longer debug-only.

> +void WKCACFLayer::layout()
> +{
> +    if (m_owner)
> +        m_owner->layout();
> +}

GraphicsLayers have no notion of layout. I think this should just be a stub.

> +void WKCACFLayer::becomeLayerForContext(CACFContextRef context)
> +{
> +    CACFContextSetLayer(context, layer());
> +    setNeedsCommit();
> +}

I've no idea what this means. Does it make this layer the root layer for that context,
or something else? It needs a comment, and a better name.

> +CGPoint WKCACFLayer::convertPoint(const CGPoint& p, const WKCACFLayer* toLayer) const
> +{
> +    CGPoint convertedPoint = p;
> +    CACFLayerConvertPoint(layer(), toLayer ? toLayer->layer() : 0, &convertedPoint);
> +    return convertedPoint;
> +}
> +
> +CGRect WKCACFLayer::convertRect(const CGRect& r, const WKCACFLayer* toLayer) const
> +{
> +    CGRect convertedRect = r;
> +    CACFLayerConvertRect(layer(), toLayer ? toLayer->layer() : 0, &convertedRect);
> +    return convertedRect;
> +}

I don't think we'll ever need these.

> +bool WKCACFLayer::isDescendantOf(WKCACFLayer* ancestor) const

Will return true when called with itself. That should be more explicit in a comment
or in the method name.

> +void WKCACFLayer::replaceSublayer(WKCACFLayer* reference, PassRefPtr<WKCACFLayer> newLayer)
> +{
> +    //RefPtr<WKCACFLayer> newLayerRef = newLayer;

Don't check in commented code.

> +void WKCACFLayer::removeFromSuperlayer()
> +{
> +    WKCACFLayer* superlayer = this->superlayer();
> +    if (!superlayer)
> +        return;
> +
> +    superlayer->removeSublayer(this);
> +    CACFLayerRemoveFromSuperlayer(layer());
> +    setNeedsCommit();
> +}

Why is setNeedsCommit() called on this, and not on the superlayer?

> +WKCACFLayer* WKCACFLayer::ancestorOrSelfWithSuperlayer(WKCACFLayer* superlayer) const

Is this used in a way that isDescendantOf() would not work for?

> +void WKCACFLayer::removeAllSublayers()
> +{
> +    CACFLayerSetSublayers(layer(), 0);
> +}

I don't grok why setNeedsCommit() isn't needed here either.

> +void WKCACFLayer::setSublayers(Vector<RefPtr<WKCACFLayer> >& sublayers)

Can that parameter be const?

> +{
> +    if (sublayers.isEmpty())
> +        CACFLayerSetSublayers(layer(), 0);
> +    else {
> +        // Create a vector of CACFLayers.
> +        Vector<const void*> layers;
> +        for (size_t i = 0; i < sublayers.size(); i++)
> +            layers.append(sublayers[i]->layer());
> +    
> +        RetainPtr<CFArrayRef> layersArray(AdoptCF, CFArrayCreate(0, layers.data(), layers.size(), 0));
> +        CACFLayerSetSublayers(layer(), layersArray.get());

Does this work? Does CACFLayerSetSublayers() just copy out of the array and do its own retaining, or is it just
taking ownership of the array? If the latter, then the array members are not being retained.

> +void WKCACFLayer::moveSublayers(WKCACFLayer* fromLayer, WKCACFLayer* toLayer)
> +{
> +    if (!fromLayer || !toLayer)
> +        return;
> +
> +    CACFLayerSetSublayers(toLayer->layer(), CACFLayerGetSublayers(fromLayer->layer()));

Why no setNeedsCommit()?

> +void WKCACFLayer::print()
> +{
> +    //CARenderShow(CACFLayerCopyRenderValue(layer()));
> +}

Why commented out?

> Index: WebCore/platform/graphics/win/WKCACFLayer.h
> ===================================================================

> +    void becomeLayerForContext(CACFContextRef);

This needs a comment explaining what it means.

> +    using RefCounted<WKCACFLayer>::ref;
> +    using RefCounted<WKCACFLayer>::deref;

Are these still required?

> +    CGPoint anchorPoint() { return CACFLayerGetAnchorPoint(layer()); }

> +    CGFloat anchorPointZ() { return CACFLayerGetAnchorPointZ(layer()); }

> +    CFStringRef contentsGravity() { return CACFLayerGetContentsGravity(layer()); }

These should all be const.

> +    void setNeedsDisplay(const CGRect& dirtyRect)
> +    {
> +        if (m_owner)
> +            CACFLayerSetNeedsDisplay(layer(), &dirtyRect);
> +        setNeedsCommit();
> +    }
> +
> +    void setNeedsDisplay()
> +    {
> +        if (m_owner)
> +            CACFLayerSetNeedsDisplay(layer(), 0);
> +        setNeedsCommit();
> +    }

I don't think these should be inline. I'd prefer to see all the calls to setNeedsCommit() in the .cpp file.

> +    void setOpacity(float opacity)
> +    {
> +        CACFLayerSetOpacity(layer(), opacity);
> +        setNeedsCommit();
> +    }

Ditto.

> +    void setSublayers(Vector<RefPtr<WKCACFLayer> >&);

const Vector?

> +    int findSublayer(const WKCACFLayer*);

Badly named. What is the return value? Should be const.

> Index: WebCore/platform/graphics/win/WKCACFLayerWindow.cpp
> ===================================================================

> +inline static CGRect winRectToCGRect(RECT rc)
> +{
> +    return CGRectMake((float)rc.left, (float)rc.top, (float)(rc.right - rc.left), (float)(rc.bottom - rc.top));

Use static_cast?

> +inline static CGRect winRectToCGRect(RECT rc, RECT relativeToRect)
> +{
> +    return CGRectMake((float)rc.left, (float)(relativeToRect.bottom-rc.bottom), (float)(rc.right - rc.left), (float)(rc.bottom - rc.top));

Ditto.

> +static ContextToWindowMap& windowsForContexts()
> +{
> +    static ContextToWindowMap& map = *new ContextToWindowMap;
> +    return map;

Use DEFINE_STATIC_LOCAL.

> +WKCACFLayerWindow::WKCACFLayerWindow()
> +    : m_triedToCreateD3DRenderer(false)
> +    , m_renderContext(0)
> +    , m_renderer(0)
> +    , m_hostWindow(0)
> +    , m_renderTimer(this, &WKCACFLayerWindow::renderTimerFired)
> +    , m_scrollFrameWidth(1)
> +    , m_scrollFrameHeight(1)

Are those reasonable defaults for m_scrollFrameWidth and m_scrollFrameHeight?

> +void WKCACFLayerWindow::createRenderer()

> +    CGColorRef debugColor = createCGColor(Color(255, 0, 0, 204));
> +    m_rootLayer->setBackgroundColor(debugColor);
> +    CGColorRelease(debugColor);

Do you really always want to do this?

> +void WKCACFLayerWindow::createRenderer()

> +    D3DXMATRIXA16 projection;
> +    D3DXMatrixOrthoOffCenterRH(&projection, rect.left, rect.right, rect.top, rect.bottom, -1.0f, 1.0f);

Why isn't this done with a call to initD3DGeometry() or resetDevice()?

> Index: WebCore/platform/graphics/win/WKCACFLayerWindow.h
> ===================================================================

This needs some comments explaining what it does, its lifetime, how many there are etc.

> +class WKCACFLayerWindow : Noncopyable {

Should be public Noncopyable.

> Index: WebKit/win/WebView.cpp
> ===================================================================

> +void WebView::setAcceleratedCompositing(bool accelerated)
> +{
> +    if (m_isAcceleratedCompositing == accelerated)
> +        return;
> +
> +    m_isAcceleratedCompositing = accelerated;
> +    if (m_isAcceleratedCompositing) {
> +        // Create the root layer
> +        ASSERT(m_viewWindow);
> +        m_layerWindow.setHostWindow(m_viewWindow);
> +        setRootLayerContents();
> +    } else {
> +        // Tear down the root layer
> +        m_layerWindow.destroyRenderer();
> +    }
> +
> +    //invalidateBackingStore(0);
> +    //::UpdateWindow(m_viewWindow);
> +    paint(0, 0);

Should not commit commented code.

> +}
> +
> +void WebView::setRootLayerContents()

It's weird having a set() method with no parameters. Maybe updateRootLayerContents()?


> +    RetainPtr<CFDataRef> data(AdoptCF, 
> +                                CFDataCreateWithBytesNoCopy(
> +                                        0, static_cast<UInt8*>(bitmap.bmBits),
> +                                        bmSize, kCFAllocatorNull));

> +    CGDataProviderRef cgData = CGDataProviderCreateWithCFData(data.get());
> +    CGColorSpaceRef space = CGColorSpaceCreateDeviceRGB();

Would be good to use RetainPtr consistently here, for these two.

> Index: WebKit/win/WebCoreSupport/WebChromeClient.cpp
> ===================================================================

> +void WebChromeClient::attachRootGraphicsLayer(Frame* frame, GraphicsLayer* graphicsLayer)
> +{
> +	m_webView->setRootChildLayer(graphicsLayer ? graphicsLayer->platformLayer() : 0);

Tab here.

> Index: WebKit/win/WebKit.vcproj/WebKit.vcproj
> ===================================================================

>  			</File>
>  			<File
> -				RelativePath="..\WebScriptWorld.h"
> -				>
> -			</File>
> -			<File
>  				RelativePath="..\WebKitStatistics.h"
>  				>
>  			</File>
> @@ -794,6 +790,10 @@
>  				>
>  			</File>
>  			<File
> +				RelativePath="..\WebScriptWorld.h"
> +				>
> +			</File>
> +			<File
>  				RelativePath="..\WebScrollBar.h"
>  				>
>  			</File>
> @@ -1094,10 +1094,6 @@
>  				>
>  			</File>
>  			<File
> -				RelativePath="..\WebScriptWorld.cpp"
> -				>
> -			</File>
> -			<File
>  				RelativePath="..\WebKitStatistics.cpp"
>  				>
>  			</File>
> @@ -1130,6 +1126,10 @@
>  				>
>  			</File>
>  			<File
> +				RelativePath="..\WebScriptWorld.cpp"
> +				>
> +			</File>
> +			<File
>  				RelativePath="..\WebScrollBar.cpp"
>  				>
>  			</File>

Maybe edit out thes unwanted changes.

> Index: WebKitTools/Scripts/webkitdirs.pm
> ===================================================================
> --- WebKitTools/Scripts/webkitdirs.pm	(revision 51122)
> +++ WebKitTools/Scripts/webkitdirs.pm	(working copy)
> @@ -647,7 +647,8 @@ sub checkWebCoreSVGSupport
>  
>  sub hasAcceleratedCompositingSupport
>  {
> -    return 0 if isCygwin() || isQt();
> +	# On platforms other than Mac the Skipped files are used to skip compositing tests
> +    return 1 if !isAppleMacWebKit();

Tabs here.

>      my $path = shift;
>      return libraryContainsSymbol($path, "GraphicsLayer");
> @@ -667,7 +668,8 @@ sub checkWebCoreAcceleratedCompositingSu
>  
>  sub has3DRenderingSupport
>  {
> -    return 0 if isQt();
> +	# On platforms other than Mac the Skipped files are used to skip 3D tests
> +    return 1 if !isAppleMacWebKit();

Tabs here

r=me with all those changes addressed, but I would prefer that GraphicsLayerCACF stays closer to GraphicsLayerCA.

[Adam Roben (:aroben)]

Comment on attachment 43743 [details]
Replacement patch for all 3 previous patches

Having one IDirect3DDevice9 and one CARenderOGLContext per window will bloat
memory. You can have a single instance of each for all WebViews.

[Adam Roben (:aroben)]

Comment on attachment 43743 [details]
Replacement patch for all 3 previous patches

WKCACFLayerWindow is kind of a confusing name, since it doesn't own or represent a window. It's more like the layer-related aspects of WebView.

It looks like you aren't going through WKCACFLayerWindow::render in response to receiving a WM_PAINT message in WebView. This can lead to garbage pixels being displayed in the WebView (e.g., when dragging another window around in front of the WebView).

[Simon Fraser (smfr)]

Would WKCACFLayerWindow be better as WKCACFLayerRenderer?

[Chris Marrin]

I've addressed all the issues raised by Simon and Adam, with some exceptions:

1) Framesets are not handled properly
(https://bugs.webkit.org/show_bug.cgi?id=31837)

2) There is extra glop in the vcproj files, from odd file movement I didn't do.
I fear trying to avoid this by editing the vcproj file!

3) the add/remove matching issue in WKCACFContextFlusher is copied from Adam's
version. When we unify this will be better handled.

4) I've asked John about whether or not CACFLayerSetSublayers retains the
CACFLayers passed to it.

5) I don't understand Simon's question about D3DXMatrixOrthoOffCenterRH ("Why
isn't this done with a call to initD3DGeometry() or resetDevice()?"). This call
is also made in the two calls mentioned. Again, this will all be resolved when
we use common code for my and Adam's implementations.

[Chris Marrin]

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