Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2981073002:
Move BrowserAccessibilityRelation code to the ui/accessibility/ (Closed)
|
Created:
3 years, 5 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. |
DescriptionMove BrowserAccessibilityRelation code to the ui/accessibility/
BUG=703369
Review-Url: https://codereview.chromium.org/2981073002
Cr-Commit-Position: refs/heads/master@{#487984}
Committed: https://chromium.googlesource.com/chromium/src/+/3e1d55ad37f669ddf349799cea5874e015bbe95a
Patch Set 1 #Patch Set 2 : Move the IA2. #Patch Set 3 : Have IA methods calculate relationships on demand. #Patch Set 4 : rebase #Patch Set 5 : Force tests to calculate relationships. #
Messages
Total messages: 44 (37 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: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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.
dougt@chromium.org changed reviewers: + dmazzoni@google.com
dmazzoni, ptal note that basic functional tests for this exists in BrowserAccessibilityTest::TestIAccessible2Relations()
Description was changed from ========== Move BrowserAccessibilityRelation code to the ui/accessibility/ BUG=703369 ========== to ========== Move BrowserAccessibilityRelation code to the ui/accessibility/ BUG=703369 ==========
dougt@chromium.org changed reviewers: + dmazzoni@chromium.org - dmazzoni@google.com
lgtm Great! Could you add a comment somewhere indicating where the test coverage is? Otherwise it looks like it's not covered. Also, could you try adding a single really trivial unit test to ax_platform_node_win_unittest.cc? It doesn't need to be comprehensive at all, but I'd like to see that our unit testing infrastructure is sufficient to be able to test this type of thing outside of content/ - in particular, it depends on being able to look up a node by ID in the test code. I was thinking of trying to rewrite this code anyway, to make it compute stuff more lazily and to compute the reverse relationships on the fly instead of storing them. This refactoring potentially helps with that a lot and being able to unit-test it entirely within ui/accessibility would be fantastic.
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: 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...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/07/17 15:46:11, dmazzoni wrote: > lgtm > > Great! > > Could you add a comment somewhere indicating where the test > coverage is? Otherwise it looks like it's not covered. > > Also, could you try adding a single really trivial unit test > to ax_platform_node_win_unittest.cc? It doesn't need to > be comprehensive at all, but I'd like to see that our > unit testing infrastructure is sufficient to be able to test > this type of thing outside of content/ - in particular, > it depends on being able to look up a node by ID in > the test code. > > I was thinking of trying to rewrite this code anyway, > to make it compute stuff more lazily and to compute > the reverse relationships on the fly instead of storing > them. This refactoring potentially helps with that a lot > and being able to unit-test it entirely within ui/accessibility > would be fantastic. Hmm. Good point. Take a look at this last change which makes us recalculate the relationships on demand. WDYT?
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: 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_...)
On 2017/07/18 19:23:17, dougt wrote: > On 2017/07/17 15:46:11, dmazzoni wrote: > > lgtm > > > > Great! > > > > Could you add a comment somewhere indicating where the test > > coverage is? Otherwise it looks like it's not covered. > > > > Also, could you try adding a single really trivial unit test > > to ax_platform_node_win_unittest.cc? It doesn't need to > > be comprehensive at all, but I'd like to see that our > > unit testing infrastructure is sufficient to be able to test > > this type of thing outside of content/ - in particular, > > it depends on being able to look up a node by ID in > > the test code. > > > > I was thinking of trying to rewrite this code anyway, > > to make it compute stuff more lazily and to compute > > the reverse relationships on the fly instead of storing > > them. This refactoring potentially helps with that a lot > > and being able to unit-test it entirely within ui/accessibility > > would be fantastic. > > > Hmm. Good point. Take a look at this last change which makes us recalculate > the relationships on demand. WDYT? I guess that will not work. I'd have to calculate over the entire tree which isn't something I'd want to do in every call.
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: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
On 2017/07/19 15:12:14, dougt wrote: > On 2017/07/18 19:23:17, dougt wrote: > > On 2017/07/17 15:46:11, dmazzoni wrote: > > > lgtm > > > > > > Great! > > > > > > Could you add a comment somewhere indicating where the test > > > coverage is? Otherwise it looks like it's not covered. > > > > > > Also, could you try adding a single really trivial unit test > > > to ax_platform_node_win_unittest.cc? It doesn't need to > > > be comprehensive at all, but I'd like to see that our > > > unit testing infrastructure is sufficient to be able to test > > > this type of thing outside of content/ - in particular, > > > it depends on being able to look up a node by ID in > > > the test code. > > > > > > I was thinking of trying to rewrite this code anyway, > > > to make it compute stuff more lazily and to compute > > > the reverse relationships on the fly instead of storing > > > them. This refactoring potentially helps with that a lot > > > and being able to unit-test it entirely within ui/accessibility > > > would be fantastic. > > > > > > Hmm. Good point. Take a look at this last change which makes us recalculate > > the relationships on demand. WDYT? > > I guess that will not work. I'd have to calculate over the entire tree which > isn't something I'd want to do in every call. Yeah, it's probably better to stick with this for now. What I was imagining is that natively in AXTree we'd keep track of relations and reverse relations in a global map. Then it'd be trivial to implement the IAccessibleRelation interface on-demand based on that, without constantly creating and destroying objects. Until then, just porting the existing code sounds best, but a unit-test that only uses AXPlatformNodeWin would help a lot with that future refactoring.
Patchset #6 (id:100001) has been deleted
Patchset #5 (id:80001) has been deleted
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
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2981073002/#ps120001 (title: "Force tests to calculate relationships.")
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": 120001, "attempt_start_ts": 1500498428245230,
"parent_rev": "75542186279074de8d13aa11d7e9b0c658624835", "commit_rev":
"3e1d55ad37f669ddf349799cea5874e015bbe95a"}
Message was sent while issue was closed.
Description was changed from ========== Move BrowserAccessibilityRelation code to the ui/accessibility/ BUG=703369 ========== to ========== Move BrowserAccessibilityRelation code to the ui/accessibility/ BUG=703369 Review-Url: https://codereview.chromium.org/2981073002 Cr-Commit-Position: refs/heads/master@{#487984} Committed: https://chromium.googlesource.com/chromium/src/+/3e1d55ad37f669ddf349799cea58... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/3e1d55ad37f669ddf349799cea58... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
