Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2731403007:
add voice interaction shortcut. (Closed)
|
Created:
3 years, 9 months ago by Muyuan Modified:
3 years, 9 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, oshima+watch_chromium.org, viettrungluu+watch_chromium.org, qsr+mojo_chromium.org, hidehiko+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, darin (slow to review), davemoore+watch_chromium.org, yusukes+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionadd voice interaction shortcut.
BUG=699309
Test=Meta + a starts voice interaction session
in CrOS.
Review-Url: https://codereview.chromium.org/2731403007
Cr-Commit-Position: refs/heads/master@{#457985}
Committed: https://chromium.googlesource.com/chromium/src/+/d8b8f945e7c2c694b202b9a0cd2e63db34ac30c1
Patch Set 1 #
Total comments: 20
Patch Set 2 : Updated docs for mojom fields; Changed coding according to comments. #
Total comments: 25
Patch Set 3 : Changed mojom comments #
Total comments: 37
Patch Set 4 : address issues according to comments #Patch Set 5 : removed debugging prints #
Total comments: 33
Patch Set 6 : address review comments #Patch Set 7 : add voice interaction shortcut. #
Total comments: 19
Patch Set 8 : address review comments #Patch Set 9 : split 2 mojom host. #
Total comments: 4
Patch Set 10 : minor edits to address review comments. #Patch Set 11 : rebased #Patch Set 12 : rebased #
Total comments: 4
Patch Set 13 : fix build and address review comments #
Total comments: 4
Patch Set 14 : addressed review comments #Messages
Total messages: 63 (29 generated)
Description was changed from ========== add voice interaction shortcut. BUG=699309 Test=Meta + a starts voice interaction session in CrOS. ========== to ========== add voice interaction shortcut. BUG=699309 Test=Meta + a starts voice interaction session in CrOS. ==========
muyuanli@chromium.org changed reviewers: + lhchavez@chromium.org, xiaohuic@chromium.org, 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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
first round. https://codereview.chromium.org/2731403007/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_voice_interaction_service.cc (right): https://codereview.chromium.org/2731403007/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:22: constexpr std::string kEnableVoiceInteraction = "enable-voice-interaction"; std::string does not support being a constexpr. This needs to be changed to constexpr char kEnableVoiceInteraction[] = ...; https://codereview.chromium.org/2731403007/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:45: home_observer_ = base::MakeUnique<ArcHomeObserver>(this); nit: you can also initialize the observers on the line above. https://codereview.chromium.org/2731403007/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:53: arc_bridge_service()->vi_capture()->RemoveObserver(capture_observer_.get()); nit: Remove the observers in the opposite order in which they are added. https://codereview.chromium.org/2731403007/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_voice_interaction_service.h (right): https://codereview.chromium.org/2731403007/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_voice_interaction_service.h:19: class ArcHomeObserver This can be forward-declared in the private section of ArcVoiceInteractionService, and declared in the .cc file: class ArcVoiceInteractionService : ... { ... private: class ArcHomeObserver; class CaptureObserver; std::unique_ptr<ArcHomeObserver> home_observer_; std::unique_ptr<CaptureObserver> capture_observer_; }; https://codereview.chromium.org/2731403007/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_voice_interaction_service.h:23: public: in Chromium/Google, the order is public, protected, private. source: https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.chromium.org/2731403007/diff/1/components/arc/arc_bridge_s... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/2731403007/diff/1/components/arc/arc_bridge_s... components/arc/arc_bridge_service.h:96: InstanceHolder<mojom::VoiceInteractionArcHomeInstance>* vi_home() { please make these voice_interaction_home and voice_interaction_capture https://codereview.chromium.org/2731403007/diff/1/components/arc/common/voice... File components/arc/common/voice_interaction.mojom (right): https://codereview.chromium.org/2731403007/diff/1/components/arc/common/voice... components/arc/common/voice_interaction.mojom:10: bool success; You need to document all these fields. https://codereview.chromium.org/2731403007/diff/1/components/arc/common/voice... components/arc/common/voice_interaction.mojom:41: StartVoiceInteractionSession@1(); Can this fail?
https://codereview.chromium.org/2731403007/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_voice_interaction_service.cc (right): https://codereview.chromium.org/2731403007/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:78: accelerators.push_back(std::move(acc)); nit: You can use {} syntax to save creating a vector. e.g. https://cs.chromium.org/chromium/src/ash/accelerators/accelerator_controller_... https://codereview.chromium.org/2731403007/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:86: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Do we need to unregister the accelerator (or have a flag to return proper value from CanHandleAccelerators) when this happens?
https://codereview.chromium.org/2731403007/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_voice_interaction_service.cc (right): https://codereview.chromium.org/2731403007/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:22: constexpr std::string kEnableVoiceInteraction = "enable-voice-interaction"; On 2017/03/08 15:22:40, Luis Héctor Chávez wrote: > std::string does not support being a constexpr. This needs to be changed to > > constexpr char kEnableVoiceInteraction[] = ...; Done. https://codereview.chromium.org/2731403007/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:45: home_observer_ = base::MakeUnique<ArcHomeObserver>(this); On 2017/03/08 15:22:40, Luis Héctor Chávez wrote: > nit: you can also initialize the observers on the line above. Done. https://codereview.chromium.org/2731403007/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:53: arc_bridge_service()->vi_capture()->RemoveObserver(capture_observer_.get()); On 2017/03/08 15:22:40, Luis Héctor Chávez wrote: > nit: Remove the observers in the opposite order in which they are added. Done. https://codereview.chromium.org/2731403007/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:78: accelerators.push_back(std::move(acc)); On 2017/03/08 17:37:59, xiyuan wrote: > nit: You can use {} syntax to save creating a vector. > > e.g. > https://cs.chromium.org/chromium/src/ash/accelerators/accelerator_controller_... Done. https://codereview.chromium.org/2731403007/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:86: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2017/03/08 17:37:59, xiyuan wrote: > Do we need to unregister the accelerator (or have a flag to return proper value > from CanHandleAccelerators) when this happens? Done. https://codereview.chromium.org/2731403007/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_voice_interaction_service.h (right): https://codereview.chromium.org/2731403007/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_voice_interaction_service.h:19: class ArcHomeObserver On 2017/03/08 15:22:40, Luis Héctor Chávez wrote: > This can be forward-declared in the private section of > ArcVoiceInteractionService, and declared in the .cc file: > > class ArcVoiceInteractionService : ... { > ... > private: > class ArcHomeObserver; > class CaptureObserver; > > std::unique_ptr<ArcHomeObserver> home_observer_; > std::unique_ptr<CaptureObserver> capture_observer_; > }; Done. https://codereview.chromium.org/2731403007/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_voice_interaction_service.h:23: public: On 2017/03/08 15:22:40, Luis Héctor Chávez wrote: > in Chromium/Google, the order is public, protected, private. source: > https://google.github.io/styleguide/cppguide.html#Declaration_Order Done. https://codereview.chromium.org/2731403007/diff/1/components/arc/arc_bridge_s... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/2731403007/diff/1/components/arc/arc_bridge_s... components/arc/arc_bridge_service.h:96: InstanceHolder<mojom::VoiceInteractionArcHomeInstance>* vi_home() { On 2017/03/08 15:22:40, Luis Héctor Chávez wrote: > please make these voice_interaction_home and voice_interaction_capture Done. https://codereview.chromium.org/2731403007/diff/1/components/arc/common/voice... File components/arc/common/voice_interaction.mojom (right): https://codereview.chromium.org/2731403007/diff/1/components/arc/common/voice... components/arc/common/voice_interaction.mojom:10: bool success; On 2017/03/08 15:22:40, Luis Héctor Chávez wrote: > You need to document all these fields. Done. https://codereview.chromium.org/2731403007/diff/1/components/arc/common/voice... components/arc/common/voice_interaction.mojom:41: StartVoiceInteractionSession@1(); On 2017/03/08 15:22:40, Luis Héctor Chávez wrote: > Can this fail? I plan to handle it in Android side and prints an error msg. But given that it's a feature guarded by USE flag, it shouldn't happen..
https://codereview.chromium.org/2731403007/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2731403007/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_service_launcher.cc:141: base::MakeUnique<ArcVoiceInteractionService>(arc_bridge_service)); Can we use the flag to guard the creation of the service? https://codereview.chromium.org/2731403007/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_voice_interaction_service.cc (right): https://codereview.chromium.org/2731403007/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:117: mojom::VoiceInteractionCaptureInstance* capture_instance_ = no surfix _ for local variable https://codereview.chromium.org/2731403007/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:122: capture_instance_->StartVoiceInteractionSession(); I think we need to explicitly check null here. otherwise it would crash browser if user hit meta+a quickly after login. https://codereview.chromium.org/2731403007/diff/20001/components/arc/common/v... File components/arc/common/voice_interaction.mojom (right): https://codereview.chromium.org/2731403007/diff/20001/components/arc/common/v... components/arc/common/voice_interaction.mojom:10: struct VoiceInteractionStructure { still needs some more documentation on the fields. https://codereview.chromium.org/2731403007/diff/20001/components/arc/common/v... components/arc/common/voice_interaction.mojom:14: // geometry of the view element what is a view element? https://codereview.chromium.org/2731403007/diff/20001/components/arc/common/v... components/arc/common/voice_interaction.mojom:17: int32 width; dp or pixel? https://codereview.chromium.org/2731403007/diff/20001/components/arc/common/v... components/arc/common/voice_interaction.mojom:20: // text properties what is text properties? https://codereview.chromium.org/2731403007/diff/20001/components/arc/common/v... components/arc/common/voice_interaction.mojom:22: array<uint8> text; what's the encoding of text? https://codereview.chromium.org/2731403007/diff/20001/components/arc/common/v... components/arc/common/voice_interaction.mojom:23: int32 color; what's the format of color, rgba or argb what not? https://codereview.chromium.org/2731403007/diff/20001/components/arc/common/v... components/arc/common/voice_interaction.mojom:33: int32 end_selection; is start/end inclusive or not? https://codereview.chromium.org/2731403007/diff/20001/components/arc/common/v... components/arc/common/voice_interaction.mojom:35: // fake Android view class name of the element what do we need this? https://codereview.chromium.org/2731403007/diff/20001/components/arc/common/v... components/arc/common/voice_interaction.mojom:38: array<VoiceInteractionStructure> children; what is children?
https://codereview.chromium.org/2731403007/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2731403007/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_service_launcher.cc:141: base::MakeUnique<ArcVoiceInteractionService>(arc_bridge_service)); On 2017/03/09 05:05:47, xc wrote: > Can we use the flag to guard the creation of the service? Not quite sure about this. Going to check with luis@ https://codereview.chromium.org/2731403007/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_voice_interaction_service.cc (right): https://codereview.chromium.org/2731403007/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:117: mojom::VoiceInteractionCaptureInstance* capture_instance_ = On 2017/03/09 05:05:47, xc wrote: > no surfix _ for local variable Done. https://codereview.chromium.org/2731403007/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:122: capture_instance_->StartVoiceInteractionSession(); On 2017/03/09 05:05:47, xc wrote: > I think we need to explicitly check null here. otherwise it would crash browser > if user hit meta+a quickly after login. The key is registered only after the Arc instance starts, which means it should never be null... https://codereview.chromium.org/2731403007/diff/20001/components/arc/common/v... File components/arc/common/voice_interaction.mojom (right): https://codereview.chromium.org/2731403007/diff/20001/components/arc/common/v... components/arc/common/voice_interaction.mojom:10: struct VoiceInteractionStructure { On 2017/03/09 05:05:47, xc wrote: > still needs some more documentation on the fields. Done. https://codereview.chromium.org/2731403007/diff/20001/components/arc/common/v... components/arc/common/voice_interaction.mojom:14: // geometry of the view element On 2017/03/09 05:05:47, xc wrote: > what is a view element? Done. https://codereview.chromium.org/2731403007/diff/20001/components/arc/common/v... components/arc/common/voice_interaction.mojom:17: int32 width; On 2017/03/09 05:05:47, xc wrote: > dp or pixel? Done. https://codereview.chromium.org/2731403007/diff/20001/components/arc/common/v... components/arc/common/voice_interaction.mojom:20: // text properties On 2017/03/09 05:05:47, xc wrote: > what is text properties? Done. https://codereview.chromium.org/2731403007/diff/20001/components/arc/common/v... components/arc/common/voice_interaction.mojom:22: array<uint8> text; On 2017/03/09 05:05:47, xc wrote: > what's the encoding of text? Done. https://codereview.chromium.org/2731403007/diff/20001/components/arc/common/v... components/arc/common/voice_interaction.mojom:23: int32 color; On 2017/03/09 05:05:47, xc wrote: > what's the format of color, rgba or argb what not? Done. https://codereview.chromium.org/2731403007/diff/20001/components/arc/common/v... components/arc/common/voice_interaction.mojom:33: int32 end_selection; On 2017/03/09 05:05:47, xc wrote: > is start/end inclusive or not? Done. https://codereview.chromium.org/2731403007/diff/20001/components/arc/common/v... components/arc/common/voice_interaction.mojom:35: // fake Android view class name of the element On 2017/03/09 05:05:47, xc wrote: > what do we need this? Done. https://codereview.chromium.org/2731403007/diff/20001/components/arc/common/v... components/arc/common/voice_interaction.mojom:38: array<VoiceInteractionStructure> children; On 2017/03/09 05:05:47, xc wrote: > what is children? Done.
hidehiko@chromium.org changed reviewers: + hidehiko@chromium.org
quick drive-by; https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_voice_interaction_service.cc (right): https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:15: #include "components/arc/arc_features.h" nit: Looks unused? https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:64: arc_bridge_service()->voice_interaction_capture()->AddObserver( Optional: How about moving these into each Observer impl's ctor (And ditto for dtor?) for more encapsulation? Maybe you'd like to swap binding and observer initialization order. https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:117: mojom::VoiceInteractionCaptureInstance* capture_instance = DCHECK_CURRENTLY_ON(UI) here, too? https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:132: LOG(ERROR) << "ArcVoiceInteractionSerivce::GetVoiceInteractionStructure " Ditto. https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:139: callback.Run(std::move(structure)); nit: Please #include <utility> https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:144: LOG(ERROR) Ditto. https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:149: callback.Run(result); nit: can be callback.Run({}); ? https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_voice_interaction_service.h (right): https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.h:11: #include "components/arc/instance_holder.h" nit: Looks unused. Move to .cc? https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.h:17: // Lives on the UI thread. Could you document what is the responsibility of this class? https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.h:45: std::unique_ptr<ArcHomeObserver> home_observer_; nit: Please #include <memory> https://codereview.chromium.org/2731403007/diff/40001/components/arc/common/a... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/2731403007/diff/40001/components/arc/common/a... components/arc/common/arc_bridge.mojom:121: [MinVersion=21] OnVoiceInteractionArcHomeInstanceReady@128( MinVersion = 22 (looks rebase mistake?) https://codereview.chromium.org/2731403007/diff/40001/components/arc/common/v... File components/arc/common/voice_interaction.mojom (right): https://codereview.chromium.org/2731403007/diff/40001/components/arc/common/v... components/arc/common/voice_interaction.mojom:15: bool success; Is there a case that only some part of children has "success = false"? If so it's good to be noted here with when it happens. If not, probably instead of the member here, the method signature will be, either; - GetVoiceInteractionStructure@1() => (bool success, VoiceInteractionStructure structure); - GetVoiceInteractionStructure@1() => (Status status, VoiceInteractionStructure structure); // status represent success or error code incl reason., or - GetVoiceInteractionStructure@1() => (VoiceInteractionStructure? structure); // return null structure on failure. or something? https://codereview.chromium.org/2731403007/diff/40001/components/arc/common/v... components/arc/common/voice_interaction.mojom:23: // Text of the view in utf-16 encoding Could you comment the reason why uint8 is used to store utf-16? Also, maybe endian if it is not trivial, too? https://codereview.chromium.org/2731403007/diff/40001/components/arc/common/v... components/arc/common/voice_interaction.mojom:37: // Start and end position of the selection, inclusive. Because of inconsistency of encoding "utf-16" and the storage "uint8", could you comment the index is for which? https://codereview.chromium.org/2731403007/diff/40001/components/arc/common/v... components/arc/common/voice_interaction.mojom:52: CaptureFocusedWindow@0() => (array<uint8> png_data); because mojo is an interface between Chrome and ARC instance, could you comment what these methods do and their responsibility explicitly as document?
https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_voice_interaction_service.cc (right): https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:24: const ui::Accelerator kVoiceInteractionAccelerator(ui::VKEY_A, Move this close to where it is used since it is only used in one place. Also, only POD is allowed as global variables. https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:38: ArcVoiceInteractionService* service_; nit: ArcVoiceInteractionService* const service_; since |service_| is not expected to change during lifetime of this class. https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:52: ArcVoiceInteractionService* service_; nit: ditto, use const ptr
https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_voice_interaction_service.cc (right): https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:15: #include "components/arc/arc_features.h" On 2017/03/10 06:14:49, hidehiko wrote: > nit: Looks unused? Done. https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:24: const ui::Accelerator kVoiceInteractionAccelerator(ui::VKEY_A, On 2017/03/10 18:01:16, xiyuan wrote: > Move this close to where it is used since it is only used in one place. > > Also, only POD is allowed as global variables. > > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables Done. https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:38: ArcVoiceInteractionService* service_; On 2017/03/10 18:01:16, xiyuan wrote: > nit: ArcVoiceInteractionService* const service_; > since |service_| is not expected to change during lifetime of this class. Done. https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:52: ArcVoiceInteractionService* service_; On 2017/03/10 18:01:16, xiyuan wrote: > nit: ditto, use const ptr Done. https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:64: arc_bridge_service()->voice_interaction_capture()->AddObserver( On 2017/03/10 06:14:49, hidehiko wrote: > Optional: How about moving these into each Observer impl's ctor (And ditto for > dtor?) for more encapsulation? > Maybe you'd like to swap binding and observer initialization order. Done. https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:117: mojom::VoiceInteractionCaptureInstance* capture_instance = On 2017/03/10 06:14:49, hidehiko wrote: > DCHECK_CURRENTLY_ON(UI) here, too? Done. https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:132: LOG(ERROR) << "ArcVoiceInteractionSerivce::GetVoiceInteractionStructure " On 2017/03/10 06:14:49, hidehiko wrote: > Ditto. Done. https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:139: callback.Run(std::move(structure)); On 2017/03/10 06:14:49, hidehiko wrote: > nit: Please > #include <utility> Done. https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:144: LOG(ERROR) On 2017/03/10 06:14:49, hidehiko wrote: > Ditto. Done. https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:149: callback.Run(result); On 2017/03/10 06:14:49, hidehiko wrote: > nit: can be callback.Run({}); ? Done. https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_voice_interaction_service.h (right): https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.h:11: #include "components/arc/instance_holder.h" On 2017/03/10 06:14:50, hidehiko wrote: > nit: Looks unused. Move to .cc? Done. https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.h:17: // Lives on the UI thread. On 2017/03/10 06:14:49, hidehiko wrote: > Could you document what is the responsibility of this class? Done. https://codereview.chromium.org/2731403007/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.h:45: std::unique_ptr<ArcHomeObserver> home_observer_; On 2017/03/10 06:14:50, hidehiko wrote: > nit: Please > #include <memory> Done. https://codereview.chromium.org/2731403007/diff/40001/components/arc/common/a... File components/arc/common/arc_bridge.mojom (right): https://codereview.chromium.org/2731403007/diff/40001/components/arc/common/a... components/arc/common/arc_bridge.mojom:121: [MinVersion=21] OnVoiceInteractionArcHomeInstanceReady@128( On 2017/03/10 06:14:50, hidehiko wrote: > MinVersion = 22 (looks rebase mistake?) Done. https://codereview.chromium.org/2731403007/diff/40001/components/arc/common/v... File components/arc/common/voice_interaction.mojom (right): https://codereview.chromium.org/2731403007/diff/40001/components/arc/common/v... components/arc/common/voice_interaction.mojom:15: bool success; On 2017/03/10 06:14:50, hidehiko wrote: > Is there a case that only some part of children has "success = false"? > If so it's good to be noted here with when it happens. > If not, probably instead of the member here, the method signature will be, > either; > > - GetVoiceInteractionStructure@1() => (bool success, VoiceInteractionStructure > structure); > - GetVoiceInteractionStructure@1() => (Status status, VoiceInteractionStructure > structure); // status represent success or error code incl reason., or > - GetVoiceInteractionStructure@1() => (VoiceInteractionStructure? structure); > // return null structure on failure. > > or something? Sure. How about using Optional<T> for this? https://codereview.chromium.org/2731403007/diff/40001/components/arc/common/v... components/arc/common/voice_interaction.mojom:23: // Text of the view in utf-16 encoding On 2017/03/10 06:14:50, hidehiko wrote: > Could you comment the reason why uint8 is used to store utf-16? Also, maybe > endian if it is not trivial, too? In Chromium, strings seem to be always little endian (verified on minnie). Also it seems that all strings in AXStructure are string16, but I can't seem to pass string16 directly through mojo calls. https://codereview.chromium.org/2731403007/diff/40001/components/arc/common/v... components/arc/common/voice_interaction.mojom:37: // Start and end position of the selection, inclusive. On 2017/03/10 06:14:50, hidehiko wrote: > Because of inconsistency of encoding "utf-16" and the storage "uint8", could you > comment the index is for which? Done. https://codereview.chromium.org/2731403007/diff/40001/components/arc/common/v... components/arc/common/voice_interaction.mojom:52: CaptureFocusedWindow@0() => (array<uint8> png_data); On 2017/03/10 06:14:50, hidehiko wrote: > because mojo is an interface between Chrome and ARC instance, > could you comment what these methods do and their responsibility explicitly as > document? Done.
second round. https://codereview.chromium.org/2731403007/diff/40001/components/arc/common/v... File components/arc/common/voice_interaction.mojom (right): https://codereview.chromium.org/2731403007/diff/40001/components/arc/common/v... components/arc/common/voice_interaction.mojom:15: bool success; On 2017/03/10 22:36:10, Muyuan wrote: > On 2017/03/10 06:14:50, hidehiko wrote: > > Is there a case that only some part of children has "success = false"? > > If so it's good to be noted here with when it happens. > > If not, probably instead of the member here, the method signature will be, > > either; > > > > - GetVoiceInteractionStructure@1() => (bool success, VoiceInteractionStructure > > structure); > > - GetVoiceInteractionStructure@1() => (Status status, > VoiceInteractionStructure > > structure); // status represent success or error code incl reason., or > > - GetVoiceInteractionStructure@1() => (VoiceInteractionStructure? structure); > > // return null structure on failure. > > > > or something? > > Sure. How about using Optional<T> for this? Optional<T> makes more sense. https://codereview.chromium.org/2731403007/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/BUILD.gn (right): https://codereview.chromium.org/2731403007/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/BUILD.gn:237: "arc/arc_voice_interaction_service.cc", nit: can you move these files to a new directory? arc/voice_interaction sounds like a good candidate. https://codereview.chromium.org/2731403007/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_voice_interaction_service.cc (right): https://codereview.chromium.org/2731403007/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:23: constexpr char kEnableVoiceInteraction[] = "enable-voice-interaction"; This should be in chromeos/chromeos_switches.h https://codereview.chromium.org/2731403007/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:42: ArcVoiceInteractionService* service_; Please make a note that ArcVoiceInteractionService owns this object. Same in L63. https://codereview.chromium.org/2731403007/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:101: LOG(ERROR) << "Registering key: " this is not an error. use VLOG(1) or DVLOG(1). https://codereview.chromium.org/2731403007/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:104: if (base::CommandLine::ForCurrentProcess()->HasSwitch( nit: use the guard clause pattern: if (!base::CommandLine::...) return; // Register accelerator. https://codereview.chromium.org/2731403007/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:113: if (base::CommandLine::ForCurrentProcess()->HasSwitch( Same as above. https://codereview.chromium.org/2731403007/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_voice_interaction_service.h (right): https://codereview.chromium.org/2731403007/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.h:19: // to Arc to be used by VoiceInteractionSession. This class Lives on the UI nit: ARC. https://codereview.chromium.org/2731403007/diff/80001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/2731403007/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_service.h:97: voice_interaction_home() { sorry for the bad suggestion in the previous iteration :( After reading through the design doc, this should be called voice_interaction_arc_home. https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... File components/arc/common/voice_interaction.mojom (right): https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... components/arc/common/voice_interaction.mojom:15: bool success; remove per the Optional<T> discussion. https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... components/arc/common/voice_interaction.mojom:18: int32 x; Can this be a https://cs.chromium.org/chromium/src/ui/gfx/geometry/mojo/geometry.mojom?l=27 instead? https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... components/arc/common/voice_interaction.mojom:24: array<uint8> text; Can this be https://cs.chromium.org/chromium/src/mojo/common/string16.mojom instead? That way we can deal with base::string16 instead of having to manually perform conversions. https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... components/arc/common/voice_interaction.mojom:39: int32 end_selection; Can you extract the start_selection and end_selection into a different structure and add a nullable reference to such structure here to avoid the |has_selection| bool? https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... components/arc/common/voice_interaction.mojom:54: CaptureFocusedWindow@0() => (array<uint8> png_data); What is the relationship between CaptureFocusedWindow and GetVoiceInteractionStructure? Will they always be called in tandem? Can they be collapsed to a single method to reduce raciness? If they cannot be collapsed to a single method (possibly because they are called from different instances), would it be possible to split the host such that each class only implements the methods that the instance can call?
https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... File components/arc/common/voice_interaction.mojom (right): https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... components/arc/common/voice_interaction.mojom:75: remove trailing empty lines.
https://codereview.chromium.org/2731403007/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_voice_interaction_service.cc (right): https://codereview.chromium.org/2731403007/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:23: constexpr char kEnableVoiceInteraction[] = "enable-voice-interaction"; On 2017/03/13 17:39:34, Luis Héctor Chávez wrote: > This should be in chromeos/chromeos_switches.h Done. https://codereview.chromium.org/2731403007/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:42: ArcVoiceInteractionService* service_; On 2017/03/13 17:39:34, Luis Héctor Chávez wrote: > Please make a note that ArcVoiceInteractionService owns this object. Same in > L63. Done. https://codereview.chromium.org/2731403007/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:101: LOG(ERROR) << "Registering key: " On 2017/03/13 17:39:34, Luis Héctor Chávez wrote: > this is not an error. use VLOG(1) or DVLOG(1). removed. https://codereview.chromium.org/2731403007/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:104: if (base::CommandLine::ForCurrentProcess()->HasSwitch( On 2017/03/13 17:39:34, Luis Héctor Chávez wrote: > nit: use the guard clause pattern: > > if (!base::CommandLine::...) > return; > // Register accelerator. Done. https://codereview.chromium.org/2731403007/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:113: if (base::CommandLine::ForCurrentProcess()->HasSwitch( On 2017/03/13 17:39:34, Luis Héctor Chávez wrote: > Same as above. Done. https://codereview.chromium.org/2731403007/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_voice_interaction_service.h (right): https://codereview.chromium.org/2731403007/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_voice_interaction_service.h:19: // to Arc to be used by VoiceInteractionSession. This class Lives on the UI On 2017/03/13 17:39:34, Luis Héctor Chávez wrote: > nit: ARC. Done. https://codereview.chromium.org/2731403007/diff/80001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/2731403007/diff/80001/components/arc/arc_brid... components/arc/arc_bridge_service.h:97: voice_interaction_home() { On 2017/03/13 17:39:34, Luis Héctor Chávez wrote: > sorry for the bad suggestion in the previous iteration :( After reading through > the design doc, this should be called voice_interaction_arc_home. Done. https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... File components/arc/common/voice_interaction.mojom (right): https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... components/arc/common/voice_interaction.mojom:15: bool success; On 2017/03/13 17:39:34, Luis Héctor Chávez wrote: > remove per the Optional<T> discussion. OK.. after a second look, it seems that the mojo generator in Java does not understand optional<T> yet? https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... components/arc/common/voice_interaction.mojom:18: int32 x; does geometry exists in Java side? https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... components/arc/common/voice_interaction.mojom:24: array<uint8> text; same question as above.. https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... components/arc/common/voice_interaction.mojom:39: int32 end_selection; On 2017/03/13 17:39:35, Luis Héctor Chávez wrote: > Can you extract the start_selection and end_selection into a different structure > and add a nullable reference to such structure here to avoid the |has_selection| > bool? So looks like optional is not implemented in the Java codegen.. Is there any alternative to represent nullable object? https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... components/arc/common/voice_interaction.mojom:54: CaptureFocusedWindow@0() => (array<uint8> png_data); On 2017/03/13 17:39:34, Luis Héctor Chávez wrote: > What is the relationship between CaptureFocusedWindow and > GetVoiceInteractionStructure? Will they always be called in tandem? Can they be > collapsed to a single method to reduce raciness? > > If they cannot be collapsed to a single method (possibly because they are called > from different instances), would it be possible to split the host such that each > class only implements the methods that the instance can call? These are relatively independent by now.. One is called by system_server and the other is called by ArcHomeService (which is not implemented atm). But the problem here is that the future shape of both calls and additional functionalities are not defined by UX.. so I don't know if separating them atm is a good idea.. https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... components/arc/common/voice_interaction.mojom:75: On 2017/03/13 17:41:02, Luis Héctor Chávez wrote: > remove trailing empty lines. Done.
https://codereview.chromium.org/2731403007/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2731403007/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_service_launcher.cc:141: base::MakeUnique<ArcVoiceInteractionService>(arc_bridge_service)); On 2017/03/09 20:44:44, Muyuan wrote: > On 2017/03/09 05:05:47, xc wrote: > > Can we use the flag to guard the creation of the service? > > Not quite sure about this. Going to check with luis@ Yes, that also works. https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... File components/arc/common/voice_interaction.mojom (right): https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... components/arc/common/voice_interaction.mojom:15: bool success; On 2017/03/14 01:38:14, Muyuan wrote: > On 2017/03/13 17:39:34, Luis Héctor Chávez wrote: > > remove per the Optional<T> discussion. > > OK.. after a second look, it seems that the mojo generator in Java does not > understand optional<T> yet? it does: it generates a null Object. unfortunately java.util.Optional<T> is new to Java 8, so we'll have to live with the current behavior until we drop support for Marshmallow across the board. https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... components/arc/common/voice_interaction.mojom:18: int32 x; On 2017/03/14 01:38:14, Muyuan wrote: > does geometry exists in Java side? on further inspection, you should be able to use this instead: https://cs.chromium.org/chromium/src/components/arc/common/screen_rect.mojom it already maps to the correct types on both sides. https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... components/arc/common/voice_interaction.mojom:24: array<uint8> text; On 2017/03/14 01:38:14, Muyuan wrote: > same question as above.. you might need to do a similar trick as with ScreenRect to be able to map it to an array<uint16> https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... components/arc/common/voice_interaction.mojom:54: CaptureFocusedWindow@0() => (array<uint8> png_data); On 2017/03/14 01:38:14, Muyuan wrote: > On 2017/03/13 17:39:34, Luis Héctor Chávez wrote: > > What is the relationship between CaptureFocusedWindow and > > GetVoiceInteractionStructure? Will they always be called in tandem? Can they > be > > collapsed to a single method to reduce raciness? > > > > If they cannot be collapsed to a single method (possibly because they are > called > > from different instances), would it be possible to split the host such that > each > > class only implements the methods that the instance can call? > > These are relatively independent by now.. One is called by system_server and the > other is called by ArcHomeService (which is not implemented atm). But the > problem here is that the future shape of both calls and additional > functionalities are not defined by UX.. so I don't know if separating them atm > is a good idea.. the problem is that substantially changing interfaces after the fact is considerably more expensive. is there any way we can finalize the design before committing to an interface?
https://codereview.chromium.org/2731403007/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_voice_interaction_service.cc (right): https://codereview.chromium.org/2731403007/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:40: ArcVoiceInteractionService* service_; nit: ArcVoiceInteractionService* const service_; const ptr since |service_| is not expected to change during the lifetime of ArcHomeObserver. https://codereview.chromium.org/2731403007/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:62: ArcVoiceInteractionService* service_; nit: ditto, use const pointer https://codereview.chromium.org/2731403007/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:102: return; nit: wrap with {} since condition takes more than one line. https://codereview.chromium.org/2731403007/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:103: ash::WmShell::Get()->accelerator_controller()->Register( nit: insert an empty line before https://codereview.chromium.org/2731403007/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:111: return; nit: wrap with {} https://codereview.chromium.org/2731403007/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:112: ash::WmShell::Get()->accelerator_controller()->Unregister( nit: accelerator_controller()->UnregisterAll(this) ? Then we don't need to repeat the Accelerator again. https://codereview.chromium.org/2731403007/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:136: LOG(ERROR) << "ArcVoiceInteractionSerivce::GetVoiceInteractionStructure " nit: NOTIMPLEMENTED(); https://codereview.chromium.org/2731403007/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:149: LOG(ERROR) nit: NOTIMPLEMENTED(); https://codereview.chromium.org/2731403007/diff/120001/components/arc/common/... File components/arc/common/voice_interaction.mojom (right): https://codereview.chromium.org/2731403007/diff/120001/components/arc/common/... components/arc/common/voice_interaction.mojom:24: array<uint8> text; Do we have to pass utf16 like this? It is not a good way to pass such data, things such as endianess might cause tricky bugs. Have we considered using mojo.common.mojom.String16 ? https://cs.chromium.org/chromium/src/mojo/common/string16.mojom?rcl=bbd052efa...
https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... File components/arc/common/voice_interaction.mojom (right): https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... components/arc/common/voice_interaction.mojom:15: bool success; On 2017/03/14 02:11:33, Luis Héctor Chávez wrote: > On 2017/03/14 01:38:14, Muyuan wrote: > > On 2017/03/13 17:39:34, Luis Héctor Chávez wrote: > > > remove per the Optional<T> discussion. > > > > OK.. after a second look, it seems that the mojo generator in Java does not > > understand optional<T> yet? > > it does: it generates a null Object. unfortunately java.util.Optional<T> is new > to Java 8, so we'll have to live with the current behavior until we drop support > for Marshmallow across the board. Done. https://codereview.chromium.org/2731403007/diff/80001/components/arc/common/v... components/arc/common/voice_interaction.mojom:18: int32 x; On 2017/03/14 02:11:33, Luis Héctor Chávez wrote: > On 2017/03/14 01:38:14, Muyuan wrote: > > does geometry exists in Java side? > > on further inspection, you should be able to use this instead: > https://cs.chromium.org/chromium/src/components/arc/common/screen_rect.mojom > > it already maps to the correct types on both sides. Done. https://codereview.chromium.org/2731403007/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_voice_interaction_service.cc (right): https://codereview.chromium.org/2731403007/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:40: ArcVoiceInteractionService* service_; On 2017/03/14 17:55:31, xiyuan wrote: > nit: ArcVoiceInteractionService* const service_; > > const ptr since |service_| is not expected to change during the lifetime of > ArcHomeObserver. Done. https://codereview.chromium.org/2731403007/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:62: ArcVoiceInteractionService* service_; On 2017/03/14 17:55:31, xiyuan wrote: > nit: ditto, use const pointer Done. https://codereview.chromium.org/2731403007/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:102: return; On 2017/03/14 17:55:31, xiyuan wrote: > nit: wrap with {} since condition takes more than one line. Done. https://codereview.chromium.org/2731403007/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:103: ash::WmShell::Get()->accelerator_controller()->Register( On 2017/03/14 17:55:31, xiyuan wrote: > nit: insert an empty line before Done. https://codereview.chromium.org/2731403007/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:111: return; On 2017/03/14 17:55:31, xiyuan wrote: > nit: wrap with {} Done. https://codereview.chromium.org/2731403007/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:112: ash::WmShell::Get()->accelerator_controller()->Unregister( On 2017/03/14 17:55:31, xiyuan wrote: > nit: accelerator_controller()->UnregisterAll(this) ? Then we don't need to > repeat the Accelerator again. Done. https://codereview.chromium.org/2731403007/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:136: LOG(ERROR) << "ArcVoiceInteractionSerivce::GetVoiceInteractionStructure " On 2017/03/14 17:55:31, xiyuan wrote: > nit: NOTIMPLEMENTED(); Done. https://codereview.chromium.org/2731403007/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_voice_interaction_service.cc:149: LOG(ERROR) On 2017/03/14 17:55:31, xiyuan wrote: > nit: NOTIMPLEMENTED(); Done. https://codereview.chromium.org/2731403007/diff/120001/components/arc/common/... File components/arc/common/voice_interaction.mojom (right): https://codereview.chromium.org/2731403007/diff/120001/components/arc/common/... components/arc/common/voice_interaction.mojom:24: array<uint8> text; On 2017/03/14 17:55:31, xiyuan wrote: > Do we have to pass utf16 like this? It is not a good way to pass such data, > things such as endianess might cause tricky bugs. > > Have we considered using mojo.common.mojom.String16 ? > > https://cs.chromium.org/chromium/src/mojo/common/string16.mojom?rcl=bbd052efa... String16 does not seem to be implemented in Java side (got codegen error) and my experiment with string16 indicates that it's UTF-16 LE regardless of architecture.
lgtm But please wait for Luis. And think you need one of //ipc/SECURITY_OWNERS for the mojom files. https://codereview.chromium.org/2731403007/diff/120001/components/arc/common/... File components/arc/common/voice_interaction.mojom (right): https://codereview.chromium.org/2731403007/diff/120001/components/arc/common/... components/arc/common/voice_interaction.mojom:24: array<uint8> text; On 2017/03/14 21:37:01, Muyuan wrote: > On 2017/03/14 17:55:31, xiyuan wrote: > > Do we have to pass utf16 like this? It is not a good way to pass such data, > > things such as endianess might cause tricky bugs. > > > > Have we considered using mojo.common.mojom.String16 ? > > > > > https://cs.chromium.org/chromium/src/mojo/common/string16.mojom?rcl=bbd052efa... > > String16 does not seem to be implemented in Java side (got codegen error) and my > experiment with string16 indicates that it's UTF-16 LE regardless of > architecture. Acknowledged.
Thanks, this looks much better! https://codereview.chromium.org/2731403007/diff/160001/components/arc/common/... File components/arc/common/voice_interaction_arc_home.mojom (right): https://codereview.chromium.org/2731403007/diff/160001/components/arc/common/... components/arc/common/voice_interaction_arc_home.mojom:52: interface VoiceInteractionArcHomeHost { Maybe remove this file until we actually need it. https://codereview.chromium.org/2731403007/diff/160001/components/arc/common/... File components/arc/common/voice_interaction_framework.mojom (right): https://codereview.chromium.org/2731403007/diff/160001/components/arc/common/... components/arc/common/voice_interaction_framework.mojom:12: // Returns a screenshot of current focused window or empty bytes if nit: s/current focused/currently focused/, s/empty bytes/an empty array/
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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
https://codereview.chromium.org/2731403007/diff/160001/components/arc/common/... File components/arc/common/voice_interaction_arc_home.mojom (right): https://codereview.chromium.org/2731403007/diff/160001/components/arc/common/... components/arc/common/voice_interaction_arc_home.mojom:52: interface VoiceInteractionArcHomeHost { On 2017/03/16 16:30:20, Luis Héctor Chávez wrote: > Maybe remove this file until we actually need it. Acknowledged. https://codereview.chromium.org/2731403007/diff/160001/components/arc/common/... File components/arc/common/voice_interaction_framework.mojom (right): https://codereview.chromium.org/2731403007/diff/160001/components/arc/common/... components/arc/common/voice_interaction_framework.mojom:12: // Returns a screenshot of current focused window or empty bytes if On 2017/03/16 16:30:20, Luis Héctor Chávez wrote: > nit: s/current focused/currently focused/, s/empty bytes/an empty array/ Done.
FYI: build failure looks caused by conflict. Could you rebase on ToT? (Then you'll need to update the ID of the new methods.)
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...
On 2017/03/16 20:49:34, hidehiko wrote: > FYI: build failure looks caused by conflict. > Could you rebase on ToT? (Then you'll need to update the ID of the new methods.) rebased.
muyuanli@chromium.org changed reviewers: + dcheng@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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...
lgtm with a nit. Please keep in mind that security reviewers are not super fans of TODOs in IPC implementations (for the record, neither am I :P), so you'll also have to add them on the changes where you are actually adding the implementation. https://codereview.chromium.org/2731403007/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2731403007/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.cc:146: chromeos::switches::kEnableVoiceInteraction)) { nit: we prefer for a single class to have complete ownership of flags. Can you create a static function in your class (called IsVoiceInteractionEnabled)?
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_...)
LGTM https://codereview.chromium.org/2731403007/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2731403007/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:71: // details needs to be implemented in the future. I don't think this is likely to have a security impact, but please add an ipc security owner on the followup CL that implements this.
The CQ bit was checked by muyuanli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, lhchavez@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2731403007/#ps240001 (title: "fix build and address review comments")
https://codereview.chromium.org/2731403007/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2731403007/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.cc:146: chromeos::switches::kEnableVoiceInteraction)) { On 2017/03/17 21:56:02, Luis Héctor Chávez wrote: > nit: we prefer for a single class to have complete ownership of flags. Can you > create a static function in your class (called IsVoiceInteractionEnabled)? Done. https://codereview.chromium.org/2731403007/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2731403007/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:71: // details needs to be implemented in the future. On 2017/03/17 22:55:16, dcheng wrote: > I don't think this is likely to have a security impact, but please add an ipc > security owner on the followup CL that implements this. Acknowledged.
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by muyuanli@chromium.org
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_...)
https://codereview.chromium.org/2731403007/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2731403007/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:22: bool ArcVoiceInteractionFrameworkService::IsVoiceInteractionEnabled() { nit: add // static above https://codereview.chromium.org/2731403007/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h (right): https://codereview.chromium.org/2731403007/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h:45: // Whether enable-voice-interaction switch presents. nit: s/presents/is present/.
https://codereview.chromium.org/2731403007/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2731403007/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:22: bool ArcVoiceInteractionFrameworkService::IsVoiceInteractionEnabled() { On 2017/03/18 03:02:12, Luis Héctor Chávez wrote: > nit: add > > // static > > above Done. https://codereview.chromium.org/2731403007/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h (right): https://codereview.chromium.org/2731403007/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h:45: // Whether enable-voice-interaction switch presents. On 2017/03/18 03:02:12, Luis Héctor Chávez wrote: > nit: s/presents/is present/. Done.
The CQ bit was checked by muyuanli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, lhchavez@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2731403007/#ps260001 (title: "addressed review comments")
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": 260001, "attempt_start_ts": 1489883064963180,
"parent_rev": "bc2ff95391de6df8d51574d4357da49eda666678", "commit_rev":
"d8b8f945e7c2c694b202b9a0cd2e63db34ac30c1"}
Message was sent while issue was closed.
Description was changed from ========== add voice interaction shortcut. BUG=699309 Test=Meta + a starts voice interaction session in CrOS. ========== to ========== add voice interaction shortcut. BUG=699309 Test=Meta + a starts voice interaction session in CrOS. Review-Url: https://codereview.chromium.org/2731403007 Cr-Commit-Position: refs/heads/master@{#457985} Committed: https://chromium.googlesource.com/chromium/src/+/d8b8f945e7c2c694b202b9a0cd2e... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/d8b8f945e7c2c694b202b9a0cd2e...
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/2757103002/ by jam@chromium.org. The reason for reverting is: Breaks debug chromeos build https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.chromiumos%2FLi... FAILED: obj/chrome/browser/chromeos/chromeos/arc_voice_interaction_framework_service.o /b/c/goma_client/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/chrome/browser/chromeos/chromeos/arc_voice_interaction_framework_service.o.d -DV8_DEPRECATION_WARNINGS -DUSE_UDEV -DUSE_ASH=1 -DUSE_AURA=1 -DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -DUSE_X11=1 -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD -DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED -DCR_CLANG_REVISION=\"296321-1\" -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DCOMPONENT_BUILD -DOS_CHROMEOS -D_DEBUG -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -D_GLIBCXX_DEBUG=1 -DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_32 -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_26 -DGL_GLEXT_PROTOTYPES -DUSE_GLX -DUSE_EGL -DTOOLKIT_VIEWS=1 -DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DHAVE_PTHREAD -DPROTOBUF_USE_DLLS -DSK_IGNORE_DW_GRAY_FIX -DSK_IGNORE_DIRECTWRITE_GASP_FIX -DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DSKIA_DLL -DGR_GL_IGNORE_ES3_MSAA=0 -DSK_SUPPORT_GPU=1 -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -DV8_USE_EXTERNAL_STARTUP_DATA -DBORINGSSL_SHARED_LIBRARY -DUSING_V8_SHARED -DLEVELDB_PLATFORM_CHROMIUM=1 -DFEATURE_ENABLE_VOICEMAIL -DEXPAT_RELATIVE_PATH -DGTEST_RELATIVE_PATH -DNO_MAIN_THREAD_WRAPPING -DNO_SOUND_SYSTEM -DWEBRTC_CHROMIUM_BUILD -DWEBRTC_POSIX -DWEBRTC_LINUX -DCHROMEOS -DUSING_V8_SHARED -I../.. -Igen -I../../build/linux/ubuntu_precise_amd64-sysroot/usr/include/glib-2.0 -I../../build/linux/ubuntu_precise_amd64-sysroot/usr/lib/x86_64-linux-gnu/glib-2.0/include -I../../third_party/khronos -I../../gpu -I../../third_party/libwebp -I../../third_party/protobuf/src -Igen/protoc_out -I../../third_party/protobuf/src -I../../skia/config -I../../skia/ext -I../../third_party/skia/include/c -I../../third_party/skia/include/config -I../../third_party/skia/include/core -I../../third_party/skia/include/effects -I../../third_party/skia/include/images -I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops -I../../third_party/skia/include/pdf -I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils -I../../third_party/skia/include/gpu -I../../third_party/skia/src/gpu -I../../third_party/skia/src/sksl -I../../third_party/ced/src -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n -Igen -I../../third_party/mesa/src/include -I../../third_party/libwebm/source -I../../third_party/boringssl/src/include -I../../build/linux/ubuntu_precise_amd64-sysroot/usr/include/nss -I../../build/linux/ubuntu_precise_amd64-sysroot/usr/include/nspr -Igen -I../../third_party/WebKit -Igen/third_party/WebKit -I../../v8/include -Igen/v8/include -Igen/components/metrics/proto -I../../third_party/re2/src -Igen -I../../build/linux/ubuntu_precise_amd64-sysroot/usr/include/dbus-1.0 -I../../build/linux/ubuntu_precise_amd64-sysroot/usr/lib/x86_64-linux-gnu/dbus-1.0/include -Igen -I../../third_party/cacheinvalidation/overrides -I../../third_party/cacheinvalidation/src -I../../third_party/leveldatabase -I../../third_party/leveldatabase/src -I../../third_party/leveldatabase/src/include -I../../third_party/libusb/src/libusb -I../../third_party/webrtc_overrides -I../../testing/gtest/include -I../../third_party -I../../third_party/webrtc_overrides -I../../third_party -I../../third_party/zlib -I../../v8/include -Igen/v8/include -fno-strict-aliasing -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -funwind-tables -fPIC -pipe -B../../third_party/binutils/Linux_x64/Release/bin -fcolor-diagnostics -m64 -march=x86-64 -pthread -Wall -Werror -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -Wno-shift-negative-value -Wno-undefined-var-template -Wno-nonportable-include-path -Wno-address-of-packed-member -Wno-unused-lambda-capture -Wno-user-defined-warnings -O0 -fno-omit-frame-pointer -g2 -gsplit-dwarf --sysroot=../../build/linux/ubuntu_precise_amd64-sysroot -fvisibility=hidden -Xclang -load -Xclang ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang check-auto-raw-pointer -Xclang -plugin-arg-find-bad-constructs -Xclang check-ipc -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -Wexit-time-destructors -Wno-header-guard -fvisibility-inlines-hidden -Wno-undefined-bool-conversion -Wno-tautological-undefined-compare -std=gnu++11 -Wno-reserved-user-defined-literal -fno-rtti -fno-exceptions -c ../../chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc -o obj/chrome/browser/chromeos/chromeos/arc_voice_interaction_framework_service.o ../../chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:78:16: error: chosen constructor is explicit in copy-initialization callback.Run({}); ^~ ../../build/linux/ubuntu_precise_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/debug/vector:77:7: note: explicit constructor declared here vector(const _Allocator& __a = _Allocator()) ^ ../../base/callback.h:81:17: note: passing argument to parameter 'args' here R Run(Args... args) const { ^ 1 error generated..
Message was sent while issue was closed.
Findit confirmed this CL at revision 457985 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
On 2017/03/19 01:28:20, jam wrote: > A revert of this CL (patchset #14 id:260001) has been created in > https://codereview.chromium.org/2757103002/ by mailto:jam@chromium.org. > > The reason for reverting is: Breaks debug chromeos build > > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.chromiumos%2FLi... > > FAILED: > obj/chrome/browser/chromeos/chromeos/arc_voice_interaction_framework_service.o > /b/c/goma_client/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++ > -MMD -MF > obj/chrome/browser/chromeos/chromeos/arc_voice_interaction_framework_service.o.d > -DV8_DEPRECATION_WARNINGS -DUSE_UDEV -DUSE_ASH=1 -DUSE_AURA=1 -DUSE_PANGO=1 > -DUSE_CAIRO=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -DUSE_X11=1 -DFULL_SAFE_BROWSING > -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD > -DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED > -DCR_CLANG_REVISION=\"296321-1\" -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE > -D_LARGEFILE64_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS > -DCOMPONENT_BUILD -DOS_CHROMEOS -D_DEBUG -DDYNAMIC_ANNOTATIONS_ENABLED=1 > -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -D_GLIBCXX_DEBUG=1 > -DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_32 > -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_26 -DGL_GLEXT_PROTOTYPES -DUSE_GLX > -DUSE_EGL -DTOOLKIT_VIEWS=1 -DGOOGLE_PROTOBUF_NO_RTTI > -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DHAVE_PTHREAD -DPROTOBUF_USE_DLLS > -DSK_IGNORE_DW_GRAY_FIX -DSK_IGNORE_DIRECTWRITE_GASP_FIX > -DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DSKIA_DLL -DGR_GL_IGNORE_ES3_MSAA=0 > -DSK_SUPPORT_GPU=1 -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 > -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -DV8_USE_EXTERNAL_STARTUP_DATA > -DBORINGSSL_SHARED_LIBRARY -DUSING_V8_SHARED -DLEVELDB_PLATFORM_CHROMIUM=1 > -DFEATURE_ENABLE_VOICEMAIL -DEXPAT_RELATIVE_PATH -DGTEST_RELATIVE_PATH > -DNO_MAIN_THREAD_WRAPPING -DNO_SOUND_SYSTEM -DWEBRTC_CHROMIUM_BUILD > -DWEBRTC_POSIX -DWEBRTC_LINUX -DCHROMEOS -DUSING_V8_SHARED -I../.. -Igen > -I../../build/linux/ubuntu_precise_amd64-sysroot/usr/include/glib-2.0 > -I../../build/linux/ubuntu_precise_amd64-sysroot/usr/lib/x86_64-linux-gnu/glib-2.0/include > -I../../third_party/khronos -I../../gpu -I../../third_party/libwebp > -I../../third_party/protobuf/src -Igen/protoc_out > -I../../third_party/protobuf/src -I../../skia/config -I../../skia/ext > -I../../third_party/skia/include/c -I../../third_party/skia/include/config > -I../../third_party/skia/include/core -I../../third_party/skia/include/effects > -I../../third_party/skia/include/images -I../../third_party/skia/include/lazy > -I../../third_party/skia/include/pathops -I../../third_party/skia/include/pdf > -I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports > -I../../third_party/skia/include/utils -I../../third_party/skia/include/gpu > -I../../third_party/skia/src/gpu -I../../third_party/skia/src/sksl > -I../../third_party/ced/src -I../../third_party/icu/source/common > -I../../third_party/icu/source/i18n -Igen -I../../third_party/mesa/src/include > -I../../third_party/libwebm/source -I../../third_party/boringssl/src/include > -I../../build/linux/ubuntu_precise_amd64-sysroot/usr/include/nss > -I../../build/linux/ubuntu_precise_amd64-sysroot/usr/include/nspr -Igen > -I../../third_party/WebKit -Igen/third_party/WebKit -I../../v8/include > -Igen/v8/include -Igen/components/metrics/proto -I../../third_party/re2/src > -Igen -I../../build/linux/ubuntu_precise_amd64-sysroot/usr/include/dbus-1.0 > -I../../build/linux/ubuntu_precise_amd64-sysroot/usr/lib/x86_64-linux-gnu/dbus-1.0/include > -Igen -I../../third_party/cacheinvalidation/overrides > -I../../third_party/cacheinvalidation/src -I../../third_party/leveldatabase > -I../../third_party/leveldatabase/src > -I../../third_party/leveldatabase/src/include > -I../../third_party/libusb/src/libusb -I../../third_party/webrtc_overrides > -I../../testing/gtest/include -I../../third_party > -I../../third_party/webrtc_overrides -I../../third_party > -I../../third_party/zlib -I../../v8/include -Igen/v8/include > -fno-strict-aliasing -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= > -D__TIMESTAMP__= -funwind-tables -fPIC -pipe > -B../../third_party/binutils/Linux_x64/Release/bin -fcolor-diagnostics -m64 > -march=x86-64 -pthread -Wall -Werror -Wextra -Wno-missing-field-initializers > -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default > -Wno-deprecated-register -Wno-unneeded-internal-declaration > -Wno-inconsistent-missing-override -Wno-shift-negative-value > -Wno-undefined-var-template -Wno-nonportable-include-path > -Wno-address-of-packed-member -Wno-unused-lambda-capture > -Wno-user-defined-warnings -O0 -fno-omit-frame-pointer -g2 -gsplit-dwarf > --sysroot=../../build/linux/ubuntu_precise_amd64-sysroot -fvisibility=hidden > -Xclang -load -Xclang > ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang > -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs > -Xclang check-auto-raw-pointer -Xclang -plugin-arg-find-bad-constructs -Xclang > check-ipc -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare > -Wexit-time-destructors -Wno-header-guard -fvisibility-inlines-hidden > -Wno-undefined-bool-conversion -Wno-tautological-undefined-compare -std=gnu++11 > -Wno-reserved-user-defined-literal -fno-rtti -fno-exceptions -c > ../../chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc > -o > obj/chrome/browser/chromeos/chromeos/arc_voice_interaction_framework_service.o > ../../chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:78:16: > error: chosen constructor is explicit in copy-initialization > callback.Run({}); > ^~ > ../../build/linux/ubuntu_precise_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/debug/vector:77:7: > note: explicit constructor declared here > vector(const _Allocator& __a = _Allocator()) > ^ > ../../base/callback.h:81:17: note: passing argument to parameter 'args' here > R Run(Args... args) const { > ^ > 1 error generated.. Just out of curiosity, why CQ is unable to catch this? Seems they are different configurations? |
