[webkit-reviews] review denied: [Bug 51311] SegmentedString should provide column position : [Attachment 76962] Proposed Patch.

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Peter Rybin <peter.rybin at gmail.com>'s
request for review:
Bug 51311: SegmentedString should provide column position
https://bugs.webkit.org/show_bug.cgi?id=51311

Attachment 76962: Proposed Patch.
https://bugs.webkit.org/attachment.cgi?id=76962&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=76962&action=review

> WebCore/html/parser/HTMLDocumentParser.cpp:82
> -    , m_treeBuilder(HTMLTreeBuilder::create(m_tokenizer.get(), document,
reportErrors, usePreHTML5ParserQuirks(document)))
> +    , m_treeBuilder(HTMLTreeBuilder::create(this, m_tokenizer.get(),
document, reportErrors, usePreHTML5ParserQuirks(document)))

Hum...	I'm not sure we want to teach the tree builder about the document
parser...

> WebCore/html/parser/HTMLInputStream.h:140
> +	   // Generated script part inherits current position.

Comments should be complete sentences.

> WebCore/html/parser/HTMLTreeBuilder.cpp:2783
> -    TextPosition0 position =
TextPosition0(WTF::ZeroBasedNumber::fromZeroBasedInt(m_tokenizer->lineNumber())
, WTF::ZeroBasedNumber::base());
> +    ScriptableDocumentParser* parserUpcast = m_parser;
> +    TextPosition0 position = parserUpcast->textPosition();

Why do we need to do this?  HTMLDocumentParser inherits from
ScriptableDocumentParser...  Rather than pass in the whole HTMLDocumentParser,
we should just pass in the textPosition.

> WebCore/html/parser/HTMLTreeBuilder.h:248
> +    HTMLDocumentParser* m_parser;

What's the point of storing this object in a member variable if you only ever
use it in the constructor?

> WebCore/platform/text/SegmentedString.cpp:56
> +    m_lineBeginCharactersConsumedValue =
other.m_lineBeginCharactersConsumedValue;

m_lineBeginCharactersConsumedValue is a mouthful.  What does this variable
represent?

> WebCore/platform/text/SegmentedString.cpp:248
> +	     ++lineNumber;
> +	     ++m_currentLine;

Four-space indent please.

> WebCore/platform/text/SegmentedString.cpp:249
> +	     m_lineBeginCharactersConsumedValue = numberOfCharactersConsumed();


I see.	This probably should be called
m_numberOfCharactersConsumedPriorToCurrentLine to match
m_numberOfCharactersConsumedPriorToCurrentString.

> WebCore/platform/text/SegmentedString.cpp:262
> +WTF::ZeroBasedNumber SegmentedString::currentColumn() const

So, this function is moderately slow.  Might be worth mentioning in the header.


> WebCore/platform/text/SegmentedString.cpp:272
> +void SegmentedString::setCurrentPosition(WTF::ZeroBasedNumber line,
WTF::ZeroBasedNumber column, int offset)
> +{
> +    m_currentLine = line.zeroBasedInt();
> +    m_lineBeginCharactersConsumedValue = numberOfCharactersConsumedSlow() +
offset - column.zeroBasedInt();
> +}

This offset business is pretty mysterious.  Can we use a more descriptive
variable name?

> WebCore/platform/text/SegmentedString.h:167
> +	       if (newLineFlag)
> +		   m_lineBeginCharactersConsumedValue =
numberOfCharactersConsumed();

Isn't the whole point of this codepath to avoid branching on newLineFlag?

> WebCore/platform/text/SegmentedString.h:193
> -	       lineNumber += (*m_currentString.m_current == '\n') &
m_currentString.doNotExcludeLineNumbers();
> +	       int newLineFlag = (*m_currentString.m_current == '\n') &
m_currentString.doNotExcludeLineNumbers();
> +	       lineNumber += newLineFlag;
> +	       m_currentLine += newLineFlag;
> +	       if (newLineFlag)
> +		   m_lineBeginCharactersConsumedValue =
numberOfCharactersConsumed() + 1;

Same here.  These branches are bad for performance.  Did you benchmark this
patch using the html-parser benchmark in WebCore/benchmarks ?