Bug 102761 – Call directly RenderBlock::deleteLineBoxTree

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Igor Trindade Oliveira]

Instead of implementing deleteLineBoxTree logic inside RenderBlock::determineStartPosition, we can reuse the code.

[Igor Trindade Oliveira]

Created attachment 175126 [details]
Proposed patch

[Brent Fulgham]

Comment on attachment 175126 [details]
Proposed patch

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

Certainly the coding of this change is fine. However, I am concerned that we will start seeing flakiness in the 'fast/repaint' test cases again.  Please revise the ChangeLog (and/or the bug) to indicate why this is no longer a concern and I'll happily r+ it for you.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:-1814
> -        // crashes for fast/repaint tests.

What has changed since this comment was made that makes it possible to use 'deleteLineBoxTree' directly?  Is the comment no longer true -- i.e., have other bug fixes have rendered it unnecessary to avoid calling the method directly?  If so, please indicate this in the ChangeLog. Ideally, we would identify the change that fixed the original problem but that's not critical.

On the other hand, if we have no reason to believe that the crash is now resolved, I don't think it wise to move to this new implementation.

[Igor Trindade Oliveira]

The commentaries added in the changeset http://trac.webkit.org/changeset/86628 are not valid anymore.

nextLineBox:
InlineFlowBox* nextLineBox() const { return m_nextLineBox; }

and

nextRootBox():

RootInlineBox* nextRootBox() const { return static_cast<RootInlineBox*>(m_nextLineBox); }

nextRootBox is just casting m_nextLineBox.

Additionally,  RenderLineBoxList::deleteLineBoxTree is doing the same thing that the old code is doing. The main difference is because we need to set curr = 0, because it is used later in the code.
A conservative change will be instead of calling deleteLineBoxTree() we should call     m_lineBoxes.deleteLineBoxTree(renderArena());

What do you think?

(In reply to comment #2)
> (From update of attachment 175126 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175126&action=review
> 
> Certainly the coding of this change is fine. However, I am concerned that we will start seeing flakiness in the 'fast/repaint' test cases again.  Please revise the ChangeLog (and/or the bug) to indicate why this is no longer a concern and I'll happily r+ it for you.
> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:-1814
> > -        // crashes for fast/repaint tests.
> 
> What has changed since this comment was made that makes it possible to use 'deleteLineBoxTree' directly?  Is the comment no longer true -- i.e., have other bug fixes have rendered it unnecessary to avoid calling the method directly?  If so, please indicate this in the ChangeLog. Ideally, we would identify the change that fixed the original problem but that's not critical.
> 
> On the other hand, if we have no reason to believe that the crash is now resolved, I don't think it wise to move to this new implementation.

[Igor Trindade Oliveira]

Created attachment 175143 [details]
Proposed patch

Use a conservative approach.

[Antonio Gomes]

Comment on attachment 175143 [details]
Proposed patch

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

> Source/WebCore/ChangeLog:9
> +        RenderBlock::determineStartPosition, we can reuse the code. The commentaries added

typo:comments

[WebKit Commit Bot]

Comment on attachment 175143 [details]
Proposed patch

Clearing flags on attachment: 175143

Committed r148468: <http://trac.webkit.org/changeset/148468>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.