[Webkit-unassigned] [Bug 106929] Text Autosizing: extend preOrderTraversal to handle different traversal modes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=106929





--- Comment #7 from John Mellor <johnme at chromium.org>  2013-01-15 16:45:21 PST ---
(From update of attachment 182813)
View in context: https://bugs.webkit.org/attachment.cgi?id=182813&action=review

(In reply to comment #5)
> This is makes render tree traversal less like the traversal we do for the DOM tree. But instead we want it to be more like the DOM tree traversal. I really like the work Antti has done in NodeTraversal.h and I’d like to see our render tree traversal use similar patterns.

There may have been a slight mis-communication: this wasn't modifying the core render tree traversal functions defined in RenderObject.h, but merely Text Autosizing's private traversal function. But I agree that we should try to stay in sync with the core traversal functions, and avoid unnecessary duplication.

(In reply to comment #4)
> I would need more context to understand how this patch helps in any ways.

Agree, more explanation would have been helpful. The current patch to bug 106792 uses a custom tree traversal function because it wants to traverse the descendants of a container, skipping descendants of renderers that are themselves containers (so far this is identical to nextInPreOrderSkippingDescendantsOfContainers), but it also wants to skip descendants of <a> elements. Currently nextInPreOrderSkippingDescendantsOfContainers doesn't provide any easy way of skipping arbitrary subtrees, and it seemed a feature that might be useful in other places too, so I suggested that Tim make nextInPreOrderSkippingDescendantsOfContainers slightly more flexible.

In hindsight I realise that this was unnecessary, as I'd forgotten that RenderObject.h provides nextInPreOrderAfterChildren(stayWithin) which has exactly the functionality needed for this case. So this patch can probably be dropped.

Instead, to reduce duplication, at some point we might want to reimplement nextInPreOrderSkippingDescendantsOfContainers as simply:

RenderObject* TextAutosizer::nextInPreOrderSkippingDescendantsOfContainers(const RenderObject* current, const RenderObject* stayWithin)
{
    if (current == stayWithin || !isAutosizingContainer(current))
        return current->nextInPreOrder(stayWithin);
    return current->nextInPreOrderAfterChildren(stayWithin);
}

>> Source/WebCore/rendering/TextAutosizer.cpp:-366
>> -    if (current == stayWithin || !isAutosizingContainer(current))
> 
> You removed the stayWithin check which means that you can potentially continue iterating if (current == stayWithin)

For the record, no, this stayWithin check was because while we normally want to skip descendants of containers, we usually start a traversal by passing in a container as |current|, and in that specific case we didn't want to skip descendants of containers (because the whole point is to traverse the container). Instead Tim used NormalTraversal when starting the traversals above, so this special case was no longer necessary which is arguably cleaner (though might make it easier to misuse the traversal?).

The stayWithin check in the for loop below is the one that actually enforces the stayWithin.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.