657719 - warning: 'virtual nsresult nsAccessible::GetIndexInParent(PRInt32*)' was hidden [...] by 'virtual PRInt32 nsXULTreeItemAccessibleBase::GetIndexInParent() const' (also: GetChildAtPoint, IsSelected, IsItemSelected)

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Daniel Holbert [:dholbert]]

The tinderbox log linked in URL field has 45 instances of warnings for methods on nsAccessible being hidden by methods with the same name but a different signature in base classes. e.g.:

{
In file included from ../../../../accessible/src/base/nsAccDocManager.cpp:44:0:
../../../../accessible/src/base/nsAccessible.h:108:3088: warning: 'virtual nsresult nsAccessible::GetChildAtPoint(PRInt32, PRInt32, nsIAccessible**)' was hidden
../../../../accessible/src/base/nsOuterDocAccessible.h:73:25: warning:   by 'virtual nsAccessible* nsOuterDocAccessible::GetChildAtPoint(PRInt32, PRInt32, nsAccessible::EWhichChildAtPoint)'
In file included from ../../../../accessible/src/base/../xul/nsXULTreeAccessible.h:44:0,
                 from ../../../../accessible/src/base/nsRootAccessible.h:46,
                 from ../../../../accessible/src/base/../atk/nsRootAccessibleWrap.h:44,
                 from ../../../../accessible/src/base/nsAccDocManager.cpp:45:
../../../../accessible/src/base/nsAccessible.h:417:16: warning: 'virtual bool nsAccessible::IsSelected()' was hidden
../../../../accessible/src/base/../xul/nsXULListboxAccessible.h:159:1042: warning:   by 'virtual nsresult nsXULListCellAccessible::IsSelected(PRBool*)'
In file included from ../../../../accessible/src/base/nsRootAccessible.h:46:0,
                 from ../../../../accessible/src/base/../atk/nsRootAccessibleWrap.h:44,
                 from ../../../../accessible/src/base/nsAccDocManager.cpp:45:
../../../../accessible/src/base/nsAccessible.h:108:1049: warning: 'virtual nsresult nsAccessible::GetIndexInParent(PRInt32*)' was hidden
../../../../accessible/src/base/../xul/nsXULTreeAccessible.h:210:19: warning:   by 'virtual PRInt32 nsXULTreeItemAccessibleBase::GetIndexInParent() const'
}

Assuming an average of 5 lines of output per warning (starting with "In file included from..."), that's 45*5 = 225 lines of warning spam resulting from this.

Generally it looks like this points to line 108 of nsAccessible, which is...
> 108   NS_DECL_NSIACCESSIBLE
...so this really boils down to subclasses of nsIAccessible colliding on method names.

IIRC, this is easily solvable with a "using" declaration (e.g. "using nsAccessible::GetIndexInParent") to explicitly request inheriting of the methods with names that collide.

Comment 1

[Trevor Saunders (:tbsaunde)]

yeah, this is known about :(   Fortunately this should get fixed by already planned dexpcoming.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> yeah, this is known about :(   Fortunately this should get fixed by already
> planned dexpcoming.

correct

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: part1

lets keep using the strategy that the xpcom free version of a method is ::Foo and the xpcom one is ::GetFoo similar to State() and Description()

Comment 4

[Trevor Saunders (:tbsaunde)]

Attachment: part 2

Comment 5

[Trevor Saunders (:tbsaunde)]

Attachment: part 3

I can't immediately see any reason for that function to be virtual and its small enough being inlineable seems reasonable.

Comment 6

[Trevor Saunders (:tbsaunde)]

Attachment: part 4

so far as I can see IsSelected() describes that method as well as IsItemSelected()

Comment 7

[Trevor Saunders (:tbsaunde)]

Attachment: part 5

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 539124 [details] [diff] [review]
part1

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

r=me

::: accessible/src/msaa/nsAccessibleWrap.cpp
@@ +1381,5 @@
>  STDMETHODIMP
>  nsAccessibleWrap::get_indexInParent(long *aIndexInParent)
>  {
>  __try {
>    *aIndexInParent = -1;

while you're here can you add null check please?

@@ +1383,5 @@
>  {
>  __try {
>    *aIndexInParent = -1;
> +  if (IsDefunct())
> +    return S_FALSE;

do E_FAIL

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 539137 [details] [diff] [review]
part 3

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

r=me, I don't see why we'd need to keep in virtual until nobody overrides it, don't recall if we have existing bugs where we might need it

::: accessible/src/base/nsAccessible.h
@@ +425,5 @@
>  
>    /**
>     * Return true if the link currently has the focus.
>     */
> +  inline bool IsSelected()

should be const, right?

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 539138 [details] [diff] [review]
part 4

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

IsSelected is a method of nsAccessible class (when it's hyperlink), this method name isn't related with hyperlink interface. So I don't think it's good idea to keep the same name for different methods for objects that are in inheritance.

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 539130 [details] [diff] [review]
part 2

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

::: accessible/src/base/nsApplicationAccessible.h
@@ +62,5 @@
>  class nsApplicationAccessible: public nsAccessibleWrap,
>                                 public nsIAccessibleApplication
>  {
>  public:
> +  using nsAccessible::ChildAtPoint;

I don't think we need this with your patch, it was introduced in bug 583076 to fix similar problem you work on.

::: accessible/src/base/nsBaseWidgetAccessible.h
@@ +54,5 @@
>    */
>  class nsLeafAccessible : public nsAccessibleWrap
>  {
>  public:
> +  using nsAccessible::ChildAtPoint;

too

::: accessible/src/html/nsHTMLImageMapAccessible.cpp
@@ +247,1 @@
>                                        EWhichChildAtPoint aWhichChild)

please fix indentation

::: accessible/src/html/nsHTMLImageMapAccessible.h
@@ +81,5 @@
>   */
>  class nsHTMLAreaAccessible : public nsHTMLLinkAccessible
>  {
>  public:
> +  using nsAccessible::ChildAtPoint;

no need

::: accessible/src/msaa/nsAccessibleWrap.cpp
@@ +900,5 @@
>  __try {
>    VariantInit(pvarChild);
>  
> +  if (IsDefunct())
> +    return S_FALSE;

E_FAIL

@@ +909,2 @@
>    xLeft = xLeft;
>    yTop = yTop;

fix it while you're, I don't XXX comments required

@@ +912,5 @@
> +    ChildAtPoint(xLeft, yTop, eDirectChild);
> +  // if we got something
> +  if (accessible) {
> +    pvarChild->vt = VT_I4;
> +      pvarChild->lVal = static_cast<IAccessible>(accessible);

this shouldn't work, query instead

@@ +913,5 @@
> +  // if we got something
> +  if (accessible) {
> +    pvarChild->vt = VT_I4;
> +      pvarChild->lVal = static_cast<IAccessible>(accessible);
> +      NS_ADDREF(accessible);

fix indentation please

@@ -917,5 @@
> -  if (xpAccessible) {
> -    // if the child is us
> -    if (xpAccessible == static_cast<nsIAccessible*>(this)) {
> -      pvarChild->vt = VT_I4;
> -      pvarChild->lVal = CHILDID_SELF;

this part shouldn't be removed

::: accessible/src/xul/nsXULTreeAccessible.h
@@ +65,5 @@
>  {
>  public:
>    using nsAccessible::GetChildCount;
>    using nsAccessible::GetChildAt;
> +  using nsAccessible::ChildAtPoint;

no need

::: accessible/src/xul/nsXULTreeGridAccessible.h
@@ +76,5 @@
>  {
>  public:
>    using nsAccessible::GetChildCount;
>    using nsAccessible::GetChildAt;
> +  using nsAccessible::ChildAtPoint;

too

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 539140 [details] [diff] [review]
part 5

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

::: accessible/src/base/nsAccessible.cpp
@@ +2939,5 @@
>    return 1;
>  }
>  
>  nsAccessible*
> +nsAccessible::Anchor(PRUint32 aAnchorIndex)

I think it should be named as AnchorAt

@@ +2946,5 @@
>    return aAnchorIndex == 0 ? this : nsnull;
>  }
>  
>  already_AddRefed<nsIURI>
>  nsAccessible::GetAnchorURI(PRUint32 aAnchorIndex)

while you're here, could you change it too?

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 539138 [details] [diff] [review]
part 4

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

r=me if you get rid ambiguity, for example, prefix all all hyperlink methods by 'Link' work.

Comment 14

[Trevor Saunders (:tbsaunde)]

(In reply to comment #12)
> Comment on attachment 539140 [details] [diff] [review] [review]
> part 5
> 
> Review of attachment 539140 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/nsAccessible.cpp
> @@ +2939,5 @@
> >    return 1;
> >  }
> >  
> >  nsAccessible*
> > +nsAccessible::Anchor(PRUint32 aAnchorIndex)
> 
> I think it should be named as AnchorAt

I like that idea :)

> >  already_AddRefed<nsIURI>
> >  nsAccessible::GetAnchorURI(PRUint32 aAnchorIndex)
> 
> while you're here, could you change it too?

GetAnchorURIAt() or AnchorURIAt()?  I think I prefer the second but whatever.

Comment 15

[Trevor Saunders (:tbsaunde)]

Attachment: part 1v2

fix nits

Comment 16

[Trevor Saunders (:tbsaunde)]

Attachment: bug 657719 - nsAccessible::IsSelected() hidden

Comment 17

[Trevor Saunders (:tbsaunde)]

Attachment: bug 657719 - nsXformsSelectableAccessible::IsItemSelected() -> IsSelected()

Comment 18

[Trevor Saunders (:tbsaunde)]

Attachment: part 2v2

hopefully addresses comments

Comment 19

[Trevor Saunders (:tbsaunde)]

> > while you're here, could you change it too?
> 
> GetAnchorURIAt() or AnchorURIAt()?  I think I prefer the second but whatever.

friendly poke ;)

Comment 20

[Trevor Saunders (:tbsaunde)]

Comment on attachment 540601 [details] [diff] [review]
bug 657719 - nsAccessible::IsSelected() hidden

it became nsAccessible::IsLinkSelected()

Comment 21

[alexander :surkov (:asurkov)]

Comment on attachment 540693 [details] [diff] [review]
part 2v2

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

r=me

::: accessible/src/msaa/nsAccessibleWrap.cpp
@@ +904,3 @@
>  
> +  nsAccessible* accessible = nsAccUtils::MustPrune(this) ? this :
> +    ChildAtPoint(xLeft, yTop, eDirectChild);

you don't need MustPrune since it's part of ChildAtPoint

@@ +913,5 @@
>        pvarChild->lVal = CHILDID_SELF;
>      } else { // its not create an Accessible for it.
>        pvarChild->vt = VT_DISPATCH;
> +      pvarChild->pdispVal = NativeAccessible(accessible);
> +      if (accessible->IsDefunct()) {

no need to keep defunct check, just skip it

Comment 22

[alexander :surkov (:asurkov)]

Comment on attachment 540602 [details] [diff] [review]
bug 657719 - nsXformsSelectableAccessible::IsItemSelected() -> IsSelected()

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

it appears identical to part4 patch, what's the reason of review request?

Comment 23

[alexander :surkov (:asurkov)]

Comment on attachment 540601 [details] [diff] [review]
bug 657719 - nsAccessible::IsSelected() hidden

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

r=me, are you going to deal with other methods in another patch? btw, IsLinkSelected isn't in connection to IsHyperLink (Link vs HyperLink). I'm fine to keep shorter names until there's confusion.

Comment 24

[alexander :surkov (:asurkov)]

(In reply to comment #19)
> > > while you're here, could you change it too?
> > 
> > GetAnchorURIAt() or AnchorURIAt()?  I think I prefer the second but whatever.
> 
> friendly poke ;)

AnchorURIAt() of course

Comment 25

[Trevor Saunders (:tbsaunde)]

Attachment: part 1 v3

Comment 26

[Trevor Saunders (:tbsaunde)]

Attachment: part 3 followup

Comment 27

[Trevor Saunders (:tbsaunde)]

Attachment: part 5 v2

change GetAnchorURI() -> AnchorURIAt()

Comment 28

[alexander :surkov (:asurkov)]

Comment on attachment 540974 [details] [diff] [review]
part 3 followup

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

r=me

::: accessible/src/base/nsAccessible.h
@@ +420,5 @@
>  
>    /**
>     * Return true if the link is valid (e. g. points to a valid URL).
>     */
> +  virtual bool IsLinkValid();

let's keep it inline and not virtual please

Comment 29

[alexander :surkov (:asurkov)]

Comment on attachment 540975 [details] [diff] [review]
part 5 v2

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

Comment 30

[Trevor Saunders (:tbsaunde)]

Attachment: part 4 to land

Comment 31

[Trevor Saunders (:tbsaunde)]

(In reply to comment #28)
> Comment on attachment 540974 [details] [diff] [review] [review]
> part 3 followup
> 
> Review of attachment 540974 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> r=me
> 
> ::: accessible/src/base/nsAccessible.h
> @@ +420,5 @@
> >  
> >    /**
> >     * Return true if the link is valid (e. g. points to a valid URL).
> >     */
> > +  virtual bool IsLinkValid();
> 
> let's keep it inline and not virtual please

INLINING THAT MEANS THAT NSaCCESSIBLE.H NEEDS TO INCLUDE sTATES.H WHICH MEANS EXPORTING THAT HEADER ARE YOU OK WITH THAT?

Comment 32

[Trevor Saunders (:tbsaunde)]

(In reply to comment #9)
> Comment on attachment 539137 [details] [diff] [review] [review]
> part 3
> 
> Review of attachment 539137 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> r=me, I don't see why we'd need to keep in virtual until nobody overrides
> it, don't recall if we have existing bugs where we might need it
> 
> ::: accessible/src/base/nsAccessible.h
> @@ +425,5 @@
> >  
> >    /**
> >     * Return true if the link currently has the focus.
> >     */
> > +  inline bool IsSelected()
> 
> should be const, right?

except that making that causes problems with the assertion, I got
../../../dist/include/nsAccessible.h: In member function 'bool nsAccessible::IsLinkSelected() const':
../../../dist/include/nsAccessible.h:441:5: error: passing 'const nsAccessible' as 'this' argument of 'virtual bool nsAccessible::IsHyperLink()' discards qualif
iers

on a linux debug try build.
I'm not sure why I didn't have a problem on a opt build locally or why it doesn't complain about State()

Comment 33

[alexander :surkov (:asurkov)]

(In reply to comment #32)

> > > +  inline bool IsSelected()
> > 
> > should be const, right?
> 
> except that making that causes problems with the assertion

ok, fair enough, btw, should IsHyperLink be renamed to IsLink?

Comment 34

[alexander :surkov (:asurkov)]

(In reply to comment #31)

> > > +  virtual bool IsLinkValid();
> > 
> > let's keep it inline and not virtual please
> 
> INLINING THAT MEANS THAT NSaCCESSIBLE.H NEEDS TO INCLUDE sTATES.H WHICH
> MEANS EXPORTING THAT HEADER ARE YOU OK WITH THAT?

I think this is ok but it should be wrapped by a11y namespace, maybe with specific export options. Please file bug for that.

Comment 35

[Trevor Saunders (:tbsaunde)]

Attachment: IsHyperlink()

Comment 36

[alexander :surkov (:asurkov)]

Comment on attachment 541599 [details] [diff] [review]
IsHyperlink()

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

in atk please consider to use 2 spaces indentation

Comment 37

[Trevor Saunders (:tbsaunde)]

Attachment: bug 657719 - rename IsHyperlink() to IsLink()

Comment 38

[alexander :surkov (:asurkov)]

Comment on attachment 541606 [details] [diff] [review]
bug 657719 - rename IsHyperlink() to IsLink()

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

::: accessible/src/atk/nsMaiHyperlink.cpp
@@ +137,5 @@
>  
>  AtkHyperlink *
>  MaiHyperlink::GetAtkHyperlink(void)
>  {
> +  NS_ENSURE_TRUE(mHyperlink, nsnull);

new line please

@@ +139,5 @@
>  MaiHyperlink::GetAtkHyperlink(void)
>  {
> +  NS_ENSURE_TRUE(mHyperlink, nsnull);
> +  if (mMaiAtkHyperlink)
> +    return mMaiAtkHyperlink;

new line please

::: accessible/src/atk/nsMaiInterfaceHyperlinkImpl.cpp
@@ +50,5 @@
>  
>  AtkHyperlink*
>  getHyperlinkCB(AtkHyperlinkImpl *aImpl)
>  {
> +  nsAccessibleWrap *accWrap = GetAccessibleWrap(ATK_OBJECT(aImpl));

no space between * and class name like nsAccessibleWrap* accWrap

@@ +57,2 @@
>  
> +  NS_ENSURE_TRUE(accWrap->IsLink(), nsnull);

I'd preserve new line here

@@ +57,3 @@
>  
> +  NS_ENSURE_TRUE(accWrap->IsLink(), nsnull);
> +  MaiHyperlink *maiHyperlink = accWrap->GetMaiHyperlink();

same

::: accessible/src/base/Makefile.in
@@ +86,5 @@
>    nsAccessibilityService.h \
>    nsAccessible.h \
>    nsAccessNode.h \
>    nsARIAMap.h \
> +	States.h \

spaces

but I bet this is from another patch, right?
do you think it's ok to make 'states' namespace visible in files outside a11y (I assume this header is included automatically in exported nsAccessible.h)

::: accessible/src/base/nsAccessible.h
@@ +406,5 @@
>  
>    /**
>     * Return true if the accessible is hyper link accessible.
>     */
> +  virtual bool IsLink();

make it a const please

Comment 39

[Trevor Saunders (:tbsaunde)]

> > INLINING THAT MEANS THAT NSaCCESSIBLE.H NEEDS TO INCLUDE sTATES.H WHICH
> > MEANS EXPORTING THAT HEADER ARE YOU OK WITH THAT?
> 
> I think this is ok but it should be wrapped by a11y namespace, maybe with
> specific export options. Please file bug for that.

done bug 666863

Comment 40

[Trevor Saunders (:tbsaunde)]

> but I bet this is from another patch, right?
> do you think it's ok to make 'states' namespace visible in files outside
> a11y (I assume this header is included automatically in exported
> nsAccessible.h)

yes, its a different patch.  I think it isn't great but probably livable for now.  See comment 34.

> 
> ::: accessible/src/base/nsAccessible.h
> @@ +406,5 @@
> >  
> >    /**
> >     * Return true if the accessible is hyper link accessible.
> >     */
> > +  virtual bool IsLink();
> 
> make it a const please

I can't until nsAccUtils::IsEmbededObject() is const afaict

Comment 41

[alexander :surkov (:asurkov)]

(In reply to comment #40)
> > > +  virtual bool IsLink();
> > 
> > make it a const please
> 
> I can't until nsAccUtils::IsEmbededObject() is const afaict

why? const means this instance can't be changed, it doesn't mean that static method of some object can't be called.

Comment 42

[Trevor Saunders (:tbsaunde)]

(In reply to comment #41)
> (In reply to comment #40)
> > > > +  virtual bool IsLink();
> > > 
> > > make it a const please
> > 
> > I can't until nsAccUtils::IsEmbededObject() is const afaict
> 
> why? const means this instance can't be changed, it doesn't mean that static
> method of some object can't be called.

sure, but we pass in this, which gcc atleast sees as a const nsAccessible* const  where  IsEmbededdObject() wants a nsIAccessible* so gcc complains there is no such conversion.

Comment 43

[Trevor Saunders (:tbsaunde)]

Attachment: more whitespace cleanup

Comment 44

[alexander :surkov (:asurkov)]

(In reply to comment #42)

> > why? const means this instance can't be changed, it doesn't mean that static
> > method of some object can't be called.
> 
> sure, but we pass in this, which gcc atleast sees as a const nsAccessible*
> const  where  IsEmbededdObject() wants a nsIAccessible* so gcc complains
> there is no such conversion.

OK, I see, then up to you to change it or not.

Comment 45

[Trevor Saunders (:tbsaunde)]

(In reply to comment #44)
> (In reply to comment #42)
> 
> > > why? const means this instance can't be changed, it doesn't mean that static
> > > method of some object can't be called.
> > 
> > sure, but we pass in this, which gcc atleast sees as a const nsAccessible*
> > const  where  IsEmbededdObject() wants a nsIAccessible* so gcc complains
> > there is no such conversion.
> 
> OK, I see, then up to you to change it or not.

well, changing it would seem to require changing nsAccUtils::IsEmbededObject() which intern depends on changing nsAccessible::Role().  So while makeing this and Role() and state(0 etc const seems nice since they don't actually mutate the accessible I think we probably 
 want to get all these warnings fixed and leave that as a longer term thing.

Comment 46

[Trevor Saunders (:tbsaunde)]

landed
http://hg.mozilla.org/mozilla-central/rev/a874d24d19b6
http://hg.mozilla.org/mozilla-central/rev/0f5f486a0a23
http://hg.mozilla.org/mozilla-central/rev/740916b2c1ea
http://hg.mozilla.org/mozilla-central/rev/66814c1e99bb
http://hg.mozilla.org/mozilla-central/rev/db49c5e4335e
http://hg.mozilla.org/mozilla-central/rev/0bfaaf7972d7
http://hg.mozilla.org/mozilla-central/rev/fc7d76664c79