Closed
Bug 455443
Opened 16 years ago
Closed 13 years ago
cache the parent for the accessibilityAttributeValue(NSAccessibilityParentAttribute)
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: surkov, Assigned: hub)
References
Details
Attachments
(1 file, 5 obsolete files)
4.71 KB,
patch
|
davidb
:
checkin+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•16 years ago
|
||
Attachment #338789 -
Flags: review?(aaronleventhal)
Reporter | ||
Updated•16 years ago
|
Attachment #338789 -
Flags: review?(hwaara)
Comment 2•16 years ago
|
||
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.
Attachment #338789 -
Flags: review?(aaronleventhal)
Reporter | ||
Comment 3•16 years ago
|
||
(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?
Reporter | ||
Comment 4•16 years ago
|
||
Hakan, iirc they use HIAccessibility, it seems it's for Carbon but mozilla is cocoa application. Can we use it?
Comment 5•16 years ago
|
||
(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 6•16 years ago
|
||
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?
Assignee | ||
Comment 7•13 years ago
|
||
Same patch, but that applies cleanly.
Attachment #338789 -
Attachment is obsolete: true
Attachment #577354 -
Flags: review?
Attachment #338789 -
Flags: review?(hwaara)
Assignee | ||
Comment 8•13 years ago
|
||
Assignee: surkov.alexander → hub
Attachment #577354 -
Attachment is obsolete: true
Attachment #579172 -
Flags: review?(surkov.alexander)
Attachment #577354 -
Flags: review?
Reporter | ||
Comment 9•13 years ago
|
||
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
Attachment #579172 -
Flags: review?(surkov.alexander) → review+
Assignee | ||
Comment 10•13 years ago
|
||
> 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.
Assignee | ||
Comment 11•13 years ago
|
||
Addressed comments.
Attachment #579172 -
Attachment is obsolete: true
Attachment #579413 -
Flags: checkin?
Comment 12•13 years ago
|
||
(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.
Reporter | ||
Comment 13•13 years ago
|
||
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
Attachment #579413 -
Flags: checkin?
Reporter | ||
Comment 14•13 years ago
|
||
(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?
Reporter | ||
Comment 15•13 years ago
|
||
(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.
Comment 16•13 years ago
|
||
(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);
Assignee | ||
Comment 17•13 years ago
|
||
> it appears we should fallback into block below if anything goes wrong. Isn't
> it?
That's what the current patch does.
Assignee | ||
Comment 18•13 years ago
|
||
changed the Obj-C class declaration to follow Alex recommendation.
Attachment #579413 -
Attachment is obsolete: true
Attachment #579793 -
Flags: review?(surkov.alexander)
Attachment #579793 -
Flags: checkin?
Reporter | ||
Updated•13 years ago
|
Attachment #579793 -
Attachment is patch: true
Reporter | ||
Comment 19•13 years ago
|
||
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);
Attachment #579793 -
Flags: review?(surkov.alexander) → review+
Assignee | ||
Comment 20•13 years ago
|
||
Attachment #580109 -
Flags: checkin?(bolterbugz)
Assignee | ||
Updated•13 years ago
|
Attachment #579793 -
Flags: checkin?
Assignee | ||
Updated•13 years ago
|
Attachment #579793 -
Attachment is obsolete: true
Comment 21•13 years ago
|
||
Comment on attachment 580109 [details] [diff] [review] reviewed patch v3.1 Inbound land: https://hg.mozilla.org/integration/mozilla-inbound/rev/fc3e43e28979
Attachment #580109 -
Flags: checkin?(bolterbugz) → checkin+
Comment 22•13 years ago
|
||
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();
Reporter | ||
Comment 23•13 years ago
|
||
Hub, please make sure you addressed all reviewer comments before landing.
Comment 24•13 years ago
|
||
Leaving open for the changes in comment 22 to be addressed. https://hg.mozilla.org/mozilla-central/rev/fc3e43e28979
Target Milestone: --- → mozilla11
Assignee | ||
Comment 25•13 years ago
|
||
Addressed in this one: https://hg.mozilla.org/integration/mozilla-inbound/rev/c2370a1dba1d
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•