[webkit-reviews] review denied: [Bug 113752] [Texmap] Update a dirty region which is not covered with keepRect. : [Attachment 197026] Patch

[bugzilla-daemon at webkit.org]

Jocelyn Turcotte <jocelyn.turcotte at digia.com> has denied JungJik Lee
<jungjik.lee at samsung.com>'s request for review:
Bug 113752: [Texmap] Update a dirty region which is not covered with keepRect.
https://bugs.webkit.org/show_bug.cgi?id=113752

Attachment 197026: Patch
https://bugs.webkit.org/attachment.cgi?id=197026&action=review

------- Additional Comments from Jocelyn Turcotte <jocelyn.turcotte at digia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=197026&action=review


> Source/WebCore/ChangeLog:11
> +	   and the dirty will not be invalidated until the tile is updated 

dirty *region*
until the tile is *recreated*

> Source/WebCore/ChangeLog:12
> +	   forcibly. We must make keepRect to fit to tile grid to append a

This sentence is difficult to read, suggestion:
"We must expand the keep rect to its intersecting tiles to make sure that the
dirty region is applied to existing tiles."

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:85
> +    IntRect keepRectFitToTileSize =
tileRectForCoordinate(tileCoordinateForPoint(m_keepRect.location()));
> +   
keepRectFitToTileSize.unite(tileRectForCoordinate(tileCoordinateForPoint(innerB
ottomRight(m_keepRect))));

Ok I see the issue now, this makes sense.

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:98
> +	       ASSERT(currentTile->rect().intersects(m_keepRect));
> +

This was an investigation suggestion, I think that the ASSERT shouldn't be
committed in the code.

> LayoutTests/ChangeLog:9
> +	   If we do not miss a dirty outside of keepRect, the red box will be
displayed.

If you see red in tests it usually means bad. Please make it a green box.

> LayoutTests/ChangeLog:11
> +	   * compositing/tiling/update-tile-outside-keeprect-expected.html:
Added.

I'm not sure what this is, you can't have an HTML expected file.

> LayoutTests/compositing/tiling/update-tile-outside-keeprect.html:50
> +	   div.keepRect
> +	   {
> +	       height:1700px;

This kind of hard-coding makes me wonder about the value of this test. At least
for Qt, this test only helped me understand your issue, it never showed the bug
itself.
Maybe No'am could give his opinion, but I think that you would be better
leaving out a test that doesn't test anything.