Closed Bug 455443 Opened 16 years ago Closed 13 years ago

cache the parent for the accessibilityAttributeValue(NSAccessibilityParentAttribute)

Categories

(Core :: Disability Access APIs, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: surkov, Assigned: hub)

References

Details

Attachments

(1 file, 5 obsolete files)

      No description provided.
Attached patch patch (obsolete) — Splinter Review
Attachment #338789 - Flags: review?(aaronleventhal)
Attachment #338789 - Flags: review?(hwaara)
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)
(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?
Depends on: 705404
Attached file updated patch (obsolete) —
Same patch, but that applies cleanly.
Attachment #338789 - Attachment is obsolete: true
Attachment #577354 - Flags: review?
Attachment #338789 - Flags: review?(hwaara)
Attached patch Patch from patch queue (obsolete) — Splinter Review
Assignee: surkov.alexander → hub
Attachment #577354 - Attachment is obsolete: true
Attachment #579172 - Flags: review?(surkov.alexander)
Attachment #577354 - Flags: review?
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+
> 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.
Attached patch Patch from patch queue v2 (obsolete) — Splinter Review
Addressed comments.
Attachment #579172 - Attachment is obsolete: true
Attachment #579413 - Flags: checkin?
(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
Attachment #579413 - Flags: checkin?
(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.
Attached patch proposed patch v3 (obsolete) — Splinter Review
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?
No longer depends on: 705404
Attachment #579793 - Attachment is patch: true
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+
Attachment #580109 - Flags: checkin?(bolterbugz)
Attachment #579793 - Flags: checkin?
Attachment #579793 - Attachment is obsolete: true
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
Target Milestone: --- → mozilla11
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.

Attachment

General

Created:
Updated:
Size: