Bug 64728 – TiledDrawingArea: Use tile sets to paint old content while rendering for a new scale.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Jocelyn Turcotte]

TiledDrawingArea: Use tile sets to paint old content while rendering for a new scale.

[Jocelyn Turcotte]

Created attachment 101163 [details]
Patch

[Jocelyn Turcotte]

Created attachment 101165 [details]
Move to own file patch

Tell me if you would prefer them squashed.

[Benjamin Poulain]

Comment on attachment 101163 [details]
Patch

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

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.h:183
> +    TiledDrawingAreaTileSet* m_currentTiles;
> +    TiledDrawingAreaTileSet* m_previousScaleTiles;

OwnPtr!!! :)

[Benjamin Poulain]

(In reply to comment #2)
> Created an attachment (id=101165) [details]
> Move to own file patch
> 
> Tell me if you would prefer them squashed.

Yep, that would be nice actually.

[Benjamin Poulain]

Comment on attachment 101163 [details]
Patch

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

Quick comments:

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:60
> +{
> +}

It'd be nice to have ASSERT(m_proxy) here so that fails early.

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.h:78
> +    TileMap& tiles() { return m_tiles; }

This is breaking the abstraction of TiledDrawingAreaTileSet.

It looks to me like TiledDrawingAreaProxy::updateTileBuffers() does not use any attribute so it could be a method of TiledDrawingAreaTileSet.
The case of TiledDrawingAreaProxy::tileBufferUpdateComplete() is a bit more tricky. If you manage to refactor it to avoid knowing about the tiles, the TiledDrawingAreaTileSet would be the only owner of m_tiles making a nicer abstraction.

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.h:82
> +    void setTile(const TiledDrawingAreaTile::Coordinate&, PassRefPtr<TiledDrawingAreaTile>);

Should this be private? It looks for me like the TiledDrawingAreaTileSet has the responsibility to manage its TiledDrawingAreaTile.

[Jocelyn Turcotte]

(In reply to comment #5)
> > Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:60
> > +{
> > +}
> 
> It'd be nice to have ASSERT(m_proxy) here so that fails early.
> 
Good idea.

> > Source/WebKit2/UIProcess/TiledDrawingAreaProxy.h:78
> > +    TileMap& tiles() { return m_tiles; }
> 
> This is breaking the abstraction of TiledDrawingAreaTileSet.
> 
> It looks to me like TiledDrawingAreaProxy::updateTileBuffers() does not use any attribute so it could be a method of TiledDrawingAreaTileSet.
> The case of TiledDrawingAreaProxy::tileBufferUpdateComplete() is a bit more tricky. If you manage to refactor it to avoid knowing about the tiles, the TiledDrawingAreaTileSet would be the only owner of m_tiles making a nicer abstraction.
> 
> > Source/WebKit2/UIProcess/TiledDrawingAreaProxy.h:82
> > +    void setTile(const TiledDrawingAreaTile::Coordinate&, PassRefPtr<TiledDrawingAreaTile>);
> 
> Should this be private? It looks for me like the TiledDrawingAreaTileSet has the responsibility to manage its TiledDrawingAreaTile.

Humm, at first I wanted to split responsibilities but I soon realized that there is no apparent reason to do so, handling tiles, their updates and painting them are not things easy to split apart and will introduce complexity with few benefits if done so.
The tile set was then just supposed to be a tile container with an attached scale ratio and a bare minimum of methods in it.

Though since this seems to annoy you as well, having this result in the end might give the impression of an incomplete lazy half-opened class. It might be better to just have the tile set as a struct { tiles; scale; }; and move those functions back to the TDAProxy with a tile set struct reference parameter. What do you think? Or tell me if you have other ideas.
Because anyway the only need I have is to group those tiles together.

[Jocelyn Turcotte]

(In reply to comment #6)
Humm wait, better than that, move the contentsScale attribute in the Tile along with knowledge of it's contentsRect in the page and just have the tile set replace by a normal container.

I think I'll give that one a try.

[Jocelyn Turcotte]

(In reply to comment #7)
> (In reply to comment #6)
> Humm wait, better than that, move the contentsScale attribute in the Tile along with knowledge of it's contentsRect in the page and just have the tile set replace by a normal container.
> 
> I think I'll give that one a try.

Ahhh... this won't work since I would need to remove the gc.setScale(invert) trick at the beginning of paint(). I tried that last weeks and had an annoying slight scaling with the opengl graphics system that I couldn't get rid of as I told you.

Anyway, please tell me what you think about the struct { tiles; scale; } idea.
I could also move the scale into the tile class as said and do a if (tile->scale() != lastScale) { gc.setScale(lastScale / tile->scale(); }, that might be clean actually since I would paint tiles with the same scale all at once.

[Jocelyn Turcotte]

Created attachment 101988 [details]
Patch

Removed unnecessary stuff from the TileSet and changed add/remove tile for attach/detach in the proxy.
Tell me what you think.

[Benjamin Poulain]

Comment on attachment 101988 [details]
Patch

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

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:49
> +    typedef HashMap<TiledDrawingAreaTile::Coordinate, RefPtr<TiledDrawingAreaTile> > TileMap;

Totally unrelated to your patch but I wonder why TiledDrawingAreaTile is refcounted. It looks like total ownership by TiledDrawingAreaTileSet to me. I guess it is simply because HashMap does not handle OwnPtr due to the lack of copy constructor?

That is something to keep in mind because RefPtr are a bit more complicated to handle right. We could probably have a destructor for TiledDrawingAreaTileSet that call deleteAllPairSeconds() and get rid of RefPtr.

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:350
> +

Uh?

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:366
> +    float rectArea = dirtyRect.width() * dirtyRect.height();

Can dirty rect be empty here?
In which case the return would likely be +inf (which might fine with the callers? :))

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.h:158
> +    OwnPtr<TiledDrawingAreaTileSet> m_currentTiles;
> +    OwnPtr<TiledDrawingAreaTileSet> m_previousScaleTiles;

I think it'd be nice to rename them to m_currentTileSet and m_previousTileSet.

> Source/WebKit2/UIProcess/TiledDrawingAreaTile.h:53
> +    void setProxy(TiledDrawingAreaProxy*);

I am not convinced by this new API of TiledDrawingAreaTile.

[Benjamin Poulain]

Comment on attachment 101988 [details]
Patch

Clearing the review flag. We discussed with Jocelyn and he wants to do some improvement.

[Jocelyn Turcotte]

Created attachment 102118 [details]
Patch

[Jocelyn Turcotte]

(In reply to comment #10)
> Totally unrelated to your patch but I wonder why TiledDrawingAreaTile is refcounted. It looks like total ownership by TiledDrawingAreaTileSet to me. I guess it is simply because HashMap does not handle OwnPtr due to the lack of copy constructor?
> 
> That is something to keep in mind because RefPtr are a bit more complicated to handle right. We could probably have a destructor for TiledDrawingAreaTileSet that call deleteAllPairSeconds() and get rid of RefPtr.
> 
Right now I don't think there is any reason why we need them refcounted. What do you mean by "to handle right", what is the cost/risk? 

> > Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:350
> > +
> 
> Uh?
> 
Oops

> > Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:366
> > +    float rectArea = dirtyRect.width() * dirtyRect.height();
> 
> Can dirty rect be empty here?
> In which case the return would likely be +inf (which might fine with the callers? :))
> 
It shouldn't be empty, but if it does, +inf would be the correct return value.

> > Source/WebKit2/UIProcess/TiledDrawingAreaProxy.h:158
> > +    OwnPtr<TiledDrawingAreaTileSet> m_currentTiles;
> > +    OwnPtr<TiledDrawingAreaTileSet> m_previousScaleTiles;
> 
> I think it'd be nice to rename them to m_currentTileSet and m_previousTileSet.
> 
Changed

> > Source/WebKit2/UIProcess/TiledDrawingAreaTile.h:53
> > +    void setProxy(TiledDrawingAreaProxy*);
> 
> I am not convinced by this new API of TiledDrawingAreaTile.
Changed with Tile::disableUpdates() that calls Proxy::unregisterTile.
Also called disableUpdates() in the tile's destructor so that deleting the tile object will also cleanup the proxy's tileByID map.

[WebKit Review Bot]

Comment on attachment 102118 [details]
Patch

Attachment 102118 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9247736

New failing tests:
fast/canvas/webgl/framebuffer-test.html

[Benjamin Poulain]

> > That is something to keep in mind because RefPtr are a bit more complicated to handle right. We could probably have a destructor for TiledDrawingAreaTileSet that call deleteAllPairSeconds() and get rid of RefPtr.
> > 
> Right now I don't think there is any reason why we need them refcounted. What do you mean by "to handle right", what is the cost/risk? 

See this discussion: https://lists.webkit.org/pipermail/webkit-dev/2011-June/017178.html

It is really easy to write code that look correct and is wrong with PassRefPtr.

[Benjamin Poulain]

Comment on attachment 102118 [details]
Patch

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

Looks good :)

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:396
> +    if (m_currentTileSet->contentsScale() == scale)

I doubt this is gonna be hit often :)

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:558
> +void TiledDrawingAreaProxy::removeAllTiles()

I don't think you need this method at all now. The tiles are cleared when the old tileset is not needed anymore.

[Jocelyn Turcotte]

Thx! As told in the corridor, removeAllTiles was already used by setTileSize so I kept it there.

Committed r91979: <http://trac.webkit.org/changeset/91979>

[Jocelyn Turcotte]

The patch caused a RefPtr assert in debug (hummm see comment #15).
Fixed it in http://trac.webkit.org/changeset/91990 .

There is also a segmentation fault on shutdown that was introduced by the tileset patch, working on it.

[Jocelyn Turcotte]

Fixed the segfault in http://trac.webkit.org/changeset/91994