Re-land: Implement NativeViewAccessibilityWin using AXPlatformNodeWin.

Re-land: Implement NativeViewAccessibilityWin using AXPlatformNodeWin.

AXPlatformNodeWin is an implementation of Windows accessibility APIs
that doesn't know anything about content/ or ui/views/ - it just
exposes an abstract tree of AXNodeData nodes and uses a delegate
interface for everything else.

This patch completely removes the Windows-specific views accessibility
code and implements it using AXPlatformNodeWin instead. A future
step will be to do the same with BrowserAccessibilityWin in content/.

This patch also includes a new unit test for AXPlatformNodeWin,
since it's now possible to test it without any views or web
content code. Hooray!

BUG=457564
Committed: https://crrev.com/f12318afda4d5bfd2f8a48eda4f0d7107e11f400
Cr-Commit-Position: refs/heads/master@{#317878}

Committed: https://crrev.com/08b8639e0a7c0db180bf6ff8e2a4c54b103df3ef
Cr-Commit-Position: refs/heads/master@{#317939}

[dmazzoni, 2015-02-11 22:33:27]

dmazzoni@chromium.org changed reviewers:
+ dtseng@chromium.org, nektar@chromium.org

[dmazzoni, 2015-02-11 22:33:28]

This change is large, sorry! Some of it is just moving code, but mostly it's
decoupling the Windows accessibility code from the Views accessibility code.
worth it, and it'd be very hard to do that in pieces.

Nektarios, please review this - it's a great way to get more familiar with some
of this code - but David should approve too. Thanks.

[David Tseng, 2015-02-13 20:10:11]

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node.cc (right):

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node.cc:26: return NULL;
nit: nullptr

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node.h (right):

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node.h:27: // Cast a
gfx::NativeViewAccessible to an AXPlatformNode if it is one,
Is the relationship a cast or a lookup by the delegate?

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node.h:43: virtual void
NotifyAccessibilityEvent(ui::AXEvent event_type) = 0;
Any thoughts to how actions are handled? (e.g. setting value)?

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_base.cc (right):

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:52: return delegate_ ?
delegate_->GetIndexInParent() : 0;
Is this index 0-based?

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:55: // AXPlatformNode.
?

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:93: if (index_in_parent + 1 <
parent->GetChildCount()) {
No lower bound?

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:101: bool
AXPlatformNodeBase::IsDescendant(AXPlatformNodeBase* descendant) {
I find this param name confusing. Perhaps just node?

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:107:
AXPlatformNode::FromNativeViewAccessible(descendant->GetParent()));
Suggestion: refactor this static_cast... pattern into a separate function.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:122: 
nit: extra line

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:132: }
The below method is a generalization of this one. Just call it in this impl.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:156: }
Same for this one. Just use the return value of the second overload.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:167: }
ditto.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:180: }
ditto

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:191: }
ditto

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:201: }
ditto

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:225: }
ditto

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:237: }
ditto

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_delegate.h (right):

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_delegate.h:26: // to provide
accessibility. So when you're implementing
?

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_delegate.h:57: virtual
gfx::NativeViewAccessible HitTest(int x, int y) = 0;
Maybe HitTestSync?

[nektarios, 2015-02-13 22:44:29]

Submitted a few comments.
As a first pass, it looks fine overall. However, I would prefer it this patch
was broken into smaller pieces so that it would be clearer which parts of the
code were copied from somewhere else and which parts were indeed changed.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_base.cc (right):

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:52: return delegate_ ?
delegate_->GetIndexInParent() : 0;
On 2015/02/13 at 20:10:10, David Tseng wrote:
> Is this index 0-based?

Yes.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:70: AXPlatformNodeBase*
parent = static_cast<AXPlatformNodeBase*>(
Are all the static_casts in this file really necessary?

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:76: if (index_in_parent > 0
&&
Use >= 1 and <= .count instead. Clearer.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:92: int index_in_parent =
GetIndexInParent();
Do the plus one here. Clearer.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:93: if (index_in_parent + 1 <
parent->GetChildCount()) {
On 2015/02/13 at 20:10:10, David Tseng wrote:
> No lower bound?

Non-user provided. Supposed to be error-free.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:114: for (size_t i = 0; i <
data.bool_attributes.size(); ++i) {
Instead of these for loops I prefer a function that returns an iterator to the
first element whose ->first matches. Perhaps std::find(attributes.begin(),
attributes.end(), IsFirstEqual(...));

[David Tseng, 2015-02-13 23:34:25]

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_base.cc (right):

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:52: return delegate_ ?
delegate_->GetIndexInParent() : 0;
On 2015/02/13 22:44:29, nektarios wrote:
> On 2015/02/13 at 20:10:10, David Tseng wrote:
> > Is this index 0-based?
> 
> Yes.


Concern is 0 is also being used as the "error" value when delegate_ is NULL.

[David Tseng, 2015-02-14 00:09:01]

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_win.cc (right):

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_win.cc:24:
base::LazyInstance<UniqueIdWinMap> g_unique_id_win_map =
What are you mapping here? hwnd's to AXPlatformWin objects? Comments please.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_win.cc:27: typedef
base::hash_set<AXPlatformNodeWin*> AXPlatformNodeWinSet;
nit: comment

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_win.cc:33: LONG
GetNextNegativeUniqueIdForWinAccessibility(IAccessible* obj) {
Want to wrap these in the anonymous namespace?

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_win.cc:72: unique_id_win_ =
GetNextNegativeUniqueIdForWinAccessibility(this);
CAn't you do this in an initializer list?

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_win.cc:80: // AXPlatformNode.
... implementation?

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_win.cc:116: if (!delegate_)
How about a macro to capture this pattern?

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_win_unittest.cc (right):

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_win_unittest.cc:70: AXNode*
RootNode() {
nit: GetRootNode

[dmazzoni, 2015-02-17 22:48:06]

Ready for another look.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node.cc (right):

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node.cc:26: return NULL;
On 2015/02/13 20:10:10, David Tseng wrote:
> nit: nullptr

Fixed throughout.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node.h (right):

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node.h:27: // Cast a
gfx::NativeViewAccessible to an AXPlatformNode if it is one,
On 2015/02/13 20:10:10, David Tseng wrote:
> Is the relationship a cast or a lookup by the delegate?

It's a cast, like the comment says. The actual implementation of the check will
vary by platform - on Windows we use QueryInterface to determine if it's an
instance of this particular class, on Mac we may use a custom selector, etc.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node.h:43: virtual void
NotifyAccessibilityEvent(ui::AXEvent event_type) = 0;
On 2015/02/13 20:10:10, David Tseng wrote:
> Any thoughts to how actions are handled? (e.g. setting value)?

Yes, each subclass of AXPlatformNode implelments platform-specific actions and
calls the appropriate AXPlatformNodeDelegate function to handle the action. This
changelist implements support for setting the value.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_base.cc (right):

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:52: return delegate_ ?
delegate_->GetIndexInParent() : 0;
On 2015/02/13 23:34:25, David Tseng wrote:
> On 2015/02/13 22:44:29, nektarios wrote:
> > On 2015/02/13 at 20:10:10, David Tseng wrote:
> > > Is this index 0-based?
> > 
> > Yes.
> 
> 
> Concern is 0 is also being used as the "error" value when delegate_ is NULL.

Good point. Thinking about this some more, trying to provide values when
delegate_ is NULL is just delaying any potential errors. I stuck a
CHECK(delegate_) at the top of every function instead. All of the public API
functions like get_accName should be checking whether delegate_ is null first
thing and doing nothing if so, so the CHECK would only get hit if there's a bug
in our code and we're trying to actually use an object that's not valid anymore.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:55: // AXPlatformNode.
On 2015/02/13 20:10:10, David Tseng wrote:
> ?

Changed to "AXPlatformNode overrides". Some Chrome code just says the name of
the superclass.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:70: AXPlatformNodeBase*
parent = static_cast<AXPlatformNodeBase*>(
On 2015/02/13 22:44:29, nektarios wrote:
> Are all the static_casts in this file really necessary?

Good point - I added an additional helper function to this file that returns an
AXPlatformNodeBase* so now there are fewer casts.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:76: if (index_in_parent > 0
&&
On 2015/02/13 22:44:29, nektarios wrote:
> Use >= 1 and <= .count instead. Clearer.

Agreed about the first, but I think that the second is clearer as-is because
it's highlighting the fact that we're checking that the previous sibling index
is valid.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:92: int index_in_parent =
GetIndexInParent();
On 2015/02/13 22:44:29, nektarios wrote:
> Do the plus one here. Clearer.

Great idea.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:93: if (index_in_parent + 1 <
parent->GetChildCount()) {
On 2015/02/13 22:44:29, nektarios wrote:
> On 2015/02/13 at 20:10:10, David Tseng wrote:
> > No lower bound?
> 
> Non-user provided. Supposed to be error-free.

Done.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:93: if (index_in_parent + 1 <
parent->GetChildCount()) {
On 2015/02/13 20:10:10, David Tseng wrote:
> No lower bound?

Done.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:107:
AXPlatformNode::FromNativeViewAccessible(descendant->GetParent()));
On 2015/02/13 20:10:10, David Tseng wrote:
> Suggestion: refactor this static_cast... pattern into a separate function.

Done.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:114: for (size_t i = 0; i <
data.bool_attributes.size(); ++i) {
On 2015/02/13 22:44:29, nektarios wrote:
> Instead of these for loops I prefer a function that returns an iterator to the
> first element whose ->first matches. Perhaps std::find(attributes.begin(),
> attributes.end(), IsFirstEqual(...));

OK. I created a templated helper function FindInVectorOfPairs because then the
compiler can infer the types. If I tried to do it just using std::find_if
directly I'd need to specify the templated types each time and it'd be far less
readable. Does this look okay?

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:122: 
On 2015/02/13 20:10:10, David Tseng wrote:
> nit: extra line

Done.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:132: }
On 2015/02/13 20:10:10, David Tseng wrote:
> The below method is a generalization of this one. Just call it in this impl.

Sure. Done for bool, int, and float. I kept the GetStringAttribute functions
separate because one form returns a const reference and the other copies.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:156: }
On 2015/02/13 20:10:10, David Tseng wrote:
> Same for this one. Just use the return value of the second overload.

Done.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_delegate.h (right):

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_delegate.h:26: // to provide
accessibility. So when you're implementing
On 2015/02/13 20:10:11, David Tseng wrote:
> ?

Done.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_delegate.h:57: virtual
gfx::NativeViewAccessible HitTest(int x, int y) = 0;
On 2015/02/13 20:10:11, David Tseng wrote:
> Maybe HitTestSync?

Done.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_win.cc (right):

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_win.cc:24:
base::LazyInstance<UniqueIdWinMap> g_unique_id_win_map =
On 2015/02/14 00:09:01, David Tseng wrote:
> What are you mapping here? hwnd's to AXPlatformWin objects? Comments please.

Done.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_win.cc:27: typedef
base::hash_set<AXPlatformNodeWin*> AXPlatformNodeWinSet;
On 2015/02/14 00:09:01, David Tseng wrote:
> nit: comment

Done.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_win.cc:33: LONG
GetNextNegativeUniqueIdForWinAccessibility(IAccessible* obj) {
On 2015/02/14 00:09:01, David Tseng wrote:
> Want to wrap these in the anonymous namespace?

Done.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_win.cc:72: unique_id_win_ =
GetNextNegativeUniqueIdForWinAccessibility(this);
On 2015/02/14 00:09:01, David Tseng wrote:
> CAn't you do this in an initializer list?

Done.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_win.cc:80: // AXPlatformNode.
On 2015/02/14 00:09:01, David Tseng wrote:
> ... implementation?

Done.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_win.cc:116: if (!delegate_)
On 2015/02/14 00:09:01, David Tseng wrote:
> How about a macro to capture this pattern?

Done.

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_win_unittest.cc (right):

https://codereview.chromium.org/909143003/diff/100001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_win_unittest.cc:70: AXNode*
RootNode() {
On 2015/02/14 00:09:01, David Tseng wrote:
> nit: GetRootNode

Done.

[David Tseng, 2015-02-18 19:51:19]

https://codereview.chromium.org/909143003/diff/120001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_win_unittest.cc (right):

https://codereview.chromium.org/909143003/diff/120001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_win_unittest.cc:132:
EXPECT_EQ(L"Help", base::string16(help));
You can throw in some negative test scenarios in these tests too (e.g. invalid
args).

https://codereview.chromium.org/909143003/diff/120001/ui/views/accessibility/...
File ui/views/accessibility/native_view_accessibility_win_unittest.cc (left):

https://codereview.chromium.org/909143003/diff/120001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility_win_unittest.cc:84:
TEST_F(NativeViewAcccessibilityWinTest, UnattachedWebView) {
What happened to this test?

[dmazzoni, 2015-02-18 23:20:20]

https://codereview.chromium.org/909143003/diff/120001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_win_unittest.cc (right):

https://codereview.chromium.org/909143003/diff/120001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_win_unittest.cc:132:
EXPECT_EQ(L"Help", base::string16(help));
On 2015/02/18 19:51:19, David Tseng wrote:
> You can throw in some negative test scenarios in these tests too (e.g. invalid
> args).

Good idea, done.

https://codereview.chromium.org/909143003/diff/120001/ui/views/accessibility/...
File ui/views/accessibility/native_view_accessibility_win_unittest.cc (left):

https://codereview.chromium.org/909143003/diff/120001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility_win_unittest.cc:84:
TEST_F(NativeViewAcccessibilityWinTest, UnattachedWebView) {
On 2015/02/18 19:51:19, David Tseng wrote:
> What happened to this test?

Deleted on purpose because I got rid of the code that registers WebViews.

In the early days of Win Aura, I added code to keep track of WebViews so that
get_accChild could handle unique ids inside web content, not just in views. It
was roundabout and ugly because ui/views can't depend on content/ and content/
can't depend on ui/views.

We ended up not needing that code because we created LegacyRenderWidgetHostHWND
and put the web content accessibility inside that HWND. However, if we end up
needing something similar in the future it will be much cleaner because we can
implement things in ui/accessibility that are shared by both content/ and
ui/views.

[David Tseng, 2015-02-18 23:26:20]

lgtm

[dmazzoni, 2015-02-18 23:47:01]

dmazzoni@chromium.org changed reviewers:
+ sky@chromium.org

[dmazzoni, 2015-02-18 23:47:02]

+sky for ui/views/

[dmazzoni, 2015-02-19 08:21:07]

New patchsets have been uploaded after l-g-t-m from dtseng@chromium.org

[sky, 2015-02-19 16:40:33]

https://codereview.chromium.org/909143003/diff/160001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_delegate.h (right):

https://codereview.chromium.org/909143003/diff/160001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_delegate.h:35: // is mostly to
implement support for walking the accessibility tree.
You should document lifetime of the returned object and when it might change.

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
File ui/views/accessibility/native_view_accessibility.cc (right):

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:104: return NULL;
nullptr

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:113: if
(!view_->GetWidget())
Why the ! here? Seems like you need it else 114 is going to crash, right?

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:122: return 0;
If there is no parent than wouldn't -1 make more sense?

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:123: return
view_->parent()->GetIndexOf(view_);
Does this work for child widgets?

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:132: return NULL;
nullptr

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:136: if
(view_->GetWidget()->GetRootView() == view_) {
I think a IsRootView() would make this code more readable (you have the
GetRootView() check in a number of places. Another option is to always call
PopulateChildWidgetVector and have it no do anything if view_ isn't a root view.
I suspect that would give you fewer ifs entirely.

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:139: for (size_t i = 0; i <
child_widgets.size(); ++i) {
Use new for loop.

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:152: // If the point is not
inside this view, return NULL.
This comment just describes the code and isn't helpful.

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:154: return NULL;
nullptr

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:165: if
(child_view->HitTestPoint(point_in_child_coords))
Why do you only descend one level here?

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:175: View* focused_view =
focus_manager ? focus_manager->GetFocusedView() : NULL;
nullptr

[chromium-reviews, 2015-02-19 17:39:18]

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...


> ui/views/accessibility/native_view_accessibility.cc:122: return 0;
> If there is no parent than wouldn't -1 make more sense?

The spec says.
indexInParent
0 based; -1 indicates there is no parent; the upper bound is the value 
returned by the parent's IAccessible::get_accChildCount.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2015-02-23 18:54:20]

On 2015/02/19 16:40:33, sky wrote:
>
https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
> ui/views/accessibility/native_view_accessibility.cc:123: return
> view_->parent()->GetIndexOf(view_);
> Does this work for child widgets?

Good catch. It looks like the previous code didn't work correctly for child
widgets either, and this is a great excuse to fix it.

I added a test and tried to fix it, but it's not obvious to me how to figure out
if a widget is owned by any other widgets using only views code (i.e. not aura).
Is there a way?

My first thought was to call widget->GetTopLevelWidget, but that only seems to
work in some cases, like for child widgets but not owned top-level widgets.

[sky, 2015-02-23 21:46:02]

On Mon, Feb 23, 2015 at 10:54 AM,  <dmazzoni@chromium.org> wrote:
> On 2015/02/19 16:40:33, sky wrote:
>
>
https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
>>
>> ui/views/accessibility/native_view_accessibility.cc:123: return
>> view_->parent()->GetIndexOf(view_);
>> Does this work for child widgets?
>
>
> Good catch. It looks like the previous code didn't work correctly for child
> widgets either, and this is a great excuse to fix it.
>
> I added a test and tried to fix it, but it's not obvious to me how to figure
> out
> if a widget is owned by any other widgets using only views code (i.e. not
> aura).
> Is there a way?

I don't believe we've had a need to track this. So, I think you're
right, there likely is no way to query if a widget is owned by another
widget.

  -Scott

>
> My first thought was to call widget->GetTopLevelWidget, but that only seems
> to
> work in some cases, like for child widgets but not owned top-level widgets.
>
>
> https://codereview.chromium.org/909143003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2015-02-24 04:53:59]

https://codereview.chromium.org/909143003/diff/160001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_delegate.h (right):

https://codereview.chromium.org/909143003/diff/160001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_delegate.h:35: // is mostly to
implement support for walking the accessibility tree.
On 2015/02/19 16:40:32, sky wrote:
> You should document lifetime of the returned object and when it might change.

Good idea. Even better, I made it a reference so it doesn't even
need to be documented.

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
File ui/views/accessibility/native_view_accessibility.cc (right):

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:113: if
(!view_->GetWidget())
On 2015/02/19 16:40:32, sky wrote:
> Why the ! here? Seems like you need it else 114 is going to crash, right?

Ack, good catch. I didn't mean to change the Mac behavior here.

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:122: return 0;
On 2015/02/19 16:40:32, sky wrote:
> If there is no parent than wouldn't -1 make more sense?

Done.

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:123: return
view_->parent()->GetIndexOf(view_);
On 2015/02/19 16:40:32, sky wrote:
> Does this work for child widgets?

Fixed now, we explicitly keep track of the parent widget.

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:136: if
(view_->GetWidget()->GetRootView() == view_) {
On 2015/02/19 16:40:32, sky wrote:
> I think a IsRootView() would make this code more readable (you have the
> GetRootView() check in a number of places. Another option is to always call
> PopulateChildWidgetVector and have it no do anything if view_ isn't a root
view.
> I suspect that would give you fewer ifs entirely.

I went with the second option, PopulateChildWidgetVector just does nothing if
it's not a root view.

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:139: for (size_t i = 0; i <
child_widgets.size(); ++i) {
On 2015/02/19 16:40:32, sky wrote:
> Use new for loop.

Can you clarify?

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:165: if
(child_view->HitTestPoint(point_in_child_coords))
On 2015/02/19 16:40:32, sky wrote:
> Why do you only descend one level here?

This will be called recursively when we return any value other than this or
NULL. (That's necessary in order to handle hit testing that spans multiple
implementation classes.)

Worth a comment!

[sky, 2015-02-24 16:45:35]

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
File ui/views/accessibility/native_view_accessibility.cc (right):

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:139: for (size_t i = 0; i <
child_widgets.size(); ++i) {
On 2015/02/24 04:53:59, dmazzoni wrote:
> On 2015/02/19 16:40:32, sky wrote:
> > Use new for loop.
> 
> Can you clarify?

for (Widget* child_widget : child_widgets) {
}

[dmazzoni, 2015-02-24 17:48:44]

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
File ui/views/accessibility/native_view_accessibility.cc (right):

https://codereview.chromium.org/909143003/diff/160001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:139: for (size_t i = 0; i <
child_widgets.size(); ++i) {
On 2015/02/24 16:45:34, sky wrote:
> On 2015/02/24 04:53:59, dmazzoni wrote:
> > On 2015/02/19 16:40:32, sky wrote:
> > > Use new for loop.
> > 
> > Can you clarify?
> 
> for (Widget* child_widget : child_widgets) {
> }

Done.

[sky, 2015-02-24 18:11:19]

LGTM

[dmazzoni, 2015-02-24 18:49:58]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-02-24 18:49:59]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org
Link to the patchset: https://codereview.chromium.org/909143003/#ps320001
(title: "New for loop syntax")

[commit-bot: I haz the power, 2015-02-24 18:51:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/909143003/320001

[commit-bot: I haz the power, 2015-02-24 20:35:38]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2015-02-24 20:36:19]

Patchset 17 (id:??) landed as
https://crrev.com/f12318afda4d5bfd2f8a48eda4f0d7107e11f400
Cr-Commit-Position: refs/heads/master@{#317878}

[DaleCurtis, 2015-02-24 21:10:26]

A revert of this CL (patchset #17 id:320001) has been created in
https://codereview.chromium.org/949333002/ by dalecurtis@chromium.org.

The reason for reverting is: Broke the Windows GN builder:
http://build.chromium.org/p/chromium.win/builders/Win8%20GN/builds/5049/steps....

[dmazzoni, 2015-02-24 23:55:09]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-02-24 23:55:09]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/909143003/#ps340001
(title: "Update GN build")

[commit-bot: I haz the power, 2015-02-24 23:56:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/909143003/340001

[commit-bot: I haz the power, 2015-02-25 01:25:32]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2015-02-25 01:26:17]

Patchset 18 (id:??) landed as
https://crrev.com/08b8639e0a7c0db180bf6ff8e2a4c54b103df3ef
Cr-Commit-Position: refs/heads/master@{#317939}