Last Comment Bug 455443 - cache the parent for the accessibilityAttributeValue(NSAccessibilityParentAttribute)
: cache the parent for the accessibilityAttributeValue(NSAccessibilityParentAtt...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla11
Assigned To: Hubert Figuiere [:hub]
:
: alexander :surkov
Mentors:
Depends on:
Blocks: 454202
  Show dependency treegraph
 
Reported: 2008-09-15 20:18 PDT by alexander :surkov
Modified: 2012-02-01 13:58 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.58 KB, patch)
2008-09-15 20:29 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
updated patch (3.60 KB, text/plain)
2011-11-28 13:37 PST, Hubert Figuiere [:hub]
no flags Details
Patch from patch queue (3.74 KB, patch)
2011-12-05 17:50 PST, Hubert Figuiere [:hub]
surkov.alexander: review+
Details | Diff | Splinter Review
Patch from patch queue v2 (4.15 KB, patch)
2011-12-06 12:33 PST, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
proposed patch v3 (4.73 KB, patch)
2011-12-07 12:38 PST, Hubert Figuiere [:hub]
surkov.alexander: review+
Details | Diff | Splinter Review
reviewed patch v3.1 (4.71 KB, patch)
2011-12-08 11:10 PST, Hubert Figuiere [:hub]
dbolter: checkin+
Details | Diff | Splinter Review

Description alexander :surkov 2008-09-15 20:18:19 PDT

    
Comment 1 alexander :surkov 2008-09-15 20:29:52 PDT
Created attachment 338789 [details] [diff] [review]
patch
Comment 2 Aaron Leventhal 2008-09-16 01:34:21 PDT
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.
Comment 3 alexander :surkov 2008-09-16 02:06:31 PDT
(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?
Comment 4 alexander :surkov 2008-09-16 02:42:58 PDT
Hakan, iirc they use HIAccessibility, it seems it's for Carbon but mozilla is cocoa application. Can we use it?
Comment 5 Håkan Waara 2008-09-16 08:00:48 PDT
(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 Håkan Waara 2008-09-16 08:11:30 PDT
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?
Comment 7 Hubert Figuiere [:hub] 2011-11-28 13:37:44 PST
Created attachment 577354 [details]
updated patch

Same patch, but that applies cleanly.
Comment 8 Hubert Figuiere [:hub] 2011-12-05 17:50:18 PST
Created attachment 579172 [details] [diff] [review]
Patch from patch queue
Comment 9 alexander :surkov 2011-12-05 23:13:57 PST
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
Comment 10 Hubert Figuiere [:hub] 2011-12-06 11:05:33 PST
> 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.
Comment 11 Hubert Figuiere [:hub] 2011-12-06 12:33:17 PST
Created attachment 579413 [details] [diff] [review]
Patch from patch queue v2

Addressed comments.
Comment 12 Trevor Saunders (:tbsaunde) 2011-12-06 18:37:28 PST
(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 13 alexander :surkov 2011-12-06 21:33:44 PST
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
Comment 14 alexander :surkov 2011-12-06 21:38:12 PST
(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?
Comment 15 alexander :surkov 2011-12-06 21:45:03 PST
(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 Trevor Saunders (:tbsaunde) 2011-12-06 22:18:45 PST
(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);
Comment 17 Hubert Figuiere [:hub] 2011-12-07 11:55:08 PST
> it appears we should fallback into block below if anything goes wrong. Isn't
> it?

That's what the current patch does.
Comment 18 Hubert Figuiere [:hub] 2011-12-07 12:38:29 PST
Created attachment 579793 [details] [diff] [review]
proposed patch v3

changed the Obj-C class declaration to follow Alex recommendation.
Comment 19 alexander :surkov 2011-12-07 21:45:12 PST
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 20 Hubert Figuiere [:hub] 2011-12-08 11:10:41 PST
Created attachment 580109 [details] [diff] [review]
reviewed patch v3.1
Comment 21 David Bolter [:davidb] 2011-12-08 13:20:59 PST
Comment on attachment 580109 [details] [diff] [review]
reviewed patch v3.1

Inbound land: https://hg.mozilla.org/integration/mozilla-inbound/rev/fc3e43e28979
Comment 22 Trevor Saunders (:tbsaunde) 2011-12-08 21:07:17 PST
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();
Comment 23 alexander :surkov 2011-12-08 21:54:23 PST
Hub, please make sure you addressed all reviewer comments before landing.
Comment 24 Ed Morley [:emorley] 2011-12-09 06:55:15 PST
Leaving open for the changes in comment 22 to be addressed.

https://hg.mozilla.org/mozilla-central/rev/fc3e43e28979
Comment 25 Hubert Figuiere [:hub] 2011-12-09 14:05:02 PST
Addressed in this one:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2370a1dba1d
Comment 26 Ed Morley [:emorley] 2011-12-10 20:39:51 PST
https://hg.mozilla.org/mozilla-central/rev/c2370a1dba1d

Note You need to log in before you can comment on or make changes to this bug.