LayoutRepainter has a parameter for oldBounds that is never used, with the old bounds instead being assigned by default from clippedOverflowRectForRepaint. The patch removes this paramater and stores the old bounds and old outline box as LayoutUnits.
Created attachment 133769 [details] Patch
Comment on attachment 133769 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133769&action=review > Source/WebCore/ChangeLog:9 > + Removing an optional parameter for old bounds in LayoutRepainter's constructor that > + was never used. The old bounds were instead always gleaned from the renderer's That statement is wrong. The parameter was used by SVG when LayoutRepainter was introduced but got removed over time (at least that's what git blame is telling me). Please amend this statement to say that it is unused now. > Source/WebCore/rendering/LayoutRepainter.h:49 > + LayoutRect m_oldBounds; > + LayoutRect m_oldOutlineBox; It's weird to store them as LayoutRect even if they correspond to paint information (ie should be device pixels). Your ChangeLog entry seems to mention detecting sub-pixel layout change, could you explain me how you are going to handle those, especially with respect to the repaint logic? (Where will you do the rounding / snapping? Where will you actually use IntRect?...)
Comment on attachment 133769 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133769&action=review >> Source/WebCore/ChangeLog:9 >> + was never used. The old bounds were instead always gleaned from the renderer's > > That statement is wrong. The parameter was used by SVG when LayoutRepainter was introduced but got removed over time (at least that's what git blame is telling me). Please amend this statement to say that it is unused now. Sorry, that was the point I intended to make. Incorrect choice of words. Either way, it's not unnecessary. >> Source/WebCore/rendering/LayoutRepainter.h:49 >> + LayoutRect m_oldOutlineBox; > > It's weird to store them as LayoutRect even if they correspond to paint information (ie should be device pixels). Your ChangeLog entry seems to mention detecting sub-pixel layout change, could you explain me how you are going to handle those, especially with respect to the repaint logic? (Where will you do the rounding / snapping? Where will you actually use IntRect?...) We currently evaluate whether or not to repaint after layout using sub-pixel bounds. This is the safest possible strategy, as we chose not to try and optimize this early. The rects passed to the embedder to repaint are pixel snapped, if that's the question you're asking.
Comment on attachment 133769 [details] Patch OK. Obviously you should update per Julien's comments.
> We currently evaluate whether or not to repaint after layout using sub-pixel bounds. This is the safest possible strategy, as we chose not to try and optimize this early. That sounds like a good comment to add to the LayoutRepainter declaration. I would say, repainting AFAICT only makes sense on a device pixel perspective bit it's better to be conservative here. > The rects passed to the embedder to repaint are pixel snapped, if that's the question you're asking. OK, I think I see what you mean here. Correct me if I am wrong: WebCore repainting logic will always use LayoutRect, snapped before calling our platform to do the invalidation. That sounds very similar to painting: it will be a lazily (late) conversion. I would love 1 line on the wiki about that :-)
(In reply to comment #5) > > We currently evaluate whether or not to repaint after layout using sub-pixel bounds. This is the safest possible strategy, as we chose not to try and optimize this early. > > That sounds like a good comment to add to the LayoutRepainter declaration. I would say, repainting AFAICT only makes sense on a device pixel perspective bit it's better to be conservative here. > > > The rects passed to the embedder to repaint are pixel snapped, if that's the question you're asking. > > OK, I think I see what you mean here. Correct me if I am wrong: WebCore repainting logic will always use LayoutRect, snapped before calling our platform to do the invalidation. That sounds very similar to painting: it will be a lazily (late) conversion. I would love 1 line on the wiki about that :-) That's a fantastic idea as usual :) I'll update the changelog and Wiki. Thanks guys!
(In reply to comment #5) > > We currently evaluate whether or not to repaint after layout using sub-pixel bounds. This is the safest possible strategy, as we chose not to try and optimize this early. > > That sounds like a good comment to add to the LayoutRepainter declaration. I would say, repainting AFAICT only makes sense on a device pixel perspective bit it's better to be conservative here. > > > The rects passed to the embedder to repaint are pixel snapped, if that's the question you're asking. > > OK, I think I see what you mean here. Correct me if I am wrong: WebCore repainting logic will always use LayoutRect, snapped before calling our platform to do the invalidation. That sounds very similar to painting: it will be a lazily (late) conversion. I would love 1 line on the wiki about that :-) No correction necessary as you're absolutely right. I'll add a comment before landing.
Comment on attachment 133769 [details] Patch Landed in http://trac.webkit.org/changeset/112243