stop using QueryInterface in CAccessibleHypertext

RESOLVED FIXED in mozilla14

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: surkov, Assigned: capella)

Tracking

unspecified
mozilla14
x86
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
same as bug 736252:

1) do static_cast<nsAccessibleHypertext*>(this) instead do_QueryObject(this)
2) rename CAccessibleHypertext to ia2AccessibleHypertext
(Assignee)

Updated

5 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
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.
Attachment #613237 - Flags: feedback?(trev.saunders)
Attachment #613237 - Flags: feedback?(trev.saunders) → feedback+
(Reporter)

Comment 2

5 years ago
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+
(Reporter)

Comment 3

5 years ago
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?
(Reporter)

Comment 5

5 years ago
(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
(Reporter)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
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?
Attachment #613237 - Attachment is obsolete: true
(Reporter)

Comment 8

5 years ago
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
(Reporter)

Comment 9

5 years ago
(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">
(Assignee)

Comment 10

5 years ago
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.
Attachment #613927 - Attachment is obsolete: true
(Reporter)

Comment 11

5 years ago
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 ':'
(Assignee)

Comment 12

5 years ago
Created attachment 614295 [details] [diff] [review]
Patch (v4)
Attachment #614205 - Attachment is obsolete: true
(Reporter)

Comment 13

5 years ago
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+
(Reporter)

Comment 14

5 years ago
Mark, you don't need new patch, I'll fix those nits before landing.
(Assignee)

Comment 15

5 years ago
Created attachment 614824 [details] [diff] [review]
Patch (v5)

Nits addressed ... build clean, test clean.
Attachment #614295 - Attachment is obsolete: true
(Reporter)

Comment 16

5 years ago
(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.
(Reporter)

Comment 17

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2411a6a8e38c
https://hg.mozilla.org/mozilla-central/rev/2411a6a8e38c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.