Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2898173006:
restricts when context could be retrieved. (Closed)
|
Created:
3 years, 7 months ago by Muyuan Modified:
3 years, 7 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org, tfarina, hidehiko+watch_chromium.org, sadrul, asvitkine+watch_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, kalyank, davemoore+watch_chromium.org, Matt Giuca Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionrestricts when context could be retrieved.
Add a timeout after user initiating voice interaction
session. All context requests can only be done with
pre-defined number of times during this time frame.
BUG=b/62065175
Review-Url: https://codereview.chromium.org/2898173006
Cr-Commit-Position: refs/heads/master@{#474917}
Committed: https://chromium.googlesource.com/chromium/src/+/e38092a8f069577f01e4d64caad1e79012cca9b6
Patch Set 1 #Patch Set 2 : restricts when context could be retrieved. #
Total comments: 16
Patch Set 3 : address review comments #
Total comments: 19
Patch Set 4 : address comments to metrics #Patch Set 5 : address review comments #Patch Set 6 : rebase #Patch Set 7 : prioritize container when user pressed shortcut #Patch Set 8 : fix shortcut key for meta + shift + a #Patch Set 9 : fix build #Patch Set 10 : fix meta layer shortcut key #
Messages
Total messages: 31 (14 generated)
Description was changed from ========== restricts when context could be retrieved. BUG=b/62065175 ========== to ========== restricts when context could be retrieved. Add a timeout after user initiating voice interaction session. All context requests can only be done with pre-defined number of times during this time frame. BUG=b/62065175 ==========
muyuanli@chromium.org changed reviewers: + derat@chromium.org, isherman@chromium.org, lhchavez@chromium.org
lhchavez@chromium.org: Please review changes in chrome/browser/chromeos/arc/* derat@chromium.org: Please review changes in chrome/browser/ui/* isherman@chromium.org: Please review changes in histograms.xml
lgtm https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/ui/ash/a... File chrome/browser/ui/ash/app_list/app_list_presenter_service.cc (right): https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/ui/ash/a... chrome/browser/ui/ash/app_list/app_list_presenter_service.cc:10: #include "chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h" it precedes this change, but this name could be shorter. the class is already in the arc namespace, so repeating "arc" in the class name feels a bit redundant.
https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:49: const int64_t kAllowedTimeMsSinceUserInteraction = 2000; nit: don't use milliseconds. It's better to use base::TimeDelta instead. Also, constexpr. https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:164: context_request_allowed_count_(0) { nit: initialize this in the .h https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:339: context_request_allowed_count_ = 2; nit: make this 2 a named constant. https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h (right): https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h:62: // VoiceInteractionFrameworkInstance::StartVoiceInteractionSessionForRegion. nit: add () to all the methods in comments. https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h:68: // initialted interaction. Logs UMA metric when it's not. nit: s/initialted/initiated/ https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h:84: // The number of allowed requests from container. Maximum is 2 (1 for nit: document that this amount resets after a while. https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h:88: int32_t context_request_allowed_count_; nit: this is not the count of allowed requests, but the number of remaining allowed requests. Maybe context_request_remaining_count_?
https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:49: const int64_t kAllowedTimeMsSinceUserInteraction = 2000; On 2017/05/25 15:55:18, Luis Héctor Chávez wrote: > nit: don't use milliseconds. It's better to use base::TimeDelta instead. > > Also, constexpr. Done. https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:164: context_request_allowed_count_(0) { On 2017/05/25 15:55:18, Luis Héctor Chávez wrote: > nit: initialize this in the .h Done. https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:339: context_request_allowed_count_ = 2; On 2017/05/25 15:55:18, Luis Héctor Chávez wrote: > nit: make this 2 a named constant. Done. https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h (right): https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h:62: // VoiceInteractionFrameworkInstance::StartVoiceInteractionSessionForRegion. On 2017/05/25 15:55:18, Luis Héctor Chávez wrote: > nit: add () to all the methods in comments. Done. https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h:68: // initialted interaction. Logs UMA metric when it's not. On 2017/05/25 15:55:18, Luis Héctor Chávez wrote: > nit: s/initialted/initiated/ Done. https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h:84: // The number of allowed requests from container. Maximum is 2 (1 for On 2017/05/25 15:55:18, Luis Héctor Chávez wrote: > nit: document that this amount resets after a while. Done. https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h:88: int32_t context_request_allowed_count_; On 2017/05/25 15:55:18, Luis Héctor Chávez wrote: > nit: this is not the count of allowed requests, but the number of remaining > allowed requests. Maybe context_request_remaining_count_? Done. https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/ui/ash/a... File chrome/browser/ui/ash/app_list/app_list_presenter_service.cc (right): https://codereview.chromium.org/2898173006/diff/20001/chrome/browser/ui/ash/a... chrome/browser/ui/ash/app_list/app_list_presenter_service.cc:10: #include "chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.h" On 2017/05/25 13:35:25, Daniel Erat wrote: > it precedes this change, but this name could be shorter. the class is already in > the arc namespace, so repeating "arc" in the class name feels a bit redundant. Acknowledged.
https://codereview.chromium.org/2898173006/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2898173006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:49: // kMaxTimeMsSinceUserInteractionForHistogram. Why is it important for this value to be strictly less than the max value for the histogram? Values above the max will be recorded to an overflow bucket. https://codereview.chromium.org/2898173006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:377: elapsed.InMilliseconds(), 0, nit: 0 is not a valid minimum bucket, and you should see a DCHECK firing for this. If you do not build with DCHECKs enabled, I'd strongly encourage you to do so. Please specify 1 instead. (Data can still go into the "0" bucket; it's just that that bucket is used for underflow, and the API expects it not to be specified explicitly.) https://codereview.chromium.org/2898173006/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2898173006/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:79827: +<histogram name="VoiceInteraction.IllegalContextRequest"> nit: Please add enum="BooleanHit". https://codereview.chromium.org/2898173006/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:79835: +<histogram name="VoiceInteraction.RequestArrivalSinceUserInteraction" Optional nit: I'd name this "UserInteractionToRequestArrivalDuration", or possibly "Time" instead of "Duration", or possibly dropping the "Duration" suffix altogether since it's a bit awkward and is clear enough from the units.
https://codereview.chromium.org/2898173006/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2898173006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:50: constexpr base::TimeDelta kAllowedTimeMsSinceUserInteraction = nit: kAllowedTimeSinceUserInteraction same below, drop the "Ms" https://codereview.chromium.org/2898173006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:55: const int32_t kContextRequestMaxRemainingCount = 2; nit: constexpr https://codereview.chromium.org/2898173006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:302: // Initiate user interaction at the point meta layer disappears. nit: Initiate user interaction when the metalayer disappears. https://codereview.chromium.org/2898173006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:307: base::Unretained(this)), Please comment why base::Unretained is safe to use. https://codereview.chromium.org/2898173006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:395: // If next request starts too soon and there is a session in action, we nit: s/a session in action/an active session/
https://codereview.chromium.org/2898173006/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2898173006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:49: // kMaxTimeMsSinceUserInteractionForHistogram. On 2017/05/25 22:21:16, Ilya Sherman wrote: > Why is it important for this value to be strictly less than the max value for > the histogram? Values above the max will be recorded to an overflow bucket. It's the allowed time for interaction realistically, meaning any time above this one will be considered to be blocked, which in theory should be a lot smaller than the upper bound for histogram. https://codereview.chromium.org/2898173006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:377: elapsed.InMilliseconds(), 0, On 2017/05/25 22:21:16, Ilya Sherman wrote: > nit: 0 is not a valid minimum bucket, and you should see a DCHECK firing for > this. If you do not build with DCHECKs enabled, I'd strongly encourage you to > do so. Please specify 1 instead. (Data can still go into the "0" bucket; it's > just that that bucket is used for underflow, and the API expects it not to be > specified explicitly.) Done. https://codereview.chromium.org/2898173006/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2898173006/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:79827: +<histogram name="VoiceInteraction.IllegalContextRequest"> On 2017/05/25 22:21:16, Ilya Sherman wrote: > nit: Please add enum="BooleanHit". Done. https://codereview.chromium.org/2898173006/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:79835: +<histogram name="VoiceInteraction.RequestArrivalSinceUserInteraction" On 2017/05/25 22:21:16, Ilya Sherman wrote: > Optional nit: I'd name this "UserInteractionToRequestArrivalDuration", or > possibly "Time" instead of "Duration", or possibly dropping the "Duration" > suffix altogether since it's a bit awkward and is clear enough from the units. Done.
Metrics LGTM % a comment. Thanks! https://codereview.chromium.org/2898173006/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2898173006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:49: // kMaxTimeMsSinceUserInteractionForHistogram. On 2017/05/25 22:32:58, Muyuan wrote: > On 2017/05/25 22:21:16, Ilya Sherman wrote: > > Why is it important for this value to be strictly less than the max value for > > the histogram? Values above the max will be recorded to an overflow bucket. > > It's the allowed time for interaction realistically, meaning any time above this > one will be considered to be blocked, which in theory should be a lot smaller > than the upper bound for histogram. Please explain this inline within the comments. (It's actually still not 100% clear to me why you need this less-than relationship. Is the idea that you want to validate the choice by looking at the real-world data recorded in the histogram?)
https://codereview.chromium.org/2898173006/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2898173006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:50: constexpr base::TimeDelta kAllowedTimeMsSinceUserInteraction = On 2017/05/25 22:32:50, Luis Héctor Chávez wrote: > nit: kAllowedTimeSinceUserInteraction > > same below, drop the "Ms" Done. https://codereview.chromium.org/2898173006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:55: const int32_t kContextRequestMaxRemainingCount = 2; On 2017/05/25 22:32:50, Luis Héctor Chávez wrote: > nit: constexpr Done. https://codereview.chromium.org/2898173006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:302: // Initiate user interaction at the point meta layer disappears. On 2017/05/25 22:32:50, Luis Héctor Chávez wrote: > nit: Initiate user interaction when the metalayer disappears. Done. https://codereview.chromium.org/2898173006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:307: base::Unretained(this)), On 2017/05/25 22:32:50, Luis Héctor Chávez wrote: > Please comment why base::Unretained is safe to use. Done. https://codereview.chromium.org/2898173006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:395: // If next request starts too soon and there is a session in action, we On 2017/05/25 22:32:50, Luis Héctor Chávez wrote: > nit: s/a session in action/an active session/ Done.
c/b/arc lgtm
The CQ bit was checked by muyuanli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org, isherman@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2898173006/#ps100001 (title: "rebase")
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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by muyuanli@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by muyuanli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org, lhchavez@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2898173006/#ps120001 (title: "prioritize container when user pressed shortcut")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by muyuanli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org, lhchavez@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2898173006/#ps140001 (title: "fix shortcut key for meta + shift + a")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by muyuanli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org, lhchavez@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2898173006/#ps180001 (title: "fix meta layer shortcut key")
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": 180001, "attempt_start_ts": 1495769697296410,
"parent_rev": "6cceae4b9017cb225d18775b0ac27b27fce45041", "commit_rev":
"e38092a8f069577f01e4d64caad1e79012cca9b6"}
Message was sent while issue was closed.
Description was changed from ========== restricts when context could be retrieved. Add a timeout after user initiating voice interaction session. All context requests can only be done with pre-defined number of times during this time frame. BUG=b/62065175 ========== to ========== restricts when context could be retrieved. Add a timeout after user initiating voice interaction session. All context requests can only be done with pre-defined number of times during this time frame. BUG=b/62065175 Review-Url: https://codereview.chromium.org/2898173006 Cr-Commit-Position: refs/heads/master@{#474917} Committed: https://chromium.googlesource.com/chromium/src/+/e38092a8f069577f01e4d64caad1... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/e38092a8f069577f01e4d64caad1... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
