707852 - [mac] remove the Objective-C wrapper

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Hubert Figuiere [:hub]]

Attachment: Patch from patch queue

Remove the objective-C wrapper as we don't need it and it brings unneeded complexity.

Comment 1

[Hubert Figuiere [:hub]]

Comment on attachment 579182 [details] [diff] [review]
Patch from patch queue

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

Changes in mozAccessible.mm were not meant for this patch.

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 579182 [details] [diff] [review]
Patch from patch queue

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

please remove mozAccessible.mm from next patch

::: accessible/src/mac/nsAccessibleWrap.h
@@ +104,5 @@
>  
> +    // Our native object.
> +    // if we are in Objective-C, we use the actual Obj-C class.
> +#if defined(__OBJC__)
> +    mozAccessible *mNativeObject;

nit: mozAccessible* mNativeObject;

@@ +106,5 @@
> +    // if we are in Objective-C, we use the actual Obj-C class.
> +#if defined(__OBJC__)
> +    mozAccessible *mNativeObject;
> +#else
> +    id mNativeObject;  

can you please give details how it works, it appears id type belongs to Objective-C

::: accessible/src/mac/nsAccessibleWrap.mm
@@ +52,5 @@
>  }
>  
>  nsAccessibleWrap::~nsAccessibleWrap()
>  {
> +  [mNativeObject release];

Shutdown is called on destruction so you don't need this

@@ +73,5 @@
>  }
>  
>  NS_IMETHODIMP
>  nsAccessibleWrap::GetNativeInterface (void **aOutInterface) 
>  {

this is XPCOM method, check arguments please

@@ +75,5 @@
>  NS_IMETHODIMP
>  nsAccessibleWrap::GetNativeInterface (void **aOutInterface) 
>  {
> +  if (mNativeObject) {
> +    *aOutInterface = (void**)mNativeObject;

nit: use explicit casting

@@ +138,5 @@
>  nsAccessibleWrap::Shutdown ()
>  {
> +  if (mNativeObject) {
> +    [mNativeObject release];
> +    mNativeObject = nil;

shouldn't you expire it?

@@ +202,1 @@
>    }

nit: don't parenthesize single if

::: accessible/src/mac/nsDocAccessibleWrap.mm
@@ +62,1 @@
>      // Create our native object using the class type specified in GetNativeType().

I'd like to see a comment why nsAccessibleWrap::Init doesn't catch here

::: accessible/src/mac/nsRootAccessibleWrap.mm
@@ +49,1 @@
>  

mozDocAccessible imports mozAccessible.h

Comment 3

[Hubert Figuiere [:hub]]

(In reply to alexander :surkov from comment #2)
> Comment on attachment 579182 [details] [diff] [review] [diff] [details] [review]
> Patch from patch queue
> 
> Review of attachment 579182 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> please remove mozAccessible.mm from next patch
> 
> ::: accessible/src/mac/nsAccessibleWrap.h
> @@ +104,5 @@
> >  
> > +    // Our native object.
> > +    // if we are in Objective-C, we use the actual Obj-C class.
> > +#if defined(__OBJC__)
> > +    mozAccessible *mNativeObject;
> 
> nit: mozAccessible* mNativeObject;
> 
> @@ +106,5 @@
> > +    // if we are in Objective-C, we use the actual Obj-C class.
> > +#if defined(__OBJC__)
> > +    mozAccessible *mNativeObject;
> > +#else
> > +    id mNativeObject;  
> 
> can you please give details how it works, it appears id type belongs to
> Objective-C

id is defined in objc/objc.h (a pure C header):
typedef struct objc_class *Class;
typedef struct objc_object {
    Class isa;
} *id;

This is C struct.

In Objective-C, id is the generic "pointer to an Obj-C object".
I could leave it at 'id' but I prefer having proper typing in Objective-C.
__OBJC__ is defined by the compiler when it compiles Objective-C or Objective-C++


> 
> ::: accessible/src/mac/nsAccessibleWrap.mm
> @@ +52,5 @@
> >  }
> >  
> >  nsAccessibleWrap::~nsAccessibleWrap()
> >  {
> > +  [mNativeObject release];
> 
> Shutdown is called on destruction so you don't need this

if mNativeObject is nil, this is a no-op. If we really want then we should assert if mNativeObject isn't nil in the destructor. I put that release as a safety measure.


> 
> @@ +73,5 @@
> >  }
> >  
> >  NS_IMETHODIMP
> >  nsAccessibleWrap::GetNativeInterface (void **aOutInterface) 
> >  {
> 
> this is XPCOM method, check arguments please
> 
> @@ +75,5 @@
> >  NS_IMETHODIMP
> >  nsAccessibleWrap::GetNativeInterface (void **aOutInterface) 
> >  {
> > +  if (mNativeObject) {
> > +    *aOutInterface = (void**)mNativeObject;
> 
> nit: use explicit casting

done. and it was the wrong type case anyway.

> 
> @@ +138,5 @@
> >  nsAccessibleWrap::Shutdown ()
> >  {
> > +  if (mNativeObject) {
> > +    [mNativeObject release];
> > +    mNativeObject = nil;
> 
> shouldn't you expire it?

Good point. Will do.


> ::: accessible/src/mac/nsDocAccessibleWrap.mm
> @@ +62,1 @@
> >      // Create our native object using the class type specified in GetNativeType().
> 
> I'd like to see a comment why nsAccessibleWrap::Init doesn't catch here

I missed it. Added

> 
> ::: accessible/src/mac/nsRootAccessibleWrap.mm
> @@ +49,1 @@
> >  
> 
> mozDocAccessible imports mozAccessible.h

Gone.

Comment 4

[Hubert Figuiere [:hub]]

Attachment: patch v2

Addressed all the comments.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Hub Figuiere [:hub] from comment #3)

> > >  nsAccessibleWrap::~nsAccessibleWrap()
> > >  {
> > > +  [mNativeObject release];
> > 
> > Shutdown is called on destruction so you don't need this
> 
> if mNativeObject is nil, this is a no-op. If we really want then we should
> assert if mNativeObject isn't nil in the destructor. I put that release as a
> safety measure.

see http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessNode.cpp#99
if you like then add assertion. The meantime you're trying to destroy mNativeObject twice.

Comment 6

[alexander :surkov (:asurkov)]

btw, is the patch ready for review?

Comment 7

[Hubert Figuiere [:hub]]

(In reply to alexander :surkov from comment #5)
> (In reply to Hub Figuiere [:hub] from comment #3)
> 
> > > >  nsAccessibleWrap::~nsAccessibleWrap()
> > > >  {
> > > > +  [mNativeObject release];
> > > 
> > > Shutdown is called on destruction so you don't need this
> > 
> > if mNativeObject is nil, this is a no-op. If we really want then we should
> > assert if mNativeObject isn't nil in the destructor. I put that release as a
> > safety measure.
> 
> see
> http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/
> nsAccessNode.cpp#99
> if you like then add assertion. The meantime you're trying to destroy
> mNativeObject twice.

No. Shutdown() set the pointer to nil so in the destructor there is no risk to over release. But I have removed the release in the destructor to follow the rules and just added a non fatal assert in case.

I'll post a newer patch ready to review.

Comment 8

[Hubert Figuiere [:hub]]

Attachment: patch v2.1

Might be the same as previous patch.

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 579814 [details] [diff] [review]
patch v2.1

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

::: accessible/src/mac/nsAccessibleWrap.mm
@@ +52,5 @@
>  }
>  
>  nsAccessibleWrap::~nsAccessibleWrap()
>  {
> +  NSCAssert(!mNativeObject, @"nsAccessibleWrap::Shutdown() was not called.");

that will trigger every time of cycle collecting (when destructor does shutdown). It sounds like all we can do is to trust the base class

@@ +77,5 @@
>  }
>  
>  NS_IMETHODIMP
>  nsAccessibleWrap::GetNativeInterface (void **aOutInterface) 
>  {

still not addressed (see comment #2)

@@ +79,5 @@
>  NS_IMETHODIMP
>  nsAccessibleWrap::GetNativeInterface (void **aOutInterface) 
>  {
> +  if (mNativeObject) {
> +    *aOutInterface = reinterpret_cast<void*>(mNativeObject);

I think I'd prefer to use static_cast for void pointers casting. Why did you choose reinterpret_cast?

Trevor, what do you think?

::: accessible/src/mac/nsDocAccessibleWrap.mm
@@ +55,5 @@
>  {
>    if (!nsDocAccessible::Init())
>      return false;
>  
> +  NS_ASSERTION(!mNativeObject, "nsDocAccessibleWrap::Init() called more than once!");

I didn't mean that. The point is nsAccessibleWrap creates mNativeObject too but since nsDocAccessible::Init() doesn't call nsAccessibleWrap::Init() then we need to create native accessible here. So if this chain is changed in the future then this comment may be not valid. I'd like to see a comment why we need to create native object here.

If you like to assert in the case of repeat Init() calles then I'd say we need to have generic mechanism for this (not mac's nsDocAccessibleWrap only).

Comment 10

[Trevor Saunders (:tbsaunde)]

> @@ +79,5 @@
> >  NS_IMETHODIMP
> >  nsAccessibleWrap::GetNativeInterface (void **aOutInterface) 
> >  {
> > +  if (mNativeObject) {
> > +    *aOutInterface = reinterpret_cast<void*>(mNativeObject);
> 
> I think I'd prefer to use static_cast for void pointers casting. Why did you
> choose reinterpret_cast?
> 
> Trevor, what do you think?

I'd say static_cast.  the c++ 98 spec seems to require in section 5.2.9#10 that static cast will do the right thing where as I see no such requirement for reinterpret_cast.

Comment 11

[Hubert Figuiere [:hub]]

(In reply to alexander :surkov from comment #2)

> @@ +73,5 @@
> >  }
> >  
> >  NS_IMETHODIMP
> >  nsAccessibleWrap::GetNativeInterface (void **aOutInterface) 
> >  {
> 
> this is XPCOM method, check arguments please

If by that you mean 
  NS_ENSURE_ARG_POINTER(aOutInterface);
Then I just added it. But nowhere else in the other platforms this is done.

Comment 12

[Hubert Figuiere [:hub]]

> ::: accessible/src/mac/nsDocAccessibleWrap.mm
> @@ +55,5 @@
> >  {
> >    if (!nsDocAccessible::Init())
> >      return false;
> >  
> > +  NS_ASSERTION(!mNativeObject, "nsDocAccessibleWrap::Init() called more than once!");
> 
> I didn't mean that. The point is nsAccessibleWrap creates mNativeObject too
> but since nsDocAccessible::Init() doesn't call nsAccessibleWrap::Init() then
> we need to create native accessible here. So if this chain is changed in the
> future then this comment may be not valid. I'd like to see a comment why we
> need to create native object here.
> 
> If you like to assert in the case of repeat Init() calles then I'd say we
> need to have generic mechanism for this (not mac's nsDocAccessibleWrap only).

I have an upcoming change for bug 705404 that will create the native object on demand. This will remove the need to do that here and it will go away.

Comment 13

[Hubert Figuiere [:hub]]

Attachment: patch v2.2

Updated patch with all the feedback.

Comment 14

[Trevor Saunders (:tbsaunde)]

(In reply to Hub Figuiere [:hub] from comment #11)
> (In reply to alexander :surkov from comment #2)
> 
> > @@ +73,5 @@
> > >  }
> > >  
> > >  NS_IMETHODIMP
> > >  nsAccessibleWrap::GetNativeInterface (void **aOutInterface) 
> > >  {
> > 
> > this is XPCOM method, check arguments please
> 
> If by that you mean 
>   NS_ENSURE_ARG_POINTER(aOutInterface);
> Then I just added it. But nowhere else in the other platforms this is done.

that's what's known as a bug ;) to be fair not very important, but still wrong.

Comment 15

[alexander :surkov (:asurkov)]

Comment on attachment 580232 [details] [diff] [review]
patch v2.2

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

::: accessible/src/mac/nsAccessibleWrap.h
@@ +102,5 @@
>     */
>    bool AncestorIsFlat();
>  
> +    // Our native object.
> +    // if we are in Objective-C, we use the actual Obj-C class.

please change the style
/**
 * NSAccessibility object. Note, if we are in Objective-C, we use the actual Obj-C class.
 */

@@ +106,5 @@
> +    // if we are in Objective-C, we use the actual Obj-C class.
> +#if defined(__OBJC__)
> +    mozAccessible* mNativeObject;
> +#else
> +    id mNativeObject;  

did we decided to name it 'mNativeObject' (refer to bug 675137)?

::: accessible/src/mac/nsAccessibleWrap.mm
@@ +85,3 @@
>      return NS_OK;
>    }
> +  *aOutInterface = nsnull;

nit: usual practice to keep it after NS_ENSURE_ARG_POINTER. But it's up to you.

@@ +209,1 @@
>    }

nit: you don't need braces

::: accessible/src/mac/nsDocAccessibleWrap.mm
@@ +55,5 @@
>  {
>    if (!nsDocAccessible::Init())
>      return false;
>  
> +  NS_ASSERTION(!mNativeObject, "nsDocAccessibleWrap::Init() called more than once!");

then just remove it (since you're going to change this code shortly after this patch). We don't need to add assertion if we have a guarantee it asserts.

Comment 16

[Hubert Figuiere [:hub]]

(In reply to alexander :surkov from comment #15)

> @@ +106,5 @@
> > +    // if we are in Objective-C, we use the actual Obj-C class.
> > +#if defined(__OBJC__)
> > +    mozAccessible* mNativeObject;
> > +#else
> > +    id mNativeObject;  
> 
> did we decided to name it 'mNativeObject' (refer to bug 675137)?

Bug 675137 does not mention how to name the member in the class. That was the name I took from day one. I'm open to suggestion. Or we can roll that into bug 675137.

Fixed the rest.

Comment 17

[Hubert Figuiere [:hub]]

Attachment: patch v2.3

Comment 18

[Ed Morley [:emorley]]

https://tbpl.mozilla.org/?tree=Try&rev=73b8ab2861e1

Will push tomorrow presuming green :-)

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cd1caaec767c

Comment 20

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks Ed!