Bug 138752 – [TexMap] Sprinkle range-based for-loops where still possible

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[TexMap] Sprinkle range-based for-loops where still possible

[Zan Dobersek]

Created attachment 241614 [details]
Patch

[Chris Dumez]

Comment on attachment 241614 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:97
> +            for (auto it = glContextDataMap().begin(), end = glContextDataMap().end(); it != end; ++it) {

nit: We could maybe use std::find_if() and a lambda function. e.g.
auto it = std::find_if(glContextDataMap().begin(), glContextDataMap().end(), [this] (const auto& item) { return item.value() == this; });
ASSERT(it != glContextDataMap().end());
glContextDataMap().remove(it);

> Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.cpp:134
> +        if (m_tiles.size() <= TileEraseThreshold)

Slight difference in behavior here. It is now possible you remove one tile even though m_tiles.size() <= TileEraseThreshold. We likely want this check before removing the title.

> Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.cpp:157
>              return texture;

nit: .release()

[Zan Dobersek]

Comment on attachment 241614 [details]
Patch

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

>> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:97
>> +            for (auto it = glContextDataMap().begin(), end = glContextDataMap().end(); it != end; ++it) {
> 
> nit: We could maybe use std::find_if() and a lambda function. e.g.
> auto it = std::find_if(glContextDataMap().begin(), glContextDataMap().end(), [this] (const auto& item) { return item.value() == this; });
> ASSERT(it != glContextDataMap().end());
> glContextDataMap().remove(it);

That would be ideal, but it's not possible at the moment because of missing std::iterator_traits specialization for HashTable iterator adapters.
Generic lambdas are also only available in C++14.

>> Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.cpp:134
>> +        if (m_tiles.size() <= TileEraseThreshold)
> 
> Slight difference in behavior here. It is now possible you remove one tile even though m_tiles.size() <= TileEraseThreshold. We likely want this check before removing the title.

Yeah, thanks for spotting this.

>> Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.cpp:157
>>              return texture;
> 
> nit: .release()

Not necessary, tile.texture() actually returns PassRefPtr<>. The previous code was not optimal.

[Zan Dobersek]

Committed r177442: <http://trac.webkit.org/changeset/177442>