[webkit-reviews] review granted: [Bug 187556] Keep Selections within Shadow DOM boundaries : [Attachment 344773] 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 187556: Keep Selections within Shadow DOM boundaries
https://bugs.webkit.org/show_bug.cgi?id=187556

Attachment 344773: Patch

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




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

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1221
> +    VisiblePosition position =
frame->visiblePositionForPoint(pointInDocument);

Why do we need this? Can't we simply call deepEquivalent() on it directly?

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

Remove redundant blank lines?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1232
> +    Position selectionStart =
frame->selection().selection().visibleStart().deepEquivalent();

Seems like we should store frame->selection().selection() in a local temporary
variable.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1236
> +	   bool wouldFlip = comparePositions(result, selectionStart) <= 0;

Instead of wouldFlip. We should probably call it resultIsBeforeStart

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

Nit: Whitespace.

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

Ditto.

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

Ditto.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1247
> +	   bool wouldFlip = comparePositions(selectionEnd, result) <= 0;

Ditto.

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

Nit: Whitespace.

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

Nit: Whitespace.