Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2826423003:
Expand Chrome OS ARC support to create one tree source per package (Closed)
|
Created:
3 years, 8 months ago by David Tseng Modified:
3 years, 7 months ago CC:
Aaron Boodman, abarth-chromium, aboxhall+watch_chromium.org, chromium-reviews, darin (slow to review), davemoore+watch_chromium.org, dmazzoni+watch_chromium.org, dougt+watch_chromium.org, dtseng+watch_chromium.org, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, je_julie, lhchavez+watch_chromium.org, nektar+watch_chromium.org, oshima+watch_chromium.org, qsr+mojo_chromium.org, victorhsieh+watch_chromium.org, viettrungluu+watch_chromium.org, yusukes+watch_chromium.org, yuzo+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionExpand Chrome OS ARC support to create one tree source per package
Chrome OS ARC support uses an AXTreeSourceArc to track the contents of the Android accessibility tree. Previously, this sufficed since we didn't need to understand which app/package we were currently in inside of Chrome.
However, in order to support whitelisting specific packages, we do need to know which packages pertain to which trees in Chrome.
We now keep a collection of trees, keyed by their task ids. Task ids are obtained by keeping a mapping from package name.
Since every AccessibilityEvent is associated with a package name (via its AccessibilityNodeInfo data), we can route events correctly to the correct package -> task id -> tree source.
BUG=708272
TEST=add --enable-chromevox-arc-support from flags. Switch between multiple ARC apps and verify instant access to tree contents.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2826423003
Cr-Commit-Position: refs/heads/master@{#471213}
Committed: https://chromium.googlesource.com/chromium/src/+/87ccf7a1cd3a9cfbc8336695f41c962929c991aa
Patch Set 1 #
Total comments: 20
Patch Set 2 : One tree per app. #
Total comments: 21
Patch Set 3 : Remove logging. #
Total comments: 8
Patch Set 4 : Mojo additions. #Patch Set 5 : Address all comments. #Patch Set 6 : 1->n mapping from package name to task ids. #
Total comments: 27
Patch Set 7 : Nits and more. #Patch Set 8 : Remove mojo change. #Patch Set 9 : Remove activation observer. #
Total comments: 8
Patch Set 10 : Add test; address nits/comments. #Patch Set 11 : Fix presubmit warnings. #Patch Set 12 : Test for tree source management. #
Total comments: 19
Patch Set 13 : Final touches/nits. #
Total comments: 11
Patch Set 14 : Address comments. #Patch Set 15 : Changes to wm_helper.cc or test fix. #
Total comments: 2
Patch Set 16 : Better fix #
Total comments: 1
Patch Set 17 : Fix typo. #Patch Set 18 : Fake out WMHelper #Dependent Patchsets: Messages
Total messages: 97 (58 generated)
dtseng@chromium.org changed reviewers: + yawano@chromium.org
PTAL. This side of the change is basically complete.
Description was changed from ========== Add support for whitelisting specific package names for Chrome OS ARC++ accessibility support BUG= ========== to ========== Add support for whitelisting specific package names for Chrome OS ARC++ accessibility support In ARC++, accessibility is either provided through Android or Chrome OS. The filter type determines which of the two is responsbielf ro accessibility given the currently active app. Active is defined as the app that last send an accessibility event. AccessibilityFilterType::OFF: either AccessibilityFilterType::WHITELISTED_PACKAGE_NAME: apps matching the whitelist (stored in ARC++), when active, will switch on Chrome OS accessibility support and off Android accessibility support. When the active app does not match, the possite occurs. Currently, only Talkback gets enabled as an accessibility service. AccessibilityFilterType::ALL: Chrome OS accessibility services run for all apps. BUG=708272 ==========
Description was changed from ========== Add support for whitelisting specific package names for Chrome OS ARC++ accessibility support In ARC++, accessibility is either provided through Android or Chrome OS. The filter type determines which of the two is responsbielf ro accessibility given the currently active app. Active is defined as the app that last send an accessibility event. AccessibilityFilterType::OFF: either AccessibilityFilterType::WHITELISTED_PACKAGE_NAME: apps matching the whitelist (stored in ARC++), when active, will switch on Chrome OS accessibility support and off Android accessibility support. When the active app does not match, the possite occurs. Currently, only Talkback gets enabled as an accessibility service. AccessibilityFilterType::ALL: Chrome OS accessibility services run for all apps. BUG=708272 ========== to ========== Add support for whitelisting specific package names for Chrome OS ARC++ accessibility support In ARC++, accessibility is either provided through Android or Chrome OS. The filter type determines which of the two is responsible for accessibility given the currently active app. Active is defined as the app that last send an accessibility event. AccessibilityFilterType::OFF: neither AccessibilityFilterType::WHITELISTED_PACKAGE_NAME: active apps matching the whitelist (stored in ARC++), will switch on Chrome OS accessibility support and off Android accessibility support. When the active app does not match, the opposite occurs. Currently, only Talkback gets toggled as an Android accessibility service. AccessibilityFilterType::ALL: Chrome OS accessibility services run for all apps and Android accessibility services are turned off. BUG=708272 ==========
https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:151: if (!tree_source_ && How about to write this function as switch (filter_type) { case ALL: case WHITELISTED_PACKAGE_NAME: // Reset tree_source_ // NotifyAccessibilityEvent break; case FOCUS: // DispatchFocusChange } ? https://codereview.chromium.org/2826423003/diff/1/components/arc/common/acces... File components/arc/common/accessibility_helper.mojom (right): https://codereview.chromium.org/2826423003/diff/1/components/arc/common/acces... components/arc/common/accessibility_helper.mojom:162: WHITELISTED_PACKAGE_NAME small nit: WHITELISTED_PACKAGE_NAMES?
hidehiko@chromium.org changed reviewers: + hidehiko@chromium.org
Drive-by. FYI. https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:136: break; nit: how about commenting // Do nothing. to explicitly say this is intentional. https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:171: tree_source_.reset(nullptr); nit: s/reset(nullptr)/reset()/ https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h (right): https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h:17: namespace { Please do not declare anonymous namespace in header. You can use nested class, instead.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
more drive-by https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:87: chromeos::switches::kEnableChromeVoxArcSupport)) nit: you cannot elide braces on this block since the condition spans multiple lines. https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:126: case arc::mojom::AccessibilityFilterType::ALL: Consider adding // fallthrough on the cases where you want to fall through. https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h (right): https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h:17: namespace { On 2017/04/21 06:27:02, hidehiko wrote: > Please do not declare anonymous namespace in header. > > You can use nested class, instead. If you don't want to use nested class, you can call |static_cast<FocusStealer*>(focus_stealer_.get())| in the .cc https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc (right): https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc:501: SendBoolPrefSettingsBroadcast( Do you need to disable the accessibility helper unconditionally? What will happen when a user that had it enabled updates to this version? Wouldn't both be enabled? https://codereview.chromium.org/2826423003/diff/1/components/arc/common/acces... File components/arc/common/accessibility_helper.mojom (right): https://codereview.chromium.org/2826423003/diff/1/components/arc/common/acces... components/arc/common/accessibility_helper.mojom:5: // Next MinVersion: 2 Next MinVersion: 3 https://codereview.chromium.org/2826423003/diff/1/components/arc/common/acces... components/arc/common/accessibility_helper.mojom:185: OnAccessibilityTreeDestroyed@2(); [MinVersion=2] OnAccessibilityTreeDestroyed@2();
Description was changed from ========== Add support for whitelisting specific package names for Chrome OS ARC++ accessibility support In ARC++, accessibility is either provided through Android or Chrome OS. The filter type determines which of the two is responsible for accessibility given the currently active app. Active is defined as the app that last send an accessibility event. AccessibilityFilterType::OFF: neither AccessibilityFilterType::WHITELISTED_PACKAGE_NAME: active apps matching the whitelist (stored in ARC++), will switch on Chrome OS accessibility support and off Android accessibility support. When the active app does not match, the opposite occurs. Currently, only Talkback gets toggled as an Android accessibility service. AccessibilityFilterType::ALL: Chrome OS accessibility services run for all apps and Android accessibility services are turned off. BUG=708272 ========== to ========== Add support for whitelisting specific package names for Chrome OS ARC++ accessibility support In ARC++, accessibility is either provided through Android or Chrome OS. The filter type determines which of the two is responsible for accessibility given the currently active app. Active is defined as the app that last send an accessibility event. AccessibilityFilterType::OFF: neither AccessibilityFilterType::WHITELISTED_PACKAGE_NAME: active apps matching the whitelist (stored in ARC++), will switch on Chrome OS accessibility support and off Android accessibility support. When the active app does not match, the opposite occurs. Currently, only Talkback gets toggled as an Android accessibility service. AccessibilityFilterType::ALL: Chrome OS accessibility services run for all apps and Android accessibility services are turned off. BUG=708272 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dtseng@chromium.org changed reviewers: + dmazzoni@chromium.org
PTAL. This is a fairly different architectural change. We got away with keeping one tree source in the Chrome end when we don't care about which tree is currently focused. However, in order to properly support whitelisting packages, we need to *not* focus apps that are not pushing data. Since the Chrome side had no way to sync up with the Android accessibility tree previously, this was subject to race conditions. For example, window activations could come before accessibility events from the previous app. This caused us to flip flop between the two apps unpredictably when whitelisting is in play. Solution is to dig through the task id and keep a map from package name to task id on creation of a task. When a task goes foreground, we keep that task id as state. Whenever an accessibility event comes through, we look for a mapping from package name (in the accessibility event), to a task id. If found, we create a new tree for that task id if needed. When window activations occur, we only focus the window if we have a task id to tree mapping for it. Thus, the Android end should only send events for apps that are whitelisted. Finally, when a task gets destroyed, we delete the tree source and remove the task id mapping. Note that each tree source now gets its own FocusStealer. The FocusStealer is still needed to ensure we only place focus on whitelisted apps and to point a client to the ARC child tree via id. Each tree source is also now its own AXHostDelegate ensuring it has a unique AXTreeID.
Took a quick look. I'll take a deeper look tomorrow. Thank you! https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:207: package_name_to_task_id_[package_name] = task_id; An app can open multiple activities. One example is Android Gmail app. If you tap compose, it opens another activity with the same package name. Are we assigning same task id for that case? Or are we assigning different task ids in that case? https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js (right): https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:331: nit: unnecessary blank line https://codereview.chromium.org/2826423003/diff/20001/components/arc/common/a... File components/arc/common/accessibility_helper.mojom (right): https://codereview.chromium.org/2826423003/diff/20001/components/arc/common/a... components/arc/common/accessibility_helper.mojom:174: // Next method ID: 3 nit: Next method ID: 2 https://codereview.chromium.org/2826423003/diff/20001/ui/views/focus/focus_ma... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/2826423003/diff/20001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:319: LOG(ERROR) << "equal focus!"; nit: Please remove logs which you have added for debug. Same with view.cc.
https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h (right): https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h:72: std::map<int32_t, AXTreeSourceArc*> task_id_to_tree_; consider using std::map<int32_t, std::unique_ptr<AXTreeSourceArc>> instead. https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc (right): https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:42: int32_t id_; nit: const int32_t id_; https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h (right): https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h:90: Delegate* delegate_; nit: Delegate* const delegate_; Also mention what the expected lifetime of |delegate_| will be. https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h:91: std::unique_ptr<FocusStealer> focus_stealer_; nit: As hidehiko@ mentioned previously, please make FocusStealer an internal class to avoid fwd-declaring stuff in the anonymous namespace in the header file. https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc (right): https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc:501: SendBoolPrefSettingsBroadcast( Do you need to disable the accessibility helper unconditionally? What will happen when a user that had it enabled updates to this version? Wouldn't both be enabled?
Nice work on this. Keeping track of the task ids and package names sounds fantastic. Maybe update the change description a bit? https://codereview.chromium.org/2826423003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:33: return -1; how about kNoTaskId? https://codereview.chromium.org/2826423003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:129: if (event_data->nodeData.size()) { nit: early-exit if the size is zero instead, to avoid nesting https://codereview.chromium.org/2826423003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:207: package_name_to_task_id_[package_name] = task_id; DCHECK that the package name doesn't already have a different task id? Is that possible, do we know for sure it's a 1:1 mapping? https://codereview.chromium.org/2826423003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc (right): https://codereview.chromium.org/2826423003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:31: parent()->NotifyAccessibilityEvent(ui::AX_EVENT_CHILDREN_CHANGED, false); Why children changed? Also, why false? I thought that the second argument to NotifyAccessibilityEvent was nearly deprecated now, it can always be true
https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:207: package_name_to_task_id_[package_name] = task_id; On 2017/04/25 09:29:23, yawano wrote: > An app can open multiple activities. One example is Android Gmail app. If you > tap compose, it opens another activity with the same package name. Are we > assigning same task id for that case? Or are we assigning different task ids in > that case? Yes, this was a concern of mine as well so I pushed this patchset out for review earlier than later. If I log OnTaskCreated, I only ever see the startup activity in Inbox. This is after tapping on compose. I tried this for several other apps, navigating through several screens (with a navigate up button). However, if you would like to verify this, would appreciate it. If we do indeed need Activity name, it is somewhat knknown how we can support this. The root problem we would need to solve is an AccessibilityEvent is only associated with a package name and not an activity. Trying to get the activity name is unfortunately just as racey as anything else.
https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:87: chromeos::switches::kEnableChromeVoxArcSupport)) On 2017/04/21 15:20:07, Luis Héctor Chávez wrote: > nit: you cannot elide braces on this block since the condition spans multiple > lines. Done (though note that this seems somewhat ambiguous since you see both styles in the codebase). https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:126: case arc::mojom::AccessibilityFilterType::ALL: On 2017/04/21 15:20:07, Luis Héctor Chávez wrote: > Consider adding > > // fallthrough > > on the cases where you want to fall through. Acknowledged. https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:136: break; On 2017/04/21 06:27:02, hidehiko wrote: > nit: how about commenting > // Do nothing. > > to explicitly say this is intentional. Acknowledged. https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:151: if (!tree_source_ && On 2017/04/21 04:26:36, yawano wrote: > How about to write this function as > > switch (filter_type) { > case ALL: > case WHITELISTED_PACKAGE_NAME: > // Reset tree_source_ > // NotifyAccessibilityEvent > break; > case FOCUS: > // DispatchFocusChange > } > > ? Acknowledged. https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:171: tree_source_.reset(nullptr); On 2017/04/21 06:27:02, hidehiko wrote: > nit: s/reset(nullptr)/reset()/ Acknowledged. https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc (right): https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc:501: SendBoolPrefSettingsBroadcast( On 2017/04/21 15:20:07, Luis Héctor Chávez wrote: > Do you need to disable the accessibility helper unconditionally? What will > happen when a user that had it enabled updates to this version? Wouldn't both be > enabled? Good question. With this change, and the associated ARC change, spoken feedback pref enables/disables accessibility helper. Accessibility helper then enables/disables Talkback. For example, if a user enables spoken feedback, then the spoken feedback pref gets synced. On the Android side, we get a filter type of whitelisted package name. This causes the accessibility helper service to enable/disable Talkback for specific package names. https://codereview.chromium.org/2826423003/diff/1/components/arc/common/acces... File components/arc/common/accessibility_helper.mojom (right): https://codereview.chromium.org/2826423003/diff/1/components/arc/common/acces... components/arc/common/accessibility_helper.mojom:162: WHITELISTED_PACKAGE_NAME On 2017/04/21 04:26:36, yawano wrote: > small nit: WHITELISTED_PACKAGE_NAMES? I thought about that but the others are singular. https://codereview.chromium.org/2826423003/diff/1/components/arc/common/acces... components/arc/common/accessibility_helper.mojom:185: OnAccessibilityTreeDestroyed@2(); On 2017/04/21 15:20:07, Luis Héctor Chávez wrote: > [MinVersion=2] OnAccessibilityTreeDestroyed@2(); Acknowledged.
Description was changed from ========== Add support for whitelisting specific package names for Chrome OS ARC++ accessibility support In ARC++, accessibility is either provided through Android or Chrome OS. The filter type determines which of the two is responsible for accessibility given the currently active app. Active is defined as the app that last send an accessibility event. AccessibilityFilterType::OFF: neither AccessibilityFilterType::WHITELISTED_PACKAGE_NAME: active apps matching the whitelist (stored in ARC++), will switch on Chrome OS accessibility support and off Android accessibility support. When the active app does not match, the opposite occurs. Currently, only Talkback gets toggled as an Android accessibility service. AccessibilityFilterType::ALL: Chrome OS accessibility services run for all apps and Android accessibility services are turned off. BUG=708272 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Expand Chrome OS ARC support to create one tree source per package Chrome OS ARC support uses an AXTreeSourceArc to track the contents of the Android accessibility tree. Previously, this sufficed since we didn't need to understand which app/package we were currently in inside of Chrome. However, in order to support whitelisting specific packages, we do need to know which packages pertain to which trees in Chrome. We now keep a a collectin of trees, keyed by their task ids. Task ids are obtained by keeping a mapping from package name. Since every AccessibilityEvent is associated with a package name (via its AccessibilityNodeInfo data), we can route events correctly to the correct package -> task id -> tree source. BUG=708272 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Expand Chrome OS ARC support to create one tree source per package Chrome OS ARC support uses an AXTreeSourceArc to track the contents of the Android accessibility tree. Previously, this sufficed since we didn't need to understand which app/package we were currently in inside of Chrome. However, in order to support whitelisting specific packages, we do need to know which packages pertain to which trees in Chrome. We now keep a a collectin of trees, keyed by their task ids. Task ids are obtained by keeping a mapping from package name. Since every AccessibilityEvent is associated with a package name (via its AccessibilityNodeInfo data), we can route events correctly to the correct package -> task id -> tree source. BUG=708272 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Expand Chrome OS ARC support to create one tree source per package Chrome OS ARC support uses an AXTreeSourceArc to track the contents of the Android accessibility tree. Previously, this sufficed since we didn't need to understand which app/package we were currently in inside of Chrome. However, in order to support whitelisting specific packages, we do need to know which packages pertain to which trees in Chrome. We now keep a a collectin of trees, keyed by their task ids. Task ids are obtained by keeping a mapping from package name. Since every AccessibilityEvent is associated with a package name (via its AccessibilityNodeInfo data), we can route events correctly to the correct package -> task id -> tree source. BUG=708272 TEST=add --enable-chromevox-arc-support from flags. Switch between multiple ARC apps and verify instant access to tree contents. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Expand Chrome OS ARC support to create one tree source per package Chrome OS ARC support uses an AXTreeSourceArc to track the contents of the Android accessibility tree. Previously, this sufficed since we didn't need to understand which app/package we were currently in inside of Chrome. However, in order to support whitelisting specific packages, we do need to know which packages pertain to which trees in Chrome. We now keep a a collectin of trees, keyed by their task ids. Task ids are obtained by keeping a mapping from package name. Since every AccessibilityEvent is associated with a package name (via its AccessibilityNodeInfo data), we can route events correctly to the correct package -> task id -> tree source. BUG=708272 TEST=add --enable-chromevox-arc-support from flags. Switch between multiple ARC apps and verify instant access to tree contents. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Expand Chrome OS ARC support to create one tree source per package Chrome OS ARC support uses an AXTreeSourceArc to track the contents of the Android accessibility tree. Previously, this sufficed since we didn't need to understand which app/package we were currently in inside of Chrome. However, in order to support whitelisting specific packages, we do need to know which packages pertain to which trees in Chrome. We now keep a collection of trees, keyed by their task ids. Task ids are obtained by keeping a mapping from package name. Since every AccessibilityEvent is associated with a package name (via its AccessibilityNodeInfo data), we can route events correctly to the correct package -> task id -> tree source. BUG=708272 TEST=add --enable-chromevox-arc-support from flags. Switch between multiple ARC apps and verify instant access to tree contents. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h (right): https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h:72: std::map<int32_t, AXTreeSourceArc*> task_id_to_tree_; On 2017/04/25 15:27:00, Luis Héctor Chávez wrote: > consider using std::map<int32_t, std::unique_ptr<AXTreeSourceArc>> instead. If we trust that ArcAppListPrefs::Observer works as advertised, I think it is unecessary. Also, I believe the map would still own the tree objects so the trees would not be deleted until object destruction anyway. I also added explicit deletion when this object gets destroyed. https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc (right): https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:42: int32_t id_; On 2017/04/25 15:27:00, Luis Héctor Chávez wrote: > nit: const int32_t id_; Done. https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h (right): https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h:90: Delegate* delegate_; On 2017/04/25 15:27:00, Luis Héctor Chávez wrote: > nit: Delegate* const delegate_; > > Also mention what the expected lifetime of |delegate_| will be. Done. https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h:91: std::unique_ptr<FocusStealer> focus_stealer_; On 2017/04/25 15:27:00, Luis Héctor Chávez wrote: > nit: As hidehiko@ mentioned previously, please make FocusStealer an internal > class to avoid fwd-declaring stuff in the anonymous namespace in the header > file. Forward declared as a private nested class. https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js (right): https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:331: On 2017/04/25 09:29:23, yawano wrote: > nit: unnecessary blank line Done. https://codereview.chromium.org/2826423003/diff/20001/components/arc/common/a... File components/arc/common/accessibility_helper.mojom (right): https://codereview.chromium.org/2826423003/diff/20001/components/arc/common/a... components/arc/common/accessibility_helper.mojom:174: // Next method ID: 3 On 2017/04/25 09:29:23, yawano wrote: > nit: Next method ID: 2 Done. https://codereview.chromium.org/2826423003/diff/20001/ui/views/focus/focus_ma... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/2826423003/diff/20001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:319: LOG(ERROR) << "equal focus!"; On 2017/04/25 09:29:23, yawano wrote: > nit: Please remove logs which you have added for debug. Same with view.cc. Acknowledged. https://codereview.chromium.org/2826423003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:33: return -1; On 2017/04/25 16:09:24, dmazzoni wrote: > how about kNoTaskId? Done. https://codereview.chromium.org/2826423003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:129: if (event_data->nodeData.size()) { On 2017/04/25 16:09:24, dmazzoni wrote: > nit: early-exit if the size is zero instead, to avoid nesting Done. https://codereview.chromium.org/2826423003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:207: package_name_to_task_id_[package_name] = task_id; On 2017/04/25 16:09:25, dmazzoni wrote: > DCHECK that the package name doesn't already have a different task id? > Is that possible, do we know for sure it's a 1:1 mapping? It's possible for the package name / task id to be removed (OnTaskDestroyed). Added code to remove the entry given the task id. Overall, it looks like, at least for the purposes of ArcAppListPrefs::Observer, we only ever get one task id per package name. I've asked Yuki to confirm. I wasn't seeing new tasks being created when navigating to new activities within the same package though. https://codereview.chromium.org/2826423003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc (right): https://codereview.chromium.org/2826423003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:31: parent()->NotifyAccessibilityEvent(ui::AX_EVENT_CHILDREN_CHANGED, false); On 2017/04/25 16:09:25, dmazzoni wrote: > Why children changed? Also, why false? I thought that the second argument > to NotifyAccessibilityEvent was nearly deprecated now, it can always be true Was having trouble getting this to work in some prior iteration. I can remove this I think safely now after testing. There are still some focusability issues, but I think this change is doing enough.
https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h (right): https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h:72: std::map<int32_t, AXTreeSourceArc*> task_id_to_tree_; On 2017/04/25 22:48:02, David Tseng wrote: > On 2017/04/25 15:27:00, Luis Héctor Chávez wrote: > > consider using std::map<int32_t, std::unique_ptr<AXTreeSourceArc>> instead. > > If we trust that ArcAppListPrefs::Observer works as advertised, I think it is > unecessary. Also, I believe the map would still own the tree objects so the > trees would not be deleted until object destruction anyway. > > I also added explicit deletion when this object gets destroyed. Right, the goal is to remove a bit of such trust and ensure things get destroyed properly no matter what. The rest of the codebase is slowly moving away from using raw pointers, so it's better to do so here too. re: the map owning the tree objects, you can replace any current delete foo; with a foo.reset(); (or even better, package_name_to_task_id_.erase(id);)
https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:207: package_name_to_task_id_[package_name] = task_id; I've tested this today, and different task ids are assigned to different windows from same package name. Inbox doesn't start new activity for compose. But Gmail Android client does. Also activity name won't help as a single package can start multiple same activities. On Android side, we have AccessibilityWindowInfo and it has its own id for each window. We may be able to use it for distinguishing them. But it's not always available for all events, and it will make the logic much more complicated. IIUC, we want to do this as we have problem with whitelisting. We won't switch between ChromeVox and TalkBack in the single package name. How about to make this mapping 1:n? 1 package name can have multiple task ids. e.g. If we get an accessibility event for packageA. Resolve package name packageA to set of task ids [1, 2, 3]. If current_task_id_ is contained in the set, we will consider the event for the task id. If not, we will discard it.
PTAL. Moved to mapping package names to multiple task ids. https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:207: package_name_to_task_id_[package_name] = task_id; On 2017/04/26 08:03:24, yawano wrote: > I've tested this today, and different task ids are assigned to different windows > from same package name. > > Inbox doesn't start new activity for compose. But Gmail Android client does. > > Also activity name won't help as a single package can start multiple same > activities. On Android side, we have AccessibilityWindowInfo and it has its own > id for each window. We may be able to use it for distinguishing them. But it's > not always available for all events, and it will make the logic much more > complicated. > Thanks for verifying. The window id won't help because it gets created by accessibility for accessibility. What we need is to associate the task id with an accessibility event within an app's context. The origin of the accessibility event (when it gets initially populated) should perhaps have the task id. > IIUC, we want to do this as we have problem with whitelisting. We won't switch > between ChromeVox and TalkBack in the single package name. There are several other reasons we need this. Performance is bad in the current case where we destroy the tree when you switch between apps. There is significant user delay when you alt-tab to an app and begin interacting. There is also the issue of retaining user position. If we destroy the tree, ChromeVox looses position so when you alt-tab back to an app, you end up at the top. > > How about to make this mapping 1:n? 1 package name can have multiple task ids. > > e.g. > If we get an accessibility event for packageA. > Resolve package name packageA to set of task ids [1, 2, 3]. > If current_task_id_ is contained in the set, we will consider the event for the > task id. > If not, we will discard it. I was hoping to avoid this, but I can live with the above lag when switching between the same package but different activities. https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h (right): https://codereview.chromium.org/2826423003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h:72: std::map<int32_t, AXTreeSourceArc*> task_id_to_tree_; On 2017/04/25 23:08:16, Luis Héctor Chávez wrote: > On 2017/04/25 22:48:02, David Tseng wrote: > > On 2017/04/25 15:27:00, Luis Héctor Chávez wrote: > > > consider using std::map<int32_t, std::unique_ptr<AXTreeSourceArc>> instead. > > > > If we trust that ArcAppListPrefs::Observer works as advertised, I think it is > > unecessary. Also, I believe the map would still own the tree objects so the > > trees would not be deleted until object destruction anyway. > > > > I also added explicit deletion when this object gets destroyed. > > Right, the goal is to remove a bit of such trust and ensure things get destroyed > properly no matter what. The rest of the codebase is slowly moving away from Ok, if this is the general trend of the codebase, then done.
https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc (right): https://codereview.chromium.org/2826423003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc:501: SendBoolPrefSettingsBroadcast( On 2017/04/25 18:29:48, David Tseng wrote: > On 2017/04/21 15:20:07, Luis Héctor Chávez wrote: > > Do you need to disable the accessibility helper unconditionally? What will > > happen when a user that had it enabled updates to this version? Wouldn't both > be > > enabled? > > Good question. With this change, and the associated ARC change, spoken feedback > pref enables/disables accessibility helper. Accessibility helper then > enables/disables Talkback. > > For example, if a user enables spoken feedback, then the spoken feedback pref > gets synced. On the Android side, we get a filter type of whitelisted package > name. This causes the accessibility helper service to enable/disable Talkback > for specific package names. > Acknowledged. https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:20: static int kNoTaskId = -1; nit: constexpr int32_t having something be static within the anonymous namespace is superfluous. https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:37: int task_id = kNoTaskId; nit: int32_t https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h (right): https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h:91: const Delegate* delegate_; nit: const Delegate* const delegate_; (or Delegate* const delegate_) the const after the pointer means that you won't change the address of the pointer. You're only setting |delegate_| on the ctor. If you use the second variant, you don't need to make Delegate::OnAction const (none of the other Delegates I've seen have const methods, so that would make it more consistent with the rest of the codebase).
Minor coding comments only. https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:113: arc::mojom::AccessibilityFilterType::WHITELISTED_PACKAGE_NAME) missing brace, for consistency with L67? https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:148: auto task_ids = task_ids_it->second; this copies whole set, which looks unexpected. maybe "const auto& task_ids = ..." https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:203: for (auto task_ids_it : package_name_to_task_ids_) { This copies entry (pair of key/value) for each element. "const auto&" Also, "_it" suffix implies iterator, but this is actually not. Maybe package_name_to_task_ids_entry ? Or just entry may be fine, here, considering its scope. https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:228: for (auto task_ids_it : package_name_to_task_ids_) { Ditto. https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:234: package_name_to_task_ids_.erase(task_ids_it.first); erase can take iterator. https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:243: auto it = package_name_to_tree_.find(package_name); package_name_to_tree_.erase(package_name); does what you want. You do not need manually reset() the value, because its dtor has the responsibility to delete the instance if necessary.
Mostly looks good for me. Thank you for working on this! https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:90: : ArcService(bridge_service), binding_(this), current_task_id_(0) { nit: current_task_id_(kNoTaskId) instead of 0? https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:114: exo::WMHelper::GetInstance()->AddActivationObserver(this); Remove this from activation observer in the destructor of ArcAccessibilityHelperBridge. https://codereview.chromium.org/2826423003/diff/100001/components/arc/common/... File components/arc/common/accessibility_helper.mojom (right): https://codereview.chromium.org/2826423003/diff/100001/components/arc/common/... components/arc/common/accessibility_helper.mojom:195: SetCurrentPackageNativeAccessibility@3(bool enabled); Wouldn't this cause race condition? i.e. current package might not match between Chrome and Android. How about to change this as setChromeAccessibilitySupportEnabled(bool enabled, string package_name)?
PTAL. Thanks all for working through this patch as it keeps changing. https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:20: static int kNoTaskId = -1; On 2017/04/26 15:34:59, Luis Héctor Chávez wrote: > nit: constexpr int32_t > > having something be static within the anonymous namespace is superfluous. Done. https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:37: int task_id = kNoTaskId; On 2017/04/26 15:34:59, Luis Héctor Chávez wrote: > nit: int32_t Done. https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:90: : ArcService(bridge_service), binding_(this), current_task_id_(0) { On 2017/04/27 10:01:27, yawano wrote: > nit: current_task_id_(kNoTaskId) instead of 0? Done. https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:113: arc::mojom::AccessibilityFilterType::WHITELISTED_PACKAGE_NAME) On 2017/04/26 17:13:55, hidehiko wrote: > missing brace, for consistency with L67? Done. https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:114: exo::WMHelper::GetInstance()->AddActivationObserver(this); On 2017/04/27 10:01:27, yawano wrote: > Remove this from activation observer in the destructor of > ArcAccessibilityHelperBridge. I don't think we can do this without potentially hitting a NOTREACHED. We would need to restructure the filter type to be updated rhater than computed. https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:148: auto task_ids = task_ids_it->second; On 2017/04/26 17:13:55, hidehiko wrote: > this copies whole set, which looks unexpected. > maybe "const auto& task_ids = ..." Done. https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:203: for (auto task_ids_it : package_name_to_task_ids_) { On 2017/04/26 17:13:55, hidehiko wrote: > This copies entry (pair of key/value) for each element. > "const auto&" > > Also, "_it" suffix implies iterator, but this is actually not. Maybe > package_name_to_task_ids_entry ? Or just entry may be fine, here, considering > its scope. Done. https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:228: for (auto task_ids_it : package_name_to_task_ids_) { On 2017/04/26 17:13:55, hidehiko wrote: > Ditto. Yes, partly. Definitely not an iterator, but cannot be const since I'm erasing below. Made into a ref. https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:234: package_name_to_task_ids_.erase(task_ids_it.first); On 2017/04/26 17:13:55, hidehiko wrote: > erase can take iterator. Done. https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:243: auto it = package_name_to_tree_.find(package_name); On 2017/04/26 17:13:55, hidehiko wrote: > package_name_to_tree_.erase(package_name); > > does what you want. > You do not need manually reset() the value, because its dtor has the > responsibility to delete the instance if necessary. True and removed though the subtle issue here was to only delete the tree source when there are no more task ids for a package. Imo, not making deletions more explicit here obscures the readability of the code, but I will adhere to the conventions within arc. https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h (right): https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h:91: const Delegate* delegate_; On 2017/04/26 15:34:59, Luis Héctor Chávez wrote: > nit: const Delegate* const delegate_; (or Delegate* const delegate_) > > the const after the pointer means that you won't change the address of the > pointer. You're only setting |delegate_| on the ctor. > > If you use the second variant, you don't need to make Delegate::OnAction const > (none of the other Delegates I've seen have const methods, so that would make it > more consistent with the rest of the codebase). Yup; I know what void* const means :). If we're strict here, then I guess both const specifiers apply since the delegate should never be modifying its own state (if one were to extend the methods it has), and I never actually reassign the pointer itself. https://codereview.chromium.org/2826423003/diff/100001/components/arc/common/... File components/arc/common/accessibility_helper.mojom (right): https://codereview.chromium.org/2826423003/diff/100001/components/arc/common/... components/arc/common/accessibility_helper.mojom:195: SetCurrentPackageNativeAccessibility@3(bool enabled); On 2017/04/27 10:01:27, yawano wrote: > Wouldn't this cause race condition? i.e. current package might not match between > Chrome and Android. How about to change this as > setChromeAccessibilitySupportEnabled(bool enabled, string package_name)? Yes, you're right. I was adding this to avoid another review, but since this ins't being called at all, I'll defer this to another cl, where it would be exercised by ChromeVox. It's pretty clear I should add a test suite for everything in this cl as well.
https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:114: exo::WMHelper::GetInstance()->AddActivationObserver(this); I might be missing something, but I think RemoveObserver checks whether the list contains it or not. It should be no problem to call RemoveObserver even for the case where we haven't added ArcAccessibilityHelperBridge to it. https://cs.chromium.org/chromium/src/base/observer_list.h?l=283
https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:114: exo::WMHelper::GetInstance()->AddActivationObserver(this); On 2017/04/28 11:17:52, yawano wrote: > I might be missing something, but I think RemoveObserver checks whether the list > contains it or not. It should be no problem to call RemoveObserver even for the > case where we haven't added ArcAccessibilityHelperBridge to it. > > https://cs.chromium.org/chromium/src/base/observer_list.h?l=283 No, you're right. I was thinking of the case where the same observer was added more than once. We're not so strict on removals. Done.
*/arc/* lgtm with two more nits. https://codereview.chromium.org/2826423003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:95: arc_bridge_service()->accessibility_helper()->RemoveObserver(this); nit: try to perform actions in the destructor in reverse order in which they are preform; this should go last, and exo::WMHelper::GetInstance()->RemoveActivationObserver(this); should go first. https://codereview.chromium.org/2826423003/diff/160001/components/arc/common/... File components/arc/common/accessibility_helper.mojom (right): https://codereview.chromium.org/2826423003/diff/160001/components/arc/common/... components/arc/common/accessibility_helper.mojom:162: WHITELISTED_PACKAGE_NAME ah i forgot: [MinVersion=2]. You'll also need to bump the MinVersion comment in line 5.
lgtm
Minor coding comments, only. https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:243: auto it = package_name_to_tree_.find(package_name); On 2017/04/28 00:27:43, David Tseng wrote: > On 2017/04/26 17:13:55, hidehiko wrote: > > package_name_to_tree_.erase(package_name); > > > > does what you want. > > You do not need manually reset() the value, because its dtor has the > > responsibility to delete the instance if necessary. > > True and removed though the subtle issue here was to only delete the tree source > when there are no more task ids for a package. Imo, not making deletions more > explicit here obscures the readability of the code, but I will adhere to the > conventions within arc. Which convention do you mean...? Anyway, commented on the latest PS. Could you check it, instead? https://codereview.chromium.org/2826423003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:65: chromeos::AccessibilityManager* accessibility_manager = nit: please move this to just before L72 (please Get the instance just before using it). (Sorry, I overlooked this in the last iteration). https://codereview.chromium.org/2826423003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:236: if (task_ids_entry.second.size() == 0) { IIUC; for (auto& task_ids_entry : ...) { if (task_ids_entry.second.erase(task_id) > 0) { if (task_ids_entry.second.empty()) { package_name_to_tree_.erase(task_ids_entry.first); package_name_to_task_ids_.erase(task_ids_entry.first); } break; } } is probably simpler way. Note: map::erase() returns the number of deleted elements, so you can check it here. package_name_to_tree_.erase() needs to be called before package_name_to_task_ids_.erase(), otherwise task_ids_entry can be invalidated first. To check the emptiness of a collection, I recommend to use empty(), rather than size() == 0, for consistency.
PTAL; also added a test. https://codereview.chromium.org/2826423003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:95: arc_bridge_service()->accessibility_helper()->RemoveObserver(this); On 2017/04/28 21:59:44, Luis Héctor Chávez wrote: > nit: try to perform actions in the destructor in reverse order in which they are > preform; this should go last, and > exo::WMHelper::GetInstance()->RemoveActivationObserver(this); should go first. Done. https://codereview.chromium.org/2826423003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:236: if (task_ids_entry.second.size() == 0) { On 2017/05/01 05:15:27, hidehiko wrote: > IIUC; > > for (auto& task_ids_entry : ...) { > if (task_ids_entry.second.erase(task_id) > 0) { > if (task_ids_entry.second.empty()) { > package_name_to_tree_.erase(task_ids_entry.first); > package_name_to_task_ids_.erase(task_ids_entry.first); > } > break; > } > } Yes, this is cleaner, but is it advisible to call erase on *all* of the value sets simply to get the code to look cleaner?> > is probably simpler way. Ok, I changed it to the above. > > Note: > map::erase() returns the number of deleted elements, so you can check it here. Thanks for pointing this out. > package_name_to_tree_.erase() needs to be called before > package_name_to_task_ids_.erase(), otherwise task_ids_entry can be invalidated > first. > To check the emptiness of a collection, I recommend to use empty(), rather than > size() == 0, for consistency. Acknowledged for the above (this wasn't an issue previously). https://codereview.chromium.org/2826423003/diff/160001/components/arc/common/... File components/arc/common/accessibility_helper.mojom (right): https://codereview.chromium.org/2826423003/diff/160001/components/arc/common/... components/arc/common/accessibility_helper.mojom:162: WHITELISTED_PACKAGE_NAME On 2017/04/28 21:59:44, Luis Héctor Chávez wrote: > ah i forgot: [MinVersion=2]. You'll also need to bump the MinVersion comment in > line 5. Done.
LGTM with style comments. Thank you very much for adding tests! https://codereview.chromium.org/2826423003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:236: if (task_ids_entry.second.size() == 0) { On 2017/05/05 19:59:21, David Tseng wrote: > On 2017/05/01 05:15:27, hidehiko wrote: > > IIUC; > > > > for (auto& task_ids_entry : ...) { > > if (task_ids_entry.second.erase(task_id) > 0) { > > if (task_ids_entry.second.empty()) { > > package_name_to_tree_.erase(task_ids_entry.first); > > package_name_to_task_ids_.erase(task_ids_entry.first); > > } > > break; > > } > > } > > > Yes, this is cleaner, but is it advisible to call erase on *all* of the value > sets simply to get the code to look cleaner?> > IMHO, here what is needed is removing "all" task_id from values (sets) in |package_name_to_task_ids_|. So, trying to erase() from all sets sounds reasonable (again IMHO). If a set gets empty, then it is necessary to remove the corresponding package_name's info from both package_name_to_tree_ and package_name_to_task_ids_ maps. That's my understanding, and why I recommend to do it. it looks ok to leave it as is to me, or adding brief comment about map::erase() works for me, too, if you'd like. > > is probably simpler way. > > Ok, I changed it to the above. > > > > > Note: > > map::erase() returns the number of deleted elements, so you can check it here. > > Thanks for pointing this out. > > > package_name_to_tree_.erase() needs to be called before > > package_name_to_task_ids_.erase(), otherwise task_ids_entry can be invalidated > > first. > > To check the emptiness of a collection, I recommend to use empty(), rather > than > > size() == 0, for consistency. > > Acknowledged for the above (this wasn't an issue previously). https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:98: if (exo::WMHelper::GetInstance()) nit/style: For consistency, could you wm_helper = exo::WMHelper::GetInstance(); if (wm_helper) wm_helper->RemoveActivationObserver(this); https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:107: if (arc_bridge_service()) Ditto. https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:121: if (!ArcAppListPrefs::Get(profile)) Ditto. https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge_unittest.cc (right): https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge_unittest.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. Wow, nice adding tests! https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge_unittest.cc:6: #include "base/command_line.h" Style: could you add an empty line between L5 and L6? https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge_unittest.cc:16: ArcAccessibilityHelperBridgeTest() {} style: s/{}/= default/ for consistency in ARC? https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge_unittest.cc:18: bridge_service_.reset(new ArcBridgeService()); Style/optional: bridge_service_ = base::MakeUnique<ArcBridgeServie>(); accessibility_helper_bridge_ = base::MakeUnique<ArcAHB>(bridge_service_.get()); for consistency? https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge_unittest.cc:35: ArcAccessibilityHelperBridge* ahb = accessibility_helper_bridge(); could you avoid "ahb" variable name? Recommendation: accessibility_helper_bridge or helper_bridge etc. https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge_unittest.cc:45: auto entry1 = package_to_ids.find("com.android.vending"); Actually, this is not entry but iterator? How about renaming? Or, another approach would be; ASSERT_EQ(1U, package_to_ids.size()); { const auto& entry = *pacakge_to_ids.begin(); EXPECT_EQ("com.android.vending", entry.first); EXPECT_EQ(1U, entry.second.size()); EXPECT_EQ(1U, entry.second.count(1)); } Ditto for below. Note: ASSERT_EQ is for early return, otherwise following EXPECTs will crash. map::count() returns the number of elements (0 or 1), so better to use _EQ, instead of _TRUE.
c/b/chromeos/arc/accessibility lgtm. Thank you for adding tests too! https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:101: if (chromeos::AccessibilityManager::Get()) { optional nit: maybe if we rewrite line 98 for consistency, this line too? AccessibilityManager* accessibility_manager = chromeos::AccessibilityManager::Get(); if (accessibility_manager) { Profile* profile = accessibility_manager->... }
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...
https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:98: if (exo::WMHelper::GetInstance()) On 2017/05/08 06:13:11, hidehiko wrote: > nit/style: For consistency, could you > > wm_helper = exo::WMHelper::GetInstance(); > if (wm_helper) > wm_helper->RemoveActivationObserver(this); Done. https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:101: if (chromeos::AccessibilityManager::Get()) { On 2017/05/08 11:08:53, yawano wrote: > optional nit: maybe if we rewrite line 98 for consistency, this line too? > > AccessibilityManager* accessibility_manager = > chromeos::AccessibilityManager::Get(); > if (accessibility_manager) { > Profile* profile = accessibility_manager->... > } Done. https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:107: if (arc_bridge_service()) On 2017/05/08 06:13:11, hidehiko wrote: > Ditto. Done. https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:121: if (!ArcAppListPrefs::Get(profile)) On 2017/05/08 06:13:11, hidehiko wrote: > Ditto. Done. https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge_unittest.cc (right): https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge_unittest.cc:6: #include "base/command_line.h" On 2017/05/08 06:13:11, hidehiko wrote: > Style: could you add an empty line between L5 and L6? Done. https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge_unittest.cc:16: ArcAccessibilityHelperBridgeTest() {} On 2017/05/08 06:13:11, hidehiko wrote: > style: s/{}/= default/ for consistency in ARC? Done. https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge_unittest.cc:18: bridge_service_.reset(new ArcBridgeService()); On 2017/05/08 06:13:11, hidehiko wrote: > Style/optional: > > bridge_service_ = base::MakeUnique<ArcBridgeServie>(); > accessibility_helper_bridge_ = > base::MakeUnique<ArcAHB>(bridge_service_.get()); > > for consistency? Done. https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge_unittest.cc:35: ArcAccessibilityHelperBridge* ahb = accessibility_helper_bridge(); On 2017/05/08 06:13:11, hidehiko wrote: > could you avoid "ahb" variable name? > Recommendation: accessibility_helper_bridge or helper_bridge etc. Done. https://codereview.chromium.org/2826423003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge_unittest.cc:45: auto entry1 = package_to_ids.find("com.android.vending"); On 2017/05/08 06:13:11, hidehiko wrote: > Actually, this is not entry but iterator? How about renaming? > Or, another approach would be; > > ASSERT_EQ(1U, package_to_ids.size()); > { > const auto& entry = *pacakge_to_ids.begin(); > EXPECT_EQ("com.android.vending", entry.first); > EXPECT_EQ(1U, entry.second.size()); > EXPECT_EQ(1U, entry.second.count(1)); > } > > Ditto for below. > Note: ASSERT_EQ is for early return, otherwise following EXPECTs will crash. > map::count() returns the number of elements (0 or 1), so better to use _EQ, > instead of _TRUE. Done. Also, converted everything to ASSERT_* since there's no real value in having the the test continue if any expectation fails.
dtseng@chromium.org changed reviewers: + dcheng@chromium.org
+ dcheng for c/a/common/accessibility_helper.mojom
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_...)
LGTM with comments addressed https://codereview.chromium.org/2826423003/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:160: if (event_data->nodeData.size() == 0) Nit: prefer .empty() to .size() == 0 Also, this is named incorrectly, but I'm not sure how hard that will be to fix here. In theory, it shouldn't require any special versioning... I think? https://codereview.chromium.org/2826423003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:169: if (package_it == node->stringProperties->end()) Same comment about naming here for "stringProperties" https://codereview.chromium.org/2826423003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:233: package_name = task_ids_entry.first; Seems wasteful to copy here. Maybe just save the iterator and pass .first to the lookup below https://codereview.chromium.org/2826423003/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h (right): https://codereview.chromium.org/2826423003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h:56: void OnTaskDestroyed(int task_id) override; Nit: the interface should be consistent about int32_t vs int https://codereview.chromium.org/2826423003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h:75: std::map<std::string, std::unique_ptr<AXTreeSourceArc>> package_name_to_tree_; For IWYU, please #include <map>, <string>, <set>
https://codereview.chromium.org/2826423003/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:160: if (event_data->nodeData.size() == 0) On 2017/05/10 06:36:42, dcheng wrote: > Nit: prefer .empty() to .size() == 0 > > Also, this is named incorrectly, but I'm not sure how hard that will be to fix > here. In theory, it shouldn't require any special versioning... I think? Done, for .empty(). Not sure about the second comment; I revised my comment above to clarify. This whole block is to get the task id. The event and nodeData fields are correctly named. https://codereview.chromium.org/2826423003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:169: if (package_it == node->stringProperties->end()) On 2017/05/10 06:36:42, dcheng wrote: > Same comment about naming here for "stringProperties" If different than above, then I guess you might mean style-wise? This is generated from the mojom. If that's not it, please reply and I'll address in a follow up; it would require lots of changes on both ends and versioning as well. https://codereview.chromium.org/2826423003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:233: package_name = task_ids_entry.first; On 2017/05/10 06:36:42, dcheng wrote: > Seems wasteful to copy here. Maybe just save the iterator and pass .first to the > lookup below Done. https://codereview.chromium.org/2826423003/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h (right): https://codereview.chromium.org/2826423003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h:56: void OnTaskDestroyed(int task_id) override; On 2017/05/10 06:36:42, dcheng wrote: > Nit: the interface should be consistent about int32_t vs int Done. https://codereview.chromium.org/2826423003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h:75: std::map<std::string, std::unique_ptr<AXTreeSourceArc>> package_name_to_tree_; On 2017/05/10 06:36:42, dcheng wrote: > For IWYU, please #include <map>, <string>, <set> Done.
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
https://codereview.chromium.org/2826423003/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:160: if (event_data->nodeData.size() == 0) On 2017/05/10 21:01:19, David Tseng wrote: > On 2017/05/10 06:36:42, dcheng wrote: > > Nit: prefer .empty() to .size() == 0 > > > > Also, this is named incorrectly, but I'm not sure how hard that will be to fix > > here. In theory, it shouldn't require any special versioning... I think? > > Done, for .empty(). > > Not sure about the second comment; I revised my comment above to clarify. This > whole block is to get the task id. The event and nodeData fields are correctly > named. Right, my comment is that the mojom should have named this node_data rather than nodeData.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/10 21:02:50, dcheng wrote: > https://codereview.chromium.org/2826423003/diff/240001/chrome/browser/chromeo... > File > chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc > (right): > > https://codereview.chromium.org/2826423003/diff/240001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:160: > if (event_data->nodeData.size() == 0) > On 2017/05/10 21:01:19, David Tseng wrote: > > On 2017/05/10 06:36:42, dcheng wrote: > > > Nit: prefer .empty() to .size() == 0 > > > > > > Also, this is named incorrectly, but I'm not sure how hard that will be to > fix > > > here. In theory, it shouldn't require any special versioning... I think? > > > > Done, for .empty(). > > > > Not sure about the second comment; I revised my comment above to clarify. This > > whole block is to get the task id. The event and nodeData fields are correctly > > named. > > Right, my comment is that the mojom should have named this node_data rather than > nodeData. accessibility_helper.mojom was always camal cased (that was what I went off of when I added to it). I can change to snake casing in a follow up in order to not conflate this cl.
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...
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_...)
Patchset #15 (id:280001) has been deleted
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_...)
Patchset #15 (id:300001) has been deleted
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
dtseng@chromium.org changed reviewers: + reveman@chromium.org
+ reveman (I need a small change to wm_helper.cc for test fix).
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_...)
Patchset #15 (id:320001) has been deleted
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/2826423003/diff/340001/components/exo/wm_help... File components/exo/wm_helper.cc (left): https://codereview.chromium.org/2826423003/diff/340001/components/exo/wm_help... components/exo/wm_helper.cc:29: DCHECK(g_instance); Should we be calling WMHelper::SetInstance in the test instead of this is actually used?
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
dtseng@chromium.org changed reviewers: - reveman@chromium.org
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2826423003/diff/340001/components/exo/wm_help... File components/exo/wm_helper.cc (left): https://codereview.chromium.org/2826423003/diff/340001/components/exo/wm_help... components/exo/wm_helper.cc:29: DCHECK(g_instance); On 2017/05/11 13:01:35, reveman wrote: > Should we be calling WMHelper::SetInstance in the test instead of this is > actually used? Thanks, that's a better way to do it. https://codereview.chromium.org/2826423003/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc (right): https://codereview.chromium.org/2826423003/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:98: // We do not unregitser ourselves from WMHelper as an ActivationObserver FYI @yawano. In response to your request to do this: ChromeBrowserMainPartsChromeOS owns ArcServiceLauncher which gets shut-down (destroys its services) *after* ChromeMainPartsExo (which owns WMHelper). ArcServiceLauncher owns ArcServiceManager which owns ArcAccessibilityHelperBridge ...
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
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, dmazzoni@chromium.org, hidehiko@chromium.org, yawano@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2826423003/#ps400001 (title: "Fake out WMHelper")
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
Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by 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
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": 400001, "attempt_start_ts": 1494560919043810,
"parent_rev": "edef2a4e69984700e09359b2f3e7234363a16d2c", "commit_rev":
"87ccf7a1cd3a9cfbc8336695f41c962929c991aa"}
Message was sent while issue was closed.
Description was changed from ========== Expand Chrome OS ARC support to create one tree source per package Chrome OS ARC support uses an AXTreeSourceArc to track the contents of the Android accessibility tree. Previously, this sufficed since we didn't need to understand which app/package we were currently in inside of Chrome. However, in order to support whitelisting specific packages, we do need to know which packages pertain to which trees in Chrome. We now keep a collection of trees, keyed by their task ids. Task ids are obtained by keeping a mapping from package name. Since every AccessibilityEvent is associated with a package name (via its AccessibilityNodeInfo data), we can route events correctly to the correct package -> task id -> tree source. BUG=708272 TEST=add --enable-chromevox-arc-support from flags. Switch between multiple ARC apps and verify instant access to tree contents. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Expand Chrome OS ARC support to create one tree source per package Chrome OS ARC support uses an AXTreeSourceArc to track the contents of the Android accessibility tree. Previously, this sufficed since we didn't need to understand which app/package we were currently in inside of Chrome. However, in order to support whitelisting specific packages, we do need to know which packages pertain to which trees in Chrome. We now keep a collection of trees, keyed by their task ids. Task ids are obtained by keeping a mapping from package name. Since every AccessibilityEvent is associated with a package name (via its AccessibilityNodeInfo data), we can route events correctly to the correct package -> task id -> tree source. BUG=708272 TEST=add --enable-chromevox-arc-support from flags. Switch between multiple ARC apps and verify instant access to tree contents. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2826423003 Cr-Commit-Position: refs/heads/master@{#471213} Committed: https://chromium.googlesource.com/chromium/src/+/87ccf7a1cd3a9cfbc8336695f41c... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:400001) as https://chromium.googlesource.com/chromium/src/+/87ccf7a1cd3a9cfbc8336695f41c... |
