Closed Bug 741703 Opened 8 years ago Closed 8 years ago

stop using QueryInterface in CAccessibleHypertext

Categories

(Core :: Disability Access APIs, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: surkov, Assigned: capella)

References

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 file, 4 obsolete files)

same as bug 736252:

1) do static_cast<nsAccessibleHypertext*>(this) instead do_QueryObject(this)
2) rename CAccessibleHypertext to ia2AccessibleHypertext
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attached patch Patch (v1) (obsolete) — Splinter Review
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.
Attachment #613237 - Flags: feedback?(trev.saunders)
Attachment #613237 - Flags: feedback?(trev.saunders) → feedback+
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.
Attachment #613237 - Flags: review+
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?
Attachment #613237 - Flags: review+
(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?
(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
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.
Attached patch Patch (v2) very rough (obsolete) — Splinter Review
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?
Attachment #613237 - Attachment is obsolete: true
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
(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">
Attached patch Patch (v3) (obsolete) — Splinter Review
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.
Attachment #613927 - Attachment is obsolete: true
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 ':'
Attached patch Patch (v4) (obsolete) — Splinter Review
Attachment #614205 - Attachment is obsolete: true
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;
Attachment #614295 - Flags: review+
Mark, you don't need new patch, I'll fix those nits before landing.
Attached patch Patch (v5)Splinter Review
Nits addressed ... build clean, test clean.
Attachment #614295 - Attachment is obsolete: true
(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.
https://hg.mozilla.org/mozilla-central/rev/2411a6a8e38c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.