767269 - ia2AccessibleText and ia2AccessibleEditbleText QI shouldn't call QI for nsIAccessibleText / EditableText

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

both of these classes can safely be static casted to HypertextAccessibleWrap, and then you can call IsTextRole(), if it has a text role then all of IAccessibleText IAccessibleEditableText and IAccessibleHypertext should be exposed if not none should be exposed.  It would be nice to avoid calling IsTextRole() 3 times when one will do, but I'm not sure what the nicest way to do that is.

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v1)

First try.

Comment 2

[Mark Capella [:capella]]

Attachment: Patch (v2)

Fixed a nit...

Comment 3

[Trevor Saunders (:tbsaunde)]

Comment on attachment 636164 [details] [diff] [review]
Patch (v2)

>+    if (static_cast<HyperTextAccessibleWrap*>(this)->IsTextRole()) {
>+      *ppv = static_cast<IAccessibleEditableText*>(this);

is the static cast really needed?

>+      (reinterpret_cast<IUnknown*>(*ppv))->AddRef(); 

just AddRef() should be fine.

>-      return E_NOINTERFACE;
>+    if (static_cast<HyperTextAccessibleWrap*>(this)->IsTextRole()) {
>+      *ppv = static_cast<IAccessibleText*>(this);
>+      (reinterpret_cast<IUnknown*>(*ppv))->AddRef();

same here.

calling IsTextRole() three times when one would do is still pretty laim, I'd be a lot happier if you reworked this stuff so we didn't need to do that.  Getting rid of the second of them should be trivial, ia2AccessibleEditableText slightly less so, but you could just move all this QI stuff to HypertextAccessibleWrap, and then it would be easy.

Comment 4

[Mark Capella [:capella]]

This might be a beginners question, but when I first made the patch, I accidentally coded it backwards:

  if (!static_cast<HyperTextAccessibleWrap*>(this)->IsTextRole()) {
..etc..

All mochitests passed ok. I spotted the logic problem while double checking it before posting, reversed the code, and re-tested with the same results.

Should I be doing something differently to test in this area?

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to Mark Capella [:capella] from comment #4)
> This might be a beginners question, but when I first made the patch, I
> accidentally coded it backwards:
> 
>   if (!static_cast<HyperTextAccessibleWrap*>(this)->IsTextRole()) {
> ..etc..
> 
> All mochitests passed ok. I spotted the logic problem while double checking
> it before posting, reversed the code, and re-tested with the same results.
> 
> Should I be doing something differently to test in this area?

no, mochitests can't really check this code, so there aren't any tests atm

Comment 6

[Mark Capella [:capella]]

Attachment: Patch (v3)

New patch addressing comment #3. 

FYI ... This is the only existing code that looks that way that I can find ... 
Everybody else does the static_cast / reinterpret_cast thing.

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsDataObj.cpp#377

I need clarification re: "calling IsTextRole() three times" ... Do you mean that someone doing a QI winds up executing IsTextRole() three times? Or do you seek to simplify that we code the call in similar / related places?

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to Mark Capella [:capella] from comment #6)
> Created attachment 636302 [details] [diff] [review]
> Patch (v3)
> 
> New patch addressing comment #3. 
> 
> FYI ... This is the only existing code that looks that way that I can find
> ... 
> Everybody else does the static_cast / reinterpret_cast thing.
> 
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsDataObj.
> cpp#377

that may be true, and in some special cases it may make sense, but it doesn't here.

> I need clarification re: "calling IsTextRole() three times" ... Do you mean
> that someone doing a QI winds up executing IsTextRole() three times? Or do
> you seek to simplify that we code the call in similar / related places?

I mean if you call QI on a HypertextAccessibleWrap it gets called three times.

Comment 8

[Mark Capella [:capella]]

Attachment: Patch (v4)

See if this does it.

Comment 9

[Trevor Saunders (:tbsaunde)]

Comment on attachment 636704 [details] [diff] [review]
Patch (v4)

this is about it, but I have some comments.

>+HyperTextAccessibleWrap::QueryInterface(REFIID aIID, void** aInstancePtr)
>+{
>+  *aInstancePtr = NULL;

technically I think you should make sure it isn't null first, although someone who calls QI with a null pointer gets what they deserve.

>+  if ((IID_IAccessibleHypertext == aIID ||
>+      IID_IAccessibleText ==aIID ||
>+      IID_IAccessibleEditableText == aIID))
>+    if (static_cast<HyperTextAccessibleWrap*>(this)->IsTextRole()) {

you don't need to cast this, since it already is that type.

>+      *aInstancePtr = this;

so, I'm a fool, but you do need a static cast here, first because C++doesn't implicitly convert pointers to and from void the way C does.  I assume msvc lets you get away with it as some sort of extension since this compiles.

  Also you want to make sure you hand out a pointer to an object whose only vtable is the one for IAccessibleFoo, which you can do by converting the type to IAccessibleFoo.

> ia2AccessibleEditableText::QueryInterface(REFIID iid, void** ppv)
> {

actually, this will never be called, so I think you can just remove it.

> STDMETHODIMP
> ia2AccessibleText::QueryInterface(REFIID iid, void** ppv)
> {

same, here, and for ia2AccessibleHypertext I think.

Comment 10

[Mark Capella [:capella]]

Attachment: Patch (v5)

I liked these suggestions, but may have gone off into left field here. Two issues:

First, in HyperTextAccessibleWrap::QueryInterface() I had to cast the below as I have it. This might be wrong, but using <IAccessibleText> the compiler threw an error C2594: 'static_cast' : ambiguous conversions ...

} else if (IID_IAccessibleText == aIID) {
      *aInstancePtr = static_cast<IAccessibleHypertext*>(this);

That got me to the linker, where it threw a bunch of error LNK2001: unresolved external symbol errors for the ::QueryInterface()'s that we removed.

Comment 11

[Mark Capella [:capella]]

Should we go in with patch (v4) and worry about the triple-call of IsTextRole() later?

Comment 12

[Trevor Saunders (:tbsaunde)]

> That got me to the linker, where it threw a bunch of error LNK2001:
> unresolved external symbol errors for the ::QueryInterface()'s that we
> removed.

it seems you  forgot to remove the declaration of those methods in the classes.

Comment 13

[Trevor Saunders (:tbsaunde)]

Comment on attachment 636740 [details] [diff] [review]
Patch (v5)

>+    if (IID_IAccessibleHypertext == aIID) {
>+      *aInstancePtr = static_cast<IAccessibleHypertext*>(this);
>+    } else if (IID_IAccessibleText == aIID) {
>+      *aInstancePtr = static_cast<IAccessibleHypertext*>(this);
>+    } else if (IID_IAccessibleEditableText == aIID) {
>+      *aInstancePtr = static_cast<IAccessibleEditableText*>(this);

nit, you don't need to brace this.

>+    }
>+    AddRef();

nit, blank line after the if stuff.

Comment 14

[Mark Capella [:capella]]

Attachment: Patch (v6)

Asking for re-review as we enhanced it from the original ...

Comment 15

[Mark Capella [:capella]]

Attachment: Patch (v7)

Comment 16

[Trevor Saunders (:tbsaunde)]

Comment on attachment 637587 [details] [diff] [review]
Patch (v7)

>+HyperTextAccessibleWrap::QueryInterface(REFIID aIID, void** aInstancePtr)
>+{
>+  if (!*aInstancePtr)
>+    return E_FAIL;

that should be !instancePtr

>+  if (IsTextRole()) {
>+    if (IID_IAccessibleHypertext == aIID || IID_IAccessibleText == aIID)
>+      *aInstancePtr = static_cast<IAccessibleHypertext*>(this);

that's not correct for the case of IAccessibleText, I'd say just keep them seperate cases.

>+    else if (IID_IAccessibleEditableText == aIID)
>+      *aInstancePtr = static_cast<IAccessibleEditableText*>(this);
>+
>+    AddRef();

that will cause an unmatched AddRef() if someone calls this method with an iid other than the three above.

>+    return S_OK;
>+  }
>+
>+  return AccessibleWrap::QueryInterface(aIID, aInstancePtr);

this will also mean interfaces like IAccessible won't be exposed  when the accessible has a text role.

Comment 17

[Mark Capella [:capella]]

Attachment: Patch (v8)

Comment 18

[Mark Capella [:capella]]

Try:
https://tbpl.mozilla.org/?tree=Try&rev=36da97bb8fe8

Comment 19

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c9db3ccb566c

Comment 20

[Ekanan Ketunuti]

http://hg.mozilla.org/mozilla-central/rev/c9db3ccb566c