[webkit-reviews] review granted: [Bug 127015] Support WebSelections in WK2 on iOS : [Attachment 224880] Block selection part2

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 127015: Support WebSelections in WK2 on iOS
https://bugs.webkit.org/show_bug.cgi?id=127015

Attachment 224880: Block selection part2
https://bugs.webkit.org/attachment.cgi?id=224880&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=224880&action=review


> Source/WebKit2/UIProcess/WebPageProxy.h:461
> +    void updateBlockSelectionWithTouches(const WebCore::IntPoint, uint32_t
touches, uint32_t handlePosition);

WithTouches -> WithTouch?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:716
> +static const int maxHitTests = 10;

If you make a single template (see comment bellow), this constant could simply
be in the common function itself.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:772
> +	   return IntPoint(box.x() + box.width() / 2, box.y());

x + width -> maxX?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:774
> +	   return IntPoint(box.maxX(), box.y() + box.height() / 2);

maxY

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:776
> +	   return IntPoint(box.x() + box.width() / 2, box.maxY());

ditto

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:778
> +	   return IntPoint(box.x(), box.y() + box.height() / 2);

ditto

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:791
> +    RefPtr<Range> best;

bestRange?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:873
> +PassRefPtr<Range> WebPage::contractedRangeFromHandle(Range* currentRange,
WKHandlePosition handlePosition, WKSelectionFlags& flags)

Any way to make expandedRangeFromHandle and contractedRangeFromHandle client of
a single template handling the loop?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:875
> +    // Shrinking with a base and extent will always give better results.  If
we only have a single element,

Two spaces after the period.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:876
> +    // see if we can break that down to a base and extent.  Shrinking base
and extent is comparatively straightforward.

Ditto.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:899
> +    RefPtr<Range> best;

bestRange?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:987
> +	   if (renderer && renderer->childrenInline() &&
(renderer->isRenderBlock() &&
toRenderBlock(renderer)->inlineElementContinuation() == nil) &&
!renderer->isTable()) {

Instead of == nil, it should use ! on the value.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:998
> +    RefPtr<Range> currentRange = m_currentBlockSelection ?
m_currentBlockSelection.get() :
frame.selection().selection().toNormalizedRange();

Why are m_currentBlockSelection and frame.selection().selection() different?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1010
> +    float current, expanded, contracted;

Need update to match WebKit's style.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1105
> +

Blank line.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1108
> +

Empty line.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1416
> +	       info.bounds =
hitNode->renderer()->absoluteBoundingBoxRect(true);

useTransform is the default, you could drop the argument here.