Closed
Bug 657719
Opened 13 years ago
Closed 13 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)
Core
Disability Access APIs
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.
Assignee | ||
Comment 1•13 years ago
|
||
yeah, this is known about :( Fortunately this should get fixed by already planned dexpcoming.
Assignee: nobody → trev.saunders
Comment 2•13 years ago
|
||
(In reply to comment #1) > yeah, this is known about :( Fortunately this should get fixed by already > planned dexpcoming. correct
Blocks: dexpcoma11y
Assignee | ||
Comment 3•13 years ago
|
||
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)
Assignee | ||
Comment 4•13 years ago
|
||
Attachment #539130 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 5•13 years ago
|
||
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)
Assignee | ||
Comment 6•13 years ago
|
||
so far as I can see IsSelected() describes that method as well as IsItemSelected()
Attachment #539138 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 7•13 years ago
|
||
Attachment #539140 -
Flags: review?(surkov.alexander)
Comment 8•13 years ago
|
||
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 9•13 years ago
|
||
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 10•13 years ago
|
||
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 11•13 years ago
|
||
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 12•13 years ago
|
||
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 13•13 years ago
|
||
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+
Assignee | ||
Comment 14•13 years ago
|
||
(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.
Assignee | ||
Comment 16•13 years ago
|
||
Assignee | ||
Comment 17•13 years ago
|
||
Assignee | ||
Comment 18•13 years ago
|
||
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)
Assignee | ||
Comment 19•13 years ago
|
||
> > while you're here, could you change it too?
>
> GetAnchorURIAt() or AnchorURIAt()? I think I prefer the second but whatever.
friendly poke ;)
Assignee | ||
Updated•13 years ago
|
Attachment #540602 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 20•13 years ago
|
||
Comment on attachment 540601 [details] [diff] [review] bug 657719 - nsAccessible::IsSelected() hidden it became nsAccessible::IsLinkSelected()
Attachment #540601 -
Flags: review?(surkov.alexander)
Comment 21•13 years ago
|
||
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 22•13 years ago
|
||
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•13 years ago
|
||
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+
Comment 24•13 years ago
|
||
(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
Assignee | ||
Comment 25•13 years ago
|
||
Attachment #540693 -
Attachment is obsolete: true
Assignee | ||
Comment 26•13 years ago
|
||
Attachment #540974 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 27•13 years ago
|
||
change GetAnchorURI() -> AnchorURIAt()
Attachment #539140 -
Attachment is obsolete: true
Attachment #540975 -
Flags: review?(surkov.alexander)
Comment 28•13 years ago
|
||
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 29•13 years ago
|
||
Comment on attachment 540975 [details] [diff] [review] part 5 v2 Review of attachment 540975 [details] [diff] [review]: -----------------------------------------------------------------
Attachment #540975 -
Flags: review?(surkov.alexander) → review+
Assignee | ||
Comment 30•13 years ago
|
||
Attachment #540974 -
Attachment is obsolete: true
Assignee | ||
Comment 31•13 years ago
|
||
(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?
Assignee | ||
Comment 32•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
(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.
Assignee | ||
Comment 35•13 years ago
|
||
Attachment #541599 -
Flags: review?(surkov.alexander)
Comment 36•13 years ago
|
||
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+
Assignee | ||
Comment 37•13 years ago
|
||
Comment 38•13 years ago
|
||
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
Assignee | ||
Comment 39•13 years ago
|
||
> > 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
Assignee | ||
Comment 40•13 years ago
|
||
> 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•13 years ago
|
||
(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.
Assignee | ||
Comment 42•13 years ago
|
||
(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.
Assignee | ||
Comment 43•13 years ago
|
||
Attachment #541606 -
Attachment is obsolete: true
Comment 44•13 years ago
|
||
(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.
Assignee | ||
Comment 45•13 years ago
|
||
(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.
Assignee | ||
Comment 46•13 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•13 years ago
|
Target Milestone: --- → mozilla7
Updated•13 years ago
|
Attachment #540602 -
Flags: review?(surkov.alexander)
You need to log in
before you can comment on or make changes to this bug.
Description
•