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

[bugzilla-daemon at webkit.org]

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





--- Comment #8 from Peter Rybin <peter.rybin at gmail.com>  2010-12-22 12:23:01 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(this, m_tokenizer.get(), document, reportErrors, usePreHTML5ParserQuirks(document)))
> 
> Hum...  I'm not sure we want to teach the tree builder about the document parser...

I need a parser to obtain a current position (line/column) when we run into script tag. Technically I only need something like position tracker, that would return me current position, but I didn't want to create a new interface.
I thought that since we already know about html tokenizer, html parser dependency doesn't make it much worse.

>> WebCore/html/parser/HTMLInputStream.h:140
>> +        // Generated script part inherits current position.
> 
> Comments should be complete sentences.

Done

>> WebCore/html/parser/HTMLTreeBuilder.cpp:2783
>> +    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.

m_parser->textPosition() does not compile, because textPosition has private access in HTMLDocumentParser (I do not know what for).
I cannot pass textPosition, because it's a variable. I need its current value when I get to <script> tag.

>> 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?

That's not true. I store it in constructor and I use in processScriptStartTag.

>> WebCore/platform/text/SegmentedString.cpp:248
>> +          ++m_currentLine;
> 
> Four-space indent please.

Sorry.

>> WebCore/platform/text/SegmentedString.cpp:249
>> +          m_lineBeginCharactersConsumedValue = numberOfCharactersConsumed();
> 
> I see.  This probably should be called m_numberOfCharactersConsumedPriorToCurrentLine to match m_numberOfCharactersConsumedPriorToCurrentString.

Thanks. Done.

>> WebCore/platform/text/SegmentedString.cpp:262
>> +WTF::ZeroBasedNumber SegmentedString::currentColumn() const
> 
> So, this function is moderately slow.  Might be worth mentioning in the header.

I put a comment. I wouldn't say it's really slow though. It has no cycles, only 4-5 arithmetics and calls.

>> WebCore/platform/text/SegmentedString.cpp:272
>> +}
> 
> This offset business is pretty mysterious.  Can we use a more descriptive variable name?

I renamed them and added a comment.

>> WebCore/platform/text/SegmentedString.h:167
>> +                m_lineBeginCharactersConsumedValue = numberOfCharactersConsumed();
> 
> Isn't the whole point of this codepath to avoid branching on newLineFlag?

Yes. I cannot avoid braching, but I tried to preserve the old code style ( += hocus). If it doesn't make sense now, let me put increment inside condition.

>> WebCore/platform/text/SegmentedString.h:193
>> +                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 ?

Yes. I posted numbers in previous reply.

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