[webkit-reviews] review denied: [Bug 65277] Make functions to find word boundaries more flexible : [Attachment 104591] Revised (alternative) fix

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Van Lam <vanlam at google.com>'s
request for review:
Bug 65277: Make functions to find word boundaries more flexible
https://bugs.webkit.org/show_bug.cgi?id=65277

Attachment 104591: Revised (alternative) fix
https://bugs.webkit.org/attachment.cgi?id=104591&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=104591&action=review


> Source/WebCore/editing/visible_units.cpp:1591
> +static VisiblePosition nextWordBoundaryBeforeWord(VisiblePosition
startingPosition, InlineBox* startingBox, int startingOffset)

This function's interface is quite confusing.  Why does this function take BOTH
startingPosition and startingBox & startingOffset.  I'd rather make an extra
call to getInlineBoxAndOffset than requiring callers to pass in the correct
arguments.  It appears that this function wants to take RenderedPosition as the
argument after all.

> Source/WebCore/editing/visible_units.cpp:1593
> +    VisiblePosition next = nextBoundary(startingPosition,
nextWordPositionBoundary);

next is a very vague name.  In this function, we're concerned about word
boundary and on which side of the word boundary we're at.  So your variable
name should reflect that.  nextPrev, nextNextPrev do not convey such
information and should therefore be be avoided.

> LayoutTests/ChangeLog:8
> +	   My change fixed this failure.

You should explain what has been fixed and why it has been fixed.