Last Comment Bug 657719 - warning: 'virtual nsresult nsAccessible::GetIndexInParent(PRInt32*)' was hidden [...] by 'virtual PRInt32 nsXULTreeItemAccessibleBase::GetIndexInParent() const' (also: GetChildAtPoint, IsSelected, IsItemSelected)
: warning: 'virtual nsresult nsAccessible::GetIndexInParent(PRInt32*)' was hidd...
Status: RESOLVED FIXED
[build_warning]
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Trevor Saunders (:tbsaunde)
:
Mentors:
http://tinderbox.mozilla.org/showlog....
Depends on:
Blocks: dexpcoma11y
  Show dependency treegraph
 
Reported: 2011-05-17 11:59 PDT by Daniel Holbert [:dholbert]
Modified: 2011-06-27 20:44 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part1 (15.91 KB, patch)
2011-06-13 22:45 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review
part 2 (22.37 KB, patch)
2011-06-13 23:18 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review-
Details | Diff | Splinter Review
part 3 (1.67 KB, patch)
2011-06-13 23:51 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review
part 4 (3.12 KB, patch)
2011-06-13 23:53 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review
part 5 (4.91 KB, patch)
2011-06-13 23:57 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review
part 1v2 (16.08 KB, patch)
2011-06-20 14:29 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
bug 657719 - nsAccessible::IsSelected() hidden (2.13 KB, patch)
2011-06-20 15:00 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review
bug 657719 - nsXformsSelectableAccessible::IsItemSelected() -> IsSelected() (3.20 KB, patch)
2011-06-20 15:00 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
part 2v2 (21.96 KB, patch)
2011-06-21 00:07 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review
part 1 v3 (21.99 KB, patch)
2011-06-21 22:58 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
part 3 followup (3.49 KB, patch)
2011-06-21 22:59 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review
part 5 v2 (11.93 KB, patch)
2011-06-21 23:01 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review
part 4 to land (4.38 KB, patch)
2011-06-22 10:25 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
IsHyperlink() (15.41 KB, patch)
2011-06-23 21:31 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review
bug 657719 - rename IsHyperlink() to IsLink() (16.84 KB, patch)
2011-06-23 22:52 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
more whitespace cleanup (16.40 KB, patch)
2011-06-24 00:55 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2011-05-17 11:59:59 PDT
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) 2011-05-17 12:08:23 PDT
yeah, this is known about :(   Fortunately this should get fixed by already planned dexpcoming.
Comment 2 alexander :surkov 2011-05-18 22:44:31 PDT
(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) 2011-06-13 22:45:46 PDT
Created attachment 539124 [details] [diff] [review]
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) 2011-06-13 23:18:44 PDT
Created attachment 539130 [details] [diff] [review]
part 2
Comment 5 Trevor Saunders (:tbsaunde) 2011-06-13 23:51:12 PDT
Created attachment 539137 [details] [diff] [review]
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) 2011-06-13 23:53:49 PDT
Created attachment 539138 [details] [diff] [review]
part 4

so far as I can see IsSelected() describes that method as well as IsItemSelected()
Comment 7 Trevor Saunders (:tbsaunde) 2011-06-13 23:57:51 PDT
Created attachment 539140 [details] [diff] [review]
part 5
Comment 8 alexander :surkov 2011-06-14 23:33:13 PDT
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 2011-06-14 23:47:25 PDT
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 2011-06-14 23:52:54 PDT
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 2011-06-15 00:10:16 PDT
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 2011-06-15 00:12:22 PDT
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 2011-06-15 00:15:04 PDT
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) 2011-06-16 16:56:49 PDT
(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) 2011-06-20 14:29:01 PDT
Created attachment 540592 [details] [diff] [review]
part 1v2

fix nits
Comment 16 Trevor Saunders (:tbsaunde) 2011-06-20 15:00:39 PDT
Created attachment 540601 [details] [diff] [review]
bug 657719 - nsAccessible::IsSelected() hidden
Comment 17 Trevor Saunders (:tbsaunde) 2011-06-20 15:00:43 PDT
Created attachment 540602 [details] [diff] [review]
bug 657719 - nsXformsSelectableAccessible::IsItemSelected() -> IsSelected()
Comment 18 Trevor Saunders (:tbsaunde) 2011-06-21 00:07:42 PDT
Created attachment 540693 [details] [diff] [review]
part 2v2

hopefully addresses comments
Comment 19 Trevor Saunders (:tbsaunde) 2011-06-21 00:15:07 PDT
> > 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) 2011-06-21 00:19:11 PDT
Comment on attachment 540601 [details] [diff] [review]
bug 657719 - nsAccessible::IsSelected() hidden

it became nsAccessible::IsLinkSelected()
Comment 21 alexander :surkov 2011-06-21 02:10:14 PDT
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 2011-06-21 02:12:32 PDT
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 2011-06-21 02:15:59 PDT
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 2011-06-21 02:21:38 PDT
(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) 2011-06-21 22:58:10 PDT
Created attachment 540973 [details] [diff] [review]
part 1 v3
Comment 26 Trevor Saunders (:tbsaunde) 2011-06-21 22:59:43 PDT
Created attachment 540974 [details] [diff] [review]
part 3 followup
Comment 27 Trevor Saunders (:tbsaunde) 2011-06-21 23:01:22 PDT
Created attachment 540975 [details] [diff] [review]
part 5 v2

change GetAnchorURI() -> AnchorURIAt()
Comment 28 alexander :surkov 2011-06-22 06:42:48 PDT
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 2011-06-22 06:49:19 PDT
Comment on attachment 540975 [details] [diff] [review]
part 5 v2

Review of attachment 540975 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 30 Trevor Saunders (:tbsaunde) 2011-06-22 10:25:51 PDT
Created attachment 541100 [details] [diff] [review]
part 4 to land
Comment 31 Trevor Saunders (:tbsaunde) 2011-06-22 23:45:20 PDT
(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) 2011-06-22 23:50:52 PDT
(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 2011-06-23 00:08:11 PDT
(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 2011-06-23 02:15:04 PDT
(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) 2011-06-23 21:31:47 PDT
Created attachment 541599 [details] [diff] [review]
IsHyperlink()
Comment 36 alexander :surkov 2011-06-23 21:34:57 PDT
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) 2011-06-23 22:52:08 PDT
Created attachment 541606 [details] [diff] [review]
bug 657719 - rename IsHyperlink() to IsLink()
Comment 38 alexander :surkov 2011-06-23 23:07:48 PDT
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) 2011-06-24 00:11:21 PDT
> > 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) 2011-06-24 00:16:49 PDT
> 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 2011-06-24 00:19:05 PDT
(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) 2011-06-24 00:37:35 PDT
(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) 2011-06-24 00:55:39 PDT
Created attachment 541623 [details] [diff] [review]
more whitespace cleanup
Comment 44 alexander :surkov 2011-06-24 02:34:18 PDT
(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) 2011-06-24 03:01:58 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.