[webkit-reviews] review denied: [Bug 51601] WML Parser should treat line/column number in a consistent way : [Attachment 77432] Proposed Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Joone Hur <joone at kldp.org>'s request
for review:
Bug 51601: WML Parser should treat line/column number in a consistent way
https://bugs.webkit.org/show_bug.cgi?id=51601

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.