Bug 159144 - Too much duplicated code in LayerTreeHosts implementations
Summary: Too much duplicated code in LayerTreeHosts implementations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 159209
  Show dependency treegraph
 
Reported: 2016-06-27 05:07 PDT by Carlos Garcia Campos
Modified: 2016-06-29 05:17 PDT (History)
4 users (show)

See Also:


Attachments
Patch (44.27 KB, patch)
2016-06-27 05:10 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix builds (44.69 KB, patch)
2016-06-27 05:16 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix EFL build (45.50 KB, patch)
2016-06-27 05:22 PDT, Carlos Garcia Campos
zan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2016-06-27 05:07:54 PDT
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.
Comment 1 Carlos Garcia Campos 2016-06-27 05:10:52 PDT
Created attachment 282120 [details]
Patch

12 files changed, 169 insertions(+), 418 deletions(-)
Comment 2 Carlos Garcia Campos 2016-06-27 05:16:45 PDT
Created attachment 282122 [details]
Try to fix builds
Comment 3 Carlos Garcia Campos 2016-06-27 05:22:46 PDT
Created attachment 282123 [details]
Try to fix EFL build
Comment 4 Zan Dobersek 2016-06-27 23:25:56 PDT
Yoon, any thoughts?
Comment 5 Gwang Yoon Hwang 2016-06-28 00:58:42 PDT
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?
Comment 6 Carlos Garcia Campos 2016-06-28 02:16:07 PDT
(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 7 Zan Dobersek 2016-06-29 04:40:40 PDT
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 8 Zan Dobersek 2016-06-29 04:41:42 PDT
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.
Comment 9 Carlos Garcia Campos 2016-06-29 05:12:22 PDT
(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!
Comment 10 Carlos Garcia Campos 2016-06-29 05:17:13 PDT
Committed r202621: <http://trac.webkit.org/changeset/202621>