[Webkit-unassigned] [Bug 78305] [Qt][Texmap] Refactor backing-store code in TextureMapper

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=78305





--- Comment #6 from Jocelyn Turcotte <jocelyn.turcotte at nokia.com>  2012-02-10 12:50:30 PST ---
View in context: https://bugs.webkit.org/attachment.cgi?id=126530&action=review

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:59
> +void TextureMapperBackingStore::createOrDestroyTilesIfNeeded(const FloatSize& size, const IntSize& tileSize)

A few comments in that method could help.

> Source/WebCore/platform/graphics/texmap/TextureMapperCompositedImage.cpp:31
> +static HashMap<int64_t, RefPtr<TextureMapperCompositedImage> > s_imageBackingStores;

The map itself wouldn't be cleared, isn't that a bit dirty?

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:103
> +    m_backingStore->updateContents(textureMapper, layer, IntRect(dirtyRect));

Tiles swapBuffers are being done as an obscure side effect of m_backingStore->updateContents inside updateBackingStore inside syncCompositingState. This is an important step and I think it would be nice if it was more explicit, directly called from LayerTreeHostProxy on the LayerBackingStore after the layer state sync.

> Source/WebKit2/UIProcess/qt/LayerBackingStore.cpp:32
> +void LayerBackingStoreTile::swapBuffers(WebCore::TextureMapper* textureMapper)

Maybe have something in the name that buffers swapping only happens if needed.

> Source/WebKit2/UIProcess/qt/LayerBackingStore.cpp:76
> +    clear();

I've spent 15 minutes trying to understand the code until I saw this line, please add a comment :)
It seems unnecessary to do all this synchronization work each time something gets updated, couldn't LayerBackingStoreTile inherit from TextureMapperTile?

> Source/WebKit2/UIProcess/qt/LayerBackingStore.cpp:79
> +    HashMap<int, LayerBackingStoreTile>::iterator end = m_tiles.end();
> +    Vector<TextureMapperTile>& tilesToPaint = getTiles();
> +    for (HashMap<int, LayerBackingStoreTile>::iterator it = m_tiles.begin(); it != end; ++it) {

This part is confusing, getTiles(), LayerBackingStore::m_tiles and TextureMapperBackingStore::m_tiles don't have much in their name to tell their purposes appart.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.