Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2457873002:
[Android] spellcheck: add latency UMA. (Closed)
|
Created:
4 years, 1 month ago by timvolodine Modified:
4 years, 1 month ago CC:
chromium-reviews, timvolodine, rlp+watch_chromium.org, rouslan+spell_chromium.org, asvitkine+watch_chromium.org, agrieve+watch_chromium.org, groby+spellwatch_chromium.org, Tobias Sargeant Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] spellcheck: add latency UMA.
Add spellchecking service latency measurement on Android.
This measures how long it takes for the spellchecker to
return results. The purpose is to understand performance
characteristics of spellchecking on various devices.
Also during testing we have encountered some slow response
times on Nexus 5 Android Lollipop device, which however were
resolved after updating the "Google Keyboard" app via Google
Play. This UMA will help us understand if there are any such
regressions in the field and how frequent they are.
BUG=660074, 629609
Committed: https://crrev.com/df000848b18809fa5dbcf0cd5e45da6a7f1e673a
Cr-Commit-Position: refs/heads/master@{#429687}
Patch Set 1 #
Total comments: 4
Patch Set 2 : address comments, use java UMA api #
Messages
Total messages: 27 (17 generated)
The CQ bit was checked by timvolodine@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.
Description was changed from ========== [Android] spellcheck: add latency UMA. Add spellchecking service latency measurement on Android. This measures how long it takes for the spellchecker to return resuls. The purpose is to understand performance characteristics of spellchecking on various devices. Also during testing we have encountered some slow response times on Nexus 5 Android Lollipop device, which however were resolved after updating the "Google Keyboard" app via Google Play. This UMA will help us understand if there are any such regressions in the field and how frequent they are. BUG=660074 ========== to ========== [Android] spellcheck: add latency UMA. Add spellchecking service latency measurement on Android. This measures how long it takes for the spellchecker to return results. The purpose is to understand performance characteristics of spellchecking on various devices. BUG=660074 ==========
Description was changed from ========== [Android] spellcheck: add latency UMA. Add spellchecking service latency measurement on Android. This measures how long it takes for the spellchecker to return results. The purpose is to understand performance characteristics of spellchecking on various devices. BUG=660074 ========== to ========== [Android] spellcheck: add latency UMA. BUG=660074 ==========
Description was changed from ========== [Android] spellcheck: add latency UMA. BUG=660074 ========== to ========== [Android] spellcheck: add latency UMA. Add spellchecking service latency measurement on Android. This measures how long it takes for the spellchecker to return results. The purpose is to understand performance characteristics of spellchecking on various devices. Also during testing we have encountered some slow response times on Nexus 5 Android Lollipop device, which however were resolved after updating the "Google Keyboard" app via Google Play. This UMA will help us understand if there are any such regressions in the field and how frequent they are. BUG=660074 ==========
timvolodine@chromium.org changed reviewers: + groby@chromium.org
timvolodine@chromium.org changed reviewers: + isherman@chromium.org
+ isherman@ for histograms.xml
https://codereview.chromium.org/2457873002/diff/1/components/spellcheck/brows... File components/spellcheck/browser/spellchecker_session_bridge_android.cc (right): https://codereview.chromium.org/2457873002/diff/1/components/spellcheck/brows... components/spellcheck/browser/spellchecker_session_bridge_android.cc:134: base::TimeDelta::FromMilliseconds(latencyMs)); Hmm, why are you adding a native call for recording this histogram, rather than using the Java API to record the histogram? (If you were simply unaware that API exists -- it does! =)) https://codereview.chromium.org/2457873002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2457873002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:60206: + spellchecking service on Android. For timing histograms, it's best to be suuuper specific about what they're measuring exactly, because often somebody will have questions about it later. So, could you add a sentence detailing the specific start and stop events for this timer?
The CQ bit was checked by timvolodine@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...
thanks for the comments Ilya, addressed them below, ptal! https://codereview.chromium.org/2457873002/diff/1/components/spellcheck/brows... File components/spellcheck/browser/spellchecker_session_bridge_android.cc (right): https://codereview.chromium.org/2457873002/diff/1/components/spellcheck/brows... components/spellcheck/browser/spellchecker_session_bridge_android.cc:134: base::TimeDelta::FromMilliseconds(latencyMs)); On 2016/10/28 18:53:54, Ilya Sherman wrote: > Hmm, why are you adding a native call for recording this histogram, rather than > using the Java API to record the histogram? (If you were simply unaware that > API exists -- it does! =)) yeey it does! (didn't realize it is available now, so did my own plumbing). anyways removed the native side completely. https://codereview.chromium.org/2457873002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2457873002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:60206: + spellchecking service on Android. On 2016/10/28 18:53:54, Ilya Sherman wrote: > For timing histograms, it's best to be suuuper specific about what they're > measuring exactly, because often somebody will have questions about it later. > So, could you add a sentence detailing the specific start and stop events for > this timer? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, thanks.
LGTM+1
Description was changed from ========== [Android] spellcheck: add latency UMA. Add spellchecking service latency measurement on Android. This measures how long it takes for the spellchecker to return results. The purpose is to understand performance characteristics of spellchecking on various devices. Also during testing we have encountered some slow response times on Nexus 5 Android Lollipop device, which however were resolved after updating the "Google Keyboard" app via Google Play. This UMA will help us understand if there are any such regressions in the field and how frequent they are. BUG=660074 ========== to ========== [Android] spellcheck: add latency UMA. Add spellchecking service latency measurement on Android. This measures how long it takes for the spellchecker to return results. The purpose is to understand performance characteristics of spellchecking on various devices. Also during testing we have encountered some slow response times on Nexus 5 Android Lollipop device, which however were resolved after updating the "Google Keyboard" app via Google Play. This UMA will help us understand if there are any such regressions in the field and how frequent they are. BUG=660074, 629609 ==========
thanks for the reviews!
The CQ bit was checked by timvolodine@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Android] spellcheck: add latency UMA. Add spellchecking service latency measurement on Android. This measures how long it takes for the spellchecker to return results. The purpose is to understand performance characteristics of spellchecking on various devices. Also during testing we have encountered some slow response times on Nexus 5 Android Lollipop device, which however were resolved after updating the "Google Keyboard" app via Google Play. This UMA will help us understand if there are any such regressions in the field and how frequent they are. BUG=660074, 629609 ========== to ========== [Android] spellcheck: add latency UMA. Add spellchecking service latency measurement on Android. This measures how long it takes for the spellchecker to return results. The purpose is to understand performance characteristics of spellchecking on various devices. Also during testing we have encountered some slow response times on Nexus 5 Android Lollipop device, which however were resolved after updating the "Google Keyboard" app via Google Play. This UMA will help us understand if there are any such regressions in the field and how frequent they are. BUG=660074, 629609 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Android] spellcheck: add latency UMA. Add spellchecking service latency measurement on Android. This measures how long it takes for the spellchecker to return results. The purpose is to understand performance characteristics of spellchecking on various devices. Also during testing we have encountered some slow response times on Nexus 5 Android Lollipop device, which however were resolved after updating the "Google Keyboard" app via Google Play. This UMA will help us understand if there are any such regressions in the field and how frequent they are. BUG=660074, 629609 ========== to ========== [Android] spellcheck: add latency UMA. Add spellchecking service latency measurement on Android. This measures how long it takes for the spellchecker to return results. The purpose is to understand performance characteristics of spellchecking on various devices. Also during testing we have encountered some slow response times on Nexus 5 Android Lollipop device, which however were resolved after updating the "Google Keyboard" app via Google Play. This UMA will help us understand if there are any such regressions in the field and how frequent they are. BUG=660074, 629609 Committed: https://crrev.com/df000848b18809fa5dbcf0cd5e45da6a7f1e673a Cr-Commit-Position: refs/heads/master@{#429687} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/df000848b18809fa5dbcf0cd5e45da6a7f1e673a Cr-Commit-Position: refs/heads/master@{#429687} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
