Bug 104142

Created attachment 177796 [details] Patch

Comment on attachment 177796 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177796&action=review > Source/WebCore/ChangeLog:8 > + This patch adds a "stack" (although the impl is just a map) which allows for retaining the chosen "implied context" for each template element. I see, it is just a stack. Is there a reason you used a HashMap rather than a Vector to implement the stack? A Vector seems both more efficient and to be a clearer representation of a stack. > Source/WebCore/html/parser/HTMLTreeBuilder.h:237 > +#if ENABLE(TEMPLATE_ELEMENT) > + HashMap<Element*, InsertionMode> m_templateInsertionModes; > +#endif Interesting! Does this need to be a hashmap? I would have expected just a Vector<InsertionMode> that we'd push and pop in parallel with the stack of open elements. (We can overwrite the top of the stack in processStartTag.

Yeah, this looks like a good way to resolve this issue (modulo implementation details).

Created attachment 178095 [details] Patch

(In reply to comment #2) > (From update of attachment 177796 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177796&action=review > > > Source/WebCore/ChangeLog:8 > > + This patch adds a "stack" (although the impl is just a map) which allows for retaining the chosen "implied context" for each template element. > > I see, it is just a stack. Is there a reason you used a HashMap rather than a Vector to implement the stack? A Vector seems both more efficient and to be a clearer representation of a stack. > > > Source/WebCore/html/parser/HTMLTreeBuilder.h:237 > > +#if ENABLE(TEMPLATE_ELEMENT) > > + HashMap<Element*, InsertionMode> m_templateInsertionModes; > > +#endif > > Interesting! Does this need to be a hashmap? I would have expected just a Vector<InsertionMode> that we'd push and pop in parallel with the stack of open elements. (We can overwrite the top of the stack in processStartTag. Switched to a vector-as-stack.

Comment on attachment 178095 [details] Patch Should we assert that the stack is empty when we're done parsing?

(In reply to comment #6) > (From update of attachment 178095 [details]) > Should we assert that the stack is empty when we're done parsing? Well, right now, it's not always empty. For example, in the fragment-parsing mode, it's always size 1 when we finish. And I haven't added code to, e.g., processEndOfFile to clear out the stack. But I am a little worried about it getting out of sync with the stack of open elements. If I were going to add such an assert, would it go in finished()?

Thanks for taking over this one as well. FWIW, I'm fine with what you guys decide here. The reason why I used a hashmap rather than a vector is that I was worried about maintaining a 'stack within a stack'. In other words, its seems to me that that the only logical stack here is the stack of open elements and that the map was encoding a property which gets applied to each <template> encountered during parsing, but that properties lifetime is tied to a single invocation of the parser.

(In reply to comment #8) > Thanks for taking over this one as well. > > FWIW, I'm fine with what you guys decide here. The reason why I used a hashmap rather than a vector is that I was worried about maintaining a 'stack within a stack'. In other words, its seems to me that that the only logical stack here is the stack of open elements and that the map was encoding a property which gets applied to each <template> encountered during parsing, but that properties lifetime is tied to a single invocation of the parser. For what it's worth, running the original (HashMap-based) patch in debug mode caused run-template.html to crash for me. I think there may be some issues with having to get ahold of the current template when wanting to find the current template insertion mode. Perhaps we should integrate this enum stack directly into the element stack?

Created attachment 179530 [details] Patch with better ASSERTs

Comment on attachment 179530 [details] Patch with better ASSERTs Seems reasonable.

The review tool seems to have lost all my comments!?!

Comment on attachment 179530 [details] Patch with better ASSERTs View in context: https://bugs.webkit.org/attachment.cgi?id=179530&action=review > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2517 > +#if ENABLE(TEMPLATE_ELEMENT) && !ASSERT_DISABLED Why is this disabled in release? > LayoutTests/html5lib/resources/template.dat:847 > +<body><template><thead></thead><template><tr></tr></template><tr></tr><tfoot></tfoot></template> Do we need additional tests (for misnested templates, etc)?

Comment on attachment 179530 [details] Patch with better ASSERTs View in context: https://bugs.webkit.org/attachment.cgi?id=179530&action=review >> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2517 >> +#if ENABLE(TEMPLATE_ELEMENT) && !ASSERT_DISABLED > > Why is this disabled in release? I had this here since only an ASSERT depends on the fact that the vector is cleared before destruction. I was worried about performance, but I guess we'll take the hit anyway when we destroy the tree builder, so I may as well remove the disable-in-release thing. >> LayoutTests/html5lib/resources/template.dat:847 >> +<body><template><thead></thead><template><tr></tr></template><tr></tr><tfoot></tfoot></template> > > Do we need additional tests (for misnested templates, etc)? There are already some mis-nested examples in this file. Did you have particular test cases you had in mind?

Created attachment 180004 [details] Patch for landing

Comment on attachment 180004 [details] Patch for landing Clearing flags on attachment: 180004 Committed r138059: <http://trac.webkit.org/changeset/138059>

All reviewed patches have been landed. Closing bug.

(In reply to comment #12) > The review tool seems to have lost all my comments!?! See https://bugs.webkit.org/show_bug.cgi?id=105252 . I've seen it too, but we need repro steps.