Bug 141687 - Adopt CAMachPort-as-layer-contents
Summary: Adopt CAMachPort-as-layer-contents
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords:
Depends on: 142116
Blocks:
  Show dependency treegraph
 
Reported: 2015-02-16 18:15 PST by Tim Horton
Modified: 2016-12-05 17:07 PST (History)
1 user (show)

See Also:


Attachments
Patch (6.38 KB, patch)
2015-02-16 18:15 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (19.75 KB, patch)
2016-12-01 23:30 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (20.54 KB, patch)
2016-12-02 01:59 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (21.96 KB, patch)
2016-12-05 12:08 PST, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2015-02-16 18:15:16 PST
Adopt CAMachPort-as-layer-contents
Comment 1 Tim Horton 2015-02-16 18:15:43 PST
Created attachment 246711 [details]
Patch
Comment 2 Tim Horton 2015-02-16 18:16:34 PST
<rdar://problem/19393233>
Comment 3 Simon Fraser (smfr) 2015-02-16 18:22:47 PST
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.
Comment 4 Tim Horton 2015-02-16 18:29:20 PST
http://trac.webkit.org/changeset/180203
Comment 5 Tim Horton 2015-02-16 23:26:48 PST
Build fix in http://trac.webkit.org/changeset/180210
Comment 6 WebKit Commit Bot 2015-02-27 16:30:24 PST
Re-opened since this is blocked by bug 142116
Comment 7 Tim Horton 2016-12-01 23:30:06 PST
Created attachment 295931 [details]
Patch
Comment 8 Tim Horton 2016-12-01 23:33:18 PST
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.
Comment 9 Tim Horton 2016-12-02 00:31:16 PST
(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.
Comment 10 Tim Horton 2016-12-02 01:59:47 PST
Created attachment 295933 [details]
Patch
Comment 11 Darin Adler 2016-12-03 11:45:24 PST
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.
Comment 12 Tim Horton 2016-12-05 12:08:27 PST
Created attachment 296170 [details]
Patch
Comment 13 Tim Horton 2016-12-05 17:07:51 PST
https://trac.webkit.org/changeset/209369