[Webkit-unassigned] [Bug 117020] Reproducible crash with triple-finger-tap "define word" gesture on a Netflix video

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=117020


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #203359|review?                     |review+, commit-queue-
               Flag|                            |




--- Comment #5 from Darin Adler <darin at apple.com>  2013-05-30 09:37:27 PST ---
(From update of attachment 203359)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.