Closed Bug 814671 Opened 7 years ago Closed 7 years ago

Stop hiding Accessible::GroupPosition

Categories

(Core :: Disability Access APIs, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 764367

People

(Reporter: jwatt, Assigned: jwatt)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
Version: 17 Branch → Trunk
Attached patch patchSplinter Review
Attachment #684678 - Flags: review?(trev.saunders)
Blocks: 814674
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?
Attachment #684678 - Flags: review?(trev.saunders)
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.
(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();
};
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
(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.
I think the binaryname approach in bug 764367 is fairly reasonable, so I'll just call this a dup.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 764367
(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)
I would go with using Accessible::GroupPosition(); approach, it's light and easy for a temporary thing.
You need to log in before you can comment on or make changes to this bug.