Bug 41084 - Get accelerated compositing working with webkit2
Summary: Get accelerated compositing working with webkit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on: 42662
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-23 11:07 PDT by Simon Fraser (smfr)
Modified: 2010-07-21 10:51 PDT (History)
7 users (show)

See Also:


Attachments
Patch (30.41 KB, patch)
2010-06-23 11:21 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (4.86 KB, patch)
2010-06-30 17:22 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
GraphicsLayer changes (3.92 KB, patch)
2010-07-01 12:05 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Add layer-backed drawing area (46.48 KB, patch)
2010-07-01 14:12 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Render server setup, and style fixes (16.51 KB, patch)
2010-07-01 15:36 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (30.29 KB, patch)
2010-07-20 14:16 PDT, Simon Fraser (smfr)
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2010-06-23 11:07:38 PDT
Bug to get accelerated compositing working with webkit2.
Comment 1 Simon Fraser (smfr) 2010-06-23 11:08:25 PDT
<rdar://problem/7660600>
Comment 2 Simon Fraser (smfr) 2010-06-23 11:21:07 PDT
Created attachment 59538 [details]
Patch
Comment 3 mitz 2010-06-23 11:50:27 PDT
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.
Comment 4 Simon Fraser (smfr) 2010-06-30 17:19:08 PDT
Comment on attachment 59538 [details]
Patch

Taking a different tack now.
Comment 5 Simon Fraser (smfr) 2010-06-30 17:22:55 PDT
Created attachment 60173 [details]
Patch
Comment 6 Simon Fraser (smfr) 2010-07-01 12:05:13 PDT
Created attachment 60268 [details]
GraphicsLayer changes
Comment 7 Darin Adler 2010-07-01 12:10:10 PDT
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.
Comment 8 Simon Fraser (smfr) 2010-07-01 12:30:35 PDT
Comment on attachment 60173 [details]
Patch

http://trac.webkit.org/changeset/62283
Comment 9 Simon Fraser (smfr) 2010-07-01 12:39:12 PDT
Comment on attachment 60268 [details]
GraphicsLayer changes

http://trac.webkit.org/changeset/62286
Comment 10 Simon Fraser (smfr) 2010-07-01 14:12:50 PDT
Created attachment 60284 [details]
Add layer-backed drawing area
Comment 11 WebKit Review Bot 2010-07-01 14:14:39 PDT
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.
Comment 12 Simon Fraser (smfr) 2010-07-01 15:24:41 PDT
Comment on attachment 60284 [details]
Add layer-backed drawing area

http://trac.webkit.org/changeset/62303
Comment 13 Simon Fraser (smfr) 2010-07-01 15:25:18 PDT
Comment on attachment 60173 [details]
Patch

Export some webcore symbols
Comment 14 Simon Fraser (smfr) 2010-07-01 15:36:46 PDT
Created attachment 60301 [details]
Render server setup, and style fixes
Comment 15 Eric Seidel (no email) 2010-07-02 03:17:07 PDT
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.
Comment 16 Eric Seidel (no email) 2010-07-02 03:17:11 PDT
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.
Comment 17 Eric Seidel (no email) 2010-07-02 03:17:15 PDT
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.
Comment 18 Simon Fraser (smfr) 2010-07-20 14:16:14 PDT
Created attachment 62110 [details]
Patch
Comment 19 WebKit Review Bot 2010-07-20 14:17:53 PDT
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.
Comment 20 Anders Carlsson 2010-07-21 09:25:07 PDT
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.
Comment 21 Simon Fraser (smfr) 2010-07-21 10:51:53 PDT
http://trac.webkit.org/changeset/63843