Bug 145788 – [CoordinatedGraphics] Remove RefCounted from Tile

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Ryuan Choi]

Tile is only used in TiledBackingStore and we don't need to use RefPtr for it since r185140

[Ryuan Choi]

Created attachment 254550 [details]
Patch

[Darin Adler]

Comment on attachment 254550 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/coordinated/TiledBackingStore.cpp:251
> +        m_tiles.set(coordinate, std::make_unique<Tile>(*this, coordinate));

Should probably be add, not set.

> Source/WebCore/platform/graphics/texmap/coordinated/TiledBackingStore.cpp:366
>      unsigned removeCount = tilesToRemove.size();
>      for (unsigned n = 0; n < removeCount; ++n)

Should use modern for loop.

> Source/WebCore/platform/graphics/texmap/coordinated/TiledBackingStore.cpp:385
>      unsigned removeCount = toRemove.size();
>      for (unsigned n = 0; n < removeCount; ++n)

Should use modern for loop.

> Source/WebCore/platform/graphics/texmap/coordinated/TiledBackingStore.h:91
> +    typedef HashMap<Tile::Coordinate, std::unique_ptr<Tile>> TileMap;

Not even sure it’s helpful to allocate these on the heap. Maybe just Tile here instead of std::unique_ptr<Tile> some day.

[Ryuan Choi]

Committed r185388: <http://trac.webkit.org/changeset/185388>

[Ryuan Choi]

Comment on attachment 254550 [details]
Patch

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

Thanks, I landed after followed your comments except Tile allocation. clearing flags.

I tried but it was not available.

>> Source/WebCore/platform/graphics/texmap/coordinated/TiledBackingStore.cpp:251
>> +        m_tiles.set(coordinate, std::make_unique<Tile>(*this, coordinate));
> 
> Should probably be add, not set.

Thanks. I fixed.

>> Source/WebCore/platform/graphics/texmap/coordinated/TiledBackingStore.cpp:366
>>      for (unsigned n = 0; n < removeCount; ++n)
> 
> Should use modern for loop.

Done.

>> Source/WebCore/platform/graphics/texmap/coordinated/TiledBackingStore.cpp:385
>>      for (unsigned n = 0; n < removeCount; ++n)
> 
> Should use modern for loop.

Done.

>> Source/WebCore/platform/graphics/texmap/coordinated/TiledBackingStore.h:91
>> +    typedef HashMap<Tile::Coordinate, std::unique_ptr<Tile>> TileMap;
> 
> Not even sure it’s helpful to allocate these on the heap. Maybe just Tile here instead of std::unique_ptr<Tile> some day.

Maybe, but now Tile is not simple stucture to keep the HashMap as instance.

Anyway, I will consider it in different bug not to allocate.