Bug 141687 – Adopt CAMachPort-as-layer-contents

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

Adopt CAMachPort-as-layer-contents

[Tim Horton]

Created attachment 246711 [details]
Patch

[Tim Horton]

<rdar://problem/19393233>

[Simon Fraser (smfr)]

Comment on attachment 246711 [details]
Patch

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

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:388
> +            layer.contents = (id)adoptCF(CAMachPortCreate(m_frontBufferSendRight.leakSendRight())).get();

This makes my brain hurt.

[Tim Horton]

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

[Tim Horton]

Build fix in http://trac.webkit.org/changeset/180210

[WebKit Commit Bot]

Re-opened since this is blocked by bug 142116

[Tim Horton]

Created attachment 295931 [details]
Patch

[Tim Horton]

Comment on attachment 295931 [details]
Patch

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

> Source/WebKit2/ChangeLog:46
> +        commit again (and a commit will restore all the backing store), and even

Need to quadruply check that the parenthetical here is true. It looks like two-years-ago-me thought that it wasn't. If it isn't, we'll need to tell the Web process to do this, and do it in the first commit. That might make this a bit more complicated.

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.h:69
> +    enum class LayerContentsType {

Naming here is a little weird because of the non-accelerated-backing-store case. This is only ever used for accelerated backing store.

> Source/WebKit2/UIProcess/mac/RemoteLayerTreeHost.mm:279
> +void RemoteLayerTreeHost::recursivelyClearLayerContents(CALayer *layer)

This can be a free function.

[Tim Horton]

(In reply to comment #8)
> Comment on attachment 295931 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295931&action=review
> 
> > Source/WebKit2/ChangeLog:46
> > +        commit again (and a commit will restore all the backing store), and even
> 
> Need to quadruply check that the parenthetical here is true. It looks like
> two-years-ago-me thought that it wasn't. If it isn't, we'll need to tell the
> Web process to do this, and do it in the first commit. That might make this
> a bit more complicated.

Two-years-ago-me was right. I'm not sure why it works ever, really. I'll have to figure this part out, or come up with a different solution to this problem (we could walk the tree and swap CAMachPorts out for actual surfaces...)

> > Source/WebKit2/Shared/mac/RemoteLayerBackingStore.h:69
> > +    enum class LayerContentsType {
> 
> Naming here is a little weird because of the non-accelerated-backing-store
> case. This is only ever used for accelerated backing store.
> 
> > Source/WebKit2/UIProcess/mac/RemoteLayerTreeHost.mm:279
> > +void RemoteLayerTreeHost::recursivelyClearLayerContents(CALayer *layer)
> 
> This can be a free function.

[Tim Horton]

Created attachment 295933 [details]
Patch

[Darin Adler]

Comment on attachment 295933 [details]
Patch

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

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.h:72
> +    enum class LayerContentsType {
> +        IOSurface,
> +        CAMachPort
> +    };

Seems like this should be a one-liner.

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:412
> +                WebCore::MachSendRight sendRight = WTFMove(m_frontBufferSendRight);
> +                m_frontBuffer.surface = WebCore::IOSurface::createFromSendRight(sendRight, sRGBColorSpaceRef());

Seems like createFromSendRight should be changed to take a MachSendRight&& as a clean-up step in the future to reflect the fact that the function consumes the send right. Then it would be better to write this as a one-liner. As it is, the idiom where we do a WTFMove into a local variable, but not into the createFromSendRight function, is peculiar.

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:416
> +
> +            layer.contents = m_frontBuffer.surface ? m_frontBuffer.surface->asLayerContents() : nil;
> +

I would omit the blank lines. Paragraphing is a bit confusing.

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:418
> +        case LayerContentsType::CAMachPort: {

Could cheat and leave out the braces since his happens to be the last case in the switch. Or take my advice below and use a one-liner, obviating the braces another way.

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:421
> +            RetainPtr<CAMachPortRef> port = adoptCF(CAMachPortCreate(m_frontBufferSendRight.leakSendRight()));
> +            layer.contents = (id)port.get();

I think this would read better as a one-liner. The combined line would be shorter than the first line.

> Source/WebKit2/UIProcess/DrawingAreaProxy.h:106
> +    virtual void prepareForAppSuspension() { }

Annoying paragraphing in this class; all these functions each in their own separate paragraph, not groups. Are they truly all one-offs with no relationship with each other?

> Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.h:83
> +    void prepareForAppSuspension() override;

I suggest final instead of override here.

> Source/WebKit2/UIProcess/mac/RemoteLayerTreeHost.h:81
>      void layerWillBeRemoved(WebCore::GraphicsLayer::PlatformLayerID);
>  
> +
>      RemoteLayerTreeDrawingAreaProxy& m_drawingArea;

I suggest not adding that new blank line.

> Source/WebKit2/UIProcess/mac/RemoteLayerTreeHost.mm:83
> +    RemoteLayerBackingStore::LayerContentsType layerContentsType = m_drawingArea.hasDebugIndicator() ? RemoteLayerBackingStore::LayerContentsType::IOSurface : RemoteLayerBackingStore::LayerContentsType::CAMachPort;

I suggest using auto here to remove one of the three instances of RemoteLayerBackingStore::LayerContentsType on this line.

[Tim Horton]

Created attachment 296170 [details]
Patch

[Tim Horton]

https://trac.webkit.org/changeset/209369