Bug 102761 - Call directly RenderBlock::deleteLineBoxTree
Summary: Call directly RenderBlock::deleteLineBoxTree
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Igor Trindade Oliveira
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-19 20:06 PST by Igor Trindade Oliveira
Modified: 2013-04-15 14:24 PDT (History)
9 users (show)

See Also:


Attachments
Proposed patch (2.22 KB, patch)
2012-11-19 20:12 PST, Igor Trindade Oliveira
bfulgham: review-
Details | Formatted Diff | Diff
Proposed patch (2.43 KB, patch)
2012-11-19 22:39 PST, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Igor Trindade Oliveira 2012-11-19 20:06:30 PST
Instead of implementing deleteLineBoxTree logic inside RenderBlock::determineStartPosition, we can reuse the code.
Comment 1 Igor Trindade Oliveira 2012-11-19 20:12:51 PST
Created attachment 175126 [details]
Proposed patch
Comment 2 Brent Fulgham 2012-11-19 21:41:00 PST
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.
Comment 3 Igor Trindade Oliveira 2012-11-19 21:53:44 PST
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.
Comment 4 Igor Trindade Oliveira 2012-11-19 22:39:39 PST
Created attachment 175143 [details]
Proposed patch

Use a conservative approach.
Comment 5 Antonio Gomes 2012-11-20 17:52:36 PST
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
Comment 6 WebKit Commit Bot 2013-04-15 14:24:15 PDT
Comment on attachment 175143 [details]
Proposed patch

Clearing flags on attachment: 175143

Committed r148468: <http://trac.webkit.org/changeset/148468>
Comment 7 WebKit Commit Bot 2013-04-15 14:24:17 PDT
All reviewed patches have been landed.  Closing bug.