[webkit-reviews] review granted: [Bug 27314] 3D CSS Transforms are not implemented in WebKit for Windows : [Attachment 41072] Second patch (hooking up accelerated compositing, but not building)

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 27314: 3D CSS Transforms are not implemented in WebKit for Windows
https://bugs.webkit.org/show_bug.cgi?id=27314

Attachment 41072: Second patch (hooking up accelerated compositing, but not
building)
https://bugs.webkit.org/attachment.cgi?id=41072&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> 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.