Last Comment Bug 741703 - stop using QueryInterface in CAccessibleHypertext
: stop using QueryInterface in CAccessibleHypertext
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 Windows 7
: -- normal (vote)
: mozilla14
Assigned To: Mark Capella [:capella]
:
Mentors:
Depends on:
Blocks: dexpcoma11y
  Show dependency treegraph
 
Reported: 2012-04-02 23:25 PDT by alexander :surkov
Modified: 2012-04-14 06:38 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (7.71 KB, patch)
2012-04-08 23:36 PDT, Mark Capella [:capella]
tbsaunde+mozbugs: feedback+
Details | Diff | Splinter Review
Patch (v2) very rough (8.54 KB, patch)
2012-04-11 02:46 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v3) (9.83 KB, patch)
2012-04-11 15:50 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v4) (11.00 KB, patch)
2012-04-12 01:02 PDT, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Splinter Review
Patch (v5) (11.10 KB, patch)
2012-04-13 09:28 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review

Description alexander :surkov 2012-04-02 23:25:24 PDT
same as bug 736252:

1) do static_cast<nsAccessibleHypertext*>(this) instead do_QueryObject(this)
2) rename CAccessibleHypertext to ia2AccessibleHypertext
Comment 1 Mark Capella [:capella] 2012-04-08 23:36:01 PDT
Created attachment 613237 [details] [diff] [review]
Patch (v1)

First draft patch ... note that there is one do_QueryObject left in place, for a winAccessNode ... I think this is planned to be covered in bug648267 so I left this portion asis.
Comment 2 alexander :surkov 2012-04-09 04:17:32 PDT
Comment on attachment 613237 [details] [diff] [review]
Patch (v1)

Review of attachment 613237 [details] [diff] [review]:
-----------------------------------------------------------------

you don't need CAccessibleHypertext::QueryInterface. I'll fix it and merge with patch from bug 539683 and land it.
Comment 3 alexander :surkov 2012-04-09 04:21:34 PDT
Comment on attachment 613237 [details] [diff] [review]
Patch (v1)

Review of attachment 613237 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/msaa/CAccessibleHypertext.cpp
@@ +51,4 @@
>  {
>    *ppv = NULL;
>    if (IID_IAccessibleHypertext == iid) {
> +    nsHyperTextAccessibleWrap* hyperAcc = static_cast<nsHyperTextAccessibleWrap*>(this);

it's a problem because nsIAccessibleHyperText can be not implemented by nsHyperTextAccessibleWrap. Trevor, ideas?
Comment 4 Trevor Saunders (:tbsaunde) 2012-04-09 04:33:43 PDT
(In reply to alexander :surkov from comment #3)
> Comment on attachment 613237 [details] [diff] [review]
> Patch (v1)
> 
> Review of attachment 613237 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/msaa/CAccessibleHypertext.cpp
> @@ +51,4 @@
> >  {
> >    *ppv = NULL;
> >    if (IID_IAccessibleHypertext == iid) {
> > +    nsHyperTextAccessibleWrap* hyperAcc = static_cast<nsHyperTextAccessibleWrap*>(this);
> 
> it's a problem because nsIAccessibleHyperText can be not implemented by
> nsHyperTextAccessibleWrap. Trevor, ideas?

i'M KIND OF DISTRACTED SO NOT SURE i FOLLOW YOUR CONCERN, BUT IS THIS THE thing were nsHyperTextAccessible doesn't always QI to nsIAccessibleHyperText depending on role?
Comment 5 alexander :surkov 2012-04-09 04:42:36 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #4)

> i'M KIND OF DISTRACTED SO NOT SURE i FOLLOW YOUR CONCERN, BUT IS THIS THE
> thing were nsHyperTextAccessible doesn't always QI to nsIAccessibleHyperText
> depending on role?

yes, http://mxr.mozilla.org/mozilla-central/source/accessible/src/html/nsHyperTextAccessible.cpp#102
Comment 6 alexander :surkov 2012-04-09 05:42:32 PDT
after talk on irc we can do:

nsARIAMap.h
namespace aria {
  bool IsTextRole(nsRoleMapEntry* aRoleMap) {
    http://mxr.mozilla.org/mozilla-central/source/accessible/src/html/nsHyperTextAccessible.cpp#102
  }
}

and then use aria::IsTextRole(mRoleMapEntry) for XPCOM and IA2 QueryInterface implementation.
Comment 7 Mark Capella [:capella] 2012-04-11 02:46:21 PDT
Created attachment 613927 [details] [diff] [review]
Patch (v2) very rough

I don't follow why the original patch had a problem, but trying to switch to your new roadmap.

I come up with the attached, where I've created the new function in nsAriaMap.h as I think you were looking for. Wrapping it in just aria was producing compile errors, so I wrapped it in mozilla / a11y / aria, and now get odd link errors.  So >yuck< either way.

Do we still plan to remove the function ia2AccessibleHypertext::QueryInterface as mentioned in comment#2? Or will we keep it but modify it to use the call to aria::IsTextRole(mRoleMapEntry)? I ask because I had a version of this I was playing with but couldn't compile/build due to failure to recognize the mRoleMapEntry var that I was trying to pass.

Finally, for my informatino, is XPCOM and IA2 QueryInterface the same thing? ie: Stuff in the msaa folder, versus mac and atk interfaces?
Comment 8 alexander :surkov 2012-04-11 04:05:36 PDT
Comment on attachment 613927 [details] [diff] [review]
Patch (v2) very rough

Review of attachment 613927 [details] [diff] [review]:
-----------------------------------------------------------------

please fix nsHyperTextAccessible::QueryInterface to use new function

::: accessible/src/base/nsARIAMap.h
@@ +266,5 @@
> + * Determine if accessible is valid as a text role.
> + */
> +bool IsTextRole(nsRoleMapEntry* aRoleMap)
> +{
> +  if (aRoleMap &&

do namespace allows inlined functions?

::: accessible/src/msaa/CAccessibleHypertext.cpp
@@ +51,5 @@
>  {
>    *ppv = NULL;
>    if (IID_IAccessibleHypertext == iid) {
> +    nsHyperTextAccessibleWrap* hyperAcc = static_cast<nsHyperTextAccessibleWrap*>(this);
> +    if (hyperAcc->IsDefunct())

interface set shouldn't depend on object state so IsDefunct check

you should do aria::IsTextRole(acc->ARIARoleMap());

pls add ARIARoleMap method to nsAccessible

@@ +65,5 @@
>  
>  // IAccessibleHypertext
>  
>  STDMETHODIMP
> +ia2AccessibleHypertext::get_nHyperlinks(long *aHyperlinkCount)

could you pls fix type* name, here and below
Comment 9 alexander :surkov 2012-04-11 04:06:20 PDT
(In reply to Mark Capella [:capella] from comment #7)
> I don't follow why the original patch had a problem, but trying to switch to
> your new roadmap.

we shouldn't expose text interfaces on things like <div role="slider">
Comment 10 Mark Capella [:capella] 2012-04-11 15:50:12 PDT
Created attachment 614205 [details] [diff] [review]
Patch (v3)

Well, Ii got this far ... it builds and tests once again. I did get stuck trying to get it built into nsariamap.h and the aria namespace.
Comment 11 alexander :surkov 2012-04-11 19:57:01 PDT
Comment on attachment 614205 [details] [diff] [review]
Patch (v3)

Review of attachment 614205 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/nsAccessible.h
@@ +201,5 @@
> +   */
> +  inline nsRoleMapEntry* ARIARoleMap() const
> +  {
> +    return mRoleMapEntry;
> +  }

nsRoleMapEntry* ARIARoleMap() const
  { return mRoleMapEntry; }

::: accessible/src/msaa/CAccessibleHypertext.cpp
@@ +53,5 @@
>  {
>    *ppv = NULL;
>    if (IID_IAccessibleHypertext == iid) {
> +    nsHyperTextAccessibleWrap* hyperAcc = static_cast<nsHyperTextAccessibleWrap*>(this);
> +    if (hyperAcc->IsDefunct() || !IsTextRole(hyperAcc->ARIARoleMap()))

again you don't need IsDefunct check, cast it to nsAccessibleWrap

@@ +134,5 @@
> + * Determine if accessible is valid as a text role.
> + */
> +bool 
> +ia2AccessibleHypertext::IsTextRole(nsRoleMapEntry* aRoleMap)
> +{

if you put it as part of ia2AccessibleHypertext then you don't need to expose ARIARoleMap. You can just to cast to nsAccessibleWrap and use mRoleMapEntry

Also you should reuse this method for nsHypertextAccessible::QueryInterface (we should avoid to dupe the logic).

If due to some reason you don't want to put it into nsARIAMap then you can keep it on nsHyperTextAccessible so that you can use it in ia2AccessibleHypertext and in nsHyperTextAccessible::QueryInterface

::: accessible/src/msaa/CAccessibleHypertext.h
@@ +46,5 @@
>  
>  #include "CAccessibleText.h"
>  #include "AccessibleHypertext.h"
>  
> +class ia2AccessibleHypertext: public CAccessibleText,

nit: space before ':'
Comment 12 Mark Capella [:capella] 2012-04-12 01:02:17 PDT
Created attachment 614295 [details] [diff] [review]
Patch (v4)
Comment 13 alexander :surkov 2012-04-13 02:55:05 PDT
Comment on attachment 614295 [details] [diff] [review]
Patch (v4)

Review of attachment 614295 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/html/nsHyperTextAccessible.cpp
@@ +91,5 @@
>      NS_ADDREF_THIS();
>      return NS_OK;
>    }
>  
> +  // ARIA roles that these interfaces are not appropriate for

nit: missed dot

@@ +2392,5 @@
>    return NS_OK;
>  }
> +
> +/**
> + * Determine if accessible is valid as a text role.

you don't need a comment here (keep in header)

::: accessible/src/html/nsHyperTextAccessible.h
@@ +201,5 @@
>                                        nsIDOMNode **aEndNode,
>                                        PRInt32 *aEndOffset);
>  
> +  /**
> +   * Determine if accessible is valid as a text role.

perhaps: Return true if the used ARIA role (if any) allows the hypertext accessible to expose text interfaces.

::: accessible/src/msaa/CAccessibleHypertext.cpp
@@ +56,1 @@
>        return E_NOINTERFACE;

I think I'd prefer

if (hypertext->IsTextRole()) {
  *ppv = static_cast<>
  return S_OK;
}

return E_NOINTERFACE;
Comment 14 alexander :surkov 2012-04-13 09:06:39 PDT
Mark, you don't need new patch, I'll fix those nits before landing.
Comment 15 Mark Capella [:capella] 2012-04-13 09:28:16 PDT
Created attachment 614824 [details] [diff] [review]
Patch (v5)

Nits addressed ... build clean, test clean.
Comment 16 alexander :surkov 2012-04-13 10:13:33 PDT
(In reply to Mark Capella [:capella] from comment #15)
> Created attachment 614824 [details] [diff] [review]
> Patch (v5)
> 
> Nits addressed ... build clean, test clean.

ok, thanks, I pushed try server build. I'll land it tomorrow.
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2012-04-14 06:38:37 PDT
https://hg.mozilla.org/mozilla-central/rev/2411a6a8e38c

Note You need to log in before you can comment on or make changes to this bug.