Keep a map of all views that have sent notifications. This ensures that Acce...

Keep a cache of all views that have sent notifications.  This ensures that AccessibleObjectFromEvent works properly.
BUG=9601
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=50302

[dmazzoni, 2010-06-16 22:51:08]

Good fix.

http://codereview.chromium.org/2823009/diff/1/2
File views/accessibility/view_accessibility.cc (right):

http://codereview.chromium.org/2823009/diff/1/2#newcode298
views/accessibility/view_accessibility.cc:298: // Child is located elsewhere in
this view's subtree.
Document the fact that positive values are 1-based indexes so you can iterate
over all children, and negative values are direct references to objects.

http://codereview.chromium.org/2823009/diff/1/4
File views/view.h (right):

http://codereview.chromium.org/2823009/diff/1/4#newcode542
views/view.h:542: virtual void NotifyAccessibilityEvent(long event_type);
Add a comment for this method.

http://codereview.chromium.org/2823009/diff/1/5
File views/view_win.cc (right):

http://codereview.chromium.org/2823009/diff/1/5#newcode32
views/view_win.cc:32: void View::NotifyAccessibilityEvent(long event_type) {
Document this method, mention specifically get_accChild.

http://codereview.chromium.org/2823009/diff/1/7
File views/widget/widget_win.h (right):

http://codereview.chromium.org/2823009/diff/1/7#newcode561
views/widget/widget_win.h:561: std::map<long, View*> accessibility_view_map_;
Document.

[David Tseng, 2010-06-17 00:03:24]

http://codereview.chromium.org/2823009/diff/1/2
File views/accessibility/view_accessibility.cc (right):

http://codereview.chromium.org/2823009/diff/1/2#newcode298
views/accessibility/view_accessibility.cc:298: // Child is located
elsewhere in this view's subtree.
Document the fact that positive values are 1-based indexes so you can
iterate over all children, and negative values are direct references to
objects.
Added comment.

http://codereview.chromium.org/2823009/diff/1/4
File views/view.h (right):

http://codereview.chromium.org/2823009/diff/1/4#newcode542
views/view.h:542: virtual void NotifyAccessibilityEvent(long
event_type);
Add a comment for this method.
Done.

http://codereview.chromium.org/2823009/diff/1/5
File views/view_win.cc (right):

http://codereview.chromium.org/2823009/diff/1/5#newcode32
views/view_win.cc:32: void View::NotifyAccessibilityEvent(long
event_type) {
Document this method, mention specifically get_accChild.
Done.
http://codereview.chromium.org/2823009/diff/1/7
File views/widget/widget_win.h (right):

http://codereview.chromium.org/2823009/diff/1/7#newcode561
views/widget/widget_win.h:561: std::map<long, View*>
accessibility_view_map_;
Document.
Done.

On 6/16/10, dmazzoni@chromium.org <dmazzoni@chromium.org> wrote:
> Good fix.
>
>
>
> http://codereview.chromium.org/2823009/diff/1/2
> File views/accessibility/view_accessibility.cc (right):
>
> http://codereview.chromium.org/2823009/diff/1/2#newcode298
> views/accessibility/view_accessibility.cc:298: // Child is located
> elsewhere in this view's subtree.
> Document the fact that positive values are 1-based indexes so you can
> iterate over all children, and negative values are direct references to
> objects.
>
> http://codereview.chromium.org/2823009/diff/1/4
> File views/view.h (right):
>
> http://codereview.chromium.org/2823009/diff/1/4#newcode542
> views/view.h:542: virtual void NotifyAccessibilityEvent(long
> event_type);
> Add a comment for this method.
>
> http://codereview.chromium.org/2823009/diff/1/5
> File views/view_win.cc (right):
>
> http://codereview.chromium.org/2823009/diff/1/5#newcode32
> views/view_win.cc:32: void View::NotifyAccessibilityEvent(long
> event_type) {
> Document this method, mention specifically get_accChild.
>
> http://codereview.chromium.org/2823009/diff/1/7
> File views/widget/widget_win.h (right):
>
> http://codereview.chromium.org/2823009/diff/1/7#newcode561
> views/widget/widget_win.h:561: std::map<long, View*>
> accessibility_view_map_;
> Document.
>
> http://codereview.chromium.org/2823009/show
>

[dmazzoni, 2010-06-17 14:47:18]

http://codereview.chromium.org/2823009/diff/9001/10003
File views/view.h (right):

http://codereview.chromium.org/2823009/diff/9001/10003#newcode544
views/view.h:544: virtual void NotifyAccessibilityEvent(long event_type);
This needs to compile on platforms other than Windows, so don't forget to
implement an empty version of this in views/view_gtk.cc.

http://codereview.chromium.org/2823009/diff/9001/10004
File views/view_win.cc (right):

http://codereview.chromium.org/2823009/diff/9001/10004#newcode40
views/view_win.cc:40: long key = reinterpret_cast<long>(this);
Rather than casting to a long, how about assigning a sequential ID to each view
as needed. Here are three possible approaches:

1. As each View is created, have it take a new accessibility ID and increment a
static next_accessibility_id_ variable. This is probably the simplest, then you
don't even need a map.

2. Or, create a fixed-size vector of 20 views, each with a fixed ID based on its
index in the vector. When notifying, search all 20 spots, and if it's not
already there, put the view in the least recently used spot. This was the idea
recommended on Mozilla's site, I think.

3. Or, use the map like you have it, but assign the IDs sequentially. To see if
a view is already in the map, you'll have to either create a reverse map, or
else iterate over the whole map - probably either is fine, since there will
never be more than a couple dozen views in one widget. If you do this, consider
putting more of this logic in widget_win.cc rather than here.

[Chris Guillory, 2010-06-17 18:52:38]

http://codereview.chromium.org/2823009/diff/9001/10001
File views/accessibility/view_accessibility.cc (right):

http://codereview.chromium.org/2823009/diff/9001/10001#newcode296
views/accessibility/view_accessibility.cc:296: return S_OK;
According to MSAA documentation we should be returning S_FALSE here.

Servers expose elements as either elements (child IDs) or full objects (
IAccessible interface pointers). If a child is an element, get_accChild returns
S_FALSE, and the parent will provide information for that child...

[David Tseng, 2010-06-17 19:06:33]

> http://codereview.chromium.org/2823009/diff/9001/10001
> File views/accessibility/view_accessibility.cc (right):
>
> http://codereview.chromium.org/2823009/diff/9001/10001#newcode296
> views/accessibility/view_accessibility.cc:296: return S_OK;
> According to MSAA documentation we should be returning S_FALSE here.
>
> Servers expose elements as either elements (child IDs) or full objects (
> IAccessible interface pointers). If a child is an element, get_accChild
> returns S_FALSE, and the parent will provide information for that
> child...
Changed.  Great catch thanks!

[David Tseng, 2010-06-18 06:02:33]

>
> http://codereview.chromium.org/2823009/diff/9001/10003
> File views/view.h (right):
>
> http://codereview.chromium.org/2823009/diff/9001/10003#newcode544
> views/view.h:544: virtual void NotifyAccessibilityEvent(long
> event_type);
> This needs to compile on platforms other than Windows, so don't forget
> to implement an empty version of this in views/view_gtk.cc.
Done; totally missed the excluded files folder under views.


> http://codereview.chromium.org/2823009/diff/9001/10004
> File views/view_win.cc (right):
>
> http://codereview.chromium.org/2823009/diff/9001/10004#newcode40
> views/view_win.cc:40: long key = reinterpret_cast<long>(this);
> Rather than casting to a long, how about assigning a sequential ID to
> each view as needed. Here are three possible approaches:
>
> 1. As each View is created, have it take a new accessibility ID and
> increment a static next_accessibility_id_ variable. This is probably the
> simplest, then you don't even need a map.
>
> 2. Or, create a fixed-size vector of 20 views, each with a fixed ID
> based on its index in the vector. When notifying, search all 20 spots,
> and if it's not already there, put the view in the least recently used
> spot. This was the idea recommended on Mozilla's site, I think.
>
> 3. Or, use the map like you have it, but assign the IDs sequentially. To
> see if a view is already in the map, you'll have to either create a
> reverse map, or else iterate over the whole map - probably either is
> fine, since there will never be more than a couple dozen views in one
> widget. If you do this, consider putting more of this logic in
> widget_win.cc rather than here.
Went with something like 3; I have a circular queue (a vector with an
index mod a max = 20).

> http://codereview.chromium.org/2823009/show
>

[dmazzoni, 2010-06-18 13:50:11]

Great, I think this is the final implementation!

One last important issue, besides a few details below: it's possible that the
View could disappear between when it's added to the vector and when AT tries to
access it - for example if a dialog box closes.

Luckily this is easy to catch: there's a method ViewHierarchyChanged that gets
called whenever a View is added, deleted, or reparented. In the implementation
of that method in widget_win.cc, go through your array and replace any view
being deleted with NULL.

http://codereview.chromium.org/2823009/diff/36002/37002
File views/accessibility/view_accessibility.cc (right):

http://codereview.chromium.org/2823009/diff/36002/37002#newcode310
views/accessibility/view_accessibility.cc:310: DCHECK(child_view);
There's a small chance this could fail legitimately, right? A DCHECK implies
that there's absolutely no chance.

http://codereview.chromium.org/2823009/diff/36002/37004
File views/view.h (right):

http://codereview.chromium.org/2823009/diff/36002/37004#newcode549
views/view.h:549: virtual void NotifyAccessibilityEvent(UINT32 event_type);
Since this is cross-platform code, this should probably be a new enum, rather
than a Windows-specific UINT32. You can define the enum in
accessibility_types.h.

http://codereview.chromium.org/2823009/diff/36002/37005
File views/view_gtk.cc (right):

http://codereview.chromium.org/2823009/diff/36002/37005#newcode24
views/view_gtk.cc:24: NOTIMPLEMENTED();
NOTIMPLEMENTED will log an error message, but this method will get triggered
normally whether there's a screenreader or not, so I think this should just be a
comment so as not to annoy Linux developers.

http://codereview.chromium.org/2823009/diff/36002/37007
File views/widget/widget_win.cc (right):

http://codereview.chromium.org/2823009/diff/36002/37007#newcode126
views/widget/widget_win.cc:126: (accessibility_view_events_counter_ +
1)%max_accessibility_view_events_;
Need spaces before and after the modulo operator.

You could use "index" instead of "counter" so that things still fit on one line
here.

http://codereview.chromium.org/2823009/diff/36002/37007#newcode129
views/widget/widget_win.cc:129: // Convert to MSAA child id.
Make sure at least one piece of code documents why negative child IDs are used
and how they correspond to the vector indices.

http://codereview.chromium.org/2823009/diff/36002/37008
File views/widget/widget_win.h (right):

http://codereview.chromium.org/2823009/diff/36002/37008#newcode96
views/widget/widget_win.h:96: // Obtain the view event with the given MSAA child
id.  Used in
Great. Also note that the ids are expected to be negative to distinguish them
from child indexes, and that this could return NULL because only the last 20
views are kept.

http://codereview.chromium.org/2823009/diff/36002/37008#newcode568
views/widget/widget_win.h:568: static const int max_accessibility_view_events_ =
20;
The naming style for constants is different. Call this
kMaxAccessibilityViewEvents.

[Chris Guillory, 2010-06-18 17:50:43]

LGTM with Dominic's comments.

[David Tseng, 2010-06-18 20:13:36]

>
> One last important issue, besides a few details below: it's possible that
> the
> View could disappear between when it's added to the vector and when AT
> tries to
> access it - for example if a dialog box closes.
>
> Luckily this is easy to catch: there's a method ViewHierarchyChanged that
> gets
> called whenever a View is added, deleted, or reparented. In the
> implementation
> of that method in widget_win.cc, go through your array and replace any view
> being deleted with NULL.
Done.


>
>
> http://codereview.chromium.org/2823009/diff/36002/37002
> File views/accessibility/view_accessibility.cc (right):
>
> http://codereview.chromium.org/2823009/diff/36002/37002#newcode310
> views/accessibility/view_accessibility.cc:310: DCHECK(child_view);
> There's a small chance this could fail legitimately, right? A DCHECK
> implies that there's absolutely no chance.
Removed (definitely more likely with the clearing of the view in
ViewHierarchyChanged).
> http://codereview.chromium.org/2823009/diff/36002/37004
> File views/view.h (right):
>
> http://codereview.chromium.org/2823009/diff/36002/37004#newcode549
> views/view.h:549: virtual void NotifyAccessibilityEvent(UINT32
> event_type);
> Since this is cross-platform code, this should probably be a new enum,
> rather than a Windows-specific UINT32. You can define the enum in
> accessibility_types.h.
Added.> http://codereview.chromium.org/2823009/diff/36002/37005
> File views/view_gtk.cc (right):
>
> http://codereview.chromium.org/2823009/diff/36002/37005#newcode24
> views/view_gtk.cc:24: NOTIMPLEMENTED();
> NOTIMPLEMENTED will log an error message, but this method will get
> triggered normally whether there's a screenreader or not, so I think
> this should just be a comment so as not to annoy Linux developers.
Currently, we only call this method in focus_manager_win, but I
removed it since it may not make sense for GTK.
> http://codereview.chromium.org/2823009/diff/36002/37007
> File views/widget/widget_win.cc (right):
>
> http://codereview.chromium.org/2823009/diff/36002/37007#newcode126
> views/widget/widget_win.cc:126: (accessibility_view_events_counter_ +
> 1)%max_accessibility_view_events_;
> Need spaces before and after the modulo operator.
>
> You could use "index" instead of "counter" so that things still fit on
> one line here.
Good suggestion; didn't want to break the line.>
http://codereview.chromium.org/2823009/diff/36002/37007#newcode129
> views/widget/widget_win.cc:129: // Convert to MSAA child id.
> Make sure at least one piece of code documents why negative child IDs
> are used and how they correspond to the vector indices.
Done in header file.
> http://codereview.chromium.org/2823009/diff/36002/37008
> File views/widget/widget_win.h (right):
>
> http://codereview.chromium.org/2823009/diff/36002/37008#newcode96
> views/widget/widget_win.h:96: // Obtain the view event with the given
> MSAA child id.  Used in
> Great. Also note that the ids are expected to be negative to distinguish
> them from child indexes, and that this could return NULL because only
> the last 20 views are kept.
Added comment.>
http://codereview.chromium.org/2823009/diff/36002/37008#newcode568
> views/widget/widget_win.h:568: static const int
> max_accessibility_view_events_ = 20;
> The naming style for constants is different. Call this
> kMaxAccessibilityViewEvents.
>
> http://codereview.chromium.org/2823009/show
>
Renamed.

[dmazzoni, 2010-06-18 20:29:07]

LGTM.

http://codereview.chromium.org/2823009/diff/53001/30003
File views/accessibility/view_accessibility.cc (right):

http://codereview.chromium.org/2823009/diff/53001/30003#newcode789
views/accessibility/view_accessibility.cc:789: return-1;
Add a space after return.

http://codereview.chromium.org/2823009/diff/53001/30010
File views/widget/widget_win.h (right):

http://codereview.chromium.org/2823009/diff/53001/30010#newcode571
views/widget/widget_win.h:571: static const int kMax_Accessibility_View_Events_
= 20;
Almost...no underscores here, just CamelCase.

[David Tseng, 2010-06-18 20:46:25]

Done and done.  Thanks!

On 6/18/10, dmazzoni@chromium.org <dmazzoni@chromium.org> wrote:
> LGTM.
>
>
>
> http://codereview.chromium.org/2823009/diff/53001/30003
> File views/accessibility/view_accessibility.cc (right):
>
> http://codereview.chromium.org/2823009/diff/53001/30003#newcode789
> views/accessibility/view_accessibility.cc:789: return-1;
> Add a space after return.
>
> http://codereview.chromium.org/2823009/diff/53001/30010
> File views/widget/widget_win.h (right):
>
> http://codereview.chromium.org/2823009/diff/53001/30010#newcode571
> views/widget/widget_win.h:571: static const int
> kMax_Accessibility_View_Events_ = 20;
> Almost...no underscores here, just CamelCase.
>
> http://codereview.chromium.org/2823009/show
>