Bug 140032

Summary: Modernize coding style of HTMLTreeBuilder
Product: WebKit Reporter: Darin Adler <darin>
Component: DOMAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, gyuyoung.kim, mitz
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 140042    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch kling: review+

Darin Adler
Reported 2015-01-01 14:28:57 PST
Modernize coding style of HTMLTreeBuilder
Attachments
Patch (157.72 KB, patch)
2015-01-01 14:58 PST, Darin Adler
no flags
Patch (157.75 KB, patch)
2015-01-01 17:01 PST, Darin Adler
kling: review+
Darin Adler
Comment 1 2015-01-01 14:58:20 PST
Darin Adler
Comment 2 2015-01-01 17:01:04 PST
Andreas Kling
Comment 3 2015-01-02 00:35:53 PST
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 4 2015-01-02 08:22:28 PST
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
Comment 5 2015-01-02 08:31:52 PST
Alexey Proskuryakov
Comment 6 2015-01-02 10:01:54 PST
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
Comment 7 2015-01-02 10:04:31 PST
Re-opened since this is blocked by bug 140042
Darin Adler
Comment 8 2015-01-02 10:11:34 PST
That was just a single bad assertion that should have been removed.
Darin Adler
Comment 9 2015-01-02 10:17:06 PST
Note You need to log in before you can comment on or make changes to this bug.