There's some code common and duplicated in all current LayerTreeHosts implementations (Gtk, Coordinated, and ThreadedCoordinated). The thing is even worse in the case of ThreadedCoordinated and Coordinated, where the former is actually a special case of the later, and it seems like code was copy pasted and then modified to add ThreadedCoordinated specific code. The problem of that approach, apart from the code duplication, is that common parts end up diverging too.
Created attachment 282120 [details] Patch 12 files changed, 169 insertions(+), 418 deletions(-)
Created attachment 282122 [details] Try to fix builds
Created attachment 282123 [details] Try to fix EFL build
Yoon, any thoughts?
Comment on attachment 282123 [details] Try to fix EFL build View in context: https://bugs.webkit.org/attachment.cgi?id=282123&action=review > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:207 > + UNUSED_PARAM(size); Maybe using ASSERT_NOT_REACHED would be nice to use here. > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.h:65 > + class CompositorClient final : public ThreadedCompositor::Client { What about to add WTF_MAKE_NONCOPYABLE to make sure? > Source/WebKit2/WebProcess/WebPage/LayerTreeHost.h:73 > virtual void sizeDidChange(const WebCore::IntSize& newSize) = 0; What about change the name of it to the viewportSizeChanged, as we discussed earlier?
(In reply to comment #5) > Comment on attachment 282123 [details] > Try to fix EFL build > > View in context: > https://bugs.webkit.org/attachment.cgi?id=282123&action=review > > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:207 > > + UNUSED_PARAM(size); > > Maybe using ASSERT_NOT_REACHED would be nice to use here. Sure. > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.h:65 > > + class CompositorClient final : public ThreadedCompositor::Client { > > What about to add WTF_MAKE_NONCOPYABLE to make sure? It's private and stack allocated only, LayerTreeHost is already non copyable, so I don't see how it can be copied. > > Source/WebKit2/WebProcess/WebPage/LayerTreeHost.h:73 > > virtual void sizeDidChange(const WebCore::IntSize& newSize) = 0; > > What about change the name of it to the viewportSizeChanged, as we discussed > earlier? That doesn't belong to this patch, this is about sharing existing code, I avoided changes like that to make sure I don't break EFL port.
Comment on attachment 282123 [details] Try to fix EFL build View in context: https://bugs.webkit.org/attachment.cgi?id=282123&action=review >>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.h:65 >>> + class CompositorClient final : public ThreadedCompositor::Client { >> >> What about to add WTF_MAKE_NONCOPYABLE to make sure? > > It's private and stack allocated only, LayerTreeHost is already non copyable, so I don't see how it can be copied. It's not stack-allocated, it's allocated on the heap by being located inside the ThreadedCoordinatedLayerTreeHost. It can be copied, ThreadedCompositor::Client is copyable and the LayerTreeHost reference doesn't prevent it either.
Comment on attachment 282123 [details] Try to fix EFL build View in context: https://bugs.webkit.org/attachment.cgi?id=282123&action=review >>>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.h:65 >>>> + class CompositorClient final : public ThreadedCompositor::Client { >>> >>> What about to add WTF_MAKE_NONCOPYABLE to make sure? >> >> It's private and stack allocated only, LayerTreeHost is already non copyable, so I don't see how it can be copied. > > It's not stack-allocated, it's allocated on the heap by being located inside the ThreadedCoordinatedLayerTreeHost. > > It can be copied, ThreadedCompositor::Client is copyable and the LayerTreeHost reference doesn't prevent it either. r=me with WTF_MAKE_NONCOPYABLE.
(In reply to comment #8) > Comment on attachment 282123 [details] > Try to fix EFL build > > View in context: > https://bugs.webkit.org/attachment.cgi?id=282123&action=review > > >>>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.h:65 > >>>> + class CompositorClient final : public ThreadedCompositor::Client { > >>> > >>> What about to add WTF_MAKE_NONCOPYABLE to make sure? > >> > >> It's private and stack allocated only, LayerTreeHost is already non copyable, so I don't see how it can be copied. > > > > It's not stack-allocated, it's allocated on the heap by being located inside the ThreadedCoordinatedLayerTreeHost. > > > > It can be copied, ThreadedCompositor::Client is copyable and the LayerTreeHost reference doesn't prevent it either. > > r=me with WTF_MAKE_NONCOPYABLE. Ok, thanks!
Committed r202621: <http://trac.webkit.org/changeset/202621>