[webkit-reviews] review granted: [Bug 90321] Convert HTML parser to handle 8-bit resources without converting to UChar* : [Attachment 154114] Updated patch with performance tunes for Parsing Tests

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 90321: Convert HTML parser to handle 8-bit resources without converting to
UChar*
https://bugs.webkit.org/show_bug.cgi?id=90321

Attachment 154114: Updated patch with performance tunes for Parsing Tests
https://bugs.webkit.org/attachment.cgi?id=154114&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=154114&action=review


Performance results look good.

Please fix the style and organization issues below before landing.

> Source/WTF/wtf/text/WTFString.h:193
> +    unsigned dataSize() const

Everything is data. How about calling this "sizeInBytes()"?

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:76
> +    if (string.is8Bit()) {
> +	   const LChar* characters = string.characters8();
> +
> +	   for (; numLeadingSpaces < length; ++numLeadingSpaces) {
> +	       if (isNotHTMLSpace(characters[numLeadingSpaces]))
> +		   break;
> +	   }
> +
> +	   if (numLeadingSpaces == length)
> +	       return string.isNull() ? string : emptyAtom.string();
>  
> +	   for (; numTrailingSpaces < length; ++numTrailingSpaces) {
> +	       if (isNotHTMLSpace(characters[length - numTrailingSpaces - 1]))
> +		   break;
> +	   }
> +    } else {
> +	   const UChar* characters = string.characters();
> +
> +	   for (; numLeadingSpaces < length; ++numLeadingSpaces) {
> +	       if (isNotHTMLSpace(characters[numLeadingSpaces]))
> +		   break;
> +	   }
> +
> +	   if (numLeadingSpaces == length)
> +	       return string.isNull() ? string : emptyAtom.string();
> +
> +	   for (; numTrailingSpaces < length; ++numTrailingSpaces) {
> +	       if (isNotHTMLSpace(characters[length - numTrailingSpaces - 1]))
> +		   break;
> +	   }
> +    }

Here we have two code paths that differ only in their character type. This
comes up a lot in 8/16 work. In this case, and cases like it, I'd rather see a
function template that abstracted out the character type, to avoid duplicating
all this code. A template will make our code more compact, and it will mean
that we only need to fix bugs in one place instead of two.

In this case, I would templatize stripLeadingAndTrailingHTMLSpaces.

> Source/WebCore/platform/text/SegmentedString.cpp:40
> +    m_advanceFunc = other.m_advanceFunc;
> +    m_advanceAndUpdateLineNumberFunc =
other.m_advanceAndUpdateLineNumberFunc;

These two assignments should use initializer syntax.

> Source/WebCore/platform/text/SegmentedString.cpp:247
> +void SegmentedString::advanceAndUpdateLineNumber8IncludeLineNumbers()

Other 8 vs 16 functions put the "8" at the end. This one should too.

Can you just remove the "IncludeLineNumbers" from the end here? I don't think
it adds anything above "UpdateLineNumber".

> Source/WebCore/platform/text/SegmentedString.cpp:259
> +void SegmentedString::advanceAndUpdateLineNumber16IncludeLineNumbers()

Ditto.

> Source/WebCore/platform/text/SegmentedString.h:151
> +	   m_advanceFunc = &SegmentedString::advanceEmpty;
> +	   m_advanceAndUpdateLineNumberFunc = &SegmentedString::advanceEmpty;

Initializer syntax, please.

> Source/WebCore/platform/text/SegmentedString.h:211
> +	   (this->*m_advanceFunc)();

No need for "this->".

> Source/WebCore/platform/text/SegmentedString.h:216
> +	   (this->*m_advanceAndUpdateLineNumberFunc)();

No need for "this->".

> Source/WebCore/platform/text/SegmentedString.h:293
> +    void setSlowCase()

WebKit style uses the "set" prefix to mean "I am passing you the value to set".
This function should be named "updateSlowCaseFunctionPointers()".

> Source/WebCore/platform/text/SegmentedString.h:306
> +    void setAdvanceFunctionPointers()

WebKit style uses the "set" prefix to mean "I am passing you the value to set".
This function should be named "updateAdvanceFunctionPointers()".