Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2768093005:
Send voice interaction structure from cros to Arc. (Closed)
DescriptionSend voice interaction structure from cros to Arc.
Bug=704684
Bug=b/35643583
Test=voice interaction context passed to ARC
(Closed in favor of https://codereview.chromium.org/2808383004/)
Patch Set 1 #
Total comments: 1
Patch Set 2 : send voice interaction structure from cros to Arc. #Patch Set 3 : address trybot errors #Patch Set 4 : address trybot error #Patch Set 5 : address trybot error #Patch Set 6 : address trybot style issue #Patch Set 7 : address trybot warning #Patch Set 8 : address trybot error #
Total comments: 38
Patch Set 9 : address review commet #
Total comments: 6
Patch Set 10 : address review comments #
Total comments: 16
Patch Set 11 : address review comments for patchset 10 #
Total comments: 8
Patch Set 12 : address review comments #
Total comments: 2
Messages
Total messages: 62 (40 generated)
https://codereview.chromium.org/2768093005/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc (right): https://codereview.chromium.org/2768093005/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:23: #include "content/browser/web_contents/web_contents_impl.h" These 2 lines cannot be included since they are not in content/browser. Besides, the following functions are really similar to the Android counterpart. I'm looking for a way to refactor that code into content/browser/android and content/public/browser/android so that they could be shared..
Description was changed from ========== [WIP] send voice interaction structure from cros to Arc. This CL should not be merged unless the include dependencies have been fixed! Bug: 704684 Bug: b/35643583 Test: voice interaction context passed to ARC ========== to ========== Send voice interaction structure from cros to Arc. Bug: 704684 Bug: b/35643583 Test: voice interaction context passed to ARC ==========
muyuanli@chromium.org changed reviewers: + hidehiko@chromium.org, lhchavez@google.com
On 2017/03/23 23:37:26, Muyuan wrote: > https://codereview.chromium.org/2768093005/diff/1/chrome/browser/chromeos/arc... > File > chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc > (right): > > https://codereview.chromium.org/2768093005/diff/1/chrome/browser/chromeos/arc... > chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:23: > #include "content/browser/web_contents/web_contents_impl.h" > These 2 lines cannot be included since they are not in content/browser. Besides, > the following functions are really similar to the Android counterpart. I'm > looking for a way to refactor that code into content/browser/android and > content/public/browser/android so that they could be shared.. Patchset 2 addressed the include dependency issue but the duplication with Android Chrome code is not addressed yet.
muyuanli@chromium.org changed reviewers: + lhchavez@chromium.org - lhchavez@google.com
The CQ bit was checked by muyuanli@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_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
muyuanli@chromium.org changed reviewers: + xiyuan@chromium.org
The CQ bit was checked by muyuanli@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_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) 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 muyuanli@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_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 muyuanli@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 muyuanli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by muyuanli@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 muyuanli@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.
This issue is ready for review.
https://codereview.chromium.org/2768093005/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc (right): https://codereview.chromium.org/2768093005/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:32: void CreateVoiceInteractionStructure( Why not use mojo typemapping ? https://codereview.chromium.org/2768093005/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:33: mojom::VoiceInteractionStructurePtr& structure, Prefer to make the function to return VoiceInteractionStructurePtr instead of passing a ref. https://codereview.chromium.org/2768093005/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:97: auto structure = mojom::VoiceInteractionStructure::New(); nit: is this used? https://codereview.chromium.org/2768093005/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:98: aura::client::ActivationClient* activation_client = nit: get rid of |activation_client| and inline it https://codereview.chromium.org/2768093005/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:106: if (!browser || browser->window()->GetNativeWindow() != focused) { Can we use browser->window()->IsActive() instead? We then don't need to mess with ash. https://codereview.chromium.org/2768093005/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.h (right): https://codereview.chromium.org/2768093005/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.h:8: #include <memory> not used? https://codereview.chromium.org/2768093005/diff/140001/components/arc/common/... File components/arc/common/voice_interaction_arc_home.mojom (right): https://codereview.chromium.org/2768093005/diff/140001/components/arc/common/... components/arc/common/voice_interaction_arc_home.mojom:13: // of the text. The indices are inclusive. Are you sure you want inclusive for both ends? It is not inline with gfx::Range, i.e. a range is [start, end). https://codereview.chromium.org/2768093005/diff/140001/components/arc/common/... components/arc/common/voice_interaction_arc_home.mojom:15: int32 start_selection; nit: start_selection -> selection_start and similarly for end_selection. start_selection reads like an verb with action, not a noun. https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... File content/public/browser/voice_interaction_helper.cc (right): https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... content/public/browser/voice_interaction_helper.cc:7: #include "base/callback.h" remove since it is already included in header ? https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... content/public/browser/voice_interaction_helper.cc:8: #include "base/logging.h" not used? https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... content/public/browser/voice_interaction_helper.cc:9: #include "content/browser/accessibility/browser_accessibility_manager.h" Should this be browser_accessibility.h instead? I don't see the manager is used. https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... content/public/browser/voice_interaction_helper.cc:19: const char* class_name = NULL; nit: NULL -> nullptr https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... content/public/browser/voice_interaction_helper.cc:105: int text_style = node->GetIntAttribute(ui::AX_ATTR_TEXT_STYLE); nit: const int https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... content/public/browser/voice_interaction_helper.cc:136: } Should we fill in |has_selection| etc member in an else clause ? Or add default initialization values to AXViewStructure. https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... File content/public/browser/voice_interaction_helper.h (right): https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... content/public/browser/voice_interaction_helper.h:11: #include <utility> nit: not used? https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... content/public/browser/voice_interaction_helper.h:27: int32_t bgcolor; nit: bgcolor -> bg_color https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... content/public/browser/voice_interaction_helper.h:47: const base::Callback<void(const AXViewStructure*)>& callback); Change "const AXViewStructure*" to either a ref "const AXViewStructure&" or passing ownership std::unique_ptr<AXViewStructure>.
https://codereview.chromium.org/2768093005/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc (right): https://codereview.chromium.org/2768093005/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:32: void CreateVoiceInteractionStructure( On 2017/04/03 19:01:07, xiyuan wrote: > Why not use mojo typemapping ? The idea here is to try to share the code with https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content... by creating an intermediate structure (which is not done in this CL yet) and Android chrome does not seem to have the mojom mapping. https://codereview.chromium.org/2768093005/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:33: mojom::VoiceInteractionStructurePtr& structure, On 2017/04/03 19:01:07, xiyuan wrote: > Prefer to make the function to return VoiceInteractionStructurePtr instead of > passing a ref. Done. https://codereview.chromium.org/2768093005/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:97: auto structure = mojom::VoiceInteractionStructure::New(); On 2017/04/03 19:01:07, xiyuan wrote: > nit: is this used? Done. https://codereview.chromium.org/2768093005/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:98: aura::client::ActivationClient* activation_client = On 2017/04/03 19:01:07, xiyuan wrote: > nit: get rid of |activation_client| and inline it Done. https://codereview.chromium.org/2768093005/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:106: if (!browser || browser->window()->GetNativeWindow() != focused) { On 2017/04/03 19:01:07, xiyuan wrote: > Can we use browser->window()->IsActive() instead? We then don't need to mess > with ash. Done. https://codereview.chromium.org/2768093005/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.h (right): https://codereview.chromium.org/2768093005/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.h:8: #include <memory> On 2017/04/03 19:01:07, xiyuan wrote: > not used? Done. https://codereview.chromium.org/2768093005/diff/140001/components/arc/common/... File components/arc/common/voice_interaction_arc_home.mojom (right): https://codereview.chromium.org/2768093005/diff/140001/components/arc/common/... components/arc/common/voice_interaction_arc_home.mojom:13: // of the text. The indices are inclusive. On 2017/04/03 19:01:08, xiyuan wrote: > Are you sure you want inclusive for both ends? It is not inline with gfx::Range, > i.e. a range is [start, end). It should be [start, end). Thx. https://codereview.chromium.org/2768093005/diff/140001/components/arc/common/... components/arc/common/voice_interaction_arc_home.mojom:15: int32 start_selection; On 2017/04/03 19:01:07, xiyuan wrote: > nit: start_selection -> selection_start and similarly for end_selection. > start_selection reads like an verb with action, not a noun. Just try to be consistent with https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... File content/public/browser/voice_interaction_helper.cc (right): https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... content/public/browser/voice_interaction_helper.cc:7: #include "base/callback.h" On 2017/04/03 19:01:08, xiyuan wrote: > remove since it is already included in header ? Done. https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... content/public/browser/voice_interaction_helper.cc:8: #include "base/logging.h" On 2017/04/03 19:01:08, xiyuan wrote: > not used? Done. https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... content/public/browser/voice_interaction_helper.cc:9: #include "content/browser/accessibility/browser_accessibility_manager.h" On 2017/04/03 19:01:08, xiyuan wrote: > Should this be browser_accessibility.h instead? I don't see the manager is used. Done. https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... content/public/browser/voice_interaction_helper.cc:19: const char* class_name = NULL; On 2017/04/03 19:01:08, xiyuan wrote: > nit: NULL -> nullptr Done. https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... content/public/browser/voice_interaction_helper.cc:105: int text_style = node->GetIntAttribute(ui::AX_ATTR_TEXT_STYLE); On 2017/04/03 19:01:08, xiyuan wrote: > nit: const int Done. https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... content/public/browser/voice_interaction_helper.cc:136: } On 2017/04/03 19:01:08, xiyuan wrote: > Should we fill in |has_selection| etc member in an else clause ? Or add default > initialization values to AXViewStructure. Done. https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... File content/public/browser/voice_interaction_helper.h (right): https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... content/public/browser/voice_interaction_helper.h:11: #include <utility> On 2017/04/03 19:01:08, xiyuan wrote: > nit: not used? Done. https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... content/public/browser/voice_interaction_helper.h:27: int32_t bgcolor; On 2017/04/03 19:01:08, xiyuan wrote: > nit: bgcolor -> bg_color same reason.. https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... https://codereview.chromium.org/2768093005/diff/140001/content/public/browser... content/public/browser/voice_interaction_helper.h:47: const base::Callback<void(const AXViewStructure*)>& callback); On 2017/04/03 19:01:08, xiyuan wrote: > Change "const AXViewStructure*" to either a ref "const AXViewStructure&" or > passing ownership std::unique_ptr<AXViewStructure>. Done.
The CQ bit was checked by muyuanli@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/2768093005/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc (right): https://codereview.chromium.org/2768093005/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:32: void CreateVoiceInteractionStructure( On 2017/04/03 20:25:35, Muyuan wrote: > On 2017/04/03 19:01:07, xiyuan wrote: > > Why not use mojo typemapping ? > > The idea here is to try to share the code with > https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content... > by creating an intermediate structure (which is not done in this CL yet) and > Android chrome does not seem to have the mojom mapping. Can you add a comment to document the intention? https://codereview.chromium.org/2768093005/diff/140001/components/arc/common/... File components/arc/common/voice_interaction_arc_home.mojom (right): https://codereview.chromium.org/2768093005/diff/140001/components/arc/common/... components/arc/common/voice_interaction_arc_home.mojom:15: int32 start_selection; On 2017/04/03 20:25:35, Muyuan wrote: > On 2017/04/03 19:01:07, xiyuan wrote: > > nit: start_selection -> selection_start and similarly for end_selection. > > start_selection reads like an verb with action, not a noun. > > Just try to be consistent with > https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... Can you document this in the comment either here for in the comment of VoiceInteractionStructure ? https://codereview.chromium.org/2768093005/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc (right): https://codereview.chromium.org/2768093005/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:64: auto structure = CreateVoiceInteractionStructure(result.get()); nit: get rid |structure| and inline the call https://codereview.chromium.org/2768093005/diff/160001/content/public/browser... File content/public/browser/voice_interaction_helper.cc (right): https://codereview.chromium.org/2768093005/diff/160001/content/public/browser... content/public/browser/voice_interaction_helper.cc:149: const base::Callback<void(const std::unique_ptr<AXViewStructure>)>& "const std::unique_ptr<>" means the callback cannot take over the ownership. Should you use just "std::unique_ptr<>" ? https://codereview.chromium.org/2768093005/diff/160001/content/public/browser... content/public/browser/voice_interaction_helper.cc:154: auto structure = nit: get rid of |structure| and inline the call
https://codereview.chromium.org/2768093005/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc (right): https://codereview.chromium.org/2768093005/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:32: void CreateVoiceInteractionStructure( On 2017/04/03 21:32:11, xiyuan wrote: > On 2017/04/03 20:25:35, Muyuan wrote: > > On 2017/04/03 19:01:07, xiyuan wrote: > > > Why not use mojo typemapping ? > > > > The idea here is to try to share the code with > > > https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content... > > by creating an intermediate structure (which is not done in this CL yet) and > > Android chrome does not seem to have the mojom mapping. > > Can you add a comment to document the intention? Done. https://codereview.chromium.org/2768093005/diff/140001/components/arc/common/... File components/arc/common/voice_interaction_arc_home.mojom (right): https://codereview.chromium.org/2768093005/diff/140001/components/arc/common/... components/arc/common/voice_interaction_arc_home.mojom:15: int32 start_selection; On 2017/04/03 21:32:11, xiyuan wrote: > On 2017/04/03 20:25:35, Muyuan wrote: > > On 2017/04/03 19:01:07, xiyuan wrote: > > > nit: start_selection -> selection_start and similarly for end_selection. > > > start_selection reads like an verb with action, not a noun. > > > > Just try to be consistent with > > > https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... > > Can you document this in the comment either here for in the comment of > VoiceInteractionStructure ? Done. https://codereview.chromium.org/2768093005/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc (right): https://codereview.chromium.org/2768093005/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:64: auto structure = CreateVoiceInteractionStructure(result.get()); On 2017/04/03 21:32:11, xiyuan wrote: > nit: get rid |structure| and inline the call Done. https://codereview.chromium.org/2768093005/diff/160001/content/public/browser... File content/public/browser/voice_interaction_helper.cc (right): https://codereview.chromium.org/2768093005/diff/160001/content/public/browser... content/public/browser/voice_interaction_helper.cc:149: const base::Callback<void(const std::unique_ptr<AXViewStructure>)>& On 2017/04/03 21:32:11, xiyuan wrote: > "const std::unique_ptr<>" means the callback cannot take over the ownership. > Should you use just "std::unique_ptr<>" ? Done. https://codereview.chromium.org/2768093005/diff/160001/content/public/browser... content/public/browser/voice_interaction_helper.cc:154: auto structure = On 2017/04/03 21:32:11, xiyuan wrote: > nit: get rid of |structure| and inline the call Done.
https://codereview.chromium.org/2768093005/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc (right): https://codereview.chromium.org/2768093005/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:59: auto c = CreateVoiceInteractionStructure(child.get()); Do the same thing you did in L68 here (inline the |c|), for consistency. https://codereview.chromium.org/2768093005/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:67: std::unique_ptr<content::AXViewStructure> result) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); https://codereview.chromium.org/2768093005/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:92: void ArcVoiceInteractionArcHomeService::OnInstanceClosed() { If you're not going to do anything here, it's better if you remove it. https://codereview.chromium.org/2768093005/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:97: const GetVoiceInteractionStructureCallback& callback) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); https://codereview.chromium.org/2768093005/diff/180001/components/arc/common/... File components/arc/common/voice_interaction_arc_home.mojom (right): https://codereview.chromium.org/2768093005/diff/180001/components/arc/common/... components/arc/common/voice_interaction_arc_home.mojom:13: // of the text [start, end). Do these represent bytes? UTF-16 code units? Unicode characters? https://codereview.chromium.org/2768093005/diff/180001/content/public/browser... File content/public/browser/voice_interaction_helper.cc (right): https://codereview.chromium.org/2768093005/diff/180001/content/public/browser... content/public/browser/voice_interaction_helper.cc:17: const char* class_name = nullptr; Since you're only setting this once per branch, how about you just return the string constant directly and avoid this variable? switch (node->GetRole()) { case ui::AX_ROLE_SEARCH_BOX: case ui::AX_ROLE_SPIN_BUTTON: case ui::AX_ROLE_TEXT_FIELD: return ui::kAXEditTextClassname; case ... } https://codereview.chromium.org/2768093005/diff/180001/content/public/browser... File content/public/browser/voice_interaction_helper.h (right): https://codereview.chromium.org/2768093005/diff/180001/content/public/browser... content/public/browser/voice_interaction_helper.h:42: AXViewStructure(); Do you need the explicit ctor/dtor? https://codereview.chromium.org/2768093005/diff/180001/content/public/browser... content/public/browser/voice_interaction_helper.h:44: DISALLOW_COPY_AND_ASSIGN(AXViewStructure); nit: make this private.
https://codereview.chromium.org/2768093005/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc (right): https://codereview.chromium.org/2768093005/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:59: auto c = CreateVoiceInteractionStructure(child.get()); On 2017/04/03 23:24:12, Luis Héctor Chávez wrote: > Do the same thing you did in L68 here (inline the |c|), for consistency. Done. https://codereview.chromium.org/2768093005/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:67: std::unique_ptr<content::AXViewStructure> result) { On 2017/04/03 23:24:12, Luis Héctor Chávez wrote: > DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Done. https://codereview.chromium.org/2768093005/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:92: void ArcVoiceInteractionArcHomeService::OnInstanceClosed() { On 2017/04/03 23:24:12, Luis Héctor Chávez wrote: > If you're not going to do anything here, it's better if you remove it. Done. https://codereview.chromium.org/2768093005/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:97: const GetVoiceInteractionStructureCallback& callback) { On 2017/04/03 23:24:12, Luis Héctor Chávez wrote: > DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Done. https://codereview.chromium.org/2768093005/diff/180001/components/arc/common/... File components/arc/common/voice_interaction_arc_home.mojom (right): https://codereview.chromium.org/2768093005/diff/180001/components/arc/common/... components/arc/common/voice_interaction_arc_home.mojom:13: // of the text [start, end). On 2017/04/03 23:24:12, Luis Héctor Chávez wrote: > Do these represent bytes? UTF-16 code units? Unicode characters? Done. https://codereview.chromium.org/2768093005/diff/180001/content/public/browser... File content/public/browser/voice_interaction_helper.cc (right): https://codereview.chromium.org/2768093005/diff/180001/content/public/browser... content/public/browser/voice_interaction_helper.cc:17: const char* class_name = nullptr; On 2017/04/03 23:24:12, Luis Héctor Chávez wrote: > Since you're only setting this once per branch, how about you just return the > string constant directly and avoid this variable? > > switch (node->GetRole()) { > case ui::AX_ROLE_SEARCH_BOX: > case ui::AX_ROLE_SPIN_BUTTON: > case ui::AX_ROLE_TEXT_FIELD: > return ui::kAXEditTextClassname; > case ... > } Done. https://codereview.chromium.org/2768093005/diff/180001/content/public/browser... File content/public/browser/voice_interaction_helper.h (right): https://codereview.chromium.org/2768093005/diff/180001/content/public/browser... content/public/browser/voice_interaction_helper.h:42: AXViewStructure(); On 2017/04/03 23:24:12, Luis Héctor Chávez wrote: > Do you need the explicit ctor/dtor? Yes.. otherwise the CQ dry run just fails with "Complex class/struct needs an explicit out-of-line constructor" https://codereview.chromium.org/2768093005/diff/180001/content/public/browser... content/public/browser/voice_interaction_helper.h:44: DISALLOW_COPY_AND_ASSIGN(AXViewStructure); On 2017/04/03 23:24:12, Luis Héctor Chávez wrote: > nit: make this private. Done.
lgtm
lgtm with one extra nit I missed. https://codereview.chromium.org/2768093005/diff/200001/content/public/browser... File content/public/browser/voice_interaction_helper.cc (right): https://codereview.chromium.org/2768093005/diff/200001/content/public/browser... content/public/browser/voice_interaction_helper.cc:136: AXViewStructure::AXViewStructure() {} nit: = default. Same in L138.
LGTM with more minor comments. https://codereview.chromium.org/2768093005/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc (right): https://codereview.chromium.org/2768093005/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:37: const content::AXViewStructure* view_structure) { Maybe: const reference instead of const pointer? https://codereview.chromium.org/2768093005/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:58: for (auto& child : view_structure->children) { nit: could you elide brace for single line body for consistency (in ARC code)? https://codereview.chromium.org/2768093005/diff/200001/content/public/browser... File content/public/browser/voice_interaction_helper.h (right): https://codereview.chromium.org/2768093005/diff/200001/content/public/browser... content/public/browser/voice_interaction_helper.h:42: AXViewStructure(); Style: Please put ctor/dtor before data members. cf) https://google.github.io/styleguide/cppguide.html#Declaration_Order
muyuanli@chromium.org changed reviewers: + avi@chromium.org, dcheng@chromium.org
https://codereview.chromium.org/2768093005/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc (right): https://codereview.chromium.org/2768093005/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:37: const content::AXViewStructure* view_structure) { On 2017/04/04 17:42:58, hidehiko wrote: > Maybe: const reference instead of const pointer? Done. https://codereview.chromium.org/2768093005/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:58: for (auto& child : view_structure->children) { On 2017/04/04 17:42:58, hidehiko wrote: > nit: could you elide brace for single line body for consistency (in ARC code)? Done. https://codereview.chromium.org/2768093005/diff/200001/content/public/browser... File content/public/browser/voice_interaction_helper.cc (right): https://codereview.chromium.org/2768093005/diff/200001/content/public/browser... content/public/browser/voice_interaction_helper.cc:136: AXViewStructure::AXViewStructure() {} On 2017/04/04 15:49:31, Luis Héctor Chávez wrote: > nit: = default. Same in L138. Done. https://codereview.chromium.org/2768093005/diff/200001/content/public/browser... File content/public/browser/voice_interaction_helper.h (right): https://codereview.chromium.org/2768093005/diff/200001/content/public/browser... content/public/browser/voice_interaction_helper.h:42: AXViewStructure(); On 2017/04/04 17:42:58, hidehiko wrote: > Style: Please put ctor/dtor before data members. > cf) https://google.github.io/styleguide/cppguide.html#Declaration_Order Done.
The CQ bit was checked by muyuanli@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.
Muyuan, When sending for review, please indicate who should be reviewing what. You want me to review the content/ change?
On 2017/04/04 20:04:47, Avi wrote: > Muyuan, > > When sending for review, please indicate who should be reviewing what. You want > me to review the content/ change? Also, do BUG= TEST= so that our automated tools know how to process the description.
https://codereview.chromium.org/2768093005/diff/220001/content/public/browser... File content/public/browser/voice_interaction_helper.h (right): https://codereview.chromium.org/2768093005/diff/220001/content/public/browser... content/public/browser/voice_interaction_helper.h:49: void GetAXViewStructure( I don't understand this file. It's named "voice interaction helper" but seems to have very generic accessibility content. My objection here is 1. The content doesn't match the file. 2. Can't we use existing accessibility code? 3. Content is for web platform code, so unless this is useful for the general web platform, this doesn't belong in content.
https://codereview.chromium.org/2768093005/diff/220001/content/public/browser... File content/public/browser/voice_interaction_helper.h (right): https://codereview.chromium.org/2768093005/diff/220001/content/public/browser... content/public/browser/voice_interaction_helper.h:49: void GetAXViewStructure( On 2017/04/04 20:11:01, Avi (slow and ooo 10-23 April) wrote: > I don't understand this file. > > It's named "voice interaction helper" but seems to have very generic > accessibility content. My objection here is > > 1. The content doesn't match the file. > 2. Can't we use existing accessibility code? > 3. Content is for web platform code, so unless this is useful for the general > web platform, this doesn't belong in content. The idea is to share the code between Android Chrome and ARC. Essentially the end goal is to move parts of web_contents_android.cc here. The naming conventions follow closely to Android Chrome: https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... The accessibility code cannot be used directly due to dependency restrictions. I'm not sure if this place is a good one. Another option would be to move the files into content/public/browser/android.
On 2017/04/04 20:43:49, Muyuan wrote: > https://codereview.chromium.org/2768093005/diff/220001/content/public/browser... > File content/public/browser/voice_interaction_helper.h (right): > > https://codereview.chromium.org/2768093005/diff/220001/content/public/browser... > content/public/browser/voice_interaction_helper.h:49: void GetAXViewStructure( > On 2017/04/04 20:11:01, Avi (slow and ooo 10-23 April) wrote: > > I don't understand this file. > > > > It's named "voice interaction helper" but seems to have very generic > > accessibility content. My objection here is > > > > 1. The content doesn't match the file. > > 2. Can't we use existing accessibility code? > > 3. Content is for web platform code, so unless this is useful for the general > > web platform, this doesn't belong in content. > > The idea is to share the code between Android Chrome and ARC. Essentially the > end goal is to move parts of web_contents_android.cc here. > > The naming conventions follow closely to Android Chrome: > https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... > > The accessibility code cannot be used directly due to dependency restrictions. > > I'm not sure if this place is a good one. Another option would be to move the > files into content/public/browser/android. Putting files into content/public/browser/android does not alleviate any of the issues I raised.
If you can extend the existing accessibility code, you should do so. Adding a second, parallel, bunch of code feels very bad.
Description was changed from ========== Send voice interaction structure from cros to Arc. Bug: 704684 Bug: b/35643583 Test: voice interaction context passed to ARC ========== to ========== Send voice interaction structure from cros to Arc. Bug=704684 Bug=b/35643583 Test=voice interaction context passed to ARC ==========
dmazzoni@chromium.org changed reviewers: + dmazzoni@chromium.org
As Avi said, we don't duplicate a bunch of existing code and then refactor it later. Let's get the design right the first time. I suggest that rather than trying to land this all in one change, you come up with a design, get buy-in from the various owners, then land it in a few smaller changes. It's okay to hack on one large change in the meantime and ask questions. Here are my suggestions: 1. First, add a public interface for WebContentsImpl::RequestAXTreeSnapshot. It's a pretty high-level interface and I don't see any reason not to make it public. 2. Refactor AXTreeSnapshotCallback and WalkAXTreeDepthFirst from web_contents_android and put them somewhere that could be used by both web_contents_android and by chrome/browser/chromeos/arc - I'm thinking that ui/accessibility/platform makes the most sense. This will require some extra work because it means taking BrowserAccessibility out of the loop entirely. We're only using BrowserAccessibility for a few small things but really web_contents_android never should have depended on it. Talk to Selim (sgurun) to coordinate on part #2, as he wrote the web_contents_android stuff. This is going to take several weeks.
Description was changed from ========== Send voice interaction structure from cros to Arc. Bug=704684 Bug=b/35643583 Test=voice interaction context passed to ARC ========== to ========== Send voice interaction structure from cros to Arc. Bug=704684 Bug=b/35643583 Test=voice interaction context passed to ARC (Closed in favor of https://codereview.chromium.org/2808383004/) ========== |
