Bug 82185 - LayoutRepainter: Remove unused constructor parameter and update to LayoutUnits
Summary: LayoutRepainter: Remove unused constructor parameter and update to LayoutUnits
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Levi Weintraub
URL:
Keywords:
Depends on:
Blocks: 60318
  Show dependency treegraph
 
Reported: 2012-03-26 03:53 PDT by Levi Weintraub
Modified: 2012-03-27 03:40 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.86 KB, patch)
2012-03-26 04:00 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Levi Weintraub 2012-03-26 03:53:30 PDT
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.
Comment 1 Levi Weintraub 2012-03-26 04:00:04 PDT
Created attachment 133769 [details]
Patch
Comment 2 Julien Chaffraix 2012-03-26 13:20:19 PDT
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 3 Levi Weintraub 2012-03-26 15:12:07 PDT
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 4 Eric Seidel (no email) 2012-03-26 15:40:10 PDT
Comment on attachment 133769 [details]
Patch

OK.  Obviously you should update per Julien's comments.
Comment 5 Julien Chaffraix 2012-03-26 16:38:13 PDT
> 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 :-)
Comment 6 Levi Weintraub 2012-03-27 02:16:41 PDT
(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!
Comment 7 Levi Weintraub 2012-03-27 03:07:59 PDT
(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 8 Levi Weintraub 2012-03-27 03:40:39 PDT
Comment on attachment 133769 [details]
Patch

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