Bug 145788 - [CoordinatedGraphics] Remove RefCounted from Tile
Summary: [CoordinatedGraphics] Remove RefCounted from Tile
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryuan Choi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-08 23:24 PDT by Ryuan Choi
Modified: 2015-06-09 15:26 PDT (History)
8 users (show)

See Also:


Attachments
Patch (9.16 KB, patch)
2015-06-08 23:38 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryuan Choi 2015-06-08 23:24:40 PDT
Tile is only used in TiledBackingStore and we don't need to use RefPtr for it since r185140
Comment 1 Ryuan Choi 2015-06-08 23:38:14 PDT
Created attachment 254550 [details]
Patch
Comment 2 Darin Adler 2015-06-09 11:17:58 PDT
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.
Comment 3 Ryuan Choi 2015-06-09 15:21:31 PDT
Committed r185388: <http://trac.webkit.org/changeset/185388>
Comment 4 Ryuan Choi 2015-06-09 15:26:54 PDT
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.