[Webkit-unassigned] [Bug 106792] Text Autosizing: don't autosize headers with multiple inline links.

[bugzilla-daemon at webkit.org]

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





--- Comment #5 from John Mellor <johnme at chromium.org>  2013-01-16 06:41:59 PST ---
(From update of attachment 182968)
View in context: https://bugs.webkit.org/attachment.cgi?id=182968&action=review

Looks good. Some nits.

> Source/WebCore/rendering/TextAutosizer.cpp:309
> +    // A "header with links" is a container for which holds:

s/header with/row of/

> Source/WebCore/rendering/TextAutosizer.cpp:312
> +    //  3. it should contain only RenderInline or RenderText elements unless they are children

It's confusing to mention RenderInline here, as you can get elements that are inline but aren't RenderInline (such as RenderApplet, RenderListMarker, RenderRubyRun and RenderTableCol). I'd recommend doing s/RenderInline or RenderText/inline/ (whether or not you remove isText below).

> Source/WebCore/rendering/TextAutosizer.cpp:321
> +            if (!renderer->isText() && !renderer->isInline())

Do you actually need the |!renderer->isText() &&| part? Unless I missed something I think isInline should return true for RenderText elements (or at least for RenderText elements with inline parents, which should be all you encounter here anytime it actually is a row of links).

> Source/WebCore/rendering/TextAutosizer.cpp:324
> +        const Node* rendererNode = renderer->node();

Nit: probably sufficient to call this just |node|, but I don't really mind...

> Source/WebCore/rendering/TextAutosizer.cpp:329
> +        renderer = (isLink || isAutosizingContainer(renderer)) ?

You can probably simplify this condition using nextInPreOrderSkippingDescendantsOfContainers, e.g.:

renderer = isLink ? renderer->nextInPreOrderAfterChildren(container) : nextInPreOrderSkippingDescendantsOfContainers(renderer, container);

Or even:

if (node && node->isElementNode() && toElement(node)->tagQName() == aTag) {
    linkCount++;
    // Skip traversing descendants of the <a>.
    renderer = renderer->nextInPreOrderAfterChildren(container);
} else
    renderer = nextInPreOrderSkippingDescendantsOfContainers(renderer, container);

> LayoutTests/fast/text-autosizing/header-li-links-autosizing-expected.html:38
> +    whereas the inline links in the header above and the accompanying time stamp

s/and the accompanying time stamp//

> LayoutTests/fast/text-autosizing/header-li-links-autosizing.html:26
> +<div>

Nit: this <div> is redundant (it's fine for the <ul> to be a direct child of the body).

> LayoutTests/fast/text-autosizing/header-links-autosizing-expected.html:18
> +    <span style="float: right">

It's a little confusing having this timestamp here, because the test ends up testing that the time stamp doesn't get autosized, but the reason for that happening is different to the reason for the links not getting autosized. Instead, how about something like this:

<div>
    <a>...</a>
    >
    <a>...</a>
    >
    <a>...</a>
    >
    <a>...</a>
    >
    <a>...</a>
    <div>
        [A bunch of text that should be autosized].
    </div>
</div>
<div>
    [Some more text that should be autosized].
</div>

That way you clearly test the fact that descendant containers are ignored.

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