Bug 140032 – Modernize coding style of HTMLTreeBuilder

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Darin Adler]

Modernize coding style of HTMLTreeBuilder

[Darin Adler]

Created attachment 243872 [details]
Patch

[Darin Adler]

Created attachment 243877 [details]
Patch

[Andreas Kling]

Comment on attachment 243877 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:19
> +        - Initialize scalars in the class definition rather than each constructor.
> +        - Use Ref/RefPtr instead of PassRefPtr.

Sweet!

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:139
> -        , m_isAll8BitData(m_text.length() && m_text.is8Bit())
> +        , m_isAll8BitData(m_text.is8Bit())

Are you sure this is cool? String::is8Bit() does not null-check String::m_impl whereas String::length() does.
(I don't know why String::is8Bit() doesn't null-check; seems like null strings could be considered 8-bit to me.)

[Darin Adler]

Comment on attachment 243877 [details]
Patch

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

>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:139
>> +        , m_isAll8BitData(m_text.is8Bit())
> 
> Are you sure this is cool? String::is8Bit() does not null-check String::m_impl whereas String::length() does.
> (I don't know why String::is8Bit() doesn't null-check; seems like null strings could be considered 8-bit to me.)

Yes. Note the ASSERT(!isEmpty()) that immediately follows this.

[Darin Adler]

Committed r177859: <http://trac.webkit.org/changeset/177859>

[Alexey Proskuryakov]

This caused many assertion failures on regression tests and API tests. Will roll out.

https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20WK2%20(Tests)/builds/1203

ASSERTION FAILED: m_insertionMode == InsertionMode::InTable
/Volumes/Data/slave/yosemite-debug/build/Source/WebCore/html/parser/HTMLTreeBuilder.cpp(1137) : void WebCore::HTMLTreeBuilder::processStartTag(WebCore::AtomicHTMLToken &)
1   0x110f920e0 WTFCrash
2   0x1130842c4 WebCore::HTMLTreeBuilder::processStartTag(WebCore::AtomicHTMLToken&)
3   0x113083987 WebCore::HTMLTreeBuilder::processToken(WebCore::AtomicHTMLToken&)
4   0x113082b8b WebCore::HTMLTreeBuilder::constructTree(WebCore::AtomicHTMLToken&)
5   0x112f81ba7 WebCore::HTMLDocumentParser::constructTreeFromHTMLToken(WebCore::HTMLToken&)
6   0x112f80fbb WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode)
7   0x112f805c9 WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode)
8   0x112f82a89 WebCore::HTMLDocumentParser::resumeParsingAfterScriptExecution()
9   0x112f82e7f WebCore::HTMLDocumentParser::notifyFinished(WebCore::CachedResource*)
10  0x112f82edf non-virtual thunk to WebCore::HTMLDocumentParser::notifyFinished(WebCore::CachedResource*)
11  0x112730922 WebCore::CachedResource::checkNotify()

[WebKit Commit Bot]

Re-opened since this is blocked by bug 140042

[Darin Adler]

That was just a single bad assertion that should have been removed.

[Darin Adler]

Committed r177863: <http://trac.webkit.org/changeset/177863>