[webkit-reviews] review granted: [Bug 134568] Selection rects sent to ServicesOverlayController are wrong : [Attachment 234317] Patch v2 - Addressing both Ryosuke's and Tim's feedback

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 134568: Selection rects sent to ServicesOverlayController are wrong
https://bugs.webkit.org/show_bug.cgi?id=134568

Attachment 234317: Patch v2 - Addressing both Ryosuke's and Tim's feedback
https://bugs.webkit.org/attachment.cgi?id=234317&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=234317&action=review


> Source/WebCore/rendering/RenderTextLineBoxes.cpp:500
> +    for (auto box = m_first; box; box = box->nextTextBox()) {

I like to use auto* in cases like this to emphasize the fact that it’s a
pointer. But auto is also OK.

> Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:139
> +    LayoutUnit leftEdge, rightEdge;
> +    bool hasLeftEdge = false, hasRightEdge = false;

Separate lines please. Also not sure we need these booleans. Could just nest
the code a bit and we wouldn’t need the locals.

> Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:147
> +	   rightEdge = gap.right().x() + gap.right().width();

Should be:

    rightEdge = gap.right().maxX();

> Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:178
> +	   rects.resize(3);

Can use shrink here instead of resize; slightly more efficient.

> Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:204
> +	   LayoutRect left, right;

Separate lines please.