Cleanup use of virtualChildren() calls

[Abhishek Arya]

Cleanup use of virtualChildren() calls

[Abhishek Arya]

From https://bugs.webkit.org/show_bug.cgi?id=85759
Comment #10 From Darin Adler 2012-05-07 13:15:17 PST (-) [reply] 
I don’t think it’s ever correct to call the virtualChildren function directly. Why is this doing that?

I looked at this code

void RenderBlock::updateFirstLetterStyle(RenderObject* firstLetterBlock, RenderObject* currentChild)
{
    RenderObject* firstLetter = currentChild->parent();
    RenderObject* firstLetterContainer = firstLetter->parent();
    RenderStyle* pseudoStyle = styleForFirstLetter(firstLetterBlock, firstLetterContainer);

and added assert to see if firstLetterContainer is always a renderblock, it is not. it can be an inline (e.g. fast/css-generated-content/002.html). Since type is not known in advance, does it make sense to keep virtualChildren() call. I don't understand how we can fix this. Same for this case in RenderObjectChildList.cpp

    if (oldContentPresent && (!newContentWanted || Node::diff(child->style(), pseudoElementStyle, owner->document()) == Node::Detach)) {
        // Nuke the child. 
        if (child->style()->styleType() == type) {
            oldContentPresent = false;
            child->destroy();
            child = (type == BEFORE) ? owner->virtualChildren()->firstChild() : owner->virtualChildren()->lastChild();
        }
    }

Place where type is known should be definitely fixed, like
// An anonymous block must be made to wrap this inline.
            RenderBlock* block = toRenderBlock(parent())->createAnonymousBlock();
            RenderObjectChildList* childlist = parent()->virtualChildren();
            childlist->insertChildNode(parent(), block, this);

What do you think Darin ?

[Darin Adler]

OK, I was wrong. It’s OK to call virtualChildren here. I misunderstood this, and thought it was done more like Node::firstChild.

[Julien Chaffraix]

(In reply to comment #2)
> OK, I was wrong. It’s OK to call virtualChildren here. I misunderstood this, and thought it was done more like Node::firstChild.

For the record, I think the use of virtualChildren is too subtle and we should think about better documenting it. In the example we have here, it's fine but it's something that slipped through the original review and got several veteran people wondering if it was right.