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.
Created attachment 150906 [details] Patch
Comment on attachment 150906 [details] Patch Attachment 150906 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13141125
Comment on attachment 150906 [details] Patch Attachment 150906 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13147031
Comment on attachment 150906 [details] Patch Attachment 150906 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13138904
I will fix build errors soon.
Comment on attachment 150906 [details] Patch Attachment 150906 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13136987
Created attachment 150923 [details] Patch
Comment on attachment 150923 [details] Patch Attachment 150923 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13143139
Created attachment 150926 [details] Patch
I don't like the name, HTMLParserCategory. Please suggest me a better name for this.
(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!
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?
(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.
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.
(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.
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.
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!
(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.
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?
(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.