Bug 137265 - [TexMap] Avoid constructing Vector when setting new children of TextureMapperLayer
Summary: [TexMap] Avoid constructing Vector when setting new children of TextureMapper...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-30 14:06 PDT by Zan Dobersek
Modified: 2015-01-07 07:41 PST (History)
9 users (show)

See Also:

Attachments
Patch (4.66 KB, patch)
2014-09-30 14:11 PDT, Zan Dobersek 		no flags Details | Formatted Diff | Diff
Patch (4.65 KB, patch)
2015-01-06 11:27 PST, Zan Dobersek 		bfulgham: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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*>&'