[Webkit-unassigned] [Bug 51311] SegmentedString should provide column position

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=51311


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #76962|review?                     |review-
               Flag|                            |




--- Comment #4 from Adam Barth <abarth at webkit.org>  2010-12-21 00:47:54 PST ---
(From update of attachment 76962)
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 ?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.