Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2968073002:
register voice interaction preference. (Closed)
|
Created:
3 years, 5 months ago by Muyuan Modified:
3 years, 5 months ago CC:
rcui, chromium-reviews, elijahtaylor+arcwatch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionadd voice interaction preference.
There should be 3 states for voice interaction
preferences: undefined, enabled, disabled. It
starts at undefined and will do a one-time-only
fetch from Android to incorporate OOBE settings.
Then the switches are managed by Chrome completely.
BUG=b:38175284
Review-Url: https://codereview.chromium.org/2968073002
Cr-Commit-Position: refs/heads/master@{#487633}
Committed: https://chromium.googlesource.com/chromium/src/+/5bd014442c0d691253b0283506649196432b05d5
Patch Set 1 #Patch Set 2 : add flag sync logic #
Total comments: 2
Patch Set 3 : address review comments #
Total comments: 25
Patch Set 4 : address review comments #
Total comments: 10
Patch Set 5 : address review comment #Patch Set 6 : rebase #
Messages
Total messages: 28 (12 generated)
muyuanli@chromium.org changed reviewers: + stevenjb@chromium.org
Description was changed from ========== register voice interaction preference. BUG=b:38175284 ========== to ========== add voice interaction preference. There should be 3 states for voice interaction preferences: undefined, enabled, disabled. It starts at undefined and will do a one-time-only fetch from Android to incorporate OOBE settings. Then the switches are managed by Chrome completely. BUG=b:38175284 ==========
muyuanli@chromium.org changed reviewers: + dcheng@chromium.org, lhchavez@google.com
muyuanli@chromium.org changed reviewers: + lhchavez@chromium.org - lhchavez@google.com
steven: could you please take a look at the settings flags lhchaves: please take a look at chrome/* dcheng: *.mojom please.
https://codereview.chromium.org/2968073002/diff/20001/components/arc/common/v... File components/arc/common/voice_interaction_framework.mojom (right): https://codereview.chromium.org/2968073002/diff/20001/components/arc/common/v... components/arc/common/voice_interaction_framework.mojom:61: => (bool defined, bool voice_interaction, bool context); 1) Please explain what |defined|, |voice_interaction|, and |context| mean in comments. 2) How would you feel about using a simple mojo struct to return these values? Seeing three bools at the call site is pretty opaque (and it'd be easy to mix up parameters by accident)
https://codereview.chromium.org/2968073002/diff/20001/components/arc/common/v... File components/arc/common/voice_interaction_framework.mojom (right): https://codereview.chromium.org/2968073002/diff/20001/components/arc/common/v... components/arc/common/voice_interaction_framework.mojom:61: => (bool defined, bool voice_interaction, bool context); On 2017/07/06 23:30:34, dcheng wrote: > 1) Please explain what |defined|, |voice_interaction|, and |context| mean in > comments. > > 2) How would you feel about using a simple mojo struct to return these values? > Seeing three bools at the call site is pretty opaque (and it'd be easy to mix up > parameters by accident) Done.
mojom lgtm
https://codereview.chromium.org/2968073002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2968073002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:326: if (!enabled) nit: you can't elide braces since the body is not a one-liner. alternatively, use the guard-clause pattern if it makes sense: if (enabled) return; // rest https://codereview.chromium.org/2968073002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:329: } Should we also clear kVoiceInteraction{Enabled,ContextEnabled} here? If so, maybe you can have the same function mentioned in L399 and call it here, too. https://codereview.chromium.org/2968073002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:390: if (!ProfileManager::GetActiveUserProfile()->GetPrefs()->GetBoolean( nit: use guard-clause pattern: if (ProfileManager::GetActiveUserProfile()->GetPrefs()->GetBoolean( prefs::kVoiceInteractionPrefSynced)) { return; } // rest. https://codereview.chromium.org/2968073002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:399: base::Bind([](mojom::VoiceInteractionStatusPtr status) { nit: make this a standalone function in the anonymous namespace. https://codereview.chromium.org/2968073002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h (right): https://codereview.chromium.org/2968073002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h:93: // Update voice interaction flags. These flags are set only once when Arc nit: s/Arc container/ARC/ https://codereview.chromium.org/2968073002/diff/40001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2968073002/diff/40001/chrome/common/pref_name... chrome/common/pref_names.cc:66: nit: remove blank line https://codereview.chromium.org/2968073002/diff/40001/chrome/common/pref_name... chrome/common/pref_names.cc:68: // from Arc. This synchronization only happens when user goes through the flow nit: s/Arc/ARC/ https://codereview.chromium.org/2968073002/diff/40001/components/arc/common/v... File components/arc/common/voice_interaction_framework.mojom (right): https://codereview.chromium.org/2968073002/diff/40001/components/arc/common/v... components/arc/common/voice_interaction_framework.mojom:33: // Indicate voice interaction configuration status. nit: s/Indicate/Indicates/
https://codereview.chromium.org/2968073002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2968073002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:399: base::Bind([](mojom::VoiceInteractionStatusPtr status) { On 2017/07/10 16:53:02, Luis Héctor Chávez wrote: > nit: make this a standalone function in the anonymous namespace. +1. We generally only use function objects like this for trivial functions (e.g. for sorting or finding). https://codereview.chromium.org/2968073002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:400: auto* prefs = ProfileManager::GetActiveUserProfile()->GetPrefs(); Prefer explicit types when the context isn't completely clear (i.e. |prefs| is actually 'PrefService', not 'Prefs' as implied by the getter). Also, even if the getter were GetPrefService we typically provide the type for clarity and reserve auto for iterators and places where the type is very clear like with MakeUnique<>). https://codereview.chromium.org/2968073002/diff/40001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2968073002/diff/40001/chrome/common/pref_name... chrome/common/pref_names.cc:67: // A preference indicating whether voice interaction settings should be read s/should be/have been/ ? https://codereview.chromium.org/2968073002/diff/40001/components/arc/common/v... File components/arc/common/voice_interaction_framework.mojom (right): https://codereview.chromium.org/2968073002/diff/40001/components/arc/common/v... components/arc/common/voice_interaction_framework.mojom:35: // Whether voice interaction is configured during OOBE flow. false if s/false/False (or |false|) https://codereview.chromium.org/2968073002/diff/40001/components/arc/common/v... components/arc/common/voice_interaction_framework.mojom:37: bool defined; 'configured' ?
https://codereview.chromium.org/2968073002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2968073002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:326: if (!enabled) On 2017/07/10 16:53:02, Luis Héctor Chávez wrote: > nit: you can't elide braces since the body is not a one-liner. > > alternatively, use the guard-clause pattern if it makes sense: > > if (enabled) > return; > // rest Done. https://codereview.chromium.org/2968073002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:329: } On 2017/07/10 16:53:02, Luis Héctor Chávez wrote: > Should we also clear kVoiceInteraction{Enabled,ContextEnabled} here? If so, > maybe you can have the same function mentioned in L399 and call it here, too. It does not matter as kVoiceInteractionPrefSynced is the master switch. But I modified the code to keep consistency. https://codereview.chromium.org/2968073002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:390: if (!ProfileManager::GetActiveUserProfile()->GetPrefs()->GetBoolean( On 2017/07/10 16:53:02, Luis Héctor Chávez wrote: > nit: use guard-clause pattern: > > if (ProfileManager::GetActiveUserProfile()->GetPrefs()->GetBoolean( > prefs::kVoiceInteractionPrefSynced)) { > return; > } > // rest. Done. https://codereview.chromium.org/2968073002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:399: base::Bind([](mojom::VoiceInteractionStatusPtr status) { On 2017/07/10 17:57:07, stevenjb wrote: > On 2017/07/10 16:53:02, Luis Héctor Chávez wrote: > > nit: make this a standalone function in the anonymous namespace. > > +1. We generally only use function objects like this for trivial functions (e.g. > for sorting or finding). Done. https://codereview.chromium.org/2968073002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:400: auto* prefs = ProfileManager::GetActiveUserProfile()->GetPrefs(); On 2017/07/10 17:57:07, stevenjb wrote: > Prefer explicit types when the context isn't completely clear (i.e. |prefs| is > actually 'PrefService', not 'Prefs' as implied by the getter). Also, even if the > getter were GetPrefService we typically provide the type for clarity and reserve > auto for iterators and places where the type is very clear like with > MakeUnique<>). Done. https://codereview.chromium.org/2968073002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h (right): https://codereview.chromium.org/2968073002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h:93: // Update voice interaction flags. These flags are set only once when Arc On 2017/07/10 16:53:02, Luis Héctor Chávez wrote: > nit: s/Arc container/ARC/ Done. https://codereview.chromium.org/2968073002/diff/40001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2968073002/diff/40001/chrome/common/pref_name... chrome/common/pref_names.cc:66: On 2017/07/10 16:53:02, Luis Héctor Chávez wrote: > nit: remove blank line Done. https://codereview.chromium.org/2968073002/diff/40001/chrome/common/pref_name... chrome/common/pref_names.cc:67: // A preference indicating whether voice interaction settings should be read On 2017/07/10 17:57:07, stevenjb wrote: > s/should be/have been/ ? Done. https://codereview.chromium.org/2968073002/diff/40001/chrome/common/pref_name... chrome/common/pref_names.cc:68: // from Arc. This synchronization only happens when user goes through the flow On 2017/07/10 16:53:03, Luis Héctor Chávez wrote: > nit: s/Arc/ARC/ Done. https://codereview.chromium.org/2968073002/diff/40001/components/arc/common/v... File components/arc/common/voice_interaction_framework.mojom (right): https://codereview.chromium.org/2968073002/diff/40001/components/arc/common/v... components/arc/common/voice_interaction_framework.mojom:33: // Indicate voice interaction configuration status. On 2017/07/10 16:53:03, Luis Héctor Chávez wrote: > nit: s/Indicate/Indicates/ Done. https://codereview.chromium.org/2968073002/diff/40001/components/arc/common/v... components/arc/common/voice_interaction_framework.mojom:35: // Whether voice interaction is configured during OOBE flow. false if On 2017/07/10 17:57:07, stevenjb wrote: > s/false/False (or |false|) Done. https://codereview.chromium.org/2968073002/diff/40001/components/arc/common/v... components/arc/common/voice_interaction_framework.mojom:37: bool defined; On 2017/07/10 17:57:07, stevenjb wrote: > 'configured' ? Done.
lgtm with a few more nits. https://codereview.chromium.org/2968073002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2968073002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:337: auto status = mojom::VoiceInteractionStatusPtr(); mojom::VoiceInteractionStatusPtr status; is simpler and you avoid an unnecessary auto. https://codereview.chromium.org/2968073002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h (right): https://codereview.chromium.org/2968073002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h:93: // Update voice interaction flags. These flags are set only once when Arc nit: s/Arc container/ARC/ https://codereview.chromium.org/2968073002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h:93: // Update voice interaction flags. These flags are set only once when Arc nit: s/Update/Updates/
https://codereview.chromium.org/2968073002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2968073002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:337: auto status = mojom::VoiceInteractionStatusPtr(); On 2017/07/17 15:10:58, Luis Héctor Chávez wrote: > mojom::VoiceInteractionStatusPtr status; > > is simpler and you avoid an unnecessary auto. Sorry, it should be mojom::VoiceInteractionStatusPtr status = mojom::VoiceInteractionStatus::New();
lgtm https://codereview.chromium.org/2968073002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2968073002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:405: return; {}
https://codereview.chromium.org/2968073002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2968073002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:337: auto status = mojom::VoiceInteractionStatusPtr(); On 2017/07/17 15:15:16, Luis Héctor Chávez wrote: > On 2017/07/17 15:10:58, Luis Héctor Chávez wrote: > > mojom::VoiceInteractionStatusPtr status; > > > > is simpler and you avoid an unnecessary auto. > > Sorry, it should be > > mojom::VoiceInteractionStatusPtr status = mojom::VoiceInteractionStatus::New(); Done. https://codereview.chromium.org/2968073002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:337: auto status = mojom::VoiceInteractionStatusPtr(); On 2017/07/17 15:10:58, Luis Héctor Chávez wrote: > mojom::VoiceInteractionStatusPtr status; > > is simpler and you avoid an unnecessary auto. Acknowledged. https://codereview.chromium.org/2968073002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:405: return; On 2017/07/17 17:00:16, stevenjb wrote: > {} Done. https://codereview.chromium.org/2968073002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h (right): https://codereview.chromium.org/2968073002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h:93: // Update voice interaction flags. These flags are set only once when Arc On 2017/07/17 15:10:58, Luis Héctor Chávez wrote: > nit: s/Update/Updates/ Done. https://codereview.chromium.org/2968073002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h:93: // Update voice interaction flags. These flags are set only once when Arc On 2017/07/17 15:10:58, Luis Héctor Chávez wrote: > nit: s/Update/Updates/ Done.
The CQ bit was checked by muyuanli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, stevenjb@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2968073002/#ps80001 (title: "address review comment")
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by muyuanli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, lhchavez@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2968073002/#ps100001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1500406986017620,
"parent_rev": "474f53bea3f44cba536a1c6f6fe8611d4d89f720", "commit_rev":
"66722f3dc8f64df127c8fab3fefe6353c53a0477"}
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1500406986017620,
"parent_rev": "504ac1c39579d5f7f54fd963309be347001e8d71", "commit_rev":
"5bd014442c0d691253b0283506649196432b05d5"}
Message was sent while issue was closed.
Description was changed from ========== add voice interaction preference. There should be 3 states for voice interaction preferences: undefined, enabled, disabled. It starts at undefined and will do a one-time-only fetch from Android to incorporate OOBE settings. Then the switches are managed by Chrome completely. BUG=b:38175284 ========== to ========== add voice interaction preference. There should be 3 states for voice interaction preferences: undefined, enabled, disabled. It starts at undefined and will do a one-time-only fetch from Android to incorporate OOBE settings. Then the switches are managed by Chrome completely. BUG=b:38175284 Review-Url: https://codereview.chromium.org/2968073002 Cr-Commit-Position: refs/heads/master@{#487633} Committed: https://chromium.googlesource.com/chromium/src/+/5bd014442c0d691253b028350664... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/5bd014442c0d691253b028350664... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
