[webkit-reviews] review denied: [Bug 37275] Accessibility support for progress element : [Attachment 52872] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Yael <yael.aharon at nokia.com>'s request
for review:
Bug 37275: Accessibility support for progress element
https://bugs.webkit.org/show_bug.cgi?id=37275

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +bool AccessibilityProgressIndicator::accessibilityIsIgnored() const
> +{
> +    AccessibilityObjectInclusion decision = accessibilityIsIgnoredBase();
> +    if (decision == IncludeObject)
> +	   return false;
> +    if (decision == IgnoreObject)
> +	   return true;
> +    
> +    return false;
> +}

How about writing it like this instead?

    return accessibilityIsIgnoredBase() == IgnoreObject;

> +    // Indneterminate progress bar should return 0.

Typo: "Indneterminate"

> +HTMLProgressElement* AccessibilityProgressIndicator::element() const
> +{
> +    return static_cast<HTMLProgressElement*>(m_renderer->node());
> +}

I don't like having a risky typecast here so far away from the code that
guarantees that the type is an HTMLProgressElement. Ideally the renderer itself
would have a function that returns an HTMLProgressElement* or we would put both
an assertion and a runtime check here.

> +    virtual ~AccessibilityProgressIndicator() { }

Should just drop this. It adds nothing.

> +    virtual AccessibilityRole roleValue() const { return
ProgressIndicatorRole; }
> +
> +    virtual bool isProgressIndicator() const { return true; }
> +
> +    virtual float valueForRange() const;
> +    virtual float maxValueForRange() const;
> +    virtual float minValueForRange() const;

These member functions should all be private.

> +protected:
> +    AccessibilityProgressIndicator(RenderObject*);

This too.