Bug 90428 - Fall thorugh in handling a "button" start tag in HTMLTreeBuilder::processStartTagForInBody(AtomicHTMLToken&)
Summary: Fall thorugh in handling a "button" start tag in HTMLTreeBuilder::processStar...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-02 20:55 PDT by Kwang Yul Seo
Modified: 2012-07-09 07:29 PDT (History)
2 users (show)

See Also:


Attachments
Patch (1.70 KB, patch)
2012-07-02 20:57 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kwang Yul Seo 2012-07-02 20:55:59 PDT
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.
Comment 1 Kwang Yul Seo 2012-07-02 20:57:59 PDT
Created attachment 150526 [details]
Patch
Comment 2 Adam Barth 2012-07-02 21:24:07 PDT
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.
Comment 3 Kwang Yul Seo 2012-07-02 22:06:09 PDT
(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;
    }
Comment 4 Kwang Yul Seo 2012-07-02 22:07:51 PDT
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.
Comment 5 Adam Barth 2012-07-02 22:21:05 PDT
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.
Comment 6 Kwang Yul Seo 2012-07-02 23:06:08 PDT
(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.
Comment 7 Kwang Yul Seo 2012-07-09 07:29:35 PDT
WONTFIX.