[webkit-reviews] review granted: [Bug 27993] AXSliders are missing required attributes and actions : [Attachment 34086] 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 27993: AXSliders are missing required attributes and actions
https://bugs.webkit.org/show_bug.cgi?id=27993

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +    if (bounds.size().width() > bounds.size().height())
> +	   return AccessibilityOrientationHorizontal;
> +    if (bounds.size().height() > bounds.size().width())
> +	   return AccessibilityOrientationVertical;
> +    return AccessibilityOrientationUnknown;

I think it would be OK to break the tie here and use horizontal. I don't think
we need a separate unknown concept since this is only a guess anyway. But if
you think there's a strong-enough benefit to distinguishing unknown, then
that's fine with me.

Not having unknown would also reduce the need to have a new type for
orientation. We could share ScrollbarOrientation -- maybe we'd have to rename
it though.

> +    ControlPart styleAppearance = style->appearance();
> +    if (styleAppearance == SliderHorizontalPart || styleAppearance ==
MediaSliderPart)
> +	   return AccessibilityOrientationHorizontal;
> +    if (styleAppearance == SliderVerticalPart)
> +	   return AccessibilityOrientationVertical;
> +    
> +    return AccessibilityOrientationUnknown;

I normally suggest using switch statements for something like this since if you
don't include a default then you get a warning about any parts not listed. That
way if someone adds a new part, they have to look at this code and it makes it
more likely someone adding a new type of slider will look here.

> +    if ([attributeName isEqualToString:NSAccessibilityOrientationAttribute])
{
> +	   AccessibilityOrientation elementOrientation =
m_object->orientation();
> +	   if (elementOrientation == AccessibilityOrientationVertical)
> +	       return NSAccessibilityVerticalOrientationValue;
> +	   if (elementOrientation == AccessibilityOrientationHorizontal)
> +	       return NSAccessibilityHorizontalOrientationValue;
> +	   return nil;
> +    }

Is it really OK to return nil?

> +enum AccessibilityOrientation {
> +    AccessibilityOrientationUnknown = 0,
> +    AccessibilityOrientationVertical,
> +    AccessibilityOrientationHorizontal,
> +};

The explicit "= 0" is not needed or helpful here.

r=me