Summary: | Too much duplicated code in LayerTreeHosts implementations | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bugs-noreply, gyuyoung.kim, yoon, zan | ||||||||
Priority: | P2 | Keywords: | Gtk | ||||||||
Version: | WebKit Local Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 159209 | ||||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2016-06-27 05:07:54 PDT
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> |