[webkit-reviews] review granted: [Bug 117020] Reproducible crash with triple-finger-tap "define word" gesture on a Netflix video : [Attachment 203359] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Thomas Deniau <deniau at apple.com>'s
request for review:
Bug 117020: Reproducible crash with triple-finger-tap "define word" gesture on
a Netflix video
https://bugs.webkit.org/show_bug.cgi?id=117020

Attachment 203359: Patch
https://bugs.webkit.org/attachment.cgi?id=203359&action=review

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


Code change is fine. Code is not written in correct WebKit style, though, so
please submit a new patch with the style. Fixes.

Why no regression test? Is there no easy way to write a test for this? Normally
we require tests for any bug fix.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:540
> +    if (!lineStart.isNull()) contextStart = lineStart;

WebKit style requires breaking this into two lines:

    if (!lineStart.isNull())
	contextStart = lineStart;

I am also a bit surprised that startOfLine doesn’t have this behavior. I would
have expected it to return the passed-in position if it can’t find a start of
line rather than returning null.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:543
> +    if (!lineEnd.isNull()) contextEnd = lineEnd;

Ditto.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:553
> +    if ((extractedRange.location == NSNotFound) || (extractedRange.length <=
0)) {
> +	   return;
> +    }

WebKit style does not use the extra parentheses or braces here. Also, range
length is an unsigned value, so <= 0 is sort of silly. For WebKit coding style
this should be:

    if (extractedRange.location == NSNotFound || !extractedRange.length)
	return;

In addition, I’m pretty sure we don’t need to check both the location and the
length. When NSNotFound is returned, the length is also 0 so the length check
alone should suffice.