LayoutRepainter: Remove unused constructor parameter and update to LayoutUnits

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Levi Weintraub]

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.

[Levi Weintraub]

Created attachment 133769 [details]
Patch

[Julien Chaffraix]

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?...)

[Levi Weintraub]

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.

[Eric Seidel (no email)]

Comment on attachment 133769 [details]
Patch

OK.  Obviously you should update per Julien's comments.

[Julien Chaffraix]

> 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 :-)

[Levi Weintraub]

(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!

[Levi Weintraub]

(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.

[Levi Weintraub]

Comment on attachment 133769 [details]
Patch

Landed in http://trac.webkit.org/changeset/112243