Bug 138752 - [TexMap] Sprinkle range-based for-loops where still possible
Summary: [TexMap] Sprinkle range-based for-loops where still possible
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-11-14 12:27 PST by Zan Dobersek
Modified: 2014-12-17 03:44 PST (History)
8 users (show)

See Also:


Attachments
Patch (9.62 KB, patch)
2014-11-14 12:32 PST, Zan Dobersek
cdumez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2014-11-14 12:27:34 PST
[TexMap] Sprinkle range-based for-loops where still possible
Comment 1 Zan Dobersek 2014-11-14 12:32:30 PST
Created attachment 241614 [details]
Patch
Comment 2 Chris Dumez 2014-11-17 10:07:14 PST
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()
Comment 3 Zan Dobersek 2014-12-17 03:35:43 PST
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.
Comment 4 Zan Dobersek 2014-12-17 03:44:05 PST
Committed r177442: <http://trac.webkit.org/changeset/177442>