The function is only called once and could easily be replaced by the usual IntPoint / LayoutPoint manipulation. Patch forthcoming.
Created attachment 142109 [details] Proposed removal 1.
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.
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.
Created attachment 142305 [details] Patch v2: Improved after Darin's comments + added more explanation on hasLayer() in ChangeLog.
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.
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.
Created attachment 142542 [details] Patch for landing
Comment on attachment 142542 [details] Patch for landing Clearing flags on attachment: 142542 Committed r117497: <http://trac.webkit.org/changeset/117497>
All reviewed patches have been landed. Closing bug.