[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 #2 from John Mellor <johnme at chromium.org>  2013-01-14 12:23:16 PST ---
(From update of attachment 182571)
View in context: https://bugs.webkit.org/attachment.cgi?id=182571&action=review

Looks pretty good. Commented on the tree traversal and some nits.

> Source/WebCore/rendering/TextAutosizer.cpp:298
> +bool TextAutosizer::isHeaderWithLinks(const RenderObject* container)

Nit: I'm not sure "header" is a good term for these. They can also occur in the footer for example. How about "isRowOfLinks"?

> Source/WebCore/rendering/TextAutosizer.cpp:302
> +    //  2. it should contain at least 2 inline link elements

Nit: I think s/link/<a>/ would be clearer.

> Source/WebCore/rendering/TextAutosizer.cpp:303
> +    //  3. it should contain only inline or text elements unless they are children

Nit: I don't think you need "or text elements", as text implicitly acts inline.

> Source/WebCore/rendering/TextAutosizer.cpp:306
> +    int numLinks = 0;

Nit: WebKit doesn't like abbreviations. Perhaps "linkCount"?

> Source/WebCore/rendering/TextAutosizer.cpp:319
> +                const String valueWithoutWhitespaces = currentNode->nodeValue().stripWhiteSpace();

Rather than going via Node, could you not just do toRenderText(current)->text()->stripWhiteSpace()->length() ?

> Source/WebCore/rendering/TextAutosizer.cpp:335
> +            if (!isHeaderWithLinksCandidate(child, stayWithin, linkCount))

I'm hesitant to add another complicated recursive tree traversal to this file.

It seems that what you need is almost exactly the same as nextInPreOrderSkippingDescendantsOfContainers, except that you want to be able to completely skip some arbitrary subtrees of the traversal, which isn't currently possible/practical. This sounds like something that could be useful elsewhere.

How about you rename nextInPreOrderSkippingDescendantsOfContainers to nextInPreOrder, and add a new SkipMode parameter, which is an enum that can either be SkipDescendantsOfContainers or SkipAllDescendants? Then you'd be able to do something like:

int numLinks = 0;
RenderObject* descendant = nextInPreOrder(SkipDescendantsOfContainers, container, container);
while (descendant) {
    ...
    if (isLink) {
        numLinks++;
        descendant = nextInPreOrder(SkipAllDescendants, descendant, container);
    }
    else
        descendant = nextInPreOrder(SkipDescendantsOfContainers, descendant, container);
}

> LayoutTests/fast/text-autosizing/header-links-autosizing-expected.html:31
> +</div>

This </div> doesn't have a matching <div>. Perhaps you didn't mean to include the earlier </div>?

> LayoutTests/fast/text-autosizing/header-links-autosizing.html:31
> +<a href="">These</a>

Test looks good (apart from the </div>). Could you also include a 2nd test which covers the <li style="display: inline"><a>...</a></li> case?

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