Adopt CAMachPort-as-layer-contents
Created attachment 246711 [details] Patch
<rdar://problem/19393233>
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.
http://trac.webkit.org/changeset/180203
Build fix in http://trac.webkit.org/changeset/180210
Re-opened since this is blocked by bug 142116
Created attachment 295931 [details] Patch
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.
(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.
Created attachment 295933 [details] Patch
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.
Created attachment 296170 [details] Patch
https://trac.webkit.org/changeset/209369