Forward BrowserAccessibility get_accState to AXPlatformNode.

Forward BrowserAccessibility get_accState to AXPlatformNode.

This is another migration from BrowserAccessibility to AXPlatformNodeWin.
In this installment, we're converting get_accState.

We attempted to land this CL in https://codereview.chromium.org/2913553002/.
The CL was reverted due to lack of adequate testing. We since have improved
our testing in bug 733437.

BUG=703369
Review-Url: https://codereview.chromium.org/2943243002
Cr-Commit-Position: refs/heads/master@{#482751}
Committed: https://chromium.googlesource.com/chromium/src/+/57c6dff1214ac4d327d0705d89534412a1743f3c

[dougt, 2017-06-17 20:49:23]

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

[commit-bot: I haz the power, 2017-06-17 20:49:26]

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-06-17 22:26:04]

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

[commit-bot: I haz the power, 2017-06-17 22:26:05]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  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_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_headless_rel 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_...)

[dougt, 2017-06-17 23:46:30]

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

[commit-bot: I haz the power, 2017-06-17 23:46:36]

Dry run: CQ is trying da patch.

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

[dougt, 2017-06-17 23:46:40]

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

[commit-bot: I haz the power, 2017-06-17 23:46:45]

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-06-18 01:04:21]

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

[commit-bot: I haz the power, 2017-06-18 01:04:22]

Dry run: This issue passed the CQ dry run.

[dougt, 2017-06-18 01:06:17]

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

[dougt, 2017-06-18 01:06:18]

dmazzoni, ptal.

aleventhal, FYI.  See ax_platform_node_win.cc. I know you're working on some
READONLY changes.

[dmazzoni, 2017-06-26 17:18:24]

https://codereview.chromium.org/2943243002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_com_win.cc (right):

https://codereview.chromium.org/2943243002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.cc:634: return
AXPlatformNodeWin::get_accState(var_id, state);
We have duplicate code to compute ia_state now, which
makes me uneasy. It's needed because of firing events,
but ideally we should just fire the events based on
internal state changes and not keep track of ia_state
in this file.

Can you do one of the following:

1.) In InitRoleAndState(), just call the AXPlatformNodeWin
functions to get the role and state?
2.) Alternatively, add an ASSERT here that the state
returned from AXPlatformNodeWin matches ia_state, and
then add a TODO to InitRoleAndState?

https://codereview.chromium.org/2943243002/diff/1/ui/accessibility/platform/a...
File ui/accessibility/platform/ax_platform_node_win.h (right):

https://codereview.chromium.org/2943243002/diff/1/ui/accessibility/platform/a...
ui/accessibility/platform/ax_platform_node_win.h:281: bool
ShouldNodeHaveReadonlyState(const AXNodeData& data) const;
It looks like these can either be static, or
maybe even just helper functions in an unnamed
namespace at the top of the C++ file

[dougt, 2017-06-26 20:01:44]

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

[commit-bot: I haz the power, 2017-06-26 20:02:09]

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-06-26 21:53:44]

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

[commit-bot: I haz the power, 2017-06-26 21:53:45]

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

[dougt, 2017-06-27 03:42:24]

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

[commit-bot: I haz the power, 2017-06-27 03:42:37]

Dry run: CQ is trying da patch.

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

[dougt, 2017-06-27 04:49:51]

https://codereview.chromium.org/2943243002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_com_win.cc (right):

https://codereview.chromium.org/2943243002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.cc:634: return
AXPlatformNodeWin::get_accState(var_id, state);
On 2017/06/26 17:18:24, dmazzoni wrote:
> We have duplicate code to compute ia_state now, which
> makes me uneasy. It's needed because of firing events,
> but ideally we should just fire the events based on
> internal state changes and not keep track of ia_state
> in this file.
> 
> Can you do one of the following:
> 
> 1.) In InitRoleAndState(), just call the AXPlatformNodeWin
> functions to get the role and state?
> 2.) Alternatively, add an ASSERT here that the state
> returned from AXPlatformNodeWin matches ia_state, and
> then add a TODO to InitRoleAndState?


Good call.  I found that MSAAState() and ia_state() differ when handling the
focus state.  Please take another look.

https://codereview.chromium.org/2943243002/diff/1/ui/accessibility/platform/a...
File ui/accessibility/platform/ax_platform_node_win.h (right):

https://codereview.chromium.org/2943243002/diff/1/ui/accessibility/platform/a...
ui/accessibility/platform/ax_platform_node_win.h:281: bool
ShouldNodeHaveReadonlyState(const AXNodeData& data) const;
On 2017/06/26 17:18:24, dmazzoni wrote:
> It looks like these can either be static, or
> maybe even just helper functions in an unnamed
> namespace at the top of the C++ file

I'll do this as a follow up. Also see CL 2962453002.

[commit-bot: I haz the power, 2017-06-27 04:52:19]

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

[commit-bot: I haz the power, 2017-06-27 04:52:20]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-06-27 21:04:53]

lgtm

Thanks, looks good to go!

[dougt, 2017-06-27 21:31:11]

The CQ bit was checked by dougt@chromium.org

[commit-bot: I haz the power, 2017-06-27 21:31: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-06-27 21:36:52]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498599071277540,
"parent_rev": "a997f57d4858b262a4036575b1e137c79dec4fc0", "commit_rev":
"57c6dff1214ac4d327d0705d89534412a1743f3c"}

[commit-bot: I haz the power, 2017-06-27 21:37:06]

Description was changed from

==========
Forward BrowserAccessibility get_accState to AXPlatformNode.

This is another migration from BrowserAccessibility to AXPlatformNodeWin.
In this installment, we're converting get_accState.

We attempted to land this CL in https://codereview.chromium.org/2913553002/.
The CL was reverted due to lack of adequate testing. We since have improved
our testing in bug 733437.

BUG=703369
==========

to

==========
Forward BrowserAccessibility get_accState to AXPlatformNode.

This is another migration from BrowserAccessibility to AXPlatformNodeWin.
In this installment, we're converting get_accState.

We attempted to land this CL in https://codereview.chromium.org/2913553002/.
The CL was reverted due to lack of adequate testing. We since have improved
our testing in bug 733437.

BUG=703369

Review-Url: https://codereview.chromium.org/2943243002
Cr-Commit-Position: refs/heads/master@{#482751}
Committed:
https://chromium.googlesource.com/chromium/src/+/57c6dff1214ac4d327d0705d8953...
==========

[commit-bot: I haz the power, 2017-06-27 21:37:08]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/57c6dff1214ac4d327d0705d8953...