Bug 41084 – Get accelerated compositing working with webkit2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Simon Fraser (smfr)]

Bug to get accelerated compositing working with webkit2.

[Simon Fraser (smfr)]

<rdar://problem/7660600>

[Simon Fraser (smfr)]

Created attachment 59538 [details]
Patch

Comment on attachment 59538 [details]
Patch

Style review:


> +        Get accelerated compositing working with webkit2

Please include the Radar URL. s/webkit2/WebKit2/.

> +        First step: display the compositing layer tree from the webkit process

s/webkit/web/

> +        (WebKit::ChunkedUpdateDrawingArea::attachCompositingContext): Non-mac stub.

Mac.

> +#if PLATFORM(MAC)
> +    SetupAcceleratedCompositingPort
> +#endif

The verb is “set up” so the name should be “SetUpAcceleratedCompositingPort”.

> +#if USE(ACCELERATED_COMPOSITING)
> +    NSView* _layerHostingView;
> +#endif

The star should go next to the name.

> +#if USE(ACCELERATED_COMPOSITING)
> +- (void)_startAcceleratedCompositing:(CALayer*)rootLayer

There should be a space before the star.

> +        NSView* hostingView = [[NSView alloc] initWithFrame:[self bounds]];

Move the star over to the other side.

> +        [self addSubview:hostingView];
> +        [hostingView release];
> +        // hostingView is owned by being a subview of self

I think the comment is redundant. Being a full sentence, it should be terminated by a period. Or you could just remove it.

> +    CALayer* viewLayer = [CALayer layer];

Star should be next to the name.

> +- (void)_startAcceleratedCompositing:(CALayer*)rootLayer;

Needs a space before the star.

> +#if USE(ACCELERATED_COMPOSITING)
> +    void setupAcceleratedCompositing();
> +#endif

s/setup/setUp/

> --- a/WebKit2/UIProcess/mac/ChunkedUpdateDrawingAreaProxyMac.mm
> +++ b/WebKit2/UIProcess/mac/ChunkedUpdateDrawingAreaProxyMac.mm
> @@ -25,11 +25,15 @@
>  
>  #include "ChunkedUpdateDrawingAreaProxy.h"
>  
> +#include <QuartzCore/QuartzCore.h>
> +
>  #include "DrawingAreaMessageKinds.h"
>  #include "DrawingAreaProxyMessageKinds.h"
>  #include "UpdateChunk.h"
>  #include "WKAPICast.h"
>  #include "WKView.h"
> +#include "WKViewInternal.h"
> +#include "WebKitSystemInterface.h"
>  #include "WebPageProxy.h"

<QuartzCore/QuartzCore.h> should not be in its own paragraph. It should be at the end of the main paragraph. You can change all of the #include directives to #import directives now that this is an Objective-C file.

> +    // Cleanup.

This comment tells me absolutely nothing, but at least it’s period-terminated.

> --- a/WebKit2/WebProcess/WebPage/mac/ChunkedUpdateDrawingAreaMac.cpp
> +++ b/WebKit2/WebProcess/WebPage/mac/ChunkedUpdateDrawingAreaMac.mm
> @@ -25,9 +25,15 @@
>  
>  #include "ChunkedUpdateDrawingArea.h"
>  
> +#include <QuartzCore/QuartzCore.h>
> +

Should not be in its own separate paragraph.

> +#include "DrawingAreaProxyMessageKinds.h"
>  #include "UpdateChunk.h"
>  #include "WebPage.h"
> +#include "WebProcess.h"
>  #include <WebCore/GraphicsContext.h>
> +#include <WebCore/GraphicsLayer.h>
> +#include "WebKitSystemInterface.h"
>  #include <wtf/RetainPtr.h>

This should be sorted.

> +    CALayer* caLayer = layer->platformLayer();

Star is on the wrong side.

> +#if PLATFORM(MAC)
> +    , m_compositingRenderServerPort(MACH_PORT_NULL)
> +#endif

Is this needed in non-accelerated-compositing builds?

> +#if PLATFORM(MAC)
> +    mach_port_t compositingRenderServerPort() const { return m_compositingRenderServerPort; }
> +#endif

Ditto.

> +#if PLATFORM(MAC)
> +    mach_port_t m_compositingRenderServerPort;
> +#endif

Ditto.

[Simon Fraser (smfr)]

Comment on attachment 59538 [details]
Patch

Taking a different tack now.

[Simon Fraser (smfr)]

Created attachment 60173 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 60268 [details]
GraphicsLayer changes

[Darin Adler]

Comment on attachment 60268 [details]
GraphicsLayer changes

I’m not a huge fan of xxxRecursive as the name for the version that includes sublayers. The "recursive" suffix seems a little vague and I don’t like it in places we already have it.

If the normal case would be to sync the sublayers, then I'd suggest calling the single-layer version syncCompositingStateForThisLayerOnly or something like that. Or you could call the new one syncCompositingStateIncludingSublayers.

[Simon Fraser (smfr)]

Comment on attachment 60173 [details]
Patch

http://trac.webkit.org/changeset/62283

[Simon Fraser (smfr)]

Comment on attachment 60268 [details]
GraphicsLayer changes

http://trac.webkit.org/changeset/62286

[Simon Fraser (smfr)]

Created attachment 60284 [details]
Add layer-backed drawing area

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit2/WebProcess/WebPage/LayerBackedDrawingArea.cpp:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/WebProcess/WebPage/LayerBackedDrawingArea.cpp:153:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebKit2/WebProcess/WebPage/LayerBackedDrawingArea.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit2/WebProcess/WebPage/LayerBackedDrawingArea.h:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit2/UIProcess/LayerBackedDrawingAreaProxy.cpp:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/LayerBackedDrawingAreaProxy.cpp:121:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 6 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 60284 [details]
Add layer-backed drawing area

http://trac.webkit.org/changeset/62303

[Simon Fraser (smfr)]

Comment on attachment 60173 [details]
Patch

Export some webcore symbols

[Simon Fraser (smfr)]

Created attachment 60301 [details]
Render server setup, and style fixes

[Eric Seidel (no email)]

Comment on attachment 60173 [details]
Patch

Cleared Sam Weinig's review+ from obsolete attachment 60173 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 60268 [details]
GraphicsLayer changes

Cleared Darin Adler's review+ from obsolete attachment 60268 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 60284 [details]
Add layer-backed drawing area

Cleared Anders Carlsson's review+ from obsolete attachment 60284 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Simon Fraser (smfr)]

Created attachment 62110 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/WebProcess/WebPage/DrawingArea.cpp:40:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Comment on attachment 62110 [details]
Patch

>  } // namespace WebKit
> diff --git a/WebKit2/UIProcess/ChunkedUpdateDrawingAreaProxy.h b/WebKit2/UIProcess/ChunkedUpdateDrawingAreaProxy.h
> index 2644abb4907b608351abaa5ccbe5c74ffa323e6d..05323b42f9dec1c1141a17d53cbb6fc5ef804c79 100644
> --- a/WebKit2/UIProcess/ChunkedUpdateDrawingAreaProxy.h
> +++ b/WebKit2/UIProcess/ChunkedUpdateDrawingAreaProxy.h
> @@ -71,6 +71,7 @@ private:
>  
>      // DrawingAreaProxy
>      virtual void didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder&);
> +    virtual void didReceiveSyncMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder&, CoreIPC::ArgumentEncoder&);
>      virtual void paint(const WebCore::IntRect&, PlatformDrawingContext);
>      virtual void setSize(const WebCore::IntSize&);
>      virtual void setPageIsVisible(bool isVisible);
> diff --git a/WebKit2/UIProcess/DrawingAreaProxy.h b/WebKit2/UIProcess/DrawingAreaProxy.h
> index 1bd7da3cf1692d013efc8d75ac0571e9ed65c3c6..3817eb1f55c70fadafbcb58045cb464edcc62d59 100644
> --- a/WebKit2/UIProcess/DrawingAreaProxy.h
> +++ b/WebKit2/UIProcess/DrawingAreaProxy.h
> @@ -55,7 +55,9 @@ typedef QPainter* PlatformDrawingContext;
>  
> +void WebPageProxy::setDrawingArea(DrawingAreaProxy* drawingArea)

Since m_drawingArea is an OwnPtr, this should take a PassOwnPtr. Also, the setDrawingArea call site should use OwnPtrs.



> +#if USE(ACCELERATED_COMPOSITING)
> +        case WebPageProxyMessage::DidChangeAcceleratedCompositing: {
> +            bool compositing;
> +            if (!arguments.decode(CoreIPC::Out(compositing)))
> +                return;
> +
> +            if (compositing)
> +                didEnterAcceleratedCompositing();
> +            else
> +                didLeaveAcceleratedCompositing();
> +
> +            DrawingAreaProxy::Type drawingAreaType = drawingArea()->type();
> +            reply.encode(CoreIPC::In(static_cast<uint32_t>(drawingAreaType)));
> +            break;

We usually don't do any work other than decoding/encoding in the didReceiveMessage case handlers. This should just call a didChangeAcceleratedCompositing function.

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/63843