Closed Bug 657719 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: dholbert, Assigned: tbsaunde)

References

()

Details

(Whiteboard: [build_warning])

Attachments

(8 files, 8 obsolete files)

16.08 KB, patch
Details | Diff | Splinter Review
2.13 KB, patch
surkov
: review+
Details | Diff | Splinter Review
3.20 KB, patch
Details | Diff | Splinter Review
21.99 KB, patch
Details | Diff | Splinter Review
11.93 KB, patch
surkov
: review+
Details | Diff | Splinter Review
4.38 KB, patch
Details | Diff | Splinter Review
15.41 KB, patch
surkov
: review+
Details | Diff | Splinter Review
16.40 KB, patch
Details | Diff | Splinter Review
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.
yeah, this is known about :(   Fortunately this should get fixed by already planned dexpcoming.
Assignee: nobody → trev.saunders
(In reply to comment #1)
> yeah, this is known about :(   Fortunately this should get fixed by already
> planned dexpcoming.

correct
Blocks: dexpcoma11y
Attached patch part1 (obsolete) — Splinter Review
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()
Attachment #539124 - Flags: review?(surkov.alexander)
Attached patch part 2 (obsolete) — Splinter Review
Attachment #539130 - Flags: review?(surkov.alexander)
Attached patch part 3 (obsolete) — Splinter Review
I can't immediately see any reason for that function to be virtual and its small enough being inlineable seems reasonable.
Attachment #539137 - Flags: review?(surkov.alexander)
Attached patch part 4 (obsolete) — Splinter Review
so far as I can see IsSelected() describes that method as well as IsItemSelected()
Attachment #539138 - Flags: review?(surkov.alexander)
Attached patch part 5 (obsolete) — Splinter Review
Attachment #539140 - Flags: review?(surkov.alexander)
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
Attachment #539124 - Flags: review?(surkov.alexander) → review+
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?
Attachment #539137 - Flags: review?(surkov.alexander) → review+
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.
Attachment #539138 - Flags: review?(surkov.alexander)
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
Attachment #539130 - Flags: review?(surkov.alexander) → review-
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?
Attachment #539140 - Flags: review?(surkov.alexander) → review+
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.
Attachment #539138 - Flags: review+
(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.
Attached patch part 1v2Splinter Review
fix nits
Attachment #539124 - Attachment is obsolete: true
Attached patch part 2v2 (obsolete) — Splinter Review
hopefully addresses comments
Attachment #539130 - Attachment is obsolete: true
Attachment #539137 - Attachment is obsolete: true
Attachment #539138 - Attachment is obsolete: true
Attachment #540693 - Flags: review?(surkov.alexander)
> > while you're here, could you change it too?
> 
> GetAnchorURIAt() or AnchorURIAt()?  I think I prefer the second but whatever.

friendly poke ;)
Attachment #540602 - Flags: review?(surkov.alexander)
Comment on attachment 540601 [details] [diff] [review]
bug 657719 - nsAccessible::IsSelected() hidden

it became nsAccessible::IsLinkSelected()
Attachment #540601 - Flags: review?(surkov.alexander)
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
Attachment #540693 - Flags: review?(surkov.alexander) → review+
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 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.
Attachment #540601 - Flags: review?(surkov.alexander) → review+
(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
Attached patch part 1 v3Splinter Review
Attachment #540693 - Attachment is obsolete: true
Attached patch part 3 followup (obsolete) — Splinter Review
Attachment #540974 - Flags: review?(surkov.alexander)
Attached patch part 5 v2Splinter Review
change GetAnchorURI() -> AnchorURIAt()
Attachment #539140 - Attachment is obsolete: true
Attachment #540975 - Flags: review?(surkov.alexander)
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
Attachment #540974 - Flags: review?(surkov.alexander) → review+
Comment on attachment 540975 [details] [diff] [review]
part 5 v2

Review of attachment 540975 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #540975 - Flags: review?(surkov.alexander) → review+
Attached patch part 4 to landSplinter Review
Attachment #540974 - Attachment is obsolete: true
(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?
(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()
(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?
(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.
Attached patch IsHyperlink()Splinter Review
Attachment #541599 - Flags: review?(surkov.alexander)
Comment on attachment 541599 [details] [diff] [review]
IsHyperlink()

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

in atk please consider to use 2 spaces indentation
Attachment #541599 - Flags: review?(surkov.alexander) → review+
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
> > 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
> 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
(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.
(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.
Attachment #541606 - Attachment is obsolete: true
(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.
(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.
Target Milestone: --- → mozilla7
Attachment #540602 - Flags: review?(surkov.alexander)
You need to log in before you can comment on or make changes to this bug.