Bug 74828 – The HTML parser doesn't enforce the "Noah's Ark condition" from the HTML5 spec

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

The HTML parser doesn't enforce the "Noah's Ark condition" from the HTML5 spec

[Adam Barth]

Created attachment 119792 [details]
Patch

[Darin Adler]

Comment on attachment 119792 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119792&action=review

> Source/WebCore/ChangeLog:10
> +        elements that can be in the list of active formating elements.  I'm not

Typo: formating instead of formatting

> Source/WebCore/ChangeLog:11
> +        entirely sure that enforcing this condition is worth the complexity,

Besides complexity, there is the performance cost. Did you measure that? I hope it’s negligible with the optimized code path you added here.

> Source/WebCore/html/parser/HTMLFormattingElementList.cpp:43
> +inline size_t attributeCount(Element* element)

Since this is in a cpp file it should have static so it gets internal linkage.

> Source/WebCore/html/parser/HTMLFormattingElementList.cpp:134
> +void HTMLFormattingElementList::tryToEnsureNoahsArkConditionQuickly(Element* newElement, Vector<Element*>& remainingCandiates)

Typo: remainingCandiates rather than remainingCandidates

> Source/WebCore/html/parser/HTMLFormattingElementList.cpp:138
> +    // Initially, use a vector with inline capacity to avoid a malloc in the

The use of the word “initially” here is confusing. I know it refers to this case vs. the non-quickly function, but it is not obvious.

> Source/WebCore/html/parser/HTMLFormattingElementList.cpp:142
> +    size_t newElementAttributeCount = attributeCount(newElement);

Is it possibly worth checking m_entries.size() for zero first, so we don’t have to fetch the attribute count at all in that case?

> Source/WebCore/html/parser/HTMLFormattingElementList.cpp:145
> +    for (size_t i = 0; i < m_entries.size(); ++i) {
> +        Entry& entry = m_entries[m_entries.size() - i - 1];

I would write the loop like this:

    for (size_t i = m_entries.size(); i; ) {
        --i;
        Entry& entry = m_entries[i];

It’s my new favorite way to write backwards loops. I think it’s easier to read this way.

> Source/WebCore/html/parser/HTMLFormattingElementList.cpp:152
> +        if (!newElement->hasTagName(candidate->tagQName()))
> +            continue;

I believe hasTagName here is incorrect and the correct check is even faster and stricter, element->tagQName() != candidate->tagQName(). Is that right?

> Source/WebCore/html/parser/HTMLFormattingElementList.cpp:192
> +            // FIXME: Technically we shouldn't read this information back from
> +            // the DOM. Instead, the parser should keep a copy of the information.

Could we be more specific about why reading back from the DOM is an issue? Maybe the reason is that a script that changes the DOM should have no effect on the Noah's Ark algorithm. Or maybe there’s some other reason that you know of and I do not.

> Source/WebCore/html/parser/HTMLFormattingElementList.cpp:206
> +    // formatting lement list gets permuted.

Typo: lement list

> Source/WebCore/html/parser/HTMLFormattingElementList.h:130
> +    // http://www.whatwg.org/specs/web-apps/current-work/multipage/parsing.html#list-of-active-formatting-elements

I think a human-readable description of what this is for could be valuable too. My comment would say something like this:

    // The "Noah's Ark" algorithm, which removes redundant mis-nested elements.

[Adam Barth]

Created attachment 119823 [details]
incorporated review comments

[Adam Barth]

> Besides complexity, there is the performance cost. Did you measure that? I hope it’s negligible with the optimized code path you added here.

Looks like about 1.5% on the HTMLParser benchmark.  That's slightly more than I would have expected but still pretty small.

[WebKit Review Bot]

Comment on attachment 119823 [details]
incorporated review comments

Clearing flags on attachment: 119823

Committed r103223: <http://trac.webkit.org/changeset/103223>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.