Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2942413003:
Forward BrowserAccessibility::get_accSelection to AXPlatformNode. (Closed)
|
Created:
3 years, 6 months ago by dougt Modified:
3 years, 5 months ago Reviewers:
dmazzoni CC:
chromium-reviews, aboxhall+watch_chromium.org, nektar+watch_chromium.org, jam, yuzo+watch_chromium.org, dougt+watch_chromium.org, darin-cc_chromium.org, aleventhal+watch_chromium.org, dtseng+watch_chromium.org, je_julie, dmazzoni+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionForward BrowserAccessibility::get_accSelection to AXPlatformNode.
BUG=703369
Review-Url: https://codereview.chromium.org/2942413003
Cr-Commit-Position: refs/heads/master@{#482460}
Committed: https://chromium.googlesource.com/chromium/src/+/9d17869d645c52f8850580b88fcf6dd221bec3e4
Patch Set 1 #Patch Set 2 : Add impl and tests #
Total comments: 6
Patch Set 3 : Break up tests and add a case for mutiple selection #
Total comments: 2
Messages
Total messages: 25 (17 generated)
The CQ bit was checked by dougt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dougt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Forward BrowserAccessibility::get_accSelection to AXPlatformNode. ========== to ========== Forward BrowserAccessibility::get_accSelection to AXPlatformNode. BUG=703369 ==========
dougt@chromium.org changed reviewers: + dmazzoni@chromium.org
dmazzoni, ptal. One area I am not sure about is selection setup. Please take a look at how I am setting a selection in the test. I am not sure if we should just build an ax tree with the selection already built, or instead use the windows accessibility api to change selection.
https://codereview.chromium.org/2942413003/diff/20001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_win_unittest.cc (right): https://codereview.chromium.org/2942413003/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_win_unittest.cc:198: // We only support AX_ROLE_LIST_BOX as this point, so, this should return I'd prefer splitting this into a bunch of separate TESTs https://codereview.chromium.org/2942413003/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_win_unittest.cc:245: // Set one of the items to be selected: One last test with multiple selections https://codereview.chromium.org/2942413003/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_win_unittest.cc:258: // We got something back, make sure that it has the corrent name. corrent -> correct
On 2017/06/23 06:56:39, dougt wrote: > dmazzoni, ptal. > > One area I am not sure about is selection setup. Please take a look at how I am > setting a selection in the test. I am not sure if we should just build an ax > tree with the selection already built, or instead use the windows accessibility > api to change selection. Building the tree with selection already set is what I had in mind
The CQ bit was checked by dougt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2942413003/diff/20001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_win_unittest.cc (right): https://codereview.chromium.org/2942413003/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_win_unittest.cc:198: // We only support AX_ROLE_LIST_BOX as this point, so, this should return On 2017/06/23 18:58:36, dmazzoni wrote: > I'd prefer splitting this into a bunch of separate TESTs Done. https://codereview.chromium.org/2942413003/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_win_unittest.cc:245: // Set one of the items to be selected: On 2017/06/23 18:58:36, dmazzoni wrote: > One last test with multiple selections Done. https://codereview.chromium.org/2942413003/diff/20001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_win_unittest.cc:258: // We got something back, make sure that it has the corrent name. On 2017/06/23 18:58:36, dmazzoni wrote: > corrent -> correct Done.
lgtm https://codereview.chromium.org/2942413003/diff/40001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_win_unittest.cc (right): https://codereview.chromium.org/2942413003/diff/40001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_win_unittest.cc:342: base::win::ScopedComPtr<IAccessible> accessible; Not necessary for this change, but I wonder if there could be some helper functions at the top of this file to remove some boilerplate from some of these checks. It's not immediately obvious what it'd be. Let's think about it.
https://codereview.chromium.org/2942413003/diff/40001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_win_unittest.cc (right): https://codereview.chromium.org/2942413003/diff/40001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_win_unittest.cc:342: base::win::ScopedComPtr<IAccessible> accessible; On 2017/06/26 16:58:31, dmazzoni wrote: > Not necessary for this change, but I wonder if there > could be some helper functions at the top of this > file to remove some boilerplate from some of these > checks. > > It's not immediately obvious what it'd be. Let's > think about it. Agreed. At the very least, if we continue adding similar checks, we should factor out the "Test that this IAccessible is the expected one".
The CQ bit was checked by dougt@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498515557239160,
"parent_rev": "0c100dc973cf378df7fdce067ff7265169a52ed5", "commit_rev":
"9d17869d645c52f8850580b88fcf6dd221bec3e4"}
Message was sent while issue was closed.
Description was changed from ========== Forward BrowserAccessibility::get_accSelection to AXPlatformNode. BUG=703369 ========== to ========== Forward BrowserAccessibility::get_accSelection to AXPlatformNode. BUG=703369 Review-Url: https://codereview.chromium.org/2942413003 Cr-Commit-Position: refs/heads/master@{#482460} Committed: https://chromium.googlesource.com/chromium/src/+/9d17869d645c52f8850580b88fcf... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/9d17869d645c52f8850580b88fcf... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
