Bug 159144 – Too much duplicated code in LayerTreeHosts implementations

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 282120 [details]
Patch

12 files changed, 169 insertions(+), 418 deletions(-)

[Carlos Garcia Campos]

Created attachment 282122 [details]
Try to fix builds

[Carlos Garcia Campos]

Created attachment 282123 [details]
Try to fix EFL build

[Zan Dobersek]

Yoon, any thoughts?

[Gwang Yoon Hwang]

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?

[Carlos Garcia Campos]

(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.

[Zan Dobersek]

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.

[Zan Dobersek]

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.

[Carlos Garcia Campos]

(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!

[Carlos Garcia Campos]

Committed r202621: <http://trac.webkit.org/changeset/202621>