[Webkit-unassigned] [Bug 51601] WML Parser should treat line/column number in a consistent way

[bugzilla-daemon at webkit.org]

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


Eric Seidel <eric at webkit.org> changed:

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




--- Comment #3 from Eric Seidel <eric at webkit.org>  2010-12-25 13:59:50 PST ---
(From update of attachment 77432)
View in context: https://bugs.webkit.org/attachment.cgi?id=77432&action=review

In general looks good.  It hink this needs one more round though, given teh nits.

And thanks peter!  Unofficial reviews are *always* welcome (and are infact "required" if you ever plan to be a reviewer)

> WebCore/dom/XMLDocumentParser.cpp:151
> +    handleError(type, m, TextPosition1(WTF::OneBasedNumber::fromOneBasedInt(lineNumber), WTF::OneBasedNumber::fromOneBasedInt(columnNumber)));

This is really the only way to construct a TextPosition1?  Seems cumbersome.

> WebCore/dom/XMLDocumentParser.h:89
> +        void handleError(ErrorType, const char* message, TextPosition1 position);

The arg name for "position" isn't needed.

> WebCore/dom/XMLDocumentParser.h:108
> +        // This method is introduced to temporary legalize existing line/column

legalize?  What law? :)

All sentences should start with a capital and end with a period. :)

> WebCore/dom/XMLDocumentParser.h:214
> +        TextPosition1 m_lastErrorPosition;

Much nicer!

> WebCore/dom/XMLDocumentParserLibxml2.cpp:560
> +    , m_lastErrorPosition(TextPosition1::belowRangePosition())

Seems we might want the default TextPosition constructor to do this, no?  Then we wouldndt' even need these lines.

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