[mac] remove the Objective-C wrapper

RESOLVED FIXED in mozilla11

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: hub, Assigned: hub)

Tracking

Trunk
mozilla11
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 579182 [details] [diff] [review]
Patch from patch queue

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

Updated

6 years ago
Attachment #579182 - Attachment is patch: true
(Assignee)

Comment 1

6 years ago
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

6 years ago
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
(Assignee)

Comment 3

6 years ago
(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.
(Assignee)

Comment 4

6 years ago
Created attachment 579543 [details] [diff] [review]
patch v2

Addressed all the comments.
Assignee: nobody → hub
Attachment #579182 - Attachment is obsolete: true

Comment 5

6 years ago
(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

6 years ago
btw, is the patch ready for review?
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Comment 8

6 years ago
Created attachment 579814 [details] [diff] [review]
patch v2.1

Might be the same as previous patch.
Attachment #579543 - Attachment is obsolete: true
Attachment #579814 - Flags: review?(surkov.alexander)

Comment 9

6 years ago
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).
> @@ +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.
(Assignee)

Comment 11

6 years ago
(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.
(Assignee)

Comment 12

6 years ago
> ::: 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.
(Assignee)

Comment 13

6 years ago
Created attachment 580232 [details] [diff] [review]
patch v2.2

Updated patch with all the feedback.
Attachment #579814 - Attachment is obsolete: true
Attachment #579814 - Flags: review?(surkov.alexander)
(Assignee)

Updated

6 years ago
Attachment #580232 - Flags: review?(surkov.alexander)
(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

6 years ago
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.
Attachment #580232 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 16

6 years ago
(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.
(Assignee)

Comment 17

6 years ago
Created attachment 580494 [details] [diff] [review]
patch v2.3
Attachment #580232 - Attachment is obsolete: true
Attachment #580494 - Flags: checkin?(bolterbugz)
https://tbpl.mozilla.org/?tree=Try&rev=73b8ab2861e1

Will push tomorrow presuming green :-)
https://hg.mozilla.org/mozilla-central/rev/cd1caaec767c
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla11

Updated

6 years ago
Attachment #580494 - Flags: checkin?(bolterbugz) → checkin+
Thanks Ed!
You need to log in before you can comment on or make changes to this bug.