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+

Description Darin Adler 2015-01-01 14:28:57 PST 
Modernize coding style of HTMLTreeBuilder
Comment 1 Darin Adler 2015-01-01 14:58:20 PST 
Created attachment 243872 [details]
Patch
Comment 2 Darin Adler 2015-01-01 17:01:04 PST 
Created attachment 243877 [details]
Patch
Comment 3 Andreas Kling 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.)
Comment 4 Darin Adler 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.
Comment 5 Darin Adler 2015-01-02 08:31:52 PST 
Committed r177859: <http://trac.webkit.org/changeset/177859>
Comment 6 Alexey Proskuryakov 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()
Comment 7 WebKit Commit Bot 2015-01-02 10:04:31 PST 
Re-opened since this is blocked by bug 140042
Comment 8 Darin Adler 2015-01-02 10:11:34 PST 
That was just a single bad assertion that should have been removed.
Comment 9 Darin Adler 2015-01-02 10:17:06 PST 
Committed r177863: <http://trac.webkit.org/changeset/177863>