Hide AXPlatformNode on ChromeOS.

Hide AXPlatformNode on ChromeOS.

AXPlatformNode is always null on ChromeOS (and cast_shell) and never
null on other toolkit-views platforms. ChromeOS just uses
AXViewObjWrapper and related classes.

This means that, although NativeViewAccessibility (and AXNodeData) is
needed on all platforms, on ChromeOS, NativeViewAccessibility will never
serve as an AXPlatformNodeDelegate, so doesn't need to implement it.
Currently, it's complicating the implementation of platform integration
since the NativeViewAccessibility always has a null AXPlatformNode.

Split NativeViewAccessibility into two; keeping the abstract interface
that views::View needs in NativeViewAccessibility and move the rest into
NativeViewAccessibilityBase, which is never compiled on ChromeOS.

BUG=610589
Review-Url: https://codereview.chromium.org/2746813002
Cr-Commit-Position: refs/heads/master@{#458978}
Committed: https://chromium.googlesource.com/chromium/src/+/2d1f02c30222c895ba0216f011a627140e2a1318

[tapted, 2017-03-13 04:57:17]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 04:57:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 05:02:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 05:02:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-03-13 05:21:53]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 05:22:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 05:26:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 05:26:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-03-13 05:41:05]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 05:41:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-13 05:46:34]

Description was changed from

==========
Hide AXPlatformNode on ChromeOS

warn


cl format


First pass: hide AXPlatformNode on ChromeOS

BUG=
==========

to

==========
Hide AXPlatformNode on ChromeOS.

AXPlatformNode is always null on ChromeOS and never null on any other
platform.

This means that, although NativeViewAccessibility (and AXNodeData) is
needed on all platforms, on ChromeOS, it will never serve as an
AXPlatformNodeDelegate.

Split NativeViewAccessibility into two; moving its implementation of
AXPlatformNodeDelegate into a PlatformNativeViewAccessibility, which is
never compiled on ChromeOS.

BUG=
==========

[commit-bot: I haz the power, 2017-03-13 05:53:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 05:53:35]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tapted, 2017-03-13 07:57:36]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 07:57:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-13 07:58:19]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 07:58:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-13 08:02:58]

Description was changed from

==========
Hide AXPlatformNode on ChromeOS.

AXPlatformNode is always null on ChromeOS and never null on any other
platform.

This means that, although NativeViewAccessibility (and AXNodeData) is
needed on all platforms, on ChromeOS, it will never serve as an
AXPlatformNodeDelegate.

Split NativeViewAccessibility into two; moving its implementation of
AXPlatformNodeDelegate into a PlatformNativeViewAccessibility, which is
never compiled on ChromeOS.

BUG=
==========

to

==========
Hide AXPlatformNode on ChromeOS.

AXPlatformNode is always null on ChromeOS (and cast_shell) and never
null on other toolkit-views platforms.

This means that, although NativeViewAccessibility (and AXNodeData) is
needed on all platforms, on ChromeOS, it will never serve as an
AXPlatformNodeDelegate.

Split NativeViewAccessibility into two; moving its implementation of
AXPlatformNodeDelegate into a PlatformNativeViewAccessibility, which is
never compiled on ChromeOS.

BUG=
==========

[tapted, 2017-03-13 08:08:30]

Description was changed from

==========
Hide AXPlatformNode on ChromeOS.

AXPlatformNode is always null on ChromeOS (and cast_shell) and never
null on other toolkit-views platforms.

This means that, although NativeViewAccessibility (and AXNodeData) is
needed on all platforms, on ChromeOS, it will never serve as an
AXPlatformNodeDelegate.

Split NativeViewAccessibility into two; moving its implementation of
AXPlatformNodeDelegate into a PlatformNativeViewAccessibility, which is
never compiled on ChromeOS.

BUG=
==========

to

==========
Hide AXPlatformNode on ChromeOS.

AXPlatformNode is always null on ChromeOS (and cast_shell) and never
null on other toolkit-views platforms.

This means that, although NativeViewAccessibility (and AXNodeData) is
needed on all platforms, on ChromeOS, NativeViewAccessibility will never
serve as an AXPlatformNodeDelegate, so doesn't need to implement it.

Split NativeViewAccessibility into two; moving its implementation of
AXPlatformNodeDelegate into a PlatformNativeViewAccessibility, which is
never compiled on ChromeOS.

BUG=
==========

[commit-bot: I haz the power, 2017-03-13 08:29:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 08:29:10]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tapted, 2017-03-20 06:39:30]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 06:39:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-20 07:21:00]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 07:21:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-20 07:31:50]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 07:32:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 07:45:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-20 07:45:15]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[tapted, 2017-03-20 09:36:01]

Patchset #9 (id:150001) has been deleted

[tapted, 2017-03-20 09:36:22]

Patchset #6 (id:90001) has been deleted

[tapted, 2017-03-20 09:36:35]

Patchset #5 (id:80001) has been deleted

[tapted, 2017-03-20 09:36:44]

Patchset #4 (id:60001) has been deleted

[tapted, 2017-03-20 09:37:07]

Patchset #3 (id:40001) has been deleted

[tapted, 2017-03-20 09:37:17]

Patchset #2 (id:20001) has been deleted

[tapted, 2017-03-20 09:37:26]

Patchset #1 (id:1) has been deleted

[tapted, 2017-03-20 09:42:08]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 09:42:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-20 10:29:20]

Description was changed from

==========
Hide AXPlatformNode on ChromeOS.

AXPlatformNode is always null on ChromeOS (and cast_shell) and never
null on other toolkit-views platforms.

This means that, although NativeViewAccessibility (and AXNodeData) is
needed on all platforms, on ChromeOS, NativeViewAccessibility will never
serve as an AXPlatformNodeDelegate, so doesn't need to implement it.

Split NativeViewAccessibility into two; moving its implementation of
AXPlatformNodeDelegate into a PlatformNativeViewAccessibility, which is
never compiled on ChromeOS.

BUG=
==========

to

==========
Hide AXPlatformNode on ChromeOS.

AXPlatformNode is always null on ChromeOS (and cast_shell) and never
null on other toolkit-views platforms. ChromeOS just uses
AXViewObjWrapper and related classes.

This means that, although NativeViewAccessibility (and AXNodeData) is
needed on all platforms, on ChromeOS, NativeViewAccessibility will never
serve as an AXPlatformNodeDelegate, so doesn't need to implement it.
Currently, it's complicating the implementation of platform integration
since the NativeViewAccessibility always has a null AXViewObjWrapper.

Split NativeViewAccessibility into two; moving the shell interface that 
views::View needs into NativeViewAccessibilityBase and the rest, which
is never compiled on ChromeOS.

BUG=610589
==========

[tapted, 2017-03-20 10:29:20]

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

[tapted, 2017-03-20 10:36:12]

Hi Dominic, please take a look

(cc patti fyi)

https://codereview.chromium.org/2746813002/diff/190001/ui/views/accessibility...
File ui/views/accessibility/native_view_accessibility_stub.cc (right):

https://codereview.chromium.org/2746813002/diff/190001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility_stub.cc:12: return nullptr;
we could also return a dummy object here, but view.cc has null checks whenever
it accesses |native_view_accessibility_|, so this should be fine.

An alternative is to provide the same "never returns null" guarantee that
AXPlatformNode now has - that might neaten up some of the call sites in view.cc.

(i.e. by implementing a concrete "stub" class that just returns null from
GetNativeObject)

[commit-bot: I haz the power, 2017-03-20 10:42:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-20 10:42:10]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-03-20 15:44:06]

Overall looks great, much appreciated. Just a couple of concerns,
then this should be good.

https://codereview.chromium.org/2746813002/diff/190001/ui/views/accessibility...
File ui/views/accessibility/native_view_accessibility_base.h (right):

https://codereview.chromium.org/2746813002/diff/190001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility_base.h:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2746813002/diff/190001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility_base.h:21: class VIEWS_EXPORT
NativeViewAccessibilityBase {
This naming pattern, while not unintuitive, seems inconsistent with the pattern
we're using
elsewhere in the code:

RenderWidgetHostView (abstract public interface)
  RenderWidgetHostViewBase (cross-platform base class with shared code)
    RenderWidgetHostViewMac (Mac-specific subclass)
    RenderWidgetHostViewAura (Aura-specific subclass)

I did the same with AXPlatformNode:

AXPlatformNode (public interface)
  AXPlatformNodeBase (cross-platform base class with shared code)
    AXPlatformNodeMac (Mac-specific subclass)
    AXPlatformNodeWin (Win-specific subclass)

So I'd prefer we don't flip "Base" to mean the abstract interface.

I'd be happy with any of these

NativeViewAccessibilityInterface -> NativeViewAccessibility
AbstractNativeViewAccessibility -> NativeViewAccessibility
NativeViewAccessibility -> NativeViewAccessibilityBase

https://codereview.chromium.org/2746813002/diff/190001/ui/views/accessibility...
File ui/views/accessibility/native_view_accessibility_stub.cc (right):

https://codereview.chromium.org/2746813002/diff/190001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility_stub.cc:12: return nullptr;
On 2017/03/20 10:36:11, tapted wrote:
> we could also return a dummy object here, but view.cc has null checks whenever
> it accesses |native_view_accessibility_|, so this should be fine.
> 
> An alternative is to provide the same "never returns null" guarantee that
> AXPlatformNode now has - that might neaten up some of the call sites in
view.cc.
> 
> (i.e. by implementing a concrete "stub" class that just returns null from
> GetNativeObject)

Semantically this approach seems more clear than a stub class - it makes more
sense
to say that we don't have or need an implementation on some platforms rather
than
saying we're going to create an instance for every view that does nothing.

https://codereview.chromium.org/2746813002/diff/190001/ui/views/controls/tabb...
File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right):

https://codereview.chromium.org/2746813002/diff/190001/ui/views/controls/tabb...
ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:146: #if
BUILDFLAG(HAS_PLATFORM_ACCESSIBILITY)
Hmmm, I understand why you did this, but the Views code that this is
testing really does get exercised on Chrome OS (via the
AXViewObjWrapper) so it'd be better to refactor it a different
way so that the test still runs.

https://codereview.chromium.org/2746813002/diff/190001/ui/views/controls/tabb...
ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:174: ui::AXNodeData data =
pnva[i]->GetData();
How about GetTabAt(i)->GetAccessibleNodeData() instead? That gets the
underlying accessibility data without either wrapper, which seems like
what we want to test.

https://codereview.chromium.org/2746813002/diff/190001/ui/views/controls/tabb...
ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:186:
pnva[0]->AccessibilityPerformAction(action);
Same here: GetTabAt(i)->HandleAccessibleAction. If that doesn't work
we should refactor that code up to live in View rather than in the wrapper.

[tapted, 2017-03-21 00:17:36]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 00:18:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-21 00:20:58]

Patchset #5 (id:210001) has been deleted

[tapted, 2017-03-21 00:26:05]

Description was changed from

==========
Hide AXPlatformNode on ChromeOS.

AXPlatformNode is always null on ChromeOS (and cast_shell) and never
null on other toolkit-views platforms. ChromeOS just uses
AXViewObjWrapper and related classes.

This means that, although NativeViewAccessibility (and AXNodeData) is
needed on all platforms, on ChromeOS, NativeViewAccessibility will never
serve as an AXPlatformNodeDelegate, so doesn't need to implement it.
Currently, it's complicating the implementation of platform integration
since the NativeViewAccessibility always has a null AXViewObjWrapper.

Split NativeViewAccessibility into two; moving the shell interface that 
views::View needs into NativeViewAccessibilityBase and the rest, which
is never compiled on ChromeOS.

BUG=610589
==========

to

==========
Hide AXPlatformNode on ChromeOS.

AXPlatformNode is always null on ChromeOS (and cast_shell) and never
null on other toolkit-views platforms. ChromeOS just uses
AXViewObjWrapper and related classes.

This means that, although NativeViewAccessibility (and AXNodeData) is
needed on all platforms, on ChromeOS, NativeViewAccessibility will never
serve as an AXPlatformNodeDelegate, so doesn't need to implement it.
Currently, it's complicating the implementation of platform integration
since the NativeViewAccessibility always has a null AXViewObjWrapper.

Split NativeViewAccessibility into two; keeping the abstract interface
that  views::View needs in NativeViewAccessibility and move the rest
into NativeViewAccessibilityBase, which is never compiled on ChromeOS.

BUG=610589
==========

[tapted, 2017-03-21 00:35:22]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 00:36:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-21 02:25:27]

Patchset #5 (id:230001) has been deleted

[tapted, 2017-03-21 02:27:49]

PTAL - and thanks for the great pointers!

https://codereview.chromium.org/2746813002/diff/190001/ui/views/accessibility...
File ui/views/accessibility/native_view_accessibility_base.h (right):

https://codereview.chromium.org/2746813002/diff/190001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility_base.h:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
On 2017/03/20 15:44:06, dmazzoni wrote:
> 2017

Done (although this is now closer to a "fork" of native_view_accessibility.h but
probably different enough that it deserves a new header).

https://codereview.chromium.org/2746813002/diff/190001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility_base.h:21: class VIEWS_EXPORT
NativeViewAccessibilityBase {
On 2017/03/20 15:44:06, dmazzoni wrote:
> This naming pattern, while not unintuitive, seems inconsistent with the
pattern
> we're using
> elsewhere in the code:
> 
> RenderWidgetHostView (abstract public interface)
>   RenderWidgetHostViewBase (cross-platform base class with shared code)
>     RenderWidgetHostViewMac (Mac-specific subclass)
>     RenderWidgetHostViewAura (Aura-specific subclass)
> 
> I did the same with AXPlatformNode:
> 
> AXPlatformNode (public interface)
>   AXPlatformNodeBase (cross-platform base class with shared code)
>     AXPlatformNodeMac (Mac-specific subclass)
>     AXPlatformNodeWin (Win-specific subclass)
> 
> So I'd prefer we don't flip "Base" to mean the abstract interface.
> 
> I'd be happy with any of these
> 
> NativeViewAccessibilityInterface -> NativeViewAccessibility
> AbstractNativeViewAccessibility -> NativeViewAccessibility
> NativeViewAccessibility -> NativeViewAccessibilityBase
> 

Done.

I was tempted by NativeViewAccessibilityInterface to keep the diff a little more
sane, but I think NativeViewAccessibility -> NativeViewAccessibilityBase makes
the most sense for consistency.

https://codereview.chromium.org/2746813002/diff/190001/ui/views/controls/tabb...
File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right):

https://codereview.chromium.org/2746813002/diff/190001/ui/views/controls/tabb...
ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:146: #if
BUILDFLAG(HAS_PLATFORM_ACCESSIBILITY)
On 2017/03/20 15:44:06, dmazzoni wrote:
> Hmmm, I understand why you did this, but the Views code that this is
> testing really does get exercised on Chrome OS (via the
> AXViewObjWrapper) so it'd be better to refactor it a different
> way so that the test still runs.

Done.

https://codereview.chromium.org/2746813002/diff/190001/ui/views/controls/tabb...
ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:174: ui::AXNodeData data =
pnva[i]->GetData();
On 2017/03/20 15:44:06, dmazzoni wrote:
> How about GetTabAt(i)->GetAccessibleNodeData() instead? That gets the
> underlying accessibility data without either wrapper, which seems like
> what we want to test.

Done.

https://codereview.chromium.org/2746813002/diff/190001/ui/views/controls/tabb...
ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:186:
pnva[0]->AccessibilityPerformAction(action);
On 2017/03/20 15:44:06, dmazzoni wrote:
> Same here: GetTabAt(i)->HandleAccessibleAction. If that doesn't work
> we should refactor that code up to live in View rather than in the wrapper.

Done.

[commit-bot: I haz the power, 2017-03-21 03:39:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 03:39:29]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-03-21 15:42:04]

lgtm

https://codereview.chromium.org/2746813002/diff/250001/ui/accessibility/BUILD.gn
File ui/accessibility/BUILD.gn (right):

https://codereview.chromium.org/2746813002/diff/250001/ui/accessibility/BUILD...
ui/accessibility/BUILD.gn:131: if (is_win) {
No concerns for this change, but just a note:

This should probably be if (has_platform_accessibility) because we
ought to have Mac tests at this layer too. Seeing this
made me realize we don't have any.

It looks like all of the Mac accessibility tests are in 
native_widget_mac_accessibility_unittest.mm, and while
there's nothing wrong with any of these tests, we should
try to do more unit-testing at the level of AXPlatformNode
too.

Tests in ui/views should focus on interactions between
accessibility and views, tests in ui/accessibility should focus
on getting all of the corner cases of the NSAccessibility
implementation right.

https://codereview.chromium.org/2746813002/diff/250001/ui/views/accessibility...
File ui/views/accessibility/native_view_accessibility_base.h (right):

https://codereview.chromium.org/2746813002/diff/250001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility_base.h:26: class VIEWS_EXPORT
NativeViewAccessibilityBase
Please add a brief class comment; this is the shared code between
all platform-specific subclasses.

[tapted, 2017-03-22 00:07:08]

Description was changed from

==========
Hide AXPlatformNode on ChromeOS.

AXPlatformNode is always null on ChromeOS (and cast_shell) and never
null on other toolkit-views platforms. ChromeOS just uses
AXViewObjWrapper and related classes.

This means that, although NativeViewAccessibility (and AXNodeData) is
needed on all platforms, on ChromeOS, NativeViewAccessibility will never
serve as an AXPlatformNodeDelegate, so doesn't need to implement it.
Currently, it's complicating the implementation of platform integration
since the NativeViewAccessibility always has a null AXViewObjWrapper.

Split NativeViewAccessibility into two; keeping the abstract interface
that  views::View needs in NativeViewAccessibility and move the rest
into NativeViewAccessibilityBase, which is never compiled on ChromeOS.

BUG=610589
==========

to

==========
Hide AXPlatformNode on ChromeOS.

AXPlatformNode is always null on ChromeOS (and cast_shell) and never
null on other toolkit-views platforms. ChromeOS just uses
AXViewObjWrapper and related classes.

This means that, although NativeViewAccessibility (and AXNodeData) is
needed on all platforms, on ChromeOS, NativeViewAccessibility will never
serve as an AXPlatformNodeDelegate, so doesn't need to implement it.
Currently, it's complicating the implementation of platform integration
since the NativeViewAccessibility always has a null AXViewObjWrapper.

Split NativeViewAccessibility into two; keeping the abstract interface
that views::View needs in NativeViewAccessibility and move the rest into
NativeViewAccessibilityBase, which is never compiled on ChromeOS.

BUG=610589
==========

[tapted, 2017-03-22 00:13:13]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 00:14:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-22 01:47:26]

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

[tapted, 2017-03-22 01:47:27]

+sky for OWNERS - I guess principally the structural stuff in:

ui/base/ui_features.gni
ui/base/BUILD.gn
ui/views/BUILD.gn

and some iwyu/refactoring in 

c/b/ui/views/location_bar/location_icon_view.cc
ui/views/../tabbed_pane_unittest.cc
ui/views/../webview.h


But other comments welcome of course :). Thanks!

https://codereview.chromium.org/2746813002/diff/250001/ui/accessibility/BUILD.gn
File ui/accessibility/BUILD.gn (right):

https://codereview.chromium.org/2746813002/diff/250001/ui/accessibility/BUILD...
ui/accessibility/BUILD.gn:131: if (is_win) {
On 2017/03/21 15:42:04, dmazzoni wrote:
> No concerns for this change, but just a note:
> 
> This should probably be if (has_platform_accessibility) because we
> ought to have Mac tests at this layer too. Seeing this
> made me realize we don't have any.
> 
> It looks like all of the Mac accessibility tests are in 
> native_widget_mac_accessibility_unittest.mm, and while
> there's nothing wrong with any of these tests, we should
> try to do more unit-testing at the level of AXPlatformNode
> too.
> 
> Tests in ui/views should focus on interactions between
> accessibility and views, tests in ui/accessibility should focus
> on getting all of the corner cases of the NSAccessibility
> implementation right.

Filed http://crbug.com/703893 for this.

https://codereview.chromium.org/2746813002/diff/250001/ui/views/accessibility...
File ui/views/accessibility/native_view_accessibility_base.h (right):

https://codereview.chromium.org/2746813002/diff/250001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility_base.h:26: class VIEWS_EXPORT
NativeViewAccessibilityBase
On 2017/03/21 15:42:04, dmazzoni wrote:
> Please add a brief class comment; this is the shared code between
> all platform-specific subclasses.

Done.

[tapted, 2017-03-22 01:48:21]

Description was changed from

==========
Hide AXPlatformNode on ChromeOS.

AXPlatformNode is always null on ChromeOS (and cast_shell) and never
null on other toolkit-views platforms. ChromeOS just uses
AXViewObjWrapper and related classes.

This means that, although NativeViewAccessibility (and AXNodeData) is
needed on all platforms, on ChromeOS, NativeViewAccessibility will never
serve as an AXPlatformNodeDelegate, so doesn't need to implement it.
Currently, it's complicating the implementation of platform integration
since the NativeViewAccessibility always has a null AXViewObjWrapper.

Split NativeViewAccessibility into two; keeping the abstract interface
that views::View needs in NativeViewAccessibility and move the rest into
NativeViewAccessibilityBase, which is never compiled on ChromeOS.

BUG=610589
==========

to

==========
Hide AXPlatformNode on ChromeOS.

AXPlatformNode is always null on ChromeOS (and cast_shell) and never
null on other toolkit-views platforms. ChromeOS just uses
AXViewObjWrapper and related classes.

This means that, although NativeViewAccessibility (and AXNodeData) is
needed on all platforms, on ChromeOS, NativeViewAccessibility will never
serve as an AXPlatformNodeDelegate, so doesn't need to implement it.
Currently, it's complicating the implementation of platform integration
since the NativeViewAccessibility always has a null AXPlatformNode.

Split NativeViewAccessibility into two; keeping the abstract interface
that views::View needs in NativeViewAccessibility and move the rest into
NativeViewAccessibilityBase, which is never compiled on ChromeOS.

BUG=610589
==========

[commit-bot: I haz the power, 2017-03-22 02:15:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 02:15:25]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT,
build hasn't started yet, builder probably lacks capacity)

[sky, 2017-03-22 15:13:25]

LGTM

https://codereview.chromium.org/2746813002/diff/270001/ui/base/ui_features.gni
File ui/base/ui_features.gni (right):

https://codereview.chromium.org/2746813002/diff/270001/ui/base/ui_features.gn...
ui/base/ui_features.gni:15: has_platform_accessibility = use_atk || is_win ||
is_mac
optional: consider naming this has_native_accessibility to better match with
other terms we use, such as nativewidget. Using 'native' also better matches the
file names too, eg native_view_accessibility*

[tapted, 2017-03-22 23:12:11]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 23:13:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-22 23:29:20]

https://codereview.chromium.org/2746813002/diff/270001/ui/base/ui_features.gni
File ui/base/ui_features.gni (right):

https://codereview.chromium.org/2746813002/diff/270001/ui/base/ui_features.gn...
ui/base/ui_features.gni:15: has_platform_accessibility = use_atk || is_win ||
is_mac
On 2017/03/22 15:13:25, sky wrote:
> optional: consider naming this has_native_accessibility to better match with
> other terms we use, such as nativewidget. Using 'native' also better matches
the
> file names too, eg native_view_accessibility*

Done.

[tapted, 2017-03-22 23:29:41]

The CQ bit was unchecked by tapted@chromium.org

[tapted, 2017-03-22 23:29:48]

The CQ bit was checked by tapted@chromium.org

[tapted, 2017-03-22 23:29:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2746813002/#ps290001
(title: "has_native_accessibility")

[commit-bot: I haz the power, 2017-03-22 23:30:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 02:05:20]

CQ is committing da patch.

Bot data: {"patchset_id": 290001, "attempt_start_ts": 1490225388169470,
"parent_rev": "1c18f0dfcf0e06987105d4aed02449328ef5d893", "commit_rev":
"2d1f02c30222c895ba0216f011a627140e2a1318"}

[commit-bot: I haz the power, 2017-03-23 02:06:10]

Description was changed from

==========
Hide AXPlatformNode on ChromeOS.

AXPlatformNode is always null on ChromeOS (and cast_shell) and never
null on other toolkit-views platforms. ChromeOS just uses
AXViewObjWrapper and related classes.

This means that, although NativeViewAccessibility (and AXNodeData) is
needed on all platforms, on ChromeOS, NativeViewAccessibility will never
serve as an AXPlatformNodeDelegate, so doesn't need to implement it.
Currently, it's complicating the implementation of platform integration
since the NativeViewAccessibility always has a null AXPlatformNode.

Split NativeViewAccessibility into two; keeping the abstract interface
that views::View needs in NativeViewAccessibility and move the rest into
NativeViewAccessibilityBase, which is never compiled on ChromeOS.

BUG=610589
==========

to

==========
Hide AXPlatformNode on ChromeOS.

AXPlatformNode is always null on ChromeOS (and cast_shell) and never
null on other toolkit-views platforms. ChromeOS just uses
AXViewObjWrapper and related classes.

This means that, although NativeViewAccessibility (and AXNodeData) is
needed on all platforms, on ChromeOS, NativeViewAccessibility will never
serve as an AXPlatformNodeDelegate, so doesn't need to implement it.
Currently, it's complicating the implementation of platform integration
since the NativeViewAccessibility always has a null AXPlatformNode.

Split NativeViewAccessibility into two; keeping the abstract interface
that views::View needs in NativeViewAccessibility and move the rest into
NativeViewAccessibilityBase, which is never compiled on ChromeOS.

BUG=610589

Review-Url: https://codereview.chromium.org/2746813002
Cr-Commit-Position: refs/heads/master@{#458978}
Committed:
https://chromium.googlesource.com/chromium/src/+/2d1f02c30222c895ba0216f011a6...
==========

[commit-bot: I haz the power, 2017-03-23 02:06:13]

Committed patchset #7 (id:290001) as
https://chromium.googlesource.com/chromium/src/+/2d1f02c30222c895ba0216f011a6...