380021 - implement IAccessibleValue

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[Aaron Leventhal]

(Surkov, there is a crash bug with value in progress meter, maybe you have a chance to look: bug 379878)

Comment 2

[Aaron Leventhal]

Comment on attachment 264087 [details] [diff] [review]
patch

r+ with the following change:
 nsresult rv = valueAcc->GetMaximumValue(&minimumValue);

s/GetMaximum/GetMinimum     !

Optional:
CANT_QUERY_ASSERTION_MSG
I guess the preference in C++ is to use:
static const char* kCantQueryAssertionMsg (not sure about the static).

Comment 3

[[e-mail]]

Comment on attachment 264087 [details] [diff] [review]
patch

>+#define CANT_QUERY_ASSERTION_MSG \
>+"Subclass of CAccessibleValue doesn't implement nsIAccessibleValue"\
If you do use a static const char[] then you'll need to wrap it in #ifdef DEBUG because it's only used in NS_ASSERTION.

>+  if (aValue.vt != VT_R8)
>+    return E_INVALIDARG;
Can you guarantee that the variant will be of the right type here?
(Someone might pass in a float, or integer, or something?)

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> (From update of attachment 264087 [details] [diff] [review])
> >+#define CANT_QUERY_ASSERTION_MSG \
> >+"Subclass of CAccessibleValue doesn't implement nsIAccessibleValue"\
> If you do use a static const char[] then you'll need to wrap it in #ifdef DEBUG
> because it's only used in NS_ASSERTION.

Good notion, thank you.

> >+  if (aValue.vt != VT_R8)
> >+    return E_INVALIDARG;
> Can you guarantee that the variant will be of the right type here?
> (Someone might pass in a float, or integer, or something?)
> 

I can't but now I have not a request where it's needed. Do you think it safely to support another types?

Comment 5

[Aaron Leventhal]

Currently ATK and IA2 only support double for value.

Comment 6

[Aaron Leventhal]

Let me revise that, actually I will go ask.

Comment 7

[Aaron Leventhal]

I've sent a request to the IA2 list for more info. In the meantime, I think we can check this in and file another bug to support more types if necessary.

The IDL says "The exact return type is implementation dependent.  Typical types are long and double."

Comment 8

[Aaron Leventhal]

One more thing, this patch is missing a few lines, marked here with a *
   if (IID_IAccessibleValue == iid) {
*    nsCOMPtr<nsIAccessibleValue> valueAcc(do_QueryInterface(this));
*    if (!valueAcc) {
*      return E_NOINTERFACE;
*    }

If you don't do that, Mozilla reports that IAccessibleValue is supported all the time.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> One more thing, this patch is missing a few lines, marked here with a *
>    if (IID_IAccessibleValue == iid) {
> *    nsCOMPtr<nsIAccessibleValue> valueAcc(do_QueryInterface(this));
> *    if (!valueAcc) {
> *      return E_NOINTERFACE;
> *    }
> 
> If you don't do that, Mozilla reports that IAccessibleValue is supported all
> the time.
> 

Ok, then I do not need an assertion when I try to query interface. I will move that assertion here.

Comment 10

[Aaron Leventhal]

That's true, I hadn't thought of that. Although, it won't be an assertion. It's perfectly normal for nsIAccessibleValue not to be supported.

Comment 11

[alexander :surkov (:asurkov)]

Attachment: patch2

checked in