Closed Bug 380021 Opened 14 years ago Closed 14 years ago

implement IAccessibleValue

Categories

(Core :: Disability Access APIs, defect)

All
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
No description provided.
Attachment #264087 - Flags: superreview?(neil)
Attachment #264087 - Flags: review?(aaronleventhal)
Attachment #264087 - Attachment is patch: true
Attachment #264087 - Attachment mime type: application/octet-stream → text/plain
(Surkov, there is a crash bug with value in progress meter, maybe you have a chance to look: bug 379878)
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).
Attachment #264087 - Flags: review?(aaronleventhal) → review+
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?)
Attachment #264087 - Flags: superreview?(neil) → superreview+
(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?
Currently ATK and IA2 only support double for value.
Let me revise that, actually I will go ask.
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."
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.
(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.
Status: NEW → ASSIGNED
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.
Attached patch patch2Splinter Review
checked in
Attachment #264087 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.