814671 - Stop hiding Accessible::GroupPosition

Status: RESOLVED DUPLICATE of bug 764367

(Core :: Disability Access APIs, defect)

Description

[Jonathan Watt [:jwatt]]

I get warnings compiling AccessibleWrap.cpp and ApplicationAccessibleWrap.cpp because ApplicationAccessible::GroupPosition hides the Accessible::GroupPosition declared in NS_DECL_NSIACCESSIBLE.

nsMaiInterfaceText.cpp
cc1plus: warnings being treated as errors
In file included from ../../../../accessible/src/atk/ApplicationAccessibleWrap.h:10:0,
                 from ../../../../accessible/src/atk/AccessibleWrap.cpp:10:
../../../../accessible/src/atk/../generic/Accessible.h:115:2397: error: 'virtual nsresult mozilla::a11y::Accessible::GroupPosition(int32_t*, int32_t*, int32_t*)' was hidden
../../../../accessible/src/atk/../generic/ApplicationAccessible.h:66:20: error:   by 'virtual mozilla::a11y::GroupPos mozilla::a11y::ApplicationAccessible::GroupPosition()'
make[7]: *** [AccessibleWrap.o] Error 1

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 684678 [details] [diff] [review]
patch

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

are you really so annoyed by warnings to change API? We do the work to disjoint XPCOM implementation and internals so all these warning should go away eventually. Btw, why wouldn't you solve the problem by using keyword?

Comment 3

[Jonathan Watt [:jwatt]]

Is this API? I thought I was fixing the non-XPCOM methods so they don't conflict with the XPCOM methods.

(In reply to alexander :surkov from comment #2)
> Btw, why wouldn't you solve the problem by using keyword?

I'm not sure what you mean.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Jonathan Watt [:jwatt] from comment #3)
> Is this API? I thought I was fixing the non-XPCOM methods so they don't
> conflict with the XPCOM methods.

Sorry, that's not real API that is supposed to be used outside. It's our internal thing but we'd like to keep it nice.

> (In reply to alexander :surkov from comment #2)
> > Btw, why wouldn't you solve the problem by using keyword?
> 
> I'm not sure what you mean.

like 

class ApplicationAccessible {
public:
  using Accessible::GroupPosition();
  virtual GroupPos GroupPosition();
};

Comment 5

[Daniel Holbert [:dholbert]]

My two cents:
I believe the "using" solution fixes the warning -- but even so, it's still pretty confusing to have two different methods with the same name, declared & implemented at different levels of a multi-level inheritance hierarchy, one of which is called by the other.  IMHO it's cleaner & clearer to simply give them different names, to clarify which method is which (and which method is the caller vs. the helper).

FWIW, we use the "helper-method-with-Internal-suffix" pattern pretty frequently across-tree to handle this. This search for "Internal(" in .h files turns up 500+ results.
https://mxr.mozilla.org/mozilla-central/search?string=Internal%28&find=.h%24&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 6

[Daniel Holbert [:dholbert]]

(Er, I meant to provide this search link, w/ case-sensitivity turned on:
https://mxr.mozilla.org/mozilla-central/search?string=Internal%28&case=on&find=.h%24&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central )

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> My two cents:
> I believe the "using" solution fixes the warning -- but even so, it's still
> pretty confusing to have two different methods with the same name, declared
> & implemented at different levels of a multi-level inheritance hierarchy,
> one of which is called by the other.  IMHO it's cleaner & clearer to simply
> give them different names, to clarify which method is which (and which
> method is the caller vs. the helper).
>
> FWIW, we use the "helper-method-with-Internal-suffix" pattern pretty
> frequently across-tree to handle this. This search for "Internal(" in .h
> files turns up 500+ results.

We are in the middle of changing the class architecture. We plan to end up with:

// Our internal API
class Accessible
{
public:
  GroupPos GroupPostion()
};

// XPCOM implementation, instance created on demand
class xpcAccessible : public nsIAccessible
{
public:
  NS_IMEHTOD GroupPosition(int*, int*, int*);

private:
  Accessible* mAccessible;
};

// IA2 implementation, instance created on demand
class ia2Accessible : public IAccessible2
{
public:
  STDMETHOD groupPosition(int*, int*, int*);
private:
  Accessible* mAccessible;
};

I admit the current state of things is confusing but changing a small bit to make a temporary thing nicer is not worth to do I think.

Comment 8

[Trevor Saunders (:tbsaunde)]

I think the binaryname approach in bug 764367 is fairly reasonable, so I'll just call this a dup.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Trevor Saunders (:tbsaunde) from comment #8)
> I think the binaryname approach in bug 764367 is fairly reasonable, so I'll
> just call this a dup.

(I disagree & think we should instead go with the strategy in this bug's patch, as I laid out in bug 764367 comment 12)

Comment 10

[alexander :surkov (:asurkov)]

I would go with using Accessible::GroupPosition(); approach, it's light and easy for a temporary thing.