[webkit-reviews] review denied: [Bug 30739] crash with AX on when an image map contains an anchor tag : [Attachment 41820] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied chris fleizach <cfleizach at apple.com>'s
request for review:
Bug 30739: crash with AX on when an image map contains an anchor tag
https://bugs.webkit.org/show_bug.cgi?id=30739

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

------- Additional Comments from Darin Adler <darin at apple.com>
>  AccessibilityImageMapLink::AccessibilityImageMapLink()
> -    : m_areaElement(0), 
> +    : m_anchorElement(0), 
>	 m_mapElement(0)

This is not using the normal formatting for WebKit. We put the commas at the
beginnings of the subsequent lines, lined up under the colons.

> +    // Ignore links that have a alt="" or nohref tag
> +    if (m_anchorElement 
> +	   && ((m_anchorElement->hasAttribute(altAttr) &&
m_anchorElement->getAttribute(altAttr).isEmpty())
> +	   || m_anchorElement->hasAttribute(nohrefAttr)))
> +	   return true;
> +    
> +    return false; 

I suggest just using a return statement rather than an if. Also, I suggest
indenting the || an additional tab level to indicate that it's in a nested
expression.

Or you can keep this simpler by using early return a bit more.

    if (!m_anchorElement)
	return false;

    if (m_anchorElement->hasAttribute(altAttr) &&
m_anchorElement->getAttribute(altAttr).isEmpty())
	return true;

    if (m_anchorElement->hasAttribute(nohrefAttr))
	return true;

    return false;

Now that I write it that way, I can see it can be done more efficiently, too:

    const AtomicString& alt = m_anchorElement->getAttribute(altAttr);
    if (alt.isEmpty() && !alt.isNull())
	return true;

And you can write some additional comments explaining why this is the correct
logic.

I see now where the crash came from -- it was from the check of isLink then
assuming that meant HTMLAreaElement. Maybe it would have been best to have one
simple patch fixing the crash that made that check do hasTagName(areaTag), and
then another more sophisticated patch adding support for <a> inside <map>.

The test you added here simply tests for the crash. It does not test the newly
added feature of having an <a> in a <map> or the new accessibilityIsIgnored
check you added.

review- because I think you should do at least some of what I suggest above. In
particular, I think the patch that just fixes the crash would be a good idea,
and a new test that tests the new features you are adding is also called for.