[webkit-reviews] review granted: [Bug 125614] Set m_pos as private in InlineIterator, and use getter and setter functions : [Attachment 220007] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Gyuyoung Kim
<gyuyoung.kim at samsung.com>'s request for review:
Bug 125614: Set m_pos as private in InlineIterator, and use getter and setter
functions
https://bugs.webkit.org/show_bug.cgi?id=125614

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=220007&action=review


This is a good iterative improvement.

But manipulating offsets using +/- 1 looks suspicious. Won't this break on
Unicode characters that don't fit in 16 bits?

> Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:924
> +		   unsigned currentPosition = m_startOfIgnoredSpaces.offset();
> +		   m_startOfIgnoredSpaces.setOffset(--currentPosition);

I don't think that the name currentPosition is helpful or even correct.

Can we write it like this?

    m_startOfIgnoredSpaces.setOffset(m_startOfIgnoredSpaces.offset() - 1);

But then again, this is an iterator, it should know how to iterate. Maybe the
right answer is to add a fastDecrementInTextNode() function (there is already
fastIncrementInTextNode()), and use that?

> Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:1085
> +		   unsigned currentPosition = endpoint.offset();
> +		   endpoint.setOffset(--currentPosition);

Same comment about bad name and iteration that should be encapsulated.

> Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:1135
> +	   unsigned currentPosition = m_lineBreak.offset();
> +	   m_lineBreak.setOffset(--currentPosition);

Same comment about bad name and iteration that should be encapsulated.

> Source/WebCore/rendering/line/TrailingObjects.cpp:46
> +	       unsigned currentPosition =
lineMidpointState.midpoints[trailingSpaceMidpoint].offset();
> +	      
lineMidpointState.midpoints[trailingSpaceMidpoint].setOffset(--currentPosition)
;

Same comment about bad name and iteration that should be encapsulated.