[Webkit-unassigned] [Bug 137345] [WinCairo] Accelerated compositing is not implemented.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=137345

Brent Fulgham <bfulgham at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #240010|review?                     |review+, commit-queue-
              Flags|                            |

--- Comment #9 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 240010
  --> https://bugs.webkit.org/attachment.cgi?id=240010
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240010&action=review

Really nice work! Are you able to activate any o the 3D CSS effects with this turned on?

I had a few minor comments and things to clean up before landing (so cq-) but I think this is pretty much ready to go!

> Source/WebKit/win/WebView.cpp:1110
>              // FIXME: We need to paint into dc (if provided). <http://webkit.org/b/52578>

I'd move this comment inside the #if USE(CA), since it only really applies to the CoreAnimation builds.

> Source/WebKit/win/WebView.cpp:6802
>      if (m_backingLayer)

Whoops (on us!)! This m_backingLayer test is not necessary (see line 6755/6794 above).

> Source/WebKit/win/WebView.h:51
> +#include "AcceleratedCompositingContext.h"

Could this just be a forward declaration?

> Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.cpp:2
> + * Copyright (C) 2014 Apple, Inc.

Shouldn't this be copyright you? If you based it off an Apple copyrighted file, just put both names.

> Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.cpp:61
> +static IntSize getWebViewSize(WebView* webView)

Can WebView ever be null? If so, check for that case. If not, let's make it a reference.

> Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.cpp:80
> +    m_rootLayer = GraphicsLayer::create(0, *this);

nullptr please.

> Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.cpp:85
> +    m_nonCompositedContentLayer = GraphicsLayer::create(0, *this);

Ditto.

> Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.cpp:164
> +    ::GetClientRect(m_window, &r);

GetClientRect can fail, leaving junk the the passed RECT. You might want to handle this case (though it's very rare).

> Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.cpp:170
> +        glClear(GL_COLOR_BUFFER_BIT);

Watch the tests to see if you run afoul of any cases where GL_SCISSOR or other switches are turned on. You might need to use the "TemporaryOpenGLSetting" class to make sure they are turned off when clearing the context (or else you'll get weird partial clears):

E.g., from GraphicsContext3D::prepareTexture:

    TemporaryOpenGLSetting scopedScissor(GL_SCISSOR_TEST, GL_FALSE);
    TemporaryOpenGLSetting scopedDither(GL_DITHER, GL_FALSE);

> Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.h:2
> + * Copyright (C) 2014 Apple, Inc.

Ditto re: Copyright by you.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141017/71f3df99/attachment-0002.html>