Bug 137265

Summary: [TexMap] Avoid constructing Vector when setting new children of TextureMapperLayer
Product: WebKit Reporter: Zan Dobersek <zan>
Component: New BugsAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, commit-queue, gyuyoung.kim, hw1008.kim, kondapallykalyan, luiz, mrobinson, noam, ossy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch bfulgham: review+

Description Zan Dobersek 2014-09-30 14:06:38 PDT
[TexMap] Avoid constructing Vector when setting new children of TextureMapperLayer
Comment 1 Zan Dobersek 2014-09-30 14:11:01 PDT
Created attachment 238959 [details]
Patch
Comment 2 Brent Fulgham 2014-10-31 11:05:03 PDT
Comment on attachment 238959 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238959&action=review

In general this seems like a good change, but I think it might be better if you simply modified the setChildren method to do this work.

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:485
> +            m_layer->addChild(toTextureMapperLayer(child));

Why not just encapsulate the operation of removing the children from the layer, and adding the new children, inside TextureMapperLayer? I.e., why couldn't setChildren just use the return value of "children()" as its argument?
Comment 3 Zan Dobersek 2015-01-06 11:21:14 PST
Comment on attachment 238959 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238959&action=review

>> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:485
>> +            m_layer->addChild(toTextureMapperLayer(child));
> 
> Why not just encapsulate the operation of removing the children from the layer, and adding the new children, inside TextureMapperLayer? I.e., why couldn't setChildren just use the return value of "children()" as its argument?

This requires GraphicsLayerTextureMapper.h to be included in TextureMapperLayer.cpp, but I think we can live with that.
Comment 4 Zan Dobersek 2015-01-06 11:27:47 PST
Created attachment 244079 [details]
Patch
Comment 5 Brent Fulgham 2015-01-06 14:23:02 PST
Comment on attachment 244079 [details]
Patch

Thank you for revising this! r=me.
Comment 6 Zan Dobersek 2015-01-07 05:14:25 PST
Committed r178034: <http://trac.webkit.org/changeset/178034>
Comment 7 Csaba Osztrogon√°c 2015-01-07 07:41:19 PST
(In reply to comment #6)
> Committed r178034: <http://trac.webkit.org/changeset/178034>

It broke the EFL build:
../../Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp: In member function 'void WebKit::CoordinatedGraphicsScene::setLayerChildrenIfNeeded(WebCore::TextureMapperLayer*, const WebCore::CoordinatedGraphicsLayerState&)':
../../Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:218:32: error: no matching function for call to 'WebCore::TextureMapperLayer::setChildren(WTF::Vector<WebCore::TextureMapperLayer*>&)'
../../Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:218:32: note: candidate is:
In file included from DerivedSources/WebKit2/include/WebCore/TextureMapperLayer.h:1:0,
                 from ../../Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.h:35,
                 from ../../Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:24:
../../Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:81:10: note: void WebCore::TextureMapperLayer::setChildren(const WTF::Vector<WebCore::GraphicsLayer*>&)
../../Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:81:10: note:   no known conversion for argument 1 from 'WTF::Vector<WebCore::TextureMapperLayer*>' to 'const WTF::Vector<WebCore::GraphicsLayer*>&'