New issue
Advanced search Search tips

Issue 910672 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 3
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug
Team-Accessibility



Sign in to add a comment

Collapse AXTreeSourceViews and AXTreeSourceAura

Project Member Reported by jamescook@chromium.org, Nov 30

Issue description

They are almost identical.

I can do this by making AutomationManagerAura own the "desktop_root_" AXRootObjWrapper and injecting it through the AXTreeSourceViews constructor.


 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 1

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/93b6b693ef284d188c769ac0a6fa7ce12e62defd

commit 93b6b693ef284d188c769ac0a6fa7ce12e62defd
Author: James Cook <jamescook@chromium.org>
Date: Sat Dec 01 00:19:02 2018

Eliminate AXTreeSourceAura by merging into AXTreeSourceViews

This pushes more functionality out of //chrome and down into
//ui/views. It will make it easier to share the code with
//ash, and possibly also for chromecast.

There are AXTreeSourceAura unit tests in //chrome that exercise
the integration of AXTreeSource with AXRootObjWrapper. I left
those in place but converted to ChromeViewsTestBase. This
eliminates a chrome-to-ash dependency. The tests can be moved
into //ui/views in a follow-up CL. I didn't do it now so the
diffs would be easier to read.

BUG= 910672 
TEST=unit_tests, views_unittests

Change-Id: Ib1483bc6b8e792409b106e0cdd12bff415032ef6
Reviewed-on: https://chromium-review.googlesource.com/c/1357499
Reviewed-by: Sergey Volk <servolk@chromium.org>
Reviewed-by: David Tseng <dtseng@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612870}
[modify] https://crrev.com/93b6b693ef284d188c769ac0a6fa7ce12e62defd/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/93b6b693ef284d188c769ac0a6fa7ce12e62defd/chrome/browser/ui/ash/accessibility/ax_tree_source_aura_unittest.cc
[modify] https://crrev.com/93b6b693ef284d188c769ac0a6fa7ce12e62defd/chrome/browser/ui/aura/accessibility/automation_manager_aura.cc
[modify] https://crrev.com/93b6b693ef284d188c769ac0a6fa7ce12e62defd/chrome/browser/ui/aura/accessibility/automation_manager_aura.h
[modify] https://crrev.com/93b6b693ef284d188c769ac0a6fa7ce12e62defd/chrome/browser/ui/aura/accessibility/automation_manager_aura_browsertest.cc
[delete] https://crrev.com/8c9abe01615efbee343a79319c7c5edee997f280/chrome/browser/ui/aura/accessibility/ax_tree_source_aura.cc
[delete] https://crrev.com/8c9abe01615efbee343a79319c7c5edee997f280/chrome/browser/ui/aura/accessibility/ax_tree_source_aura.h
[modify] https://crrev.com/93b6b693ef284d188c769ac0a6fa7ce12e62defd/chromecast/browser/ui/aura/accessibility/automation_manager_aura.cc
[modify] https://crrev.com/93b6b693ef284d188c769ac0a6fa7ce12e62defd/chromecast/browser/ui/aura/accessibility/automation_manager_aura.h
[modify] https://crrev.com/93b6b693ef284d188c769ac0a6fa7ce12e62defd/chromecast/browser/ui/aura/accessibility/ax_tree_source_aura.cc
[modify] https://crrev.com/93b6b693ef284d188c769ac0a6fa7ce12e62defd/chromecast/browser/ui/aura/accessibility/ax_tree_source_aura.h
[modify] https://crrev.com/93b6b693ef284d188c769ac0a6fa7ce12e62defd/ui/views/accessibility/ax_tree_source_views.cc
[modify] https://crrev.com/93b6b693ef284d188c769ac0a6fa7ce12e62defd/ui/views/accessibility/ax_tree_source_views.h
[modify] https://crrev.com/93b6b693ef284d188c769ac0a6fa7ce12e62defd/ui/views/accessibility/ax_tree_source_views_unittest.cc
[modify] https://crrev.com/93b6b693ef284d188c769ac0a6fa7ce12e62defd/ui/views/mus/ax_tree_source_mus.cc
[modify] https://crrev.com/93b6b693ef284d188c769ac0a6fa7ce12e62defd/ui/views/mus/ax_tree_source_mus.h

Project Member

Comment 2 by bugdroid1@chromium.org, Dec 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3bb44fe77513cc8ae5310b999cb4bc739270f9d2

commit 3bb44fe77513cc8ae5310b999cb4bc739270f9d2
Author: James Cook <jamescook@chromium.org>
Date: Mon Dec 03 23:08:56 2018

Move AXTreeSourceAura tests into AXTreeSourceViews tests

AXTreeSourceAura doesn't exist any more. The tests exercise the
integration of AXRootObjWrapper with AXTreeSourceViews, so rename the
tests ot AXTreeSourceViewsRootTest.

Note that the tests lived in //chrome/browser/ui/ash. If we need an
ash-specific tree source we could introduce an AXTreeSourceAsh either
in //ash (for mash) or //chrome/browser/ui/ash (for SingleProcessMash).

Bug:  910672 
Test: views_unittests
Change-Id: I79438345e3ad9bd1aa69d74b3c34cc35efe87142
Reviewed-on: https://chromium-review.googlesource.com/c/1358972
Reviewed-by: David Tseng <dtseng@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613308}
[delete] https://crrev.com/629fcfa30ed9752b3dd8fe9c9daff5f7d1c38d79/chrome/browser/ui/ash/accessibility/ax_tree_source_aura_unittest.cc
[modify] https://crrev.com/3bb44fe77513cc8ae5310b999cb4bc739270f9d2/chrome/test/BUILD.gn
[modify] https://crrev.com/3bb44fe77513cc8ae5310b999cb4bc739270f9d2/ui/views/accessibility/ax_tree_source_views_unittest.cc

Status: Fixed (was: Started)
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 4

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/100ecc74e6a910ca158e6f6395cb2053cd54e97a

commit 100ecc74e6a910ca158e6f6395cb2053cd54e97a
Author: Guido Urdaneta <guidou@chromium.org>
Date: Tue Dec 04 09:46:33 2018

Revert "Move AXTreeSourceAura tests into AXTreeSourceViews tests"

This reverts commit 3bb44fe77513cc8ae5310b999cb4bc739270f9d2.

Reason for revert: Suspect of introducing consistent failure on 
 Linux Chromium OS ASan LSan Tests (1) bot.

First failure:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/30399

Sample logs:
views_mus_unittests Run on OS: 'Ubuntu-14.04'
Shard duration: 0:01:41.022687
failures:
AXTreeSourceViewsRootTest.Serialize
AXTreeSourceViewsRootTest.Focus
AXTreeSourceViewsRootTest.Accessors
AXTreeSourceViewsRootTest.SerializeWindowSetsClipsChildren
AXTreeSourceViewsRootTest.DoDefault

  ==7985==ERROR: LeakSanitizer: detected memory leaks
  Direct leak of 56 byte(s) in 1 object(s) allocated from:
      #0 0x55ec150a7ed2 in operator new(unsigned long) /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm/compiler-rt/lib/asan/asan_new_delete.cc:106:3
      #1 0x55ec158f3f9e in ui::AXTreeSerializer<views::AXAuraObjWrapper*, ui::AXNodeData, ui::AXTreeData>::SerializeChangedNodes(views::AXAuraObjWrapper*, ui::AXTreeUpdateBase<ui::AXNodeData, ui::AXTreeData>*) ui/accessibility/ax_tree_serializer.h:598:35
      #2 0x55ec158f41c2 in ui::AXTreeSerializer<views::AXAuraObjWrapper*, ui::AXNodeData, ui::AXTreeData>::SerializeChangedNodes(views::AXAuraObjWrapper*, ui::AXTreeUpdateBase<ui::AXNodeData, ui::AXTreeData>*) ui/accessibility/ax_tree_serializer.h:604:12
      #3 0x55ec158f41c2 in ui::AXTreeSerializer<views::AXAuraObjWrapper*, ui::AXNodeData, ui::AXTreeData>::SerializeChangedNodes(views::AXAuraObjWrapper*, ui::AXTreeUpdateBase<ui::AXNodeData, ui::AXTreeData>*) ui/accessibility/ax_tree_serializer.h:604:12
      #4 0x55ec158f09de in ui::AXTreeSerializer<views::AXAuraObjWrapper*, ui::AXNodeData, ui::AXTreeData>::SerializeChanges(views::AXAuraObjWrapper*, ui::AXTreeUpdateBase<ui::AXNodeData, ui::AXTreeData>*) ui/accessibility/ax_tree_serializer.h:422:8
      #5 0x55ec158ee9d9 in views::(anonymous namespace)::AXTreeSourceViewsRootTest_Serialize_Test::TestBody() ui/views/accessibility/ax_tree_source_views_unittest.cc:318:17
      #6 0x55ec17bdd342 in HandleExceptionsInMethodIfSupported<testing::Test, void> third_party/googletest/src/googletest/src/gtest.cc
      #7 0x55ec17bdd342 in testing::Test::Run() third_party/googletest/src/googletest/src/gtest.cc:2522
      #8 0x55ec17bdf428 in testing::TestInfo::Run() third_party/googletest/src/googletest/src/gtest.cc:2703:11
      #9 0x55ec17be08e6 in testing::TestCase::Run() third_party/googletest/src/googletest/src/gtest.cc:2825:28
      #10 0x55ec17c08f46 in testing::internal::UnitTestImpl::RunAllTests() third_party/googletest/src/googletest/src/gtest.cc:5227:43
      #11 0x55ec17c082c5 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> third_party/googletest/src/googletest/src/gtest.cc
      #12 0x55ec17c082c5 in testing::UnitTest::Run() third_party/googletest/src/googletest/src/gtest.cc:4835
      #13 0x55ec180eb7da in RUN_ALL_TESTS third_party/googletest/src/googletest/include/gtest/gtest.h:2369:46
      #14 0x55ec180eb7da in base::TestSuite::Run() base/test/test_suite.cc:294
      #15 0x55ec180f1a34 in Run base/callback.h:99:12
      #16 0x55ec180f1a34 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::OnceCallback<int ()>, unsigned long, int, bool, base::OnceCallback<void ()>) base/test/launcher/unit_test_launcher.cc:225
      #17 0x55ec180f1500 in base::LaunchUnitTests(int, char**, base::OnceCallback<int ()>) base/test/launcher/unit_test_launcher.cc:575:10
      #18 0x55ec15220e7e in views::ViewsTestSuite::RunTests() ui/views/views_test_suite.cc:33:10
      #19 0x55ec150f46a3 in main ui/views/mus/run_all_unittests_mus.cc:8:47
      #20 0x7fd6ce976f44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21f44)
  Indirect leak of 8 byte(s) in 1 object(s) allocated from:
      #0 0x55ec150a7ed2 in operator new(unsigned long) /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm/compiler-rt/lib/asan/asan_new_delete.cc:106:3
      #1 0x55ec158f6f15 in __libcpp_allocate buildtools/third_party/libc++/trunk/include/new:254:10
      #2 0x55ec158f6f15 in allocate buildtools/third_party/libc++/trunk/include/memory:1800
      #3 0x55ec158f6f15 in allocate buildtools/third_party/libc++/trunk/include/memory:1549
      #4 0x55ec158f6f15 in __split_buffer buildtools/third_party/libc++/trunk/include/__split_buffer:311
      #5 0x55ec158f6f15 in std::__1::vector<ui::ClientTreeNode*, std::__1::allocator<ui::ClientTreeNode*> >::reserve(unsigned long) buildtools/third_party/libc++/trunk/include/vector:1576
      #6 0x55ec158f3a1b in ui::AXTreeSerializer<views::AXAuraObjWrapper*, ui::AXNodeData, ui::AXTreeData>::SerializeChangedNodes(views::AXAuraObjWrapper*, ui::AXTreeUpdateBase<ui::AXNodeData, ui::AXTreeData>*) ui/accessibility/ax_tree_serializer.h:573:25
      #7 0x55ec158f41c2 in ui::AXTreeSerializer<views::AXAuraObjWrapper*, ui::AXNodeData, ui::AXTreeData>::SerializeChangedNodes(views::AXAuraObjWrapper*, ui::AXTreeUpdateBase<ui::AXNodeData, ui::AXTreeData>*) ui/accessibility/ax_tree_serializer.h:604:12
      #8 0x55ec158f41c2 in ui::AXTreeSerializer<views::AXAuraObjWrapper*, ui::AXNodeData, ui::AXTreeData>::SerializeChangedNodes(views::AXAuraObjWrapper*, ui::AXTreeUpdateBase<ui::AXNodeData, ui::AXTreeData>*) ui/accessibility/ax_tree_serializer.h:604:12
      #9 0x55ec158f41c2 in ui::AXTreeSerializer<views::AXAuraObjWrapper*, ui::AXNodeData, ui::AXTreeData>::SerializeChangedNodes(views::AXAuraObjWrapper*, ui::AXTreeUpdateBase<ui::AXNodeData, ui::AXTreeData>*) ui/accessibility/ax_tree_serializer.h:604:12
      #10 0x55ec158f09de in ui::AXTreeSerializer<views::AXAuraObjWrapper*, ui::AXNodeData, ui::AXTreeData>::SerializeChanges(views::AXAuraObjWrapper*, ui::AXTreeUpdateBase<ui::AXNodeData, ui::AXTreeData>*) ui/accessibility/ax_tree_serializer.h:422:8
      #11 0x55ec158ee9d9 in views::(anonymous namespace)::AXTreeSourceViewsRootTest_Serialize_Test::TestBody() ui/views/accessibility/ax_tree_source_views_unittest.cc:318:17
      #12 0x55ec17bdd342 in HandleExceptionsInMethodIfSupported<testing::Test, void> third_party/googletest/src/googletest/src/gtest.cc
      #13 0x55ec17bdd342 in testing::Test::Run() third_party/googletest/src/googletest/src/gtest.cc:2522
      #14 0x55ec17bdf428 in testing::TestInfo::Run() third_party/googletest/src/googletest/src/gtest.cc:2703:11
      #15 0x55ec17be08e6 in testing::TestCase::Run() third_party/googletest/src/googletest/src/gtest.cc:2825:28
      #16 0x55ec17c08f46 in testing::internal::UnitTestImpl::RunAllTests() third_party/googletest/src/googletest/src/gtest.cc:5227:43
      #17 0x55ec17c082c5 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> third_party/googletest/src/googletest/src/gtest.cc
      #18 0x55ec17c082c5 in testing::UnitTest::Run() third_party/googletest/src/googletest/src/gtest.cc:4835
      #19 0x55ec180eb7da in RUN_ALL_TESTS third_party/googletest/src/googletest/include/gtest/gtest.h:2369:46
      #20 0x55ec180eb7da in base::TestSuite::Run() base/test/test_suite.cc:294
      #21 0x55ec180f1a34 in Run base/callback.h:99:12
      #22 0x55ec180f1a34 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::OnceCallback<int ()>, unsigned long, int, bool, base::OnceCallback<void ()>) base/test/launcher/unit_test_launcher.cc:225
      #23 0x55ec180f1500 in base::LaunchUnitTests(int, char**, base::OnceCallback<int ()>) base/test/launcher/unit_test_launcher.cc:575:10
      #24 0x55ec15220e7e in views::ViewsTestSuite::RunTests() ui/views/views_test_suite.cc:33:10
      #25 0x55ec150f46a3 in main ui/views/mus/run_all_unittests_mus.cc:8:47
      #26 0x7fd6ce976f44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21f44)



Original change's description:
> Move AXTreeSourceAura tests into AXTreeSourceViews tests
> 
> AXTreeSourceAura doesn't exist any more. The tests exercise the
> integration of AXRootObjWrapper with AXTreeSourceViews, so rename the
> tests ot AXTreeSourceViewsRootTest.
> 
> Note that the tests lived in //chrome/browser/ui/ash. If we need an
> ash-specific tree source we could introduce an AXTreeSourceAsh either
> in //ash (for mash) or //chrome/browser/ui/ash (for SingleProcessMash).
> 
> Bug:  910672 
> Test: views_unittests
> Change-Id: I79438345e3ad9bd1aa69d74b3c34cc35efe87142
> Reviewed-on: https://chromium-review.googlesource.com/c/1358972
> Reviewed-by: David Tseng <dtseng@chromium.org>
> Commit-Queue: James Cook <jamescook@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#613308}

TBR=jamescook@chromium.org,dtseng@chromium.org

Change-Id: If77dfe8f468d8f5e8514e2dbe57c9aff3cbf5a3e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  910672 
Reviewed-on: https://chromium-review.googlesource.com/c/1360591
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613487}
[add] https://crrev.com/100ecc74e6a910ca158e6f6395cb2053cd54e97a/chrome/browser/ui/ash/accessibility/ax_tree_source_aura_unittest.cc
[modify] https://crrev.com/100ecc74e6a910ca158e6f6395cb2053cd54e97a/chrome/test/BUILD.gn
[modify] https://crrev.com/100ecc74e6a910ca158e6f6395cb2053cd54e97a/ui/views/accessibility/ax_tree_source_views_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Dec 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/51e1e97e26579a5d26ff5263dbe08242f0aa983c

commit 51e1e97e26579a5d26ff5263dbe08242f0aa983c
Author: James Cook <jamescook@chromium.org>
Date: Wed Dec 12 23:53:27 2018

Reland: Move AXTreeSourceAura tests into AXTreeSourceViews tests

Additional CLs have landed that resolve the memory leak in
views_mus_unittests detected by LSAN.

Original description:
>AXTreeSourceAura doesn't exist any more. The tests exercise the
>integration of AXRootObjWrapper with AXTreeSourceViews, so rename the
>tests ot AXTreeSourceViewsRootTest.
>
>Note that the tests lived in //chrome/browser/ui/ash. If we need an
>ash-specific tree source we could introduce an AXTreeSourceAsh either
>in //ash (for mash) or //chrome/browser/ui/ash (for SingleProcessMash).
>
>Bug:  910672 
>Test: views_unittests
>Reviewed-on: https://chromium-review.googlesource.com/c/1358972
>Reviewed-by: David Tseng <dtseng@chromium.org>
>Commit-Queue: James Cook <jamescook@chromium.org>
>Cr-Commit-Position: refs/heads/master@{#613308}

TBR=dtseng@chromium.org

Change-Id: Id4dc9b6dd7bedffb6cc1184ac0c1f1e36f51ffc1
Reviewed-on: https://chromium-review.googlesource.com/c/1362193
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616111}
[delete] https://crrev.com/06c91da2e7454048cbe91e46685e9965b201d928/chrome/browser/ui/ash/accessibility/ax_tree_source_aura_unittest.cc
[modify] https://crrev.com/51e1e97e26579a5d26ff5263dbe08242f0aa983c/chrome/test/BUILD.gn
[modify] https://crrev.com/51e1e97e26579a5d26ff5263dbe08242f0aa983c/ui/views/accessibility/ax_tree_source_views_unittest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Dec 13

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d642a9fc4fb8f3f69c7448be31dd6e2fe6a30bec

commit d642a9fc4fb8f3f69c7448be31dd6e2fe6a30bec
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Thu Dec 13 11:52:32 2018

Revert "Reland: Move AXTreeSourceAura tests into AXTreeSourceViews tests"

This reverts commit 51e1e97e26579a5d26ff5263dbe08242f0aa983c.

Reason for revert: Blocks reversion of https://chromium-review.googlesource.com/c/chromium/src/+/1368828 , which is causing regressions. Sorry!

Original change's description:
> Reland: Move AXTreeSourceAura tests into AXTreeSourceViews tests
> 
> Additional CLs have landed that resolve the memory leak in
> views_mus_unittests detected by LSAN.
> 
> Original description:
> >AXTreeSourceAura doesn't exist any more. The tests exercise the
> >integration of AXRootObjWrapper with AXTreeSourceViews, so rename the
> >tests ot AXTreeSourceViewsRootTest.
> >
> >Note that the tests lived in //chrome/browser/ui/ash. If we need an
> >ash-specific tree source we could introduce an AXTreeSourceAsh either
> >in //ash (for mash) or //chrome/browser/ui/ash (for SingleProcessMash).
> >
> >Bug:  910672 
> >Test: views_unittests
> >Reviewed-on: https://chromium-review.googlesource.com/c/1358972
> >Reviewed-by: David Tseng <dtseng@chromium.org>
> >Commit-Queue: James Cook <jamescook@chromium.org>
> >Cr-Commit-Position: refs/heads/master@{#613308}
> 
> TBR=dtseng@chromium.org
> 
> Change-Id: Id4dc9b6dd7bedffb6cc1184ac0c1f1e36f51ffc1
> Reviewed-on: https://chromium-review.googlesource.com/c/1362193
> Reviewed-by: James Cook <jamescook@chromium.org>
> Commit-Queue: James Cook <jamescook@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#616111}

TBR=jamescook@chromium.org,dtseng@chromium.org

Change-Id: I319e63c00929140648028601365d8896fa55b7b8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/1375127
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616273}
[add] https://crrev.com/d642a9fc4fb8f3f69c7448be31dd6e2fe6a30bec/chrome/browser/ui/ash/accessibility/ax_tree_source_aura_unittest.cc
[modify] https://crrev.com/d642a9fc4fb8f3f69c7448be31dd6e2fe6a30bec/chrome/test/BUILD.gn
[modify] https://crrev.com/d642a9fc4fb8f3f69c7448be31dd6e2fe6a30bec/ui/views/accessibility/ax_tree_source_views_unittest.cc

Sign in to add a comment