Summary: | [TexMap] Sprinkle range-based for-loops where still possible | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Zan Dobersek <zan> | ||||
Component: | New Bugs | Assignee: | Zan Dobersek <zan> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | cdumez, cmarcelo, commit-queue, kondapallykalyan, luiz, mrobinson, noam, svillar | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Zan Dobersek
2014-11-14 12:27:34 PST
Created attachment 241614 [details]
Patch
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 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. Committed r177442: <http://trac.webkit.org/changeset/177442> |