Bug 86551 – Kill RenderLayer::relativePositionOffset(LayoutUnit& relX, LayoutUnit& relY) and cleanup RenderInline::clippedOverflowRectForRepaint

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Julien Chaffraix]

The function is only called once and could easily be replaced by the usual IntPoint / LayoutPoint manipulation.

Patch forthcoming.

[Julien Chaffraix]

Created attachment 142109 [details]
Proposed removal 1.

[Darin Adler]

Comment on attachment 142109 [details]
Proposed removal 1.

View in context: https://bugs.webkit.org/attachment.cgi?id=142109&action=review

> Source/WebCore/rendering/RenderInline.cpp:948
> -        if (inlineFlow->style()->position() == RelativePosition && inlineFlow->hasLayer())
> -            toRenderInline(inlineFlow)->layer()->relativePositionOffset(left, top);
> +        if (inlineFlow->style()->position() == RelativePosition)
> +            repaintRect.move(toRenderInline(inlineFlow)->layer()->relativePositionOffset());

Why is it OK to remove the inlineFlow->hasLayer() check? Presumably somehow you know that it has a layer, but how?

> Source/WebCore/rendering/RenderInline.cpp:968
> -        LayoutRect repaintRect(r);
> -        repaintRect.move(-cb->scrolledContentOffset()); // For overflow:auto/scroll/hidden.
> +        LayoutRect scrolledRepaintRect(repaintRect);
> +        scrolledRepaintRect.move(-cb->scrolledContentOffset()); // For overflow:auto/scroll/hidden.
>  
>          LayoutRect boxRect(LayoutPoint(), cb->cachedSizeForOverflowClip());
> -        r = intersection(repaintRect, boxRect);
> +        repaintRect = intersection(scrolledRepaintRect, boxRect);

Why do we need a separate variable for scrolledRepaintRect? We should use repaintRect directly.

> Source/WebCore/rendering/RenderInline.cpp:977
> +                LayoutRect childRect = curr->rectWithOutlineForRepaint(repaintContainer, outlineSize);
> +                repaintRect.unite(childRect);

I don’t think the local variable here helps.

> Source/WebCore/rendering/RenderInline.cpp:983
> +            LayoutRect contRect = continuation()->rectWithOutlineForRepaint(repaintContainer, outlineSize);
> +            repaintRect.unite(contRect);

I don’t think the local variable here helps.

[Julien Chaffraix]

Comment on attachment 142109 [details]
Proposed removal 1.

View in context: https://bugs.webkit.org/attachment.cgi?id=142109&action=review

>> Source/WebCore/rendering/RenderInline.cpp:948
>> +            repaintRect.move(toRenderInline(inlineFlow)->layer()->relativePositionOffset());
> 
> Why is it OK to remove the inlineFlow->hasLayer() check? Presumably somehow you know that it has a layer, but how?

Mostly because we ensure that RenderInline has a RenderLayer for position:relative. See the isRelPositioned() check in RenderInline::requiresLayer().

Also the classes inheriting from RenderInline either override clippedOverflowForRepaint (RenderSVGInline) or don't override requiresLayer (RenderRubyAsInline).

The only way we could crash here is if we have an SVG inline ancestor with a RenderInline or RenderRubyAsInline child. I don't think this is something we allow.

>> Source/WebCore/rendering/RenderInline.cpp:968
>> +        repaintRect = intersection(scrolledRepaintRect, boxRect);
> 
> Why do we need a separate variable for scrolledRepaintRect? We should use repaintRect directly.

I agree with you, I just didn't feel strongly enough to remove it. Will be changed of course.

[Julien Chaffraix]

Created attachment 142305 [details]
Patch v2: Improved after Darin's comments + added more explanation on hasLayer() in ChangeLog.

[Abhishek Arya]

Comment on attachment 142305 [details]
Patch v2: Improved after Darin's comments + added more explanation on hasLayer() in ChangeLog.

View in context: https://bugs.webkit.org/attachment.cgi?id=142305&action=review

r=me. please do bring the null check back.

> Source/WebCore/ChangeLog:3
> +        Kill RenderLayer::relativePositionOffset(LayoutUnit& relX, LayoutUnit& relY)

Do you want to add "and cleanup RenderInline::clippedOverflowRectForRepaint".

> Source/WebCore/ChangeLog:8
> +        No expected change in behavior.

Probably like 'No new tests since there is no expected change in behavior.'

> Source/WebCore/ChangeLog:14
> +        variable and removed 2 unneeded NULL-checks:

s/variable/variables.
s/NULL/null

> Source/WebCore/ChangeLog:16
> +            - we ensure that a RenderInlie with position: relative has a RenderLayer

typo RenderInlie. however we won't need this. see comment below.

> Source/WebCore/rendering/RenderInline.cpp:947
> +        if (inlineFlow->style()->position() == RelativePosition)

I think we should bring the hasLayer() check back. If some new class (like svg) inherits from renderinline and overrides hasLayer() and does not need the isRelPositioned() check, we will crash with a null.

[Julien Chaffraix]

Comment on attachment 142305 [details]
Patch v2: Improved after Darin's comments + added more explanation on hasLayer() in ChangeLog.

View in context: https://bugs.webkit.org/attachment.cgi?id=142305&action=review

>> Source/WebCore/ChangeLog:3
>> +        Kill RenderLayer::relativePositionOffset(LayoutUnit& relX, LayoutUnit& relY)
> 
> Do you want to add "and cleanup RenderInline::clippedOverflowRectForRepaint".

Sure.

>> Source/WebCore/ChangeLog:14
>> +        variable and removed 2 unneeded NULL-checks:
> 
> s/variable/variables.
> s/NULL/null

I prefer NULL in capital letters to match the C constant.

>> Source/WebCore/rendering/RenderInline.cpp:947
>> +        if (inlineFlow->style()->position() == RelativePosition)
> 
> I think we should bring the hasLayer() check back. If some new class (like svg) inherits from renderinline and overrides hasLayer() and does not need the isRelPositioned() check, we will crash with a null.

I don't think that's possible in our current code but it's likely for the better. I will add it back.

[Julien Chaffraix]

Created attachment 142542 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 142542 [details]
Patch for landing

Clearing flags on attachment: 142542

Committed r117497: <http://trac.webkit.org/changeset/117497>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.