[webkit-reviews] review granted: [Bug 52324] REGRESSION: A problem with Voiceover and finding links : [Attachment 79020] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 52324: REGRESSION: A problem with Voiceover and finding links
https://bugs.webkit.org/show_bug.cgi?id=52324

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

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:1022
> +	       IntRect frameRect = widget->frameRect();
> +	       IntPoint newPoint = point;
> +	       newPoint.setX(point.x() - frameRect.x());
> +	       newPoint.setY(point.y() - frameRect.y());
> +	       return
axObjectCache()->getOrCreate(widget)->accessibilityHitTest(newPoint);

This can be written without doing X and Y separately:

    IntPoint newPoint = point - widget->frameRect().location();

Or even:

    return axObjectCache()->getOrCreate(widget)->accessibilityHitTest(point -
widget->frameRect().location());

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1888
> +    // This should be the last check
> +    if (!helpText().isEmpty())
> +	   return false;
>      
> -    return !m_renderer->isListMarker() && !isWebArea();
> +    return true;

If it is important that the helpText check be last then you *could* write it
like this:

    return helpText().isEmpty();

Unless you think that’s less clear. Also, we prefer comments that are in
sentence style.

Also, the comment needs to say why the helpText check should be last. The
general rule of thumb is that comments say why and the code itself says what.