Bug 90428 – Fall thorugh in handling a "button" start tag in HTMLTreeBuilder::processStartTagForInBody(AtomicHTMLToken&)

• Status: RESOLVED
• Resolution: WONTFIX
• Component: DOM

[Kwang Yul Seo]

When handling a "button" start tag in HTMLTreeBuilder::processStartTagForInBody(AtomicHTMLToken&), we can fall through instead of calling processStartTag(token). It saves at least one function call.

[Kwang Yul Seo]

Created attachment 150526 [details]
Patch

[Adam Barth]

Comment on attachment 150526 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        When handling a "button" start tag in HTMLTreeBuilder::processStartTagForInBody(AtomicHTMLToken&),
> +        we can fall through instead of calling processStartTag(token). It saves at least one function call.

How do we know this is safe?  Can you explain in more detail?  The reason we have this FIXME here is because we're not sure its safe.

[Kwang Yul Seo]

(In reply to comment #2)
> How do we know this is safe?  Can you explain in more detail?  The reason we have this FIXME here is because we're not sure its safe.

Okay, I will explain it in more detail. Because I am new to parser code, maybe I misunderstood something. Please tell me if I am wrong.

    if (token.name() == buttonTag) {
        if (m_tree.openElements()->inScope(buttonTag)) {
            parseError(token);
            processFakeEndTag(buttonTag);

// We are still in the "in body" mode because processing a "button" end tag in the "in body" mode does not change the insertion mode.

// Now we need to process the token again in the "in body" mode

// I assumed that there is only one button element in the stack of open elements. The button element was popped in processFakeEndTag(buttonTag), so we don't need to check if we are in the button scope.

// So instead of calling processStartTag(token) which in turn calls processStartTagForInBody(token), we can fall through here.

            processStartTag(token); // FIXME: Could we just fall through here?
            return;
        }
        m_tree.reconstructTheActiveFormattingElements();
        m_tree.insertHTMLElement(token);
        m_framesetOk = false;
        return;
    }

[Kwang Yul Seo]

I ran the layout tests and confirmed that there is no regression. But I don't know if there is a layout test case which tests this behavior.

[Adam Barth]

Some of the complexity here comes from the fact that some of the other modes of the parser call processStartTagForInBody, so we need to analyze how the end tag is treated in all those modes as well.  I suspect the patch is correct, but it's a fair amount of work to verify it (and the benefit is very small), which is why the FIXME is there rather than the fall through.

I appreciate your posting the patch for review.  It's just that reviewing the patch is much more difficult than writing it.

[Kwang Yul Seo]

(In reply to comment #5)
Thanks for the comment. Now I got the true meaning of FIXME here. I agree to your point. The benefit is so small considering the amount of verification work.

[Kwang Yul Seo]

WONTFIX.