[webkit-reviews] review denied: [Bug 31525] WAI-ARIA roles not supported on image map <area> : [Attachment 43409] 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 31525: WAI-ARIA roles not supported on image map <area>
https://bugs.webkit.org/show_bug.cgi?id=31525

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +    String ariaRole = m_areaElement->getAttribute(roleAttr);

It's usually a slightly better idiom to put fetched attributes into const
AtomicString& local variables instead of String, because it avoids a little bit
of reference count churn.

> +static const ARIARoleMap& createARIARoleMap()

I think it's cleaner for a create function to return an ARIARoleMap* rather
than a const ARIARoleMap&. I realize you're just moving this code.

> +AccessibilityRole AccessibilityObject::ariaRoleToWebCoreRole(String& value)

The argument should be const String&, not String&.

> +    ASSERT(!value.isEmpty() && !value.isNull());

Normally we don't use && in assertions. Instead we do multiple assertions in
such cases; that way we know which one failed. Also, all null strings are also
empty, so the only reason to have both assertions is if you want to know from
the assertion which is which, and if so, the null assertion has to come first.

> -    AccessibilityRole role = ariaRoleToWebCoreRole(ariaRole);
> +    AccessibilityRole role =
AccessibilityObject::ariaRoleToWebCoreRole(ariaRole);

Do you really need to change this? Since AccessibilityRenderObject is derived
from AccessibilityObject I'd expect this to already be in scope.

review- because of the String& thing -- everything else is too minor to matter,
but that is not good