[webkit-reviews] review denied: [Bug 113752] [Texmap] Update a dirty region which is not covered with keepRect. : [Attachment 196822] 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 196822: Patch
https://bugs.webkit.org/attachment.cgi?id=196822&action=review

------- Additional Comments from Jocelyn Turcotte <jocelyn.turcotte at digia.com>
r- because of the performance regression that this patch would introduce.
It seems like something is going wrong related to m_keepRect, please
investigate.

IRC discussion:
10:01 < jturcotte> fnwinter: I'm not sure how it's possible that you have tiles
outside the keepRect, could that be the issue instead?
10:02 < jturcotte> fnwinter: Your patch is going to itterate over all the
possible tile positions, if you're zoomed in on a large page, this loop can
take more than one second to run, you don't want this.
10:06 < fnwinter> jturcotte, it's not exactly outside of keepRect. actually
it's between keepRect and tile area. there is a gap area to be missing the
dirty.
10:08 < fnwinter> jturcotte, and so first time I thought to inflate the
keepRect to 2 tile size. but we are checking the dirty in tile::invalidate
function.
10:09 < fnwinter> keepRect does not fit to tiles size, that makes this issue.
10:24 < jturcotte> fnwinter: So normally, the code already there will
invalidate all tiles that touches the dirtyRect.
tileCoordinateForPoint(innerBottomRight(coveredDirtyRect)) should return the
coordinate of the tile to the lower-right even if it touches the keepRect by
only one pixel.
10:26 < jturcotte> Another thing that you have to make sure is that all tiles
should intersect m_keepRect when invalidate is called. If you keep your patch
and add something like ASSERT(currentTile->rect().intersects(m_keepRect)); It
shouldn't fire.