Bug 90596 – Move element/tag category query functions to a separate file

• Component: DOM
• Status: RESOLVED WONTFIX

[Kwang Yul Seo]

Move element/tag category query functions such as isSpecialNode(Node*) to HTMLParserCategory.h/cpp. Currently, these functions are scattered in HTMLTokenizer.cpp and HTMLElementStack.cpp. 

After this patch, we can move HTMLTreeBuilder::furthestBlockForFormattingElement(Element*) method to HTMLElementStack class. This method naturally belongs to HTMLElementStack, but it is currently in HTMLTreeBuilder only because isSpecialNode(Element*) is defined in HTMLTreeBuilder.cpp file.

[Kwang Yul Seo]

Created attachment 150906 [details]
Patch

[Early Warning System Bot]

Comment on attachment 150906 [details]
Patch

Attachment 150906 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13141125

[Early Warning System Bot]

Comment on attachment 150906 [details]
Patch

Attachment 150906 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13147031

[Build Bot]

Comment on attachment 150906 [details]
Patch

Attachment 150906 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13138904

[Kwang Yul Seo]

I will fix build errors soon.

[WebKit Review Bot]

Comment on attachment 150906 [details]
Patch

Attachment 150906 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13136987

[Kwang Yul Seo]

Created attachment 150923 [details]
Patch

[Build Bot]

Comment on attachment 150923 [details]
Patch

Attachment 150923 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13143139

[Kwang Yul Seo]

Created attachment 150926 [details]
Patch

[Kwang Yul Seo]

I don't like the name, HTMLParserCategory. Please suggest me a better name for this.

[Kwang Yul Seo]

(In reply to comment #8)
> (From update of attachment 150923 [details])
> Attachment 150923 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/13143139

Wow. It is surprisingly hard to make all the build bots green. Modifying Xcode project file by hand is the worst experience!

[Adam Barth]

Comment on attachment 150926 [details]
Patch

I don't see the benefit of this change.  Currently, these functions are defined in the files that use them.  Do we achieve any code sharing with this change?

[Kwang Yul Seo]

(In reply to comment #12)
> (From update of attachment 150926 [details])
> I don't see the benefit of this change.  Currently, these functions are defined in the files that use them.  Do we achieve any code sharing with this change?

I wanted to move HTMLTreeBuilder::furthestBlockForFormattingElement(Element*) to HTMLElementStack. (Bug 90607). There is a FIXME comment:

// FIXME: This probably belongs on HTMLElementStack.
HTMLElementStack::ElementRecord* HTMLTreeBuilder::furthestBlockForFormattingElement(Element* formattingElement)

This method internally uses isSpecialNode(Node*).

After HTMLTreeBuilder::furthestBlockForFormattingElement(Element*) is moved to HTMLElementStack, both HTMLTreeBuilder and HTMLElementStack need to access isSpecialNode(Node*). 

So I moved isSpecialNode(Node*) to a separate file. Because isSpecialNode(Node*) depends on many other functions, I had to move them too.

I named the file HTMLParserCategory.h/cpp since these functions happen to be tag category query functions. Then I moved a few related functions too.

So this patch does not achieve much code sharing. The only code sharing it achieves is isSpecialNode(Node*).

If you think it is better to leave HTMLTreeBuilder::furthestBlockForFormattingElement(Element*) where it is, I'm okay with r-.

Thanks for you review.

[Adam Barth]

I think it's probably not worth fixing.  Perhaps we should remove the FIXME comment?  Generally, FIXME comments are often questions we weren't sure about when we wrote the code originally.  We don't necessarily want to fix all of them.  They're mostly there to help folks working on this code in the future understand what we were thinking originally.

[Kwang Yul Seo]

(In reply to comment #14)
> I think it's probably not worth fixing.  Perhaps we should remove the FIXME comment?  Generally, FIXME comments are often questions we weren't sure about when we wrote the code originally.  We don't necessarily want to fix all of them.  They're mostly there to help folks working on this code in the future understand what we were thinking originally.

Okay. I will close Bug 90607 as WONTFIX too.

[Eric Seidel (no email)]

I probably wrote the fixme.  I kinda like the change... but I suspect we would want to do these all in a header instead.  But as Adam notes, this is probably not worth teh code churn.

[Eric Seidel (no email)]

I really appreciate you looking through all these parser FIXMEs.  As Adam points out, we wrote a whole bunch of FIXMEs while writing the parser.  Not all of which we really should fix.  It's nice that someone is looking through them!

[Kwang Yul Seo]

(In reply to comment #17)
> I really appreciate you looking through all these parser FIXMEs.  As Adam points out, we wrote a whole bunch of FIXMEs while writing the parser.  Not all of which we really should fix.  It's nice that someone is looking through them!

Thanks. One particular FIXME I really want to remove is in HTMLPreloadScanner::scan()

    // FIXME: We should save and re-use these tokens in HTMLDocumentParser if
    // the pending script doesn't end up calling document.write.
    while (m_tokenizer->nextToken(m_source, m_token)) {
        processToken();
        m_token.clear();
    }   

HTMLPreloadScanner is nice, but it fails to save and reuse the tokens because it just uses an approximation of how the tree builder would update the tokenizer's state.

I want to teach HTMLDocumentParser to parse speculatively (in addition to preloading) while it is waiting for scripts. I will file a bug and share my initial design.

Looking through all these parser FIXMEs is a good way to learn the parser code base. I really appreciate your review. Thanks Adam and Eric.

[Adam Barth]

w.r.t. that FIXME, another path is to move the preload scanner to its own thread.  For either change, we really need benchmarks to tell us whether we're making things faster or slower.  Maybe we can both re-use tokens and move the scanner to its own thread?

[Kwang Yul Seo]

(In reply to comment #19)
> w.r.t. that FIXME, another path is to move the preload scanner to its own thread.  For either change, we really need benchmarks to tell us whether we're making things faster or slower.  Maybe we can both re-use tokens and move the scanner to its own thread?

It was tried before in Bug 63531. The patch was rejected because the performance gain was very small.

After we introduce speculative parsing, then maybe we can both re-use tokens and move HTML parsing (not just preload scanning) off the main thread. Firefox exactly doe this.

https://developer.mozilla.org/en/Gecko/HTML_parser_threading
https://bugzilla.mozilla.org/show_bug.cgi?id=483016

Now the discussion is off the topic. I filed Bug 90751 for further discussion.