Bug 140032 - Modernize coding style of HTMLTreeBuilder
Summary: Modernize coding style of HTMLTreeBuilder
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on: 140042
Blocks:
  Show dependency tree / graph
 
Reported: 2015-01-01 14:28 PST by Darin Adler
Modified: 2015-01-02 10:17 PST (History)
4 users (show)

See Also:

Attachments
Patch (157.72 KB, patch)
2015-01-01 14:58 PST, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (157.75 KB, patch)
2015-01-01 17:01 PST, Darin Adler 		kling: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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>