Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1662703002:
[Android, UMA, MediaFling] Add UMA for how much time users spend casting media without the media el… (Closed)
|
Created:
4 years, 10 months ago by whywhat Modified:
4 years, 10 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, feature-media-reviews_chromium.org, mcasas+watch_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android, UMA, MediaFling] Add UMA for how much time users spend casting media without the media element being alive.
BUG=583069
TEST=manual
Committed: https://crrev.com/0f3b50035949d3eecb3a843a2b45a6474ee48dd1
Cr-Commit-Position: refs/heads/master@{#374265}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Handle switching videos #
Total comments: 2
Messages
Total messages: 23 (7 generated)
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
avayvod@chromium.org changed reviewers: + aberent@chromium.org, mpearson@chromium.org
Forgot to send this for a review. Please, take a look. This is the last and perhaps the hardest to implement metric for media flinging we want to implement. Anthony: overall change Mark: histograms.xml Thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1662703002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1662703002/1
https://codereview.chromium.org/1662703002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/RecordCastAction.java (right): https://codereview.chromium.org/1662703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/remote/RecordCastAction.java:134: RecordHistogram.recordEnumeratedHistogram( What's wrong with recordPercentageHistogram? https://codereview.chromium.org/1662703002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1662703002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:4024: + time the remote playback is stopped. This will be recorded when the playback Is this "the percentage of the video played without the media element" recorded "at the time the remote playback was stopped" (regardless of whether the media element is there at the time) OR "the percentage of the video played" recorded "at the time the remote playback was stopped if the media element is not there at that moment" ? Also, does this count the percent of video that may have been played already? i.e., play some, stop, play some more, stop. Does the percentage emitted for the second stop include the part of the video watched in the first play action?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1662703002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java (right): https://codereview.chromium.org/1662703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:469: public void setMediaStateListener(MediaStateListener mediaStateListener) { I am not sure about the sequencing of setMediaStateListener() for the new video, and release() for the old video when switching videos. TEST that case! You might find that the timestamps are cleared before you record them (I am almost sure that we have other bugs in that area). https://codereview.chromium.org/1662703002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java (right): https://codereview.chromium.org/1662703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:404: super.release(); Note - You need to add this to the downstream implementation as well. https://codereview.chromium.org/1662703002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/RecordCastAction.java (right): https://codereview.chromium.org/1662703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/remote/RecordCastAction.java:128: * playback stopped), in percents. As written this includes pause time. Is this what you intend? If so, maybe clarify this in the comment.
Handle switching videos
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
PTaL https://codereview.chromium.org/1662703002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/RecordCastAction.java (right): https://codereview.chromium.org/1662703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/remote/RecordCastAction.java:128: * playback stopped), in percents. On 2016/02/04 at 16:44:25, aberent wrote: > As written this includes pause time. Is this what you intend? If so, maybe clarify this in the comment. Yes, my intent is the amount of time the media element is not available compared to the total duration of the cast session, no matter if the video is being played or not. https://codereview.chromium.org/1662703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/remote/RecordCastAction.java:134: RecordHistogram.recordEnumeratedHistogram( On 2016/02/03 at 22:44:57, Mark P (out Feb 5-8) wrote: > What's wrong with recordPercentageHistogram? Only that it's too far from the top of of the RecordHistogram file :) https://codereview.chromium.org/1662703002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1662703002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:4024: + time the remote playback is stopped. This will be recorded when the playback On 2016/02/03 at 22:44:57, Mark P (out Feb 5-8) wrote: > Is this "the percentage of the video played without the media element" recorded "at the time the remote playback was stopped" (regardless of whether the media element is there at the time) > OR > "the percentage of the video played" recorded "at the time the remote playback was stopped if the media element is not there at that moment" > ? The former. If the media element is still attached, the percentage should be 0. > > Also, does this count the percent of video that may have been played already? i.e., play some, stop, play some more, stop. Does the percentage emitted for the second stop include the part of the video watched in the first play action? This is for the total time of the remote session, so if the playback/session is stopped, there's no resuming (stop here means stop the cast application and disconnect from the device, not pause the video). I rephrased it around the remote session vs. playback to make it clearer.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1662703002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1662703002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
histograms.xml lgtm once you deal with the below comment if necessary. --mark https://codereview.chromium.org/1662703002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1662703002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4032: + percents. By implication, this is recorded on every detach event. Is that true? If so, no changes are needed.
https://codereview.chromium.org/1662703002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1662703002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4032: + percents. On 2016/02/08 at 22:59:37, Mark P (out Feb 5-8) wrote: > By implication, this is recorded on every detach event. Is that true? If so, no changes are needed. Yes. Thanks for the review!
The CQ bit was checked by avayvod@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1662703002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1662703002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Android, UMA, MediaFling] Add UMA for how much time users spend casting media without the media element being alive. BUG=583069 TEST=manual ========== to ========== [Android, UMA, MediaFling] Add UMA for how much time users spend casting media without the media element being alive. BUG=583069 TEST=manual Committed: https://crrev.com/0f3b50035949d3eecb3a843a2b45a6474ee48dd1 Cr-Commit-Position: refs/heads/master@{#374265} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/0f3b50035949d3eecb3a843a2b45a6474ee48dd1 Cr-Commit-Position: refs/heads/master@{#374265} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
