[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 #4 from Julien Chaffraix <jchaffraix at webkit.org>  2013-01-15 13:50:08 PST ---
(From update of attachment 182813)
View in context: https://bugs.webkit.org/attachment.cgi?id=182813&action=review

I would need more context to understand how this patch helps in any ways. Adding an extra parameter doesn't seem to be much of a win compared keeping separated methods as it makes the code easier to misread or misuse but also makes the code less readable (more parameters and your traversal function becomes a swiss-knife function)

>> Source/WebCore/rendering/TextAutosizer.cpp:299
>> +            (renderer == container) ? NormalTraversal : SkipDescendantsOfContainers);
> 
> Nit: WebKit doesn't often wrap long lines (except comments). Perhaps break this into two lines, where the first line sets a temporary |traversalMode| variable?

This seems to be a change in behavior as you could skip descendants where you didn't before: you will only skip descendants if renderer == container && isAutosizingContainer(renderer) where it used to be only isAutosizingContainer(renderer)

> 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)

> Source/WebCore/rendering/TextAutosizer.cpp:367
> +    bool skipChildren = (mode == SkipDescendants) || (mode == SkipDescendantsOfContainers && isAutosizingContainer(current));

Per our style guide: Precede boolean values with words like "is" and "did".

I think |shouldSkipChildren| would be more readable.

> Source/WebCore/rendering/TextAutosizer.cpp:371
> +        RenderObject* child = current->firstChild();
> +        if (child)

if (RenderObject* child = current->firstChild())
    return child;

> Source/WebCore/rendering/TextAutosizer.cpp:379
> +        RenderObject* sibling = ancestor->nextSibling();
> +        if (sibling)

Again it should be on one line.

> Source/WebCore/rendering/TextAutosizer.h:62
> +        SkipDescendants,

This new mode is not explained anywhere nor used. I would be more supportive to postpone adding it until it's actually used.

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