Comment on attachment 338789 [details] [diff] [review] patch I will rubberstamp approve if hwaara thinks it's correct. One question: Opera uses some interface that allows them to use the OS X accessibility APIs directly from C. Wouldn't that allow us to avoid creating our own Objective C proxy objects? That would be simpler in the long run -- nsAccessible already caches the parent, etc.

(In reply to comment #2) > One question: Opera uses some interface that allows them to use the OS X > accessibility APIs directly from C. Wouldn't that allow us to avoid creating > our own Objective C proxy objects? That would be simpler in the long run -- > nsAccessible already caches the parent, etc. Yes, we could use wrap classes like we do in MSAA/IA2. Hakan, you comment this?

Hakan, iirc they use HIAccessibility, it seems it's for Carbon but mozilla is cocoa application. Can we use it?

(In reply to comment #4) > Hakan, iirc they use HIAccessibility, it seems it's for Carbon but mozilla is > cocoa application. Can we use it? No, we can't. Carbon is going away for Mozilla (it's a moz2 goal) - it is not supported in 64-bit OS X (which will soon be all macs).

Comment on attachment 338789 [details] [diff] [review] patch What about my comments in bug 454202 ; what happens if the DOM tree changes? This patch assumes that once an accessible has found a parent, that it will always stay the same. Also, did you have any data on how much/what the patch improves? Basically, from what I can see, we're saved a GetUnignoredParent() call, which is just a few GetParent() on the accessible to find an "interesting" accessible object. Is that showing up in your profiles?

Same patch, but that applies cleanly.

Comment on attachment 579172 [details] [diff] [review] Patch from patch queue Review of attachment 579172 [details] [diff] [review]: ----------------------------------------------------------------- but I still think the bug 705404 is a right way to fix this one. ::: accessible/src/mac/mozAccessible.h @@ +47,5 @@ > @interface mozAccessible : NSObject <mozAccessible> > { > nsAccessibleWrap *mGeckoAccessible; // weak reference; it owns us. > NSMutableArray *mChildren; // strong ref to array of children > + mozAccessible *mParent; // weak reference to the parent mozAccessible* ::: accessible/src/mac/mozAccessible.mm @@ +357,5 @@ > if (accessibleParent) { > id nativeParent = GetNativeFromGeckoAccessible(accessibleParent); > if (nativeParent) > + mParent = GetClosestInterestingAccessible(nativeParent); > + return mParent; it appears it should be inside the if statement @@ +611,5 @@ > NS_OBJC_BEGIN_TRY_ABORT_BLOCK; > > + if (mChildren) { > + NSEnumerator *e = [mChildren objectEnumerator]; > + mozAccessible *m = nil; nit: NSEnumerator* e, mozAccessible* m

> but I still think the bug 705404 is a right way to fix this one. Let's get progressive :-) > ::: accessible/src/mac/mozAccessible.h > @@ +47,5 @@ > > @interface mozAccessible : NSObject <mozAccessible> > > { > > nsAccessibleWrap *mGeckoAccessible; // weak reference; it owns us. > > NSMutableArray *mChildren; // strong ref to array of children > > + mozAccessible *mParent; // weak reference to the parent > > mozAccessible* Expect more that one line changed then. > > ::: accessible/src/mac/mozAccessible.mm > @@ +357,5 @@ > > if (accessibleParent) { > > id nativeParent = GetNativeFromGeckoAccessible(accessibleParent); > > if (nativeParent) > > + mParent = GetClosestInterestingAccessible(nativeParent); > > + return mParent; > > it appears it should be inside the if statement The original patch had return at the same level has mParent, but without the braces. This is a good argument for requiring curly braces in every case.

Addressed comments.

(In reply to Hub Figuiere [:hub] from comment #10) > > but I still think the bug 705404 is a right way to fix this one. > > Let's get progressive :-) > > > ::: accessible/src/mac/mozAccessible.h > > @@ +47,5 @@ > > > @interface mozAccessible : NSObject <mozAccessible> > > > { > > > nsAccessibleWrap *mGeckoAccessible; // weak reference; it owns us. > > > NSMutableArray *mChildren; // strong ref to array of children > > > + mozAccessible *mParent; // weak reference to the parent > > > > mozAccessible* > > Expect more that one line changed then. > > > > > ::: accessible/src/mac/mozAccessible.mm > > @@ +357,5 @@ > > > if (accessibleParent) { > > > id nativeParent = GetNativeFromGeckoAccessible(accessibleParent); > > > if (nativeParent) > > > + mParent = GetClosestInterestingAccessible(nativeParent); > > > + return mParent; > > > > it appears it should be inside the if statement Honestly, I'd prefer return mParent = GetUnIgnoredFoo(); > > The original patch had return at the same level has mParent, but without the > braces. > This is a good argument for requiring curly braces in every case. I don't really care enough to look through the history of the patches, but I'd call this an argument for writing the code correctly the first time.

Comment on attachment 579413 [details] [diff] [review] Patch from patch queue v2 Review of attachment 579413 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/mac/mozAccessible.h @@ +47,5 @@ > @interface mozAccessible : NSObject <mozAccessible> > { > + nsAccessibleWrap* mGeckoAccessible; // weak reference; it owns us. > + NSMutableArray* mChildren; // strong ref to array of children > + mozAccessible* mParent; // weak reference to the parent then please don't have an intend between them like mozAccessible* mParent; but actually I would prefer style /** * Strong ref to array of children. */ NSMutableArray* mChildren; /** * Weak reference to the parent. */ mozAccessible* mParent; what makes us consistent to other parts of code

(In reply to Hub Figuiere [:hub] from comment #10) > > but I still think the bug 705404 is a right way to fix this one. > > Let's get progressive :-) yeah, but in nearest future I plan to get rid InvalidateChildren. So the bug 705404 should be fixed. > > > if (accessibleParent) { > > > id nativeParent = GetNativeFromGeckoAccessible(accessibleParent); > > > if (nativeParent) > > > + mParent = GetClosestInterestingAccessible(nativeParent); > > > + return mParent; > > > > it appears it should be inside the if statement > > The original patch had return at the same level has mParent, but without the > braces. it appears we should fallback into block below if anything goes wrong. Isn't it?

(In reply to Trevor Saunders (:tbsaunde) from comment #12) > (In reply to Hub Figuiere [:hub] from comment #10) > > > but I still think the bug 705404 is a right way to fix this one. > > > > Let's get progressive :-) > > > > > ::: accessible/src/mac/mozAccessible.h > > > @@ +47,5 @@ > > > > @interface mozAccessible : NSObject <mozAccessible> > > > > { > > > > nsAccessibleWrap *mGeckoAccessible; // weak reference; it owns us. > > > > NSMutableArray *mChildren; // strong ref to array of children > > > > + mozAccessible *mParent; // weak reference to the parent > > > > > > mozAccessible* > > > > Expect more that one line changed then. > > > > > > > > ::: accessible/src/mac/mozAccessible.mm > > > @@ +357,5 @@ > > > > if (accessibleParent) { > > > > id nativeParent = GetNativeFromGeckoAccessible(accessibleParent); > > > > if (nativeParent) > > > > + mParent = GetClosestInterestingAccessible(nativeParent); > > > > + return mParent; > > > > > > it appears it should be inside the if statement > > Honestly, I'd prefer return mParent = GetUnIgnoredFoo(); would you elaborate the idea. GetUnignoredFoo() returns Gecko accessible, we want to have mac object which might be different form geckoObject->mNativeObject if I get right. > > > > The original patch had return at the same level has mParent, but without the > > braces. > > This is a good argument for requiring curly braces in every case. > > I don't really care enough to look through the history of the patches, but > I'd call this an argument for writing the code correctly the first time. agree. In general we should keep empty line after single if.

(In reply to alexander :surkov from comment #15) > (In reply to Trevor Saunders (:tbsaunde) from comment #12) > > (In reply to Hub Figuiere [:hub] from comment #10) > > > > but I still think the bug 705404 is a right way to fix this one. > > > > > > Let's get progressive :-) > > > > > > > ::: accessible/src/mac/mozAccessible.h > > > > @@ +47,5 @@ > > > > > @interface mozAccessible : NSObject <mozAccessible> > > > > > { > > > > > nsAccessibleWrap *mGeckoAccessible; // weak reference; it owns us. > > > > > NSMutableArray *mChildren; // strong ref to array of children > > > > > + mozAccessible *mParent; // weak reference to the parent > > > > > > > > mozAccessible* > > > > > > Expect more that one line changed then. > > > > > > > > > > > ::: accessible/src/mac/mozAccessible.mm > > > > @@ +357,5 @@ > > > > > if (accessibleParent) { > > > > > id nativeParent = GetNativeFromGeckoAccessible(accessibleParent); > > > > > if (nativeParent) > > > > > + mParent = GetClosestInterestingAccessible(nativeParent); > > > > > + return mParent; > > > > > > > > it appears it should be inside the if statement > > > > Honestly, I'd prefer return mParent = GetUnIgnoredFoo(); > > would you elaborate the idea. GetUnignoredFoo() returns Gecko accessible, we > want to have mac object which might be different form > geckoObject->mNativeObject if I get right. I meant return mParent = GetClosestInterestingAccessible(nativeParent);

> it appears we should fallback into block below if anything goes wrong. Isn't > it? That's what the current patch does.

changed the Obj-C class declaration to follow Alex recommendation.

Comment on attachment 579793 [details] [diff] [review] proposed patch v3 Review of attachment 579793 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/mac/mozAccessible.h @@ +46,5 @@ > > @interface mozAccessible : NSObject <mozAccessible> > { > + /** > + * weak reference; it owns us. nit: please start from capital letter here and below ::: accessible/src/mac/mozAccessible.mm @@ +356,5 @@ > if (accessibleParent) { > id nativeParent = GetNativeFromGeckoAccessible(accessibleParent); > + if (nativeParent) { > + mParent = GetClosestInterestingAccessible(nativeParent); > + return mParent; I think I like Trevor's suggestion to do return mParent = GetClosestInterestingAccessible(nativeParent);

Comment on attachment 580109 [details] [diff] [review] reviewed patch v3.1 Inbound land: https://hg.mozilla.org/integration/mozilla-inbound/rev/fc3e43e28979

Comment on attachment 580109 [details] [diff] [review] reviewed patch v3.1 >+ if (nativeParent) { >+ return mParent = GetClosestInterestingAccessible(nativeParent); >+ } I don't think these braces where mentioned, but not bracing single line ifs was a review comment, and this wasn't a single line if till this patch. >- return GetClosestInterestingAccessible(nativeParent); >+ mParent = GetClosestInterestingAccessible(nativeParent); >+ return mParent; looks like you missed changing this one to return mParent = getClosestInterestingAccessible();

Hub, please make sure you addressed all reviewer comments before landing.

Leaving open for the changes in comment 22 to be addressed. https://hg.mozilla.org/mozilla-central/rev/fc3e43e28979

Addressed in this one: https://hg.mozilla.org/integration/mozilla-inbound/rev/c2370a1dba1d