|
|
Created:
3 years, 11 months ago by David Tseng Modified:
3 years, 10 months ago CC:
chromium-reviews, dmazzoni Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitial support for native accessibility in ARC
This is an initial implementation of native ARC++ accessibility.
An accessibility tree is forwarded to Chrome and Chrome forwards tree data to extensions.
TEST=manual
- in ChromeVox, in a js-background page context, verify the contents of the accessibility tree from ARC show up; when events occur, verify events are received; when the ARC+++ surface is deactivated, verify tree gets destroyed/deatched
- touch explore with ChromeVox; verify basic speech feedback
- ChromeVox object navigation works
BUG=683396
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2640123004
Cr-Commit-Position: refs/heads/master@{#452029}
Committed: https://chromium.googlesource.com/chromium/src/+/be42343c9fd8b9fbb5733faf2e93abc3ff648b7f
Patch Set 1 : m #
Total comments: 10
Patch Set 2 : address feedback #
Total comments: 4
Patch Set 3 : address feedback #Patch Set 4 : Add missing deps entry #Patch Set 5 : Simplify. #Patch Set 6 : Fix analyze #Patch Set 7 : Export. #Patch Set 8 : Fix Mac compile. #Patch Set 9 : Rebase. #Patch Set 10 : Rebase on top of yawano's changes. #Patch Set 11 : Experimental changes #
Total comments: 2
Patch Set 12 : Ready for review #Patch Set 13 : Add filter type off. #Patch Set 14 : Add boolean properties field. #Patch Set 15 : Individual settings for Talkback vs native ax. #Patch Set 16 : Remove temp id. #Patch Set 17 : Support focus! #Patch Set 18 : |window| can be nullptr #
Total comments: 22
Patch Set 19 : Address various feedback. #
Total comments: 3
Patch Set 20 : Address some changes #Patch Set 21 : Fix touch exploration (map view_hover_entered to hover on Chrome end); hover doesn't map well to fo… #Patch Set 22 : Rebase. #Patch Set 23 : override #
Total comments: 7
Patch Set 24 : Address nasko's comments. #
Total comments: 4
Patch Set 25 : Redundant include. #Patch Set 26 : Rebase. #Messages
Total messages: 142 (105 generated)
Description was changed from ========== Initial support for custom exo shell surface accessibility Here is a simple initial draft of how accessibility will hook into an exo shell surface. Potential embedders (e.g. ARC++) depend on exo, but not vice versa, so interface definitions are placed in exo itself. The tree source will belong to the embedder and significant events (e.g. accessibility, teardown) will be placed in an observer interface. BUG= ========== to ========== Initial support for custom exo shell surface accessibility Here is a simple initial draft of how accessibility will hook into an exo shell surface. Potential embedders (e.g. ARC++) depend on exo, but not vice versa, so interface definitions are placed in exo itself. The tree source will belong to the embedder and significant events (e.g. accessibility, teardown) will be placed in an observer interface. For ARC++ specifically: - automation will register inside of EnableFrame - automation will make the call to EnableFrame when it sees the ShellSurface view. - EnableFrame will have to know about ShellSurface's source tree in order to add observers. BUG= ==========
Description was changed from ========== Initial support for custom exo shell surface accessibility Here is a simple initial draft of how accessibility will hook into an exo shell surface. Potential embedders (e.g. ARC++) depend on exo, but not vice versa, so interface definitions are placed in exo itself. The tree source will belong to the embedder and significant events (e.g. accessibility, teardown) will be placed in an observer interface. For ARC++ specifically: - automation will register inside of EnableFrame - automation will make the call to EnableFrame when it sees the ShellSurface view. - EnableFrame will have to know about ShellSurface's source tree in order to add observers. BUG= ========== to ========== Initial support for custom exo shell surface accessibility Here is an initial draft of how accessibility will hook into an exo shell surface. Potential embedders (e.g. ARC++) depend on exo, but not vice versa, so interface definitions are placed in exo itself. The tree source will belong to the embedder and significant events (e.g. accessibility, teardown) will be placed in an observer interface. A stub ARC tree source implementation is included to show how things might connect to the ARC accessibility helper service/bridge. TEST=exo_unittests ==========
dtseng@chromium.org changed reviewers: + reveman@chromium.org, yawano@chromium.org
Ready for a first look. reveman: exo changes yawano: ARC++ AXTreeSourceArc stub/integration with accessibility helper. dmazzoni: FYI/general ax.
Patchset #1 (id:1) has been deleted
dmazzoni@chromium.org changed reviewers: + dmazzoni@chromium.org
https://codereview.chromium.org/2640123004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility_helper/ax_tree_source_arc.h (right): https://codereview.chromium.org/2640123004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility_helper/ax_tree_source_arc.h:16: // This class represents the accessibility tree from the focused ARC window. Just curious, why "the focused" ARC window? Could we have a separate tree source for each ARC window, or will we swap out the same tree for whichever one is focused? https://codereview.chromium.org/2640123004/diff/20001/components/exo/surface.h File components/exo/surface.h (right): https://codereview.chromium.org/2640123004/diff/20001/components/exo/surface.... components/exo/surface.h:425: AXTreeSourceSurface* ax_tree_source_; Comment who owns this. If this is owned by this class, maybe unique_ptr?
The CQ bit was checked by dtseng@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Do we have a bug for this? It would be better to file a bug for this and add BUG= line to track the work around this. https://codereview.chromium.org/2640123004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility_helper/ax_tree_source_arc.cc (right): https://codereview.chromium.org/2640123004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility_helper/ax_tree_source_arc.cc:72: surface->set_ax_tree_source(nullptr); Might we need to call AxTreeSourceArc::Reset here as this instance will be reused for the gained focused window? https://codereview.chromium.org/2640123004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility_helper/ax_tree_source_arc.cc:75: exo::Surface* focused_surface = GetArcSurface(gained_focus); nit: how about to move this line at the first line of this method and avoid calling GetArcSurface(gained_focus) 2 times? https://codereview.chromium.org/2640123004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility_helper/ax_tree_source_arc.h (right): https://codereview.chromium.org/2640123004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility_helper/ax_tree_source_arc.h:8: #include "components/exo/ax_tree_source_surface.h" Is this file already landed? I've quickly checked your in-review CLs, but I couldn't find this.
Description was changed from ========== Initial support for custom exo shell surface accessibility Here is an initial draft of how accessibility will hook into an exo shell surface. Potential embedders (e.g. ARC++) depend on exo, but not vice versa, so interface definitions are placed in exo itself. The tree source will belong to the embedder and significant events (e.g. accessibility, teardown) will be placed in an observer interface. A stub ARC tree source implementation is included to show how things might connect to the ARC accessibility helper service/bridge. TEST=exo_unittests ========== to ========== Initial support for custom exo shell surface accessibility Here is an initial draft of how accessibility will hook into an exo shell surface. Potential embedders (e.g. ARC++) depend on exo, but not vice versa, so interface definitions are placed in exo itself. The tree source will belong to the embedder and significant events (e.g. accessibility, teardown) will be placed in an observer interface. A stub ARC tree source implementation is included to show how things might connect to the ARC accessibility helper service/bridge. TEST=exo_unittests BUG=683396 ==========
https://codereview.chromium.org/2640123004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility_helper/ax_tree_source_arc.cc (right): https://codereview.chromium.org/2640123004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility_helper/ax_tree_source_arc.cc:72: surface->set_ax_tree_source(nullptr); On 2017/01/20 07:47:49, yawano wrote: > Might we need to call AxTreeSourceArc::Reset here as this instance will be > reused for the gained focused window? Done. https://codereview.chromium.org/2640123004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility_helper/ax_tree_source_arc.cc:75: exo::Surface* focused_surface = GetArcSurface(gained_focus); On 2017/01/20 07:47:49, yawano wrote: > nit: how about to move this line at the first line of this method and avoid > calling GetArcSurface(gained_focus) 2 times? Done. https://codereview.chromium.org/2640123004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility_helper/ax_tree_source_arc.h (right): https://codereview.chromium.org/2640123004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility_helper/ax_tree_source_arc.h:8: #include "components/exo/ax_tree_source_surface.h" On 2017/01/20 07:47:49, yawano wrote: > Is this file already landed? I've quickly checked your in-review CLs, but I > couldn't find this. Nope not yet. Added! https://codereview.chromium.org/2640123004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility_helper/ax_tree_source_arc.h:16: // This class represents the accessibility tree from the focused ARC window. On 2017/01/19 19:48:16, dmazzoni wrote: > Just curious, why "the focused" ARC window? Could we have a separate tree source > for each ARC window, or will we swap out the same tree for whichever one is > focused? I'm thinking we can reuse the same source tree for now. https://codereview.chromium.org/2640123004/diff/20001/components/exo/surface.h File components/exo/surface.h (right): https://codereview.chromium.org/2640123004/diff/20001/components/exo/surface.... components/exo/surface.h:425: AXTreeSourceSurface* ax_tree_source_; On 2017/01/19 19:48:16, dmazzoni wrote: > Comment who owns this. If this is owned by this class, maybe unique_ptr? Not owned by this class (commented).
The CQ bit was checked by dtseng@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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thank you! c/b/chromeos/arc/accessibility_helper lgtm with a nit (please consider to rename accessibility_helper to accessibility as I've commented.) https://codereview.chromium.org/2640123004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/BUILD.gn (right): https://codereview.chromium.org/2640123004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/BUILD.gn:211: "arc/accessibility_helper/ax_tree_source_arc.cc", Could you rename accessibility_helper to accessibility? I've renamed folder name from "accessibility_helper" to "accessibility" in my CL (https://crrev.com/2559663002) to make it consistent with c/b/chromeos/accessibility. arc/accessibility/ax_tree_source_arc.cc. https://codereview.chromium.org/2640123004/diff/40001/components/exo/ax_tree_... File components/exo/ax_tree_source_surface.cc (right): https://codereview.chromium.org/2640123004/diff/40001/components/exo/ax_tree_... components/exo/ax_tree_source_surface.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2017 instead of 2015?
https://codereview.chromium.org/2640123004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/BUILD.gn (right): https://codereview.chromium.org/2640123004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/BUILD.gn:211: "arc/accessibility_helper/ax_tree_source_arc.cc", On 2017/01/23 10:41:53, yawano wrote: > Could you rename accessibility_helper to accessibility? I've renamed folder name > from "accessibility_helper" to "accessibility" in my CL > (https://crrev.com/2559663002) to make it consistent with > c/b/chromeos/accessibility. > > arc/accessibility/ax_tree_source_arc.cc. Done. https://codereview.chromium.org/2640123004/diff/40001/components/exo/ax_tree_... File components/exo/ax_tree_source_surface.cc (right): https://codereview.chromium.org/2640123004/diff/40001/components/exo/ax_tree_... components/exo/ax_tree_source_surface.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2017/01/23 10:41:53, yawano wrote: > 2017 instead of 2015? Done.
Friendly ping reveman@.
The CQ bit was checked by dtseng@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: 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-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_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 dtseng@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 ========== Initial support for custom exo shell surface accessibility Here is an initial draft of how accessibility will hook into an exo shell surface. Potential embedders (e.g. ARC++) depend on exo, but not vice versa, so interface definitions are placed in exo itself. The tree source will belong to the embedder and significant events (e.g. accessibility, teardown) will be placed in an observer interface. A stub ARC tree source implementation is included to show how things might connect to the ARC accessibility helper service/bridge. TEST=exo_unittests BUG=683396 ========== to ========== Initial support for custom exo shell surface accessibility Here is an initial draft of how accessibility will hook into an exo shell surface. Potential embedders (e.g. ARC++) depend on exo, but not vice versa, so interface definitions are placed in exo itself. The tree source will belong to the embedder and significant events (e.g. accessibility, teardown) will be placed in an observer interface. A stub ARC tree source implementation is included to show how things might connect to the ARC accessibility helper service/bridge. TEST=exo_unittests BUG=683396 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by dtseng@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: 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-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_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 dtseng@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: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
PTAL @dmazzoni; moved around the tree registry; and minimized exo dependency.
The CQ bit was checked by dtseng@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by dtseng@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 dtseng@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 dtseng@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.
I think I need a high level description of how this will be used. I'm currently trying to wrap my head around if this only makes sense for arc++ clients or also for other exo clients? If not for other clients then why is that?
reveman@chromium.org writes: > I think I need a high level description of how this will be used. I'm > currently > trying to wrap my head around if this only makes sense for arc++ clients or > also > for other exo clients? If not for other clients then why is that? > I am open to other exo clients attaching accessibility trees to surfaces, but it's not clear to me what that means. At the moment, when an accessibility service on Chrome OS requests it, ARC forwards an accessibility tree. Exo gets looped in because we need the hosting view(s) from exo to point to the new (remote) tree from ARC. I can forward a design doc as well if that helps. > https://codereview.chromium.org/2640123004/ -- Foo X. Bar http://www.example.com -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
One question about the new interface. https://codereview.chromium.org/2640123004/diff/220001/components/arc/common/... File components/arc/common/accessibility_helper.mojom (right): https://codereview.chromium.org/2640123004/diff/220001/components/arc/common/... components/arc/common/accessibility_helper.mojom:37: array<AccessibilityNodeInfoData> nodeData; What data is expected to this nodeData field? Is this corresponding to AccessibilityEvent.getSource()? Will the whole tree of AccessibilityNodeInfo be returned? Or just the subtree from the node of AccessibilityEvent.getSource()?
Description was changed from ========== Initial support for custom exo shell surface accessibility Here is an initial draft of how accessibility will hook into an exo shell surface. Potential embedders (e.g. ARC++) depend on exo, but not vice versa, so interface definitions are placed in exo itself. The tree source will belong to the embedder and significant events (e.g. accessibility, teardown) will be placed in an observer interface. A stub ARC tree source implementation is included to show how things might connect to the ARC accessibility helper service/bridge. TEST=exo_unittests BUG=683396 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Initial support for complete accessibility in ARC This is an initial implementation of native ARC++ accessibility. An accessibility tree is forwarded to Chrome and Chrome forwards tree data to extensions. TEST=manual - in ChromeVox, in a js-background page context, verify the contents of the accessibility tree from ARC show up; when events occur, verify events are received; when the ARC+++ surface is deactivated, verify tree gets destroyed/deatched - touch explore with ChromeVox; verify basic speech feedback BUG=683396 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
dtseng@chromium.org changed reviewers: - reveman@chromium.org
PTAL. This is now ready for a full review. Note that subsequent cl's will optimize tree updates from ARC++, but it is at the moment unclear exactly what events require a full tree update. It is also possible we will need to do something like AXTreeSerializer (keeping track of the current state of the client tree). That is left for future cls as there's enough here to review and difficult to keep in sync with the ARC++ side. ywano: primary dmazzoni: ax* files. https://codereview.chromium.org/2640123004/diff/220001/components/arc/common/... File components/arc/common/accessibility_helper.mojom (right): https://codereview.chromium.org/2640123004/diff/220001/components/arc/common/... components/arc/common/accessibility_helper.mojom:37: array<AccessibilityNodeInfoData> nodeData; On 2017/02/08 11:11:27, yawano wrote: > What data is expected to this nodeData field? Is this corresponding to > AccessibilityEvent.getSource()? Will the whole tree of AccessibilityNodeInfo be > returned? Or just the subtree from the node of AccessibilityEvent.getSource()? The whole tree rooted at the source node's root will be contained in the nodeData field. See the latest patchset, which is the one ready for review, for more details. In that cl, there's a filter to only receive focus events -- in which case this nodeData will only contain the focus node data. For the "ALL" filter, we will start by sending the entire tree for every event. I don't want to prematurely optimize especially when this is all hidden behind a flag. We can then look at making optimization e.g. only sending tree contents on window content changed events (which would require we keep more state in the browser). Currently, besides the serializer, we're keeping no state.
Description was changed from ========== Initial support for complete accessibility in ARC This is an initial implementation of native ARC++ accessibility. An accessibility tree is forwarded to Chrome and Chrome forwards tree data to extensions. TEST=manual - in ChromeVox, in a js-background page context, verify the contents of the accessibility tree from ARC show up; when events occur, verify events are received; when the ARC+++ surface is deactivated, verify tree gets destroyed/deatched - touch explore with ChromeVox; verify basic speech feedback BUG=683396 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Initial support for native accessibility in ARC This is an initial implementation of native ARC++ accessibility. An accessibility tree is forwarded to Chrome and Chrome forwards tree data to extensions. TEST=manual - in ChromeVox, in a js-background page context, verify the contents of the accessibility tree from ARC show up; when events occur, verify events are received; when the ARC+++ surface is deactivated, verify tree gets destroyed/deatched - touch explore with ChromeVox; verify basic speech feedback BUG=683396 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by dtseng@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...
Description was changed from ========== Initial support for native accessibility in ARC This is an initial implementation of native ARC++ accessibility. An accessibility tree is forwarded to Chrome and Chrome forwards tree data to extensions. TEST=manual - in ChromeVox, in a js-background page context, verify the contents of the accessibility tree from ARC show up; when events occur, verify events are received; when the ARC+++ surface is deactivated, verify tree gets destroyed/deatched - touch explore with ChromeVox; verify basic speech feedback BUG=683396 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Initial support for native accessibility in ARC This is an initial implementation of native ARC++ accessibility. An accessibility tree is forwarded to Chrome and Chrome forwards tree data to extensions. TEST=manual - in ChromeVox, in a js-background page context, verify the contents of the accessibility tree from ARC show up; when events occur, verify events are received; when the ARC+++ surface is deactivated, verify tree gets destroyed/deatched - touch explore with ChromeVox; verify basic speech feedback - ChromeVox object navigation works BUG=683396 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by dtseng@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-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:71: } else { nit: maybe just return inside the first if block, to avoid indenting the rest https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h (right): https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h:45: std::unique_ptr<AXTreeSourceArc> tree_; Maybe tree_source_? https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc (right): https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:90: root_id_ = id; Any other clue that we have the root? Maybe assert that you don't get two roots? https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:187: aura::Window* focused_window = wmHelper->GetFocusedWindow(); Maybe try to refactor this out and not call it on every node? https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h (right): https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h:28: public views::View { Seems like a bit of a hack to make this a View and attach it as a child. I'm worried that could cause side effects or it could be a poorly-tested configuration. How about making the content view for the ExoShell window into a specific View subclass, and adding the GetAccessibilityNodeData override directly? It'd be essentially the same. The difference is that now everybody using ARC++ would be using that View so accessibility is less likely to break. Ping me if that isn't clear. https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h:61: int32_t id_; tree_id_? https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... File components/arc/common/accessibility_helper.mojom (right): https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... components/arc/common/accessibility_helper.mojom:20: WINDOW_STATE_CHANGED, Are these supposed to match the order of Android events? If so, document them, otherwise please sort them. https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... components/arc/common/accessibility_helper.mojom:44: // Please note the omission of Might be simpler to just map all of them even if some are unused. https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... components/arc/common/accessibility_helper.mojom:62: COPY, nit: some of these need indentation. Also, please sort all https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... components/arc/common/accessibility_helper.mojom:76: CHECKABLE, sort https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... components/arc/common/accessibility_helper.mojom:103: string contentDescription; how about map<> stringProperties? https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... components/arc/common/accessibility_helper.mojom:118: // Send a complete tree from the event source's root for every event. nit: indent https://codereview.chromium.org/2640123004/diff/360001/ui/accessibility/BUILD.gn File ui/accessibility/BUILD.gn (right): https://codereview.chromium.org/2640123004/diff/360001/ui/accessibility/BUILD... ui/accessibility/BUILD.gn:40: "ax_tree_id_registry.cc", Oh good, I like it better here anyway https://codereview.chromium.org/2640123004/diff/360001/ui/accessibility/ax_tr... File ui/accessibility/ax_tree_id_registry.h (right): https://codereview.chromium.org/2640123004/diff/360001/ui/accessibility/ax_tr... ui/accessibility/ax_tree_id_registry.h:40: int CreateID(); This is great. Should we maybe use this for the Desktop tree? That'd probably be a lot cleaner.
dtseng@chromium.org changed reviewers: + reveman@chromium.org
Replying to one question from dmazzoni. +reveman, can you please answer the question below? https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h (right): https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h:28: public views::View { On 2017/02/09 01:18:09, dmazzoni wrote: > Seems like a bit of a hack to make this a View and attach it as a child. I'm > worried that could > cause side effects or it could be a poorly-tested configuration. > > How about making the content view for the ExoShell window into a specific View > subclass, > and adding the GetAccessibilityNodeData override directly? > > It'd be essentially the same. The difference is that now everybody using ARC++ > would be > using that View so accessibility is less likely to break. > > Ping me if that isn't clear. No, its clear because I did that in the first patchsets of this change (please see). Ping again @reveman. The advantage of this approach is it makes it so we're not as tightly coupled with the exo component and don't have to introduce more dependencies for exo. There's also the need to plumb through access to ShellSurface from Surface; ShellSurface is the content view for a ShellSurfaceWidget. The downside is as you pointed out, side effects. I made the view owned by client (so we're explicitly responsible for managing it) and we're making it focusable. I'm not sure if we can do the same for the exo shell surface? I think we can mitigate the downside by writing good tests.
Thank you for working on this! https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc (right): https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:90: root_id_ = id; Can we simply force that the first element in the list must be the root node of the tree? https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... File components/arc/common/accessibility_helper.mojom (right): https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... components/arc/common/accessibility_helper.mojom:5: // Next MinVersion: 1 Update this to Next MinVersion: 2 https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... components/arc/common/accessibility_helper.mojom:102: int32 id; I think this will correspond to AccessibilityNodeInfo.getViewIdResourceName(). Would this be string? https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... components/arc/common/accessibility_helper.mojom:103: string contentDescription; +1 for map<AccessibilityStringProperty> as it will make it easier to add or remove fields of this field later. https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... components/arc/common/accessibility_helper.mojom:107: array<int32> childIds; Add [MinVersion=1] to the new fields to keep backward compatibility. https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... components/arc/common/accessibility_helper.mojom:111: enum AccessibilityFilterType { Mark this [Extensible] as we can extend this later. https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... components/arc/common/accessibility_helper.mojom:135: OnAccessibilityEvent@0(AccessibilityEventData eventData); We cannot simply change the arguments of existing method. You need to do the following things. 1. Deprecate the current method. Rename OnAccessibilityEvent@0 to OnAccessibilityEventDeperecated@0. 2. Add new method with the new interface. Add OnAccessibilityEvent@1. 3. Keep the OnAccessibilityEventDeperecated@0 work correctly. Simply proxying to the new interface is okay. GetFileSizeDeprecated in intent_helper.mojom will be an example. https://cs.chromium.org/chromium/src/components/arc/common/intent_helper.mojo...
dmazzoni@chromium.org writes: > https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... > File > chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc > (right): > > https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:71: > } else { > nit: maybe just return inside the first if block, to avoid indenting the > rest > Done > https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... > File > chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h > (right): > > https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h:45: > std::unique_ptr<AXTreeSourceArc> tree_; > Maybe tree_source_? > Done. > https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc > (right): > > https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:90: > root_id_ = id; > Any other clue that we have the root? > > Maybe assert that you don't get two roots? > Sure; done. > https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:187: > aura::Window* focused_window = wmHelper->GetFocusedWindow(); > Maybe try to refactor this out and not call it on every node? > WMHelper stores the focused window so I'll interpret your comment as a code refactor/style not the fact that this code gets called for each node (since it's on the same order cost-wise). Move this to it's own function. > https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h > (right): > > https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h:28: > public views::View { > Seems like a bit of a hack to make this a View and attach it as a child. > I'm worried that could > cause side effects or it could be a poorly-tested configuration. > > How about making the content view for the ExoShell window into a > specific View subclass, > and adding the GetAccessibilityNodeData override directly? > > It'd be essentially the same. The difference is that now everybody using > ARC++ would be > using that View so accessibility is less likely to break. > > Ping me if that isn't clear. > Summary of long offline discussion: - getWindows in an AccessibilityService only gives you the windows *within* the current app - Chrome uses task id's to uniquely identify each shell surface - View backed AccessibilityNodeInfo's don't always belong to a task id; i.e. View::getContext() doesn't always return an Activity (even when you search up context's using getBaseContext()). To reempt any other questions, here's why I did what I did in the new cl: - focusability gets controlled in the bridge because when Talkback is enabled (i.e. the command line flag is not set), we do not want the shell surface to be focused; if it were, ChromeVox would set a range and start trying to read the desktop tree. If the command line for arc chromevox support is set, then we do want the shell surface to be focusable because automation uses the focus_id computation (from desktop downward) to figure out how to treat focus events. For active/inactive observation, I kept this around and associate an ax tree id when a shell surface becomes active and remove it when it becomes inactive. This gives us the same behavior as seen by an Android service since there's only ever one app tree associated with an accessibility event. Unsure if this is changing, but this is current behavior as of N. > https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h:61: > int32_t id_; > tree_id_? > Done. > https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... > File components/arc/common/accessibility_helper.mojom (right): > > https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... > components/arc/common/accessibility_helper.mojom:20: > WINDOW_STATE_CHANGED, > Are these supposed to match the order of Android events? > > If so, document them, otherwise please sort them. > Sorted according to the order they appear in Android source. Since future maintainers will likely look at source (or should). Added comment. > https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... > components/arc/common/accessibility_helper.mojom:44: // Please note the > omission of > Might be simpler to just map all of them even if some are unused. I would like to discourage use of these. Added and commented. > > https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... > components/arc/common/accessibility_helper.mojom:62: COPY, > nit: some of these need indentation. > Done. > Also, please sort all > Not a good idea to sort as mojo uses enum offsets so re-sorting would break older versions of this enum i.e. the one with only focus event in it. > https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... > components/arc/common/accessibility_helper.mojom:76: CHECKABLE, > sort > See above. > https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... > components/arc/common/accessibility_helper.mojom:103: string > contentDescription; > how about map<> stringProperties? > I'd like to keep the interface similar to AccessibilityNodeInfo; we can do whatever we want on the Chrome end, but to reduce confusion for future changes (e.g. for O). > https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... > components/arc/common/accessibility_helper.mojom:118: // Send a complete > tree from the event source's root for every event. > nit: indent > Done. > https://codereview.chromium.org/2640123004/diff/360001/ui/accessibility/BUILD.gn > File ui/accessibility/BUILD.gn (right): > > https://codereview.chromium.org/2640123004/diff/360001/ui/accessibility/BUILD... > ui/accessibility/BUILD.gn:40: "ax_tree_id_registry.cc", > Oh good, I like it better here anyway > > https://codereview.chromium.org/2640123004/diff/360001/ui/accessibility/ax_tr... > File ui/accessibility/ax_tree_id_registry.h (right): > > https://codereview.chromium.org/2640123004/diff/360001/ui/accessibility/ax_tr... > ui/accessibility/ax_tree_id_registry.h:40: int CreateID(); > This is great. > > Should we maybe use this for the Desktop tree? That'd probably be > a lot cleaner. > Agreed; let's leave it for another change since I think we're hard coding assumptions about desktop id == 0 in multiple places. > https://codereview.chromium.org/2640123004/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
From: David Tseng <dtseng@google.com> Date: Mon, 13 Feb 2017 15:16:21 -0800 Message-ID: <xq5defz1adh6.fsf@google.com> MIME-Version: 1.0 Content-Type: text/plain --text follows this line-- PTAL. yawano@chromium.org writes: > Thank you for working on this! > > > https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc > (right): > > https://codereview.chromium.org/2640123004/diff/360001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:90: > root_id_ = id; > Can we simply force that the first element in the list must be the root > node of the tree? > We can, but I don't want to make that assumption; see the new CHECK. > https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... > File components/arc/common/accessibility_helper.mojom (right): > > https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... > components/arc/common/accessibility_helper.mojom:5: // Next MinVersion: > 1 > Update this to Next MinVersion: 2 > Done. > https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... > components/arc/common/accessibility_helper.mojom:102: int32 id; > I think this will correspond to > AccessibilityNodeInfo.getViewIdResourceName(). Would this be string? > I don't think so. We need a unique accessibility id as computed by the internal accessibility cache within Android framework. > https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... > components/arc/common/accessibility_helper.mojom:103: string > contentDescription; > +1 for map<AccessibilityStringProperty> as it will make it easier to add > or remove fields of this field later. This mirrors the way AccessibilityNodInfo works. For example, the class internally has boolean properties but no string properties. However, it does seem like version tracking would be easier if we had enums for everything, so done. > > https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... > components/arc/common/accessibility_helper.mojom:107: array<int32> > childIds; > Add [MinVersion=1] to the new fields to keep backward compatibility. > Done. > https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... > components/arc/common/accessibility_helper.mojom:111: enum > AccessibilityFilterType { > Mark this [Extensible] as we can extend this later. > Done. > https://codereview.chromium.org/2640123004/diff/360001/components/arc/common/... > components/arc/common/accessibility_helper.mojom:135: > OnAccessibilityEvent@0(AccessibilityEventData eventData); > We cannot simply change the arguments of existing method. You need to do > the following things. > > 1. Deprecate the current method. Rename OnAccessibilityEvent@0 to > OnAccessibilityEventDeperecated@0. > 2. Add new method with the new interface. Add OnAccessibilityEvent@1. > 3. Keep the OnAccessibilityEventDeperecated@0 work correctly. Simply > proxying to the new interface is okay. > Done. > GetFileSizeDeprecated in intent_helper.mojom will be an example. > https://cs.chromium.org/chromium/src/components/arc/common/intent_helper.mojo... > > https://codereview.chromium.org/2640123004/ -- -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm This looks great. Still hoping for a cleaner solution to associating accessibility events with exo shell windows in the future but I agree we should land this and try to improve it later as needed. This is a great first step. https://codereview.chromium.org/2640123004/diff/380001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc (right): https://codereview.chromium.org/2640123004/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:210: if (GetStringProperty(node, arc::mojom::AccessibilityStringProperty::TEXT, nit: make this shorter by adding "using" at the top, like: using AXStringProperty = arc::mojom::AccessibilityStringProperty;
mostly lgtm, just a few comments. https://codereview.chromium.org/2640123004/diff/380001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc (right): https://codereview.chromium.org/2640123004/diff/380001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:236: current_tree_serializer_.reset(new AXTreeArcSerializer(this)); Do we need to reset root_id_ and focused_node_id_ here? https://codereview.chromium.org/2640123004/diff/380001/components/arc/common/... File components/arc/common/accessibility_helper.mojom (right): https://codereview.chromium.org/2640123004/diff/380001/components/arc/common/... components/arc/common/accessibility_helper.mojom:141: [MinVersion=1]map<AccessibilityBooleanProperty, int32>? intProperties; AccessibilityIntProperty
FYI. I removed the window property changes in favor of the original approach of adding a child view that points to the child ax tree. It turns out that ShellSurface loses focus when you begin touching. We don't want to override that behavior in general and it is far cleaner to do it this way. dmazzoni@chromium.org writes: > lgtm > > This looks great. Still hoping for a cleaner solution > to associating accessibility events with exo shell > windows in the future but I agree we should land this > and try to improve it later as needed. This is a great > first step. > > > > https://codereview.chromium.org/2640123004/diff/380001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc > (right): > > https://codereview.chromium.org/2640123004/diff/380001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:210: if > (GetStringProperty(node, arc::mojom::AccessibilityStringProperty::TEXT, > nit: make this shorter by adding "using" at the top, like: > > using AXStringProperty = arc::mojom::AccessibilityStringProperty; Sure, done. > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
From: David Tseng <dtseng@google.com> Date: Tue, 14 Feb 2017 12:49:07 -0800 Message-ID: <xq5dshng33cs.fsf@google.com> MIME-Version: 1.0 Content-Type: text/plain --text follows this line-- yawano@chromium.org writes: > mostly lgtm, just a few comments. > > > https://codereview.chromium.org/2640123004/diff/380001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc > (right): > > https://codereview.chromium.org/2640123004/diff/380001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:236: > current_tree_serializer_.reset(new AXTreeArcSerializer(this)); > Do we need to reset root_id_ and focused_node_id_ here? > Yup; and in the NotifyAccessibilityEvent path. > https://codereview.chromium.org/2640123004/diff/380001/components/arc/common/... > File components/arc/common/accessibility_helper.mojom (right): > > https://codereview.chromium.org/2640123004/diff/380001/components/arc/common/... > components/arc/common/accessibility_helper.mojom:141: > [MinVersion=1]map<AccessibilityBooleanProperty, int32>? intProperties; > AccessibilityIntProperty > Good catch; done. > https://codereview.chromium.org/2640123004/ -- -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
dtseng@chromium.org changed reviewers: + nasko@chromium.org, oshima@chromium.org - reveman@chromium.org
+ oshima for chrome/browser/chromeos/ + nasko for content/* chromecomponents/arc/common/
The CQ bit was checked by dtseng@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-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by dtseng@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 checked by dtseng@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/2640123004/diff/460001/components/arc/common/... File components/arc/common/accessibility_helper.mojom (right): https://codereview.chromium.org/2640123004/diff/460001/components/arc/common/... components/arc/common/accessibility_helper.mojom:21: VIEW_SELECTED, The ordering doesn't seem to quite match the one in the AccessibilityEvent class defined in AccessibilityEvent.java. https://codereview.chromium.org/2640123004/diff/460001/components/arc/common/... components/arc/common/accessibility_helper.mojom:104: enum AccessibilityStringProperty { Are these enums expected to not be extensible in the future? https://codereview.chromium.org/2640123004/diff/460001/components/arc/common/... components/arc/common/accessibility_helper.mojom:145: // Filters the event type (and implicitly the data) sent by the ARC accessibility service. Please keep comment lines to 80 characters. https://codereview.chromium.org/2640123004/diff/460001/components/arc/common/... components/arc/common/accessibility_helper.mojom:170: AccessibilityNodeInfoData? eventSource); The parameters need to be aligned vertically.
https://codereview.chromium.org/2640123004/diff/460001/components/arc/common/... File components/arc/common/accessibility_helper.mojom (right): https://codereview.chromium.org/2640123004/diff/460001/components/arc/common/... components/arc/common/accessibility_helper.mojom:21: VIEW_SELECTED, On 2017/02/15 19:49:40, nasko wrote: > The ordering doesn't seem to quite match the one in the AccessibilityEvent class > defined in AccessibilityEvent.java. Good catch. Matches now except for VIEW_FOCUSED. https://codereview.chromium.org/2640123004/diff/460001/components/arc/common/... components/arc/common/accessibility_helper.mojom:104: enum AccessibilityStringProperty { On 2017/02/15 19:49:40, nasko wrote: > Are these enums expected to not be extensible in the future? Made extensible. https://codereview.chromium.org/2640123004/diff/460001/components/arc/common/... components/arc/common/accessibility_helper.mojom:170: AccessibilityNodeInfoData? eventSource); On 2017/02/15 19:49:40, nasko wrote: > The parameters need to be aligned vertically. Done.
oshima@chromium.org changed reviewers: + penghuang@chromium.org
https://codereview.chromium.org/2640123004/diff/480001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h (right): https://codereview.chromium.org/2640123004/diff/480001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h:13: #include "components/exo/wm_helper.h" Hmm, I think you're not supposed to use this outside of exo. +peng in case I'm wrong. If you simply want to get activation event, you can use ui/wm/public/activation_client.h
LGTM
https://codereview.chromium.org/2640123004/diff/480001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h (right): https://codereview.chromium.org/2640123004/diff/480001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h:13: #include "components/exo/wm_helper.h" On 2017/02/16 04:16:45, oshima wrote: > Hmm, I think you're not supposed to use this outside of exo. > > +peng in case I'm wrong. > > If you simply want to get activation event, you can use > > ui/wm/public/activation_client.h > It's also being used to get the focused window.
https://codereview.chromium.org/2640123004/diff/480001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h (right): https://codereview.chromium.org/2640123004/diff/480001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h:13: #include "components/exo/wm_helper.h" On 2017/02/16 04:16:45, oshima wrote: > Hmm, I think you're not supposed to use this outside of exo. > > +peng in case I'm wrong. > > If you simply want to get activation event, you can use > > ui/wm/public/activation_client.h > I added WMHelper to avoid using ash code in exo directly. It is because the exo and ash will live in different processes for mus+ash. If AAHB can archive the functionality without using ash and WMHelper, it will be great. Otherwise, probably it is okay to use WMHelper, or refactor WMHelper out to a common place which can be used by both here and exo. Thought?
https://codereview.chromium.org/2640123004/diff/480001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h (right): https://codereview.chromium.org/2640123004/diff/480001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h:13: #include "components/exo/wm_helper.h" On 2017/02/16 18:51:04, Peng wrote: > On 2017/02/16 04:16:45, oshima wrote: > > Hmm, I think you're not supposed to use this outside of exo. > > > > +peng in case I'm wrong. > > > > If you simply want to get activation event, you can use > > > > ui/wm/public/activation_client.h > > > > I added WMHelper to avoid using ash code in exo directly. It is because the exo > and ash will live in different processes for mus+ash. If AAHB can archive the > functionality without using ash and WMHelper, it will be great. Otherwise, > probably it is okay to use WMHelper, or refactor WMHelper out to a common place > which can be used by both here and exo. Thought? For getting the focused window, we need ash (Shell::GetPrimaryRootWindow) to get the focus client. It sounds like doing this directly in AAHB won't be sufficient for mus+ash.
On 2017/02/16 19:14:38, David Tseng wrote: > https://codereview.chromium.org/2640123004/diff/480001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h > (right): > > https://codereview.chromium.org/2640123004/diff/480001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h:13: > #include "components/exo/wm_helper.h" > On 2017/02/16 18:51:04, Peng wrote: > > On 2017/02/16 04:16:45, oshima wrote: > > > Hmm, I think you're not supposed to use this outside of exo. > > > > > > +peng in case I'm wrong. > > > > > > If you simply want to get activation event, you can use > > > > > > ui/wm/public/activation_client.h > > > > > > > I added WMHelper to avoid using ash code in exo directly. It is because the > exo > > and ash will live in different processes for mus+ash. If AAHB can archive the > > functionality without using ash and WMHelper, it will be great. Otherwise, > > probably it is okay to use WMHelper, or refactor WMHelper out to a common > place > > which can be used by both here and exo. Thought? > > For getting the focused window, we need ash (Shell::GetPrimaryRootWindow) to get > the focus client. It sounds like doing this directly in AAHB won't be sufficient > for mus+ash. You don't need primary root window. You can you any root window.
On 2017/02/16 22:49:19, oshima wrote: > On 2017/02/16 19:14:38, David Tseng wrote: > > > https://codereview.chromium.org/2640123004/diff/480001/chrome/browser/chromeo... > > File > chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h > > (right): > > > > > https://codereview.chromium.org/2640123004/diff/480001/chrome/browser/chromeo... > > > chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h:13: > > #include "components/exo/wm_helper.h" > > On 2017/02/16 18:51:04, Peng wrote: > > > On 2017/02/16 04:16:45, oshima wrote: > > > > Hmm, I think you're not supposed to use this outside of exo. > > > > > > > > +peng in case I'm wrong. > > > > > > > > If you simply want to get activation event, you can use > > > > > > > > ui/wm/public/activation_client.h > > > > > > > > > > I added WMHelper to avoid using ash code in exo directly. It is because the > > exo > > > and ash will live in different processes for mus+ash. If AAHB can archive > the > > > functionality without using ash and WMHelper, it will be great. Otherwise, > > > probably it is okay to use WMHelper, or refactor WMHelper out to a common > > place > > > which can be used by both here and exo. Thought? > > > > For getting the focused window, we need ash (Shell::GetPrimaryRootWindow) to > get > > the focus client. It sounds like doing this directly in AAHB won't be > sufficient > > for mus+ash. > > You don't need primary root window. You can you any root window. Sorry, I meant: you can use any root window.
On 2017/02/16 22:49:52, oshima wrote: > On 2017/02/16 22:49:19, oshima wrote: > > On 2017/02/16 19:14:38, David Tseng wrote: > > > > > > https://codereview.chromium.org/2640123004/diff/480001/chrome/browser/chromeo... > > > File > > chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h > > > (right): > > > > > > > > > https://codereview.chromium.org/2640123004/diff/480001/chrome/browser/chromeo... > > > > > > chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h:13: > > > #include "components/exo/wm_helper.h" > > > On 2017/02/16 18:51:04, Peng wrote: > > > > On 2017/02/16 04:16:45, oshima wrote: > > > > > Hmm, I think you're not supposed to use this outside of exo. > > > > > > > > > > +peng in case I'm wrong. > > > > > > > > > > If you simply want to get activation event, you can use > > > > > > > > > > ui/wm/public/activation_client.h > > > > > > > > > > > > > I added WMHelper to avoid using ash code in exo directly. It is because > the > > > exo > > > > and ash will live in different processes for mus+ash. If AAHB can archive > > the > > > > functionality without using ash and WMHelper, it will be great. Otherwise, > > > > probably it is okay to use WMHelper, or refactor WMHelper out to a common > > > place > > > > which can be used by both here and exo. Thought? > > > > > > For getting the focused window, we need ash (Shell::GetPrimaryRootWindow) to > > get > > > the focus client. It sounds like doing this directly in AAHB won't be > > sufficient > > > for mus+ash. > > > > You don't need primary root window. You can you any root window. > > Sorry, I meant: > > you can use any root window. What does this mean exactly? ash::Shell manages its own ActivationClient for which I can observe activations. Focused window requires a focus client. As written, I need *both* activation and focus. So, what you are suggesting is: - add a dependency on ash (to add an observer on ash::Shell::GetInstance()->activation_client()) - perhaps keep track of the active window and ask it for its focus client and then the focus client's focused window Again, this won't be portable to us+ash. Admitedly, I don't really know the plans there, but it seems like we're basically going to need WmHelper in one form or another, right? Also, WmHelper is already being used in AAHB.cc previously.
On 2017/02/17 15:26:44, David Tseng wrote: > On 2017/02/16 22:49:52, oshima wrote: > > On 2017/02/16 22:49:19, oshima wrote: > > > On 2017/02/16 19:14:38, David Tseng wrote: > > > > > > > > > > https://codereview.chromium.org/2640123004/diff/480001/chrome/browser/chromeo... > > > > File > > > chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2640123004/diff/480001/chrome/browser/chromeo... > > > > > > > > > > chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h:13: > > > > #include "components/exo/wm_helper.h" > > > > On 2017/02/16 18:51:04, Peng wrote: > > > > > On 2017/02/16 04:16:45, oshima wrote: > > > > > > Hmm, I think you're not supposed to use this outside of exo. > > > > > > > > > > > > +peng in case I'm wrong. > > > > > > > > > > > > If you simply want to get activation event, you can use > > > > > > > > > > > > ui/wm/public/activation_client.h > > > > > > > > > > > > > > > > I added WMHelper to avoid using ash code in exo directly. It is because > > the > > > > exo > > > > > and ash will live in different processes for mus+ash. If AAHB can > archive > > > the > > > > > functionality without using ash and WMHelper, it will be great. > Otherwise, > > > > > probably it is okay to use WMHelper, or refactor WMHelper out to a > common > > > > place > > > > > which can be used by both here and exo. Thought? > > > > > > > > For getting the focused window, we need ash (Shell::GetPrimaryRootWindow) > to > > > get > > > > the focus client. It sounds like doing this directly in AAHB won't be > > > sufficient > > > > for mus+ash. > > > > > > You don't need primary root window. You can you any root window. > > > > Sorry, I meant: > > > > you can use any root window. > > What does this mean exactly? ash::Shell manages its own ActivationClient for > which I can observe activations. > Focused window requires a focus client. As written, I need *both* activation and > focus. All root windows has aura clients, including activation client. > > So, what you are suggesting is: > - add a dependency on ash (to add an observer on > ash::Shell::GetInstance()->activation_client()) That's not necessary. You can get activation client form any root window from any window from any view in chromeos. > - perhaps keep track of the active window and ask it for its focus client and > then the focus client's focused window > > Again, this won't be portable to us+ash. Admitedly, I don't really know the > plans there, but it seems like we're basically going to need WmHelper in one > form or another, right? > > Also, WmHelper is already being used in AAHB.cc previously. I think what's needed is an injection mechanism to let Arc service know context aura window, like desktop aura does. Let me look into this. In any case, chrome shouldn't depend on component/exo because chrome and exo will be in separate processes in mus. (which made me wonder. Didn't you need DEPS change?)
On 2017/02/17 16:55:48, oshima wrote: > On 2017/02/17 15:26:44, David Tseng wrote: > > On 2017/02/16 22:49:52, oshima wrote: > > > On 2017/02/16 22:49:19, oshima wrote: > > > > On 2017/02/16 19:14:38, David Tseng wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2640123004/diff/480001/chrome/browser/chromeo... > > > > > File > > > > > chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2640123004/diff/480001/chrome/browser/chromeo... > > > > > > > > > > > > > > > chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h:13: > > > > > #include "components/exo/wm_helper.h" > > > > > On 2017/02/16 18:51:04, Peng wrote: > > > > > > On 2017/02/16 04:16:45, oshima wrote: > > > > > > > Hmm, I think you're not supposed to use this outside of exo. > > > > > > > > > > > > > > +peng in case I'm wrong. > > > > > > > > > > > > > > If you simply want to get activation event, you can use > > > > > > > > > > > > > > ui/wm/public/activation_client.h > > > > > > > > > > > > > > > > > > > I added WMHelper to avoid using ash code in exo directly. It is > because > > > the > > > > > exo > > > > > > and ash will live in different processes for mus+ash. If AAHB can > > archive > > > > the > > > > > > functionality without using ash and WMHelper, it will be great. > > Otherwise, > > > > > > probably it is okay to use WMHelper, or refactor WMHelper out to a > > common > > > > > place > > > > > > which can be used by both here and exo. Thought? > > > > > > > > > > For getting the focused window, we need ash > (Shell::GetPrimaryRootWindow) > > to > > > > get > > > > > the focus client. It sounds like doing this directly in AAHB won't be > > > > sufficient > > > > > for mus+ash. > > > > > > > > You don't need primary root window. You can you any root window. > > > > > > Sorry, I meant: > > > > > > you can use any root window. > > > > What does this mean exactly? ash::Shell manages its own ActivationClient for > > which I can observe activations. > > Focused window requires a focus client. As written, I need *both* activation > and > > focus. > > All root windows has aura clients, including activation client. > > > > > So, what you are suggesting is: > > - add a dependency on ash (to add an observer on > > ash::Shell::GetInstance()->activation_client()) > > That's not necessary. You can get activation client form any root window from > any > window from any view in chromeos. > > > - perhaps keep track of the active window and ask it for its focus client and > > then the focus client's focused window > > > > Again, this won't be portable to us+ash. Admitedly, I don't really know the > > plans there, but it seems like we're basically going to need WmHelper in one > > form or another, right? > > > > Also, WmHelper is already being used in AAHB.cc previously. > > I think what's needed is an injection mechanism to let Arc service know context > aura window, > like desktop aura does. Let me look into this. > > In any case, chrome shouldn't depend on component/exo because chrome and exo > will be in > separate processes in mus. (which made me wonder. Didn't you need DEPS change?) I think it already does, right? It's already being used previous to my patch and in other components, so I'm pretty sure what you're stating, though a goal, is not the current state of things. Anyway, PTAL. I moved wm helper (the interface definition) into ui/chromeos. I think that's fine for now, but am open to putting it elsewhere in ui/.
The CQ bit was checked by dtseng@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-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dtseng@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dtseng@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: linux_chromium_chromeos_ozone_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 dtseng@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...
Patchset #30 (id:600001) has been deleted
Patchset #29 (id:580001) has been deleted
Patchset #28 (id:560001) has been deleted
Patchset #27 (id:540001) has been deleted
Patchset #26 (id:520001) has been deleted
per offline chat, lgtm'ing this CL because it already depends on exo/wm_helper. We should remove the dependency because it's not necessary, but in a separate CL.
The CQ bit was checked by dtseng@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by dtseng@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: linux_chromium_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 dtseng@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 dtseng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org, yawano@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2640123004/#ps620001 (title: "Rebase.")
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": 620001, "attempt_start_ts": 1487772055052800, "parent_rev": "ee0c03ddb4a10c8c9d14244bdefc8034054e846e", "commit_rev": "be42343c9fd8b9fbb5733faf2e93abc3ff648b7f"}
Message was sent while issue was closed.
Description was changed from ========== Initial support for native accessibility in ARC This is an initial implementation of native ARC++ accessibility. An accessibility tree is forwarded to Chrome and Chrome forwards tree data to extensions. TEST=manual - in ChromeVox, in a js-background page context, verify the contents of the accessibility tree from ARC show up; when events occur, verify events are received; when the ARC+++ surface is deactivated, verify tree gets destroyed/deatched - touch explore with ChromeVox; verify basic speech feedback - ChromeVox object navigation works BUG=683396 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Initial support for native accessibility in ARC This is an initial implementation of native ARC++ accessibility. An accessibility tree is forwarded to Chrome and Chrome forwards tree data to extensions. TEST=manual - in ChromeVox, in a js-background page context, verify the contents of the accessibility tree from ARC show up; when events occur, verify events are received; when the ARC+++ surface is deactivated, verify tree gets destroyed/deatched - touch explore with ChromeVox; verify basic speech feedback - ChromeVox object navigation works BUG=683396 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2640123004 Cr-Commit-Position: refs/heads/master@{#452029} Committed: https://chromium.googlesource.com/chromium/src/+/be42343c9fd8b9fbb5733faf2e93... ==========
Message was sent while issue was closed.
Committed patchset #26 (id:620001) as https://chromium.googlesource.com/chromium/src/+/be42343c9fd8b9fbb5733faf2e93... |