Bug 92240 – Read tag names and attributes from the saved tokens in HTMLTreeBuilder::resetInsertionModeAppropriately.

• Component: DOM
• Status: RESOLVED FIXED

[Kwang Yul Seo]

This is a follow-up patch for r123577.

[Kwang Yul Seo]

Created attachment 154328 [details]
Patch

[Adam Barth]

Comment on attachment 154328 [details]
Patch

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

> Source/WebCore/html/parser/HTMLStackItem.h:89
> +        } else
> +            ASSERT_NOT_REACHED();

Generally we prefer to use switch statements for situations like this so that the compiler can tell us when we've forgotten an enum value.  In those cases, we leave off the "default" case.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:1635
> +        RefPtr<HTMLStackItem> itemForContextElement;

This point is that normally we don't need to hold a reference to a stack item because m_tree hold it for us, but in the case of the root node, we need to create an extra item?

I think it would be cleaner if item where a RefPtr.  That would mean an extra ref()/deref() in the non-root case, but I don't think this function is that hot.

[Adam Barth]

Comment on attachment 154328 [details]
Patch

This is fine, pending the style issue and an HTML parser benchmark run.  :)

[Kwang Yul Seo]

(In reply to comment #3)
> (From update of attachment 154328 [details])
> This is fine, pending the style issue and an HTML parser benchmark run.  :)

It seems this patch slightly improves the performance even with an extra ref()/deref() in the non-root case.

* Before
kseo@cat:WebKit (master)> ./Tools/Scripts/run-perf-tests Parser/html-parser.html
Running 1 tests
Running Parser/html-parser.html (1 of 1)
RESULT Parser: html-parser= 2839.0 ms
median= 2838.0 ms, stdev= 3.30151480384 ms, min= 2833.0 ms, max= 2847.0 ms

* After
kseo@cat:WebKit (master)> ./Tools/Scripts/run-perf-tests Parser/html-parser.html
Running 1 tests
Running Parser/html-parser.html (1 of 1)
RESULT Parser: html-parser= 2818.9 ms
median= 2818.0 ms, stdev= 4.21781934179 ms, min= 2812.0 ms, max= 2830.0 ms

[Kwang Yul Seo]

(In reply to comment #2)
> > Source/WebCore/html/parser/HTMLStackItem.h:89
> > +        } else
> > +            ASSERT_NOT_REACHED();
> 
> Generally we prefer to use switch statements for situations like this so that the compiler can tell us when we've forgotten an enum value.  In those cases, we leave off the "default" case.

Okay. That's a good convention.

> > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:1635
> > +        RefPtr<HTMLStackItem> itemForContextElement;
> 
> This point is that normally we don't need to hold a reference to a stack item because m_tree hold it for us, but in the case of the root node, we need to create an extra item?
> 
> I think it would be cleaner if item where a RefPtr.  That would mean an extra ref()/deref() in the non-root case, but I don't think this function is that hot.

Yeah, it is much cleaner. I confirmed that there is no observable performance penalty here.

[Kwang Yul Seo]

Committed r123671: <http://trac.webkit.org/changeset/123671>