Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2803403002:
Support region selection for voice interaction session (Closed)
|
Created:
3 years, 8 months ago by Vladislav Kaznacheev Modified:
3 years, 8 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org, viettrungluu+watch_chromium.org, hidehiko+watch_chromium.org, sadrul, abarth-chromium, Aaron Boodman, lhchavez+watch_chromium.org, yzshen+watch_chromium.org, victorhsieh+watch_chromium.org, kalyank, darin (slow to review), davemoore+watch_chromium.org, qsr+mojo_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport region selection for voice interaction session
Introducing two new ways to invoke a voice interaction
session for a specific region:
1. If kEnableVoiceInteraction is enabled, the "Capture region"
stylus tool does not take a screenshot but starts a voice
interaction session for that region.
2. Shift+Search+A invokes the "metalayer" - an alternative
region selection tool.
BUG=b:37165189
TEST="Capture region" stylus tool start voice Interaction session.
Shift+Search+A invokes the "metalayer".
Review-Url: https://codereview.chromium.org/2803403002
Cr-Commit-Position: refs/heads/master@{#464070}
Committed: https://chromium.googlesource.com/chromium/src/+/91b3f7e7b1f592052614ffd6eb61d60f87dd2571
Patch Set 1 #
Total comments: 1
Patch Set 2 : Removed 'auto' #
Total comments: 10
Patch Set 3 : Addressed comments #
Total comments: 6
Patch Set 4 : Addressed comments #Patch Set 5 : Added comments re experimental mode #
Total comments: 2
Patch Set 6 : Addressed comments #
Messages
Total messages: 41 (29 generated)
kaznacheev@chromium.org changed reviewers: + xiaohuic@chromium.org
lgtm https://codereview.chromium.org/2803403002/diff/1/chrome/browser/ui/ash/palet... File chrome/browser/ui/ash/palette_delegate_chromeos.cc (right): https://codereview.chromium.org/2803403002/diff/1/chrome/browser/ui/ash/palet... chrome/browser/ui/ash/palette_delegate_chromeos.cc:46: auto framework = I was asked to use concrete type instead of auto in this complex situation last time.
Patchset #2 (id:20001) has been deleted
kaznacheev@chromium.org changed reviewers: + dcheng@chromium.org, lhchavez@chromium.org
Please review: dcheng@ *.mojom lhchavez@ components/arc
mojo lgtm (I can't help but wonder what voice interaction has to do with screenshots though... maybe I'm misunderstanding something fundamental) https://codereview.chromium.org/2803403002/diff/40001/chrome/browser/ui/ash/p... File chrome/browser/ui/ash/palette_delegate_chromeos.cc (right): https://codereview.chromium.org/2803403002/diff/40001/chrome/browser/ui/ash/p... chrome/browser/ui/ash/palette_delegate_chromeos.cc:52: LOG(ERROR) << "Cannot connect to ARC"; Nit: please use DLOG
*/arc/* lgtm with nits. https://codereview.chromium.org/2803403002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2803403002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:84: if (accelerator.modifiers() & ui::EF_SHIFT_DOWN) { nit: accelerator.IsShiftDown() https://codereview.chromium.org/2803403002/diff/40001/chrome/browser/ui/ash/p... File chrome/browser/ui/ash/palette_delegate_chromeos.cc (right): https://codereview.chromium.org/2803403002/diff/40001/chrome/browser/ui/ash/p... chrome/browser/ui/ash/palette_delegate_chromeos.cc:40: Profile* profile_; nit: Profile* const profile_; https://codereview.chromium.org/2803403002/diff/40001/chrome/browser/ui/ash/p... chrome/browser/ui/ash/palette_delegate_chromeos.cc:52: LOG(ERROR) << "Cannot connect to ARC"; On 2017/04/11 07:39:47, dcheng wrote: > Nit: please use DLOG You can also remove this logging completely: ARC_GET_INSTANCE_FOR_METHOD already does its own logging. https://codereview.chromium.org/2803403002/diff/40001/chrome/browser/ui/ash/p... chrome/browser/ui/ash/palette_delegate_chromeos.cc:186: chromeos::switches::kEnableVoiceInteraction) && nit: ArcVoiceInteractionFrameworkService::IsVoiceInteractionEnabled() (let's try to keep switch inspection/manipulation constrained to a single class).
The CQ bit was checked by kaznacheev@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
kaznacheev@chromium.org changed reviewers: + oshima@chromium.org
Please review: oshima@ chrome/browser/ui/ash/palette_delegate_chromeos.* https://codereview.chromium.org/2803403002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2803403002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:84: if (accelerator.modifiers() & ui::EF_SHIFT_DOWN) { On 2017/04/11 16:11:16, Luis Héctor Chávez wrote: > nit: accelerator.IsShiftDown() Done. https://codereview.chromium.org/2803403002/diff/40001/chrome/browser/ui/ash/p... File chrome/browser/ui/ash/palette_delegate_chromeos.cc (right): https://codereview.chromium.org/2803403002/diff/40001/chrome/browser/ui/ash/p... chrome/browser/ui/ash/palette_delegate_chromeos.cc:40: Profile* profile_; On 2017/04/11 16:11:16, Luis Héctor Chávez wrote: > nit: Profile* const profile_; Removed as unused. https://codereview.chromium.org/2803403002/diff/40001/chrome/browser/ui/ash/p... chrome/browser/ui/ash/palette_delegate_chromeos.cc:52: LOG(ERROR) << "Cannot connect to ARC"; On 2017/04/11 07:39:47, dcheng wrote: > Nit: please use DLOG Acknowledged. https://codereview.chromium.org/2803403002/diff/40001/chrome/browser/ui/ash/p... chrome/browser/ui/ash/palette_delegate_chromeos.cc:52: LOG(ERROR) << "Cannot connect to ARC"; On 2017/04/11 16:11:16, Luis Héctor Chávez wrote: > On 2017/04/11 07:39:47, dcheng wrote: > > Nit: please use DLOG > > You can also remove this logging completely: ARC_GET_INSTANCE_FOR_METHOD already > does its own logging. Done. https://codereview.chromium.org/2803403002/diff/40001/chrome/browser/ui/ash/p... chrome/browser/ui/ash/palette_delegate_chromeos.cc:186: chromeos::switches::kEnableVoiceInteraction) && On 2017/04/11 16:11:16, Luis Héctor Chávez wrote: > nit: ArcVoiceInteractionFrameworkService::IsVoiceInteractionEnabled() > > (let's try to keep switch inspection/manipulation constrained to a single > class). Done.
The CQ bit was checked by kaznacheev@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2803403002/diff/60001/chrome/browser/ui/ash/p... File chrome/browser/ui/ash/palette_delegate_chromeos.cc (right): https://codereview.chromium.org/2803403002/diff/60001/chrome/browser/ui/ash/p... chrome/browser/ui/ash/palette_delegate_chromeos.cc:44: if (!framework) { nit: you can elide braces https://codereview.chromium.org/2803403002/diff/60001/components/arc/common/v... File components/arc/common/voice_interaction_framework.mojom (right): https://codereview.chromium.org/2803403002/diff/60001/components/arc/common/v... components/arc/common/voice_interaction_framework.mojom:27: // Starts the voice interaction session in container, with a screen region selected. nit: fit to 80 cols. https://codereview.chromium.org/2803403002/diff/60001/components/arc/common/v... components/arc/common/voice_interaction_framework.mojom:29: StartVoiceInteractionSessionForRegion@2(ScreenRect region); nit: if it fits on 80 cols, put this in the same line as the annotation for consistency with the rest of the .mojom files.
https://codereview.chromium.org/2803403002/diff/60001/chrome/browser/ui/ash/p... File chrome/browser/ui/ash/palette_delegate_chromeos.cc (right): https://codereview.chromium.org/2803403002/diff/60001/chrome/browser/ui/ash/p... chrome/browser/ui/ash/palette_delegate_chromeos.cc:44: if (!framework) { On 2017/04/11 20:02:04, Luis Héctor Chávez wrote: > nit: you can elide braces Done. https://codereview.chromium.org/2803403002/diff/60001/components/arc/common/v... File components/arc/common/voice_interaction_framework.mojom (right): https://codereview.chromium.org/2803403002/diff/60001/components/arc/common/v... components/arc/common/voice_interaction_framework.mojom:27: // Starts the voice interaction session in container, with a screen region selected. On 2017/04/11 20:02:04, Luis Héctor Chávez wrote: > nit: fit to 80 cols. Done. https://codereview.chromium.org/2803403002/diff/60001/components/arc/common/v... components/arc/common/voice_interaction_framework.mojom:29: StartVoiceInteractionSessionForRegion@2(ScreenRect region); On 2017/04/11 20:02:04, Luis Héctor Chávez wrote: > nit: if it fits on 80 cols, put this in the same line as the annotation for > consistency with the rest of the .mojom files. Done.
The CQ bit was checked by kaznacheev@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kaznacheev@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.
lgtm with nits https://codereview.chromium.org/2803403002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/palette_delegate_chromeos.cc (right): https://codereview.chromium.org/2803403002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/palette_delegate_chromeos.cc:34: private: nit: please define public ctor/dtor and then add private: DISALLOW_COPY_AND_ASSIGN(VoiceInteractionScreenshotDelegate); at the end of the class definition.
The CQ bit was checked by kaznacheev@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/2803403002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/palette_delegate_chromeos.cc (right): https://codereview.chromium.org/2803403002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/palette_delegate_chromeos.cc:34: private: On 2017/04/12 01:09:22, oshima wrote: > nit: please define public ctor/dtor and then add > > private: > DISALLOW_COPY_AND_ASSIGN(VoiceInteractionScreenshotDelegate); > > at the end of the class definition. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kaznacheev@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiaohuic@chromium.org, lhchavez@chromium.org, dcheng@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2803403002/#ps120001 (title: "Addressed 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": 120001, "attempt_start_ts": 1492015133164650,
"parent_rev": "8ce1b3a5c77dbe359a1592acb9bcfbadf3339bf0", "commit_rev":
"91b3f7e7b1f592052614ffd6eb61d60f87dd2571"}
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492015133164650,
"parent_rev": "8ce1b3a5c77dbe359a1592acb9bcfbadf3339bf0", "commit_rev":
"91b3f7e7b1f592052614ffd6eb61d60f87dd2571"}
Message was sent while issue was closed.
Description was changed from ========== Support region selection for voice interaction session Introducing two new ways to invoke a voice interaction session for a specific region: 1. If kEnableVoiceInteraction is enabled, the "Capture region" stylus tool does not take a screenshot but starts a voice interaction session for that region. 2. Shift+Search+A invokes the "metalayer" - an alternative region selection tool. BUG=b:37165189 TEST="Capture region" stylus tool start voice Interaction session. Shift+Search+A invokes the "metalayer". ========== to ========== Support region selection for voice interaction session Introducing two new ways to invoke a voice interaction session for a specific region: 1. If kEnableVoiceInteraction is enabled, the "Capture region" stylus tool does not take a screenshot but starts a voice interaction session for that region. 2. Shift+Search+A invokes the "metalayer" - an alternative region selection tool. BUG=b:37165189 TEST="Capture region" stylus tool start voice Interaction session. Shift+Search+A invokes the "metalayer". Review-Url: https://codereview.chromium.org/2803403002 Cr-Commit-Position: refs/heads/master@{#464070} Committed: https://chromium.googlesource.com/chromium/src/+/91b3f7e7b1f592052614ffd6eb61... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/91b3f7e7b1f592052614ffd6eb61... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
