[webkit-reviews] review granted: [Bug 188767] Use VisiblePosition to calculate selection ranges : [Attachment 347572] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 188767: Use VisiblePosition to calculate selection ranges
https://bugs.webkit.org/show_bug.cgi?id=188767

Attachment 347572: Patch

https://bugs.webkit.org/attachment.cgi?id=347572&action=review




--- Comment #2 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 347572
  --> https://bugs.webkit.org/attachment.cgi?id=347572
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=347572&action=review

> Source/WebKit/ChangeLog:11
> +	   when trying to snap a selection. VisiblePosition gives us the
correct information, and does not 
> +	   result is collapsed ranges.

does not result *in*?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1233
>      

This function seems to have a lot of whitespace on blank lines :(
Could you please fix them while we're at it?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1235
> +	   if (comparePositions(result.deepEquivalent(),
selectionStart.deepEquivalent()) <= 0)

This isn't right. comparePositions in Editing.h takes VisiblePositions.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1239
> +	   

Nit: Whitespace.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1243
> -	   if (comparePositions(selectionEnd, result) <= 0)
> +	   if (comparePositions(selectionEnd.deepEquivalent(),
result.deepEquivalent()) <= 0)

This isn't right. comparePositions in Editing.h takes VisiblePositions.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1247
> +	   

Nit: Whitespace.