WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
138752
[TexMap] Sprinkle range-based for-loops where still possible
https://bugs.webkit.org/show_bug.cgi?id=138752
Summary
[TexMap] Sprinkle range-based for-loops where still possible
Zan Dobersek
Reported
2014-11-14 12:27:34 PST
[TexMap] Sprinkle range-based for-loops where still possible
Attachments
Patch
(9.62 KB, patch)
2014-11-14 12:32 PST
,
Zan Dobersek
cdumez
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Zan Dobersek
Comment 1
2014-11-14 12:32:30 PST
Created
attachment 241614
[details]
Patch
Chris Dumez
Comment 2
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()
Zan Dobersek
Comment 3
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.
Zan Dobersek
Comment 4
2014-12-17 03:44:05 PST
Committed
r177442
: <
http://trac.webkit.org/changeset/177442
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug