Last Comment Bug 740696 - [Mac] we have children that are expired.
: [Mac] we have children that are expired.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86 Mac OS X
: P1 normal (vote)
: mozilla14
Assigned To: Hubert Figuiere [:hub]
:
Mentors:
Depends on:
Blocks: osxa11y 733513
  Show dependency treegraph
 
Reported: 2012-03-29 20:30 PDT by Hubert Figuiere [:hub]
Modified: 2012-04-06 11:37 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't cache AXParent value. This basically revert bug 455443. r= (3.41 KB, patch)
2012-04-04 14:11 PDT, Hubert Figuiere [:hub]
tbsaunde+mozbugs: review+
surkov.alexander: feedback+
Details | Diff | Review

Description Hubert Figuiere [:hub] 2012-03-29 20:30:09 PDT
We have children that are expired causing a ruckus in the hierarchy of accessible.
Comment 1 Hubert Figuiere [:hub] 2012-03-29 20:30:53 PDT
Must fix.
Comment 2 alexander :surkov 2012-03-29 20:45:38 PDT
Ah, I told you we need to create normal tree on mac in bug 705404 and iirc pointed some cases when current approach won't work ;)
Comment 3 Hubert Figuiere [:hub] 2012-04-02 17:25:19 PDT
One way to clearly see it is to start Firefox from the command line, pass -NSAccessibilityDebugLogLevel 1 as an argument and then start VoiceOver is needed. The console will print a lot of stuff.
Comment 4 Hubert Figuiere [:hub] 2012-04-04 14:11:16 PDT
Created attachment 612328 [details] [diff] [review]
Don't cache AXParent value. This basically revert bug 455443. r=
Comment 5 Hubert Figuiere [:hub] 2012-04-04 14:16:54 PDT
Comment on attachment 612328 [details] [diff] [review]
Don't cache AXParent value. This basically revert bug 455443. r=

After thinking about it, this is IMHO the best course of action. Alex, what do you think? I basically revert the caching done for bug 455443.
Comment 6 alexander :surkov 2012-04-04 21:02:12 PDT
Comment on attachment 612328 [details] [diff] [review]
Don't cache AXParent value. This basically revert bug 455443. r=

Review of attachment 612328 [details] [diff] [review]:
-----------------------------------------------------------------

you don't remove mParent member and this must hit the perf but in either case we need to rethink mac tree invalidation (that's what I permanently saying over months ;) ) so I'm fine with it as temporary solution so that you can continue to implement mac a11y support.
Comment 7 Trevor Saunders (:tbsaunde) 2012-04-04 21:52:52 PDT
(In reply to alexander :surkov from comment #6)
> Comment on attachment 612328 [details] [diff] [review]
> Don't cache AXParent value. This basically revert bug 455443. r=
> 
> Review of attachment 612328 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> you don't remove mParent member and this must hit the perf but in either
> case we need to rethink mac tree invalidation (that's what I permanently
> saying over months ;) ) so I'm fine with it as temporary solution so that
> you can continue to implement mac a11y support.

well, tree update is an important part of that.

The patch seems correct mod the mParent member and whatever my head made me ignore.  However its not clear to me it would be better to actually fix this tree update stuffs finally rather than going in circles.
Comment 8 alexander :surkov 2012-04-04 22:45:04 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> well, tree update is an important part of that.

having a real tree is important for performance, probably not for feature complete implementation

> The patch seems correct mod the mParent member and whatever my head made me
> ignore.  However its not clear to me it would be better to actually fix this
> tree update stuffs finally rather than going in circles.

that's my feeling I express over months. "best to loose a day learning ..." I'm saying but there's no unique way: http://www.youtube.com/watch?v=OZfPPLgCqTw :)
Comment 9 Trevor Saunders (:tbsaunde) 2012-04-05 04:26:41 PDT
(In reply to alexander :surkov from comment #8)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > well, tree update is an important part of that.
> 
> having a real tree is important for performance, probably not for feature
> complete implementation

yeah, but performance matters for users.

> > The patch seems correct mod the mParent member and whatever my head made me
> > ignore.  However its not clear to me it would be better to actually fix this
> > tree update stuffs finally rather than going in circles.
> 
> that's my feeling I express over months. "best to loose a day learning ..."
> I'm saying but there's no unique way:
> http://www.youtube.com/watch?v=OZfPPLgCqTw :)

sure, but its not good if all you do is fix x then fix y and then go back and undo x.
Comment 10 Trevor Saunders (:tbsaunde) 2012-04-05 04:28:05 PDT
Comment on attachment 612328 [details] [diff] [review]
Don't cache AXParent value. This basically revert bug 455443. r=

well, why not.
Comment 11 Hubert Figuiere [:hub] 2012-04-05 10:28:53 PDT
I definitely agree with trying to not go into circles and such.

And yes performance is also important.
Comment 12 Hubert Figuiere [:hub] 2012-04-05 14:48:02 PDT
Landed inbound
https://hg.mozilla.org/integration/mozilla-inbound/
Comment 13 Hubert Figuiere [:hub] 2012-04-05 14:48:26 PDT
I meant
https://hg.mozilla.org/integration/mozilla-inbound/rev/701495278c15
Comment 14 Matt Brubeck (:mbrubeck) 2012-04-06 11:37:02 PDT
https://hg.mozilla.org/mozilla-central/rev/701495278c15

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