Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
55193003:
Add UMAs for video casting (Closed)
|
Created:
7 years, 1 month ago by May Modified:
7 years, 1 month ago CC:
chromium-reviews, feature-media-reviews_chromium.org, Ilya Sherman, jar (doing other things), asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd UMAs for video casting feature.
Added UMAs for:
1) How many times the button is shown
2) How many times the button is selected
3) How many times the YouTube button is selected
4) How many times a video is fullscreened
5) How many times a play ends in success or error
6) The % of a video that the user watched remotely
BUG=284482
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233301
Patch Set 1 #Patch Set 2 : Removed stray MediaPlayerBridge changes #
Total comments: 1
Patch Set 3 : Removed random VirtualKeyboardLoaded action to be safe. #
Total comments: 15
Patch Set 4 : Added enums to histograms.xml #
Total comments: 2
Patch Set 5 : Update histogram description #
Messages
Total messages: 18 (0 generated)
Corresponding histogram.xml changes.
lgtm https://codereview.chromium.org/55193003/diff/30001/tools/metrics/actions/chr... File tools/metrics/actions/chromeactions.txt (right): https://codereview.chromium.org/55193003/diff/30001/tools/metrics/actions/chr... tools/metrics/actions/chromeactions.txt:1665: 0x91586aefc4a53dc7 VirtualKeyboardLoaded This is unexpected?
On 2013/11/01 11:48:19, whywhat wrote: > lgtm > > https://codereview.chromium.org/55193003/diff/30001/tools/metrics/actions/chr... > File tools/metrics/actions/chromeactions.txt (right): > > https://codereview.chromium.org/55193003/diff/30001/tools/metrics/actions/chr... > tools/metrics/actions/chromeactions.txt:1665: > 0x91586aefc4a53dc7 VirtualKeyboardLoaded > This is unexpected? Yeah it's what came out when I ran the python script. Someone must not have added it before.
Hi Mark/Ilya, Could you please review the histograms piece? Thanks!
FYI, it's fine to include the stray action in chromeactions.txt as well. https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1197: +<histogram name="Cast.Sender.CastButtonShown"> Please associate an enum like "BooleanShown" with this histogram. https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1206: +<histogram name="Cast.Sender.CastButtonShownInitialFullscreen"> Ditto. https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1217: +<histogram name="Cast.Sender.CastPlaySuccess"> Please associate an enum with this histogram as well.
In the future, if possible please add the histograms.xml changes to the change that added the histograms. Then we won't run into problems about inaccurate name choices for histograms and the like. --mark https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1197: +<histogram name="Cast.Sender.CastButtonShown"> perhaps: units="Enabled" ditto histogram below https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1202: + entirely if it's disabled, so it's possible for the false values to be 0. Wha? This histogram is inappropriately named then. Shouldn't it be called something like Cast.Sender.CastButtonEligibleToBeShown ? analogous comment on histogram below https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1210: + exit fullscreen session. The value will be true if the button is enabled, recorded on exit fullscreen then, I assume? (also, if I close the browser when fullscreeen, I assume it might not be incremented.) https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1217: +<histogram name="Cast.Sender.CastPlaySuccess"> perhaps: units="Success" https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1227: + Records the percentage of the video left by the time the remote playback is ^by the^at the Can you point me to where this histogram is emitted to? I'd like to see what the parameters (min, max, buckets, etc.) are for it.
Added enums to the histograms. https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1197: +<histogram name="Cast.Sender.CastButtonShown"> On 2013/11/01 16:16:09, Mark P wrote: > perhaps: > units="Enabled" > > ditto histogram below Done. https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1202: + entirely if it's disabled, so it's possible for the false values to be 0. So currently, the Chromecast button shows up as an overlay when the user fullscreens a video. This histogram is intended to record if the user can see the button or not in that view. Given that the button is a regular Android Button, it has the usual enabled/disabled state, which is something I wanted to record also since obviously a user can't click on a disabled button. The current UX (in beta, still subject to change) hides the button if it's not enabled, hence the warning in the description that there might be no "disabled" values recorded. On 2013/11/01 16:16:09, Mark P wrote: > Wha? This histogram is inappropriately named then. Shouldn't it be called > something like > Cast.Sender.CastButtonEligibleToBeShown > ? > > analogous comment on histogram below https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1206: +<histogram name="Cast.Sender.CastButtonShownInitialFullscreen"> On 2013/11/01 16:13:37, Ilya Sherman wrote: > Ditto. Done. https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1210: + exit fullscreen session. The value will be true if the button is enabled, No, recorded on enter fullscreen, but only once. So closing the browser while in fullscreen would still be recorded. On 2013/11/01 16:16:09, Mark P wrote: > recorded on exit fullscreen then, I assume? > (also, if I close the browser when fullscreeen, I assume it might not be > incremented.) https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1217: +<histogram name="Cast.Sender.CastPlaySuccess"> On 2013/11/01 16:16:09, Mark P wrote: > perhaps: > units="Success" Done. https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1227: + Records the percentage of the video left by the time the remote playback is I'll add you to the downstream change for this. On 2013/11/01 16:16:09, Mark P wrote: > ^by the^at the > > Can you point me to where this histogram is emitted to? I'd like to see what > the parameters (min, max, buckets, etc.) are for it.
Hi Mark, I tried adding you to the corresponding clank downstream change for this, and I can't do that because you've never logged in to the Gerrit CR tool. Can you log in so I can add you? https://gerrit-int.chromium.org/ On 2013/11/01 16:16:09, Mark P wrote: > In the future, if possible please add the histograms.xml changes to the change > that added the histograms. Then we won't run into problems about inaccurate > name choices for histograms and the like. > > --mark > > https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... > tools/metrics/histograms/histograms.xml:1197: +<histogram > name="Cast.Sender.CastButtonShown"> > perhaps: > units="Enabled" > > ditto histogram below > > https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... > tools/metrics/histograms/histograms.xml:1202: + entirely if it's disabled, so > it's possible for the false values to be 0. > Wha? This histogram is inappropriately named then. Shouldn't it be called > something like > Cast.Sender.CastButtonEligibleToBeShown > ? > > analogous comment on histogram below > > https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... > tools/metrics/histograms/histograms.xml:1210: + exit fullscreen session. The > value will be true if the button is enabled, > recorded on exit fullscreen then, I assume? > (also, if I close the browser when fullscreeen, I assume it might not be > incremented.) > > https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... > tools/metrics/histograms/histograms.xml:1217: +<histogram > name="Cast.Sender.CastPlaySuccess"> > perhaps: > units="Success" > > https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... > tools/metrics/histograms/histograms.xml:1227: + Records the percentage of the > video left by the time the remote playback is > ^by the^at the > > Can you point me to where this histogram is emitted to? I'd like to see what > the parameters (min, max, buckets, etc.) are for it.
I have logged into gerrit before (both under @google and @chromium accounts). I have one minor comment below. Other than that, things generally look good. Does the histogram I asked about (Cast.Sender.CastTimeRemainingPercentage) use the UMA_HISTOGRAM_PERCENTAGE macro? If so, I'd like like to encourage you to use a method that buckets more coarsely--do you really need 100 buckets of precision? --mark https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1202: + entirely if it's disabled, so it's possible for the false values to be 0. On 2013/11/04 16:46:14, may wrote: > So currently, the Chromecast button shows up as an overlay when the user > fullscreens a video. This histogram is intended to record if the user can see > the button or not in that view. Given that the button is a regular Android > Button, it has the usual enabled/disabled state, which is something I wanted to > record also since obviously a user can't click on a disabled button. The current > UX (in beta, still subject to change) hides the button if it's not enabled, > hence the warning in the description that there might be no "disabled" values > recorded. Okay. In this case, no change in the description and histogram name is necessary. (I would've preferred a better name in the first place, but this name is not as far wrong as I originally thought.) > > On 2013/11/01 16:16:09, Mark P wrote: > > Wha? This histogram is inappropriately named then. Shouldn't it be called > > something like > > Cast.Sender.CastButtonEligibleToBeShown > > ? > > > > analogous comment on histogram below > https://codereview.chromium.org/55193003/diff/160001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/55193003/diff/160001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1220: + exit fullscreen session. The value will be true if the button is enabled, On 2013/11/04 16:46:14, may wrote: > No, recorded on enter fullscreen, but only once. So closing the browser while in > fullscreen would still be recorded. In this case, perhaps you want to change: The value will only be recorded once per fullscreen/exit fullscreen session. -> The value is recorded upon entering fullscreen. > On 2013/11/01 16:16:09, Mark P wrote: > > recorded on exit fullscreen then, I assume? > > (also, if I close the browser when fullscreeen, I assume it might not be > > incremented.) >
Ok, this time adding you seemed to have worked. Buckets don't have to be that granular, I updated the other half of the UMA code (which you can see in gerrit) to use 10 buckets. On 2013/11/04 18:30:14, Mark P wrote: > I have logged into gerrit before (both under @google and @chromium accounts). > > I have one minor comment below. Other than that, things generally look good. > > Does the histogram I asked about (Cast.Sender.CastTimeRemainingPercentage) use > the UMA_HISTOGRAM_PERCENTAGE macro? If so, I'd like like to encourage you to > use a method that buckets more coarsely--do you really need 100 buckets of > precision? > > --mark > > https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/55193003/diff/110001/tools/metrics/histograms... > tools/metrics/histograms/histograms.xml:1202: + entirely if it's disabled, so > it's possible for the false values to be 0. > On 2013/11/04 16:46:14, may wrote: > > So currently, the Chromecast button shows up as an overlay when the user > > fullscreens a video. This histogram is intended to record if the user can see > > the button or not in that view. Given that the button is a regular Android > > Button, it has the usual enabled/disabled state, which is something I wanted > to > > record also since obviously a user can't click on a disabled button. The > current > > UX (in beta, still subject to change) hides the button if it's not enabled, > > hence the warning in the description that there might be no "disabled" values > > recorded. > > Okay. In this case, no change in the description and histogram name is > necessary. (I would've preferred a better name in the first place, but this > name is not as far wrong as I originally thought.) > > > > On 2013/11/01 16:16:09, Mark P wrote: > > > Wha? This histogram is inappropriately named then. Shouldn't it be called > > > something like > > > Cast.Sender.CastButtonEligibleToBeShown > > > ? > > > > > > analogous comment on histogram below > > > > https://codereview.chromium.org/55193003/diff/160001/tools/metrics/histograms... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/55193003/diff/160001/tools/metrics/histograms... > tools/metrics/histograms/histograms.xml:1220: + exit fullscreen session. The > value will be true if the button is enabled, > On 2013/11/04 16:46:14, may wrote: > > No, recorded on enter fullscreen, but only once. So closing the browser while > in > > fullscreen would still be recorded. > > In this case, perhaps you want to change: > The value will only be recorded once per fullscreen/exit fullscreen session. > -> > The value is recorded upon entering fullscreen. > > > On 2013/11/01 16:16:09, Mark P wrote: > > > recorded on exit fullscreen then, I assume? > > > (also, if I close the browser when fullscreeen, I assume it might not be > > > incremented.) > >
https://codereview.chromium.org/55193003/diff/160001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/55193003/diff/160001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1220: + exit fullscreen session. The value will be true if the button is enabled, On 2013/11/04 18:30:15, Mark P wrote: > On 2013/11/04 16:46:14, may wrote: > > No, recorded on enter fullscreen, but only once. So closing the browser while > in > > fullscreen would still be recorded. > > In this case, perhaps you want to change: > The value will only be recorded once per fullscreen/exit fullscreen session. > -> > The value is recorded upon entering fullscreen. > > > On 2013/11/01 16:16:09, Mark P wrote: > > > recorded on exit fullscreen then, I assume? > > > (also, if I close the browser when fullscreeen, I assume it might not be > > > incremented.) > > Done.
On Mon, Nov 4, 2013 at 11:03 AM, <maybelle@chromium.org> wrote: > Ok, this time adding you seemed to have worked. Buckets don't have to be > that > granular, I updated the other half of the UMA code (which you can see in > gerrit) > to use 10 buckets. Okay, thanks. gerrit is acting funny for me. I don't have permission to see the issue you added me to (although I get sent the -emails containing the patch set). BTW, given that the original change hasn't been submitted, maybe you can clarify the "Shown" histogram names too? --mark > > > On 2013/11/04 18:30:14, Mark P wrote: > >> I have logged into gerrit before (both under @google and @chromium >> accounts). >> > > I have one minor comment below. Other than that, things generally look >> good. >> > > Does the histogram I asked about (Cast.Sender.CastTimeRemainingPercentage) >> use >> the UMA_HISTOGRAM_PERCENTAGE macro? If so, I'd like like to encourage >> you to >> use a method that buckets more coarsely--do you really need 100 buckets of >> precision? >> > > --mark >> > > > https://codereview.chromium.org/55193003/diff/110001/ > tools/metrics/histograms/histograms.xml > >> File tools/metrics/histograms/histograms.xml (right): >> > > > https://codereview.chromium.org/55193003/diff/110001/ > tools/metrics/histograms/histograms.xml#newcode1202 > >> tools/metrics/histograms/histograms.xml:1202: + entirely if it's >> disabled, >> > so > >> it's possible for the false values to be 0. >> On 2013/11/04 16:46:14, may wrote: >> > So currently, the Chromecast button shows up as an overlay when the user >> > fullscreens a video. This histogram is intended to record if the user >> can >> > see > >> > the button or not in that view. Given that the button is a regular >> Android >> > Button, it has the usual enabled/disabled state, which is something I >> wanted >> to >> > record also since obviously a user can't click on a disabled button. The >> current >> > UX (in beta, still subject to change) hides the button if it's not >> enabled, >> > hence the warning in the description that there might be no "disabled" >> > values > >> > recorded. >> > > Okay. In this case, no change in the description and histogram name is >> necessary. (I would've preferred a better name in the first place, but >> this >> name is not as far wrong as I originally thought.) >> > >> > On 2013/11/01 16:16:09, Mark P wrote: >> > > Wha? This histogram is inappropriately named then. Shouldn't it be >> > called > >> > > something like >> > > Cast.Sender.CastButtonEligibleToBeShown >> > > ? >> > > >> > > analogous comment on histogram below >> > >> > > > https://codereview.chromium.org/55193003/diff/160001/ > tools/metrics/histograms/histograms.xml > >> File tools/metrics/histograms/histograms.xml (right): >> > > > https://codereview.chromium.org/55193003/diff/160001/ > tools/metrics/histograms/histograms.xml#newcode1220 > >> tools/metrics/histograms/histograms.xml:1220: + exit fullscreen >> session. >> > The > >> value will be true if the button is enabled, >> On 2013/11/04 16:46:14, may wrote: >> > No, recorded on enter fullscreen, but only once. So closing the browser >> > while > >> in >> > fullscreen would still be recorded. >> > > In this case, perhaps you want to change: >> The value will only be recorded once per fullscreen/exit fullscreen >> session. >> -> >> The value is recorded upon entering fullscreen. >> > > > On 2013/11/01 16:16:09, Mark P wrote: >> > > recorded on exit fullscreen then, I assume? >> > > (also, if I close the browser when fullscreeen, I assume it might not >> be >> > > incremented.) >> > >> > > > > https://codereview.chromium.org/55193003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
"CastButtonVisble"? I'm running low on name ideas today. On 2013/11/04 19:07:08, Mark P wrote: > On Mon, Nov 4, 2013 at 11:03 AM, <mailto:maybelle@chromium.org> wrote: > > > Ok, this time adding you seemed to have worked. Buckets don't have to be > > that > > granular, I updated the other half of the UMA code (which you can see in > > gerrit) > > to use 10 buckets. > > > Okay, thanks. > > gerrit is acting funny for me. I don't have permission to see the issue > you added me to (although I get sent the -emails containing the patch set). > > BTW, given that the original change hasn't been submitted, maybe you can > clarify the "Shown" histogram names too? > > --mark > > > > > > > > > > On 2013/11/04 18:30:14, Mark P wrote: > > > >> I have logged into gerrit before (both under @google and @chromium > >> accounts). > >> > > > > I have one minor comment below. Other than that, things generally look > >> good. > >> > > > > Does the histogram I asked about (Cast.Sender.CastTimeRemainingPercentage) > >> use > >> the UMA_HISTOGRAM_PERCENTAGE macro? If so, I'd like like to encourage > >> you to > >> use a method that buckets more coarsely--do you really need 100 buckets of > >> precision? > >> > > > > --mark > >> > > > > > > https://codereview.chromium.org/55193003/diff/110001/ > > tools/metrics/histograms/histograms.xml > > > >> File tools/metrics/histograms/histograms.xml (right): > >> > > > > > > https://codereview.chromium.org/55193003/diff/110001/ > > tools/metrics/histograms/histograms.xml#newcode1202 > > > >> tools/metrics/histograms/histograms.xml:1202: + entirely if it's > >> disabled, > >> > > so > > > >> it's possible for the false values to be 0. > >> On 2013/11/04 16:46:14, may wrote: > >> > So currently, the Chromecast button shows up as an overlay when the user > >> > fullscreens a video. This histogram is intended to record if the user > >> can > >> > > see > > > >> > the button or not in that view. Given that the button is a regular > >> Android > >> > Button, it has the usual enabled/disabled state, which is something I > >> wanted > >> to > >> > record also since obviously a user can't click on a disabled button. The > >> current > >> > UX (in beta, still subject to change) hides the button if it's not > >> enabled, > >> > hence the warning in the description that there might be no "disabled" > >> > > values > > > >> > recorded. > >> > > > > Okay. In this case, no change in the description and histogram name is > >> necessary. (I would've preferred a better name in the first place, but > >> this > >> name is not as far wrong as I originally thought.) > >> > > >> > On 2013/11/01 16:16:09, Mark P wrote: > >> > > Wha? This histogram is inappropriately named then. Shouldn't it be > >> > > called > > > >> > > something like > >> > > Cast.Sender.CastButtonEligibleToBeShown > >> > > ? > >> > > > >> > > analogous comment on histogram below > >> > > >> > > > > > > https://codereview.chromium.org/55193003/diff/160001/ > > tools/metrics/histograms/histograms.xml > > > >> File tools/metrics/histograms/histograms.xml (right): > >> > > > > > > https://codereview.chromium.org/55193003/diff/160001/ > > tools/metrics/histograms/histograms.xml#newcode1220 > > > >> tools/metrics/histograms/histograms.xml:1220: + exit fullscreen > >> session. > >> > > The > > > >> value will be true if the button is enabled, > >> On 2013/11/04 16:46:14, may wrote: > >> > No, recorded on enter fullscreen, but only once. So closing the browser > >> > > while > > > >> in > >> > fullscreen would still be recorded. > >> > > > > In this case, perhaps you want to change: > >> The value will only be recorded once per fullscreen/exit fullscreen > >> session. > >> -> > >> The value is recorded upon entering fullscreen. > >> > > > > > On 2013/11/01 16:16:09, Mark P wrote: > >> > > recorded on exit fullscreen then, I assume? > >> > > (also, if I close the browser when fullscreeen, I assume it might not > >> be > >> > > incremented.) > >> > > >> > > > > > > > > https://codereview.chromium.org/55193003/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On Mon, Nov 4, 2013 at 11:10 AM, <maybelle@chromium.org> wrote: > "CastButtonVisble"? I'm running low on name ideas today. > I liked inserting the word "Eligible" in there, as in my original comments, but if you don't like that you can leave it as is. --mark > > On 2013/11/04 19:07:08, Mark P wrote: > > On Mon, Nov 4, 2013 at 11:03 AM, <mailto:maybelle@chromium.org> wrote: >> > > > Ok, this time adding you seemed to have worked. Buckets don't have to be >> > that >> > granular, I updated the other half of the UMA code (which you can see in >> > gerrit) >> > to use 10 buckets. >> > > > Okay, thanks. >> > > gerrit is acting funny for me. I don't have permission to see the issue >> you added me to (although I get sent the -emails containing the patch >> set). >> > > BTW, given that the original change hasn't been submitted, maybe you can >> clarify the "Shown" histogram names too? >> > > --mark >> > > > > > > >> > >> > On 2013/11/04 18:30:14, Mark P wrote: >> > >> >> I have logged into gerrit before (both under @google and @chromium >> >> accounts). >> >> >> > >> > I have one minor comment below. Other than that, things generally look >> >> good. >> >> >> > >> > Does the histogram I asked about (Cast.Sender. >> CastTimeRemainingPercentage) >> >> use >> >> the UMA_HISTOGRAM_PERCENTAGE macro? If so, I'd like like to encourage >> >> you to >> >> use a method that buckets more coarsely--do you really need 100 >> buckets of >> >> precision? >> >> >> > >> > --mark >> >> >> > >> > >> > https://codereview.chromium.org/55193003/diff/110001/ >> > tools/metrics/histograms/histograms.xml >> > >> >> File tools/metrics/histograms/histograms.xml (right): >> >> >> > >> > >> > https://codereview.chromium.org/55193003/diff/110001/ >> > tools/metrics/histograms/histograms.xml#newcode1202 >> > >> >> tools/metrics/histograms/histograms.xml:1202: + entirely if it's >> >> disabled, >> >> >> > so >> > >> >> it's possible for the false values to be 0. >> >> On 2013/11/04 16:46:14, may wrote: >> >> > So currently, the Chromecast button shows up as an overlay when the >> user >> >> > fullscreens a video. This histogram is intended to record if the user >> >> can >> >> >> > see >> > >> >> > the button or not in that view. Given that the button is a regular >> >> Android >> >> > Button, it has the usual enabled/disabled state, which is something I >> >> wanted >> >> to >> >> > record also since obviously a user can't click on a disabled button. >> The >> >> current >> >> > UX (in beta, still subject to change) hides the button if it's not >> >> enabled, >> >> > hence the warning in the description that there might be no >> "disabled" >> >> >> > values >> > >> >> > recorded. >> >> >> > >> > Okay. In this case, no change in the description and histogram name is >> >> necessary. (I would've preferred a better name in the first place, but >> >> this >> >> name is not as far wrong as I originally thought.) >> >> > >> >> > On 2013/11/01 16:16:09, Mark P wrote: >> >> > > Wha? This histogram is inappropriately named then. Shouldn't it >> be >> >> >> > called >> > >> >> > > something like >> >> > > Cast.Sender.CastButtonEligibleToBeShown >> >> > > ? >> >> > > >> >> > > analogous comment on histogram below >> >> > >> >> >> > >> > >> > https://codereview.chromium.org/55193003/diff/160001/ >> > tools/metrics/histograms/histograms.xml >> > >> >> File tools/metrics/histograms/histograms.xml (right): >> >> >> > >> > >> > https://codereview.chromium.org/55193003/diff/160001/ >> > tools/metrics/histograms/histograms.xml#newcode1220 >> > >> >> tools/metrics/histograms/histograms.xml:1220: + exit fullscreen >> >> session. >> >> >> > The >> > >> >> value will be true if the button is enabled, >> >> On 2013/11/04 16:46:14, may wrote: >> >> > No, recorded on enter fullscreen, but only once. So closing the >> browser >> >> >> > while >> > >> >> in >> >> > fullscreen would still be recorded. >> >> >> > >> > In this case, perhaps you want to change: >> >> The value will only be recorded once per fullscreen/exit fullscreen >> >> session. >> >> -> >> >> The value is recorded upon entering fullscreen. >> >> >> > >> > > On 2013/11/01 16:16:09, Mark P wrote: >> >> > > recorded on exit fullscreen then, I assume? >> >> > > (also, if I close the browser when fullscreeen, I assume it might >> not >> >> be >> >> > > incremented.) >> >> > >> >> >> > >> > >> > >> > https://codereview.chromium.org/55193003/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > > https://codereview.chromium.org/55193003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I'd prefer the current name as is. Does the change look good otherwise? On 2013/11/04 19:13:40, Mark P wrote: > On Mon, Nov 4, 2013 at 11:10 AM, <mailto:maybelle@chromium.org> wrote: > > > "CastButtonVisble"? I'm running low on name ideas today. > > > > I liked inserting the word "Eligible" in there, as in my original comments, > but if you don't like that you can leave it as is. > > --mark > > > > > > On 2013/11/04 19:07:08, Mark P wrote: > > > > On Mon, Nov 4, 2013 at 11:03 AM, <mailto:maybelle@chromium.org> wrote: > >> > > > > > Ok, this time adding you seemed to have worked. Buckets don't have to be > >> > that > >> > granular, I updated the other half of the UMA code (which you can see in > >> > gerrit) > >> > to use 10 buckets. > >> > > > > > > Okay, thanks. > >> > > > > gerrit is acting funny for me. I don't have permission to see the issue > >> you added me to (although I get sent the -emails containing the patch > >> set). > >> > > > > BTW, given that the original change hasn't been submitted, maybe you can > >> clarify the "Shown" histogram names too? > >> > > > > --mark > >> > > > > > > > > > > > > >> > > >> > On 2013/11/04 18:30:14, Mark P wrote: > >> > > >> >> I have logged into gerrit before (both under @google and @chromium > >> >> accounts). > >> >> > >> > > >> > I have one minor comment below. Other than that, things generally look > >> >> good. > >> >> > >> > > >> > Does the histogram I asked about (Cast.Sender. > >> CastTimeRemainingPercentage) > >> >> use > >> >> the UMA_HISTOGRAM_PERCENTAGE macro? If so, I'd like like to encourage > >> >> you to > >> >> use a method that buckets more coarsely--do you really need 100 > >> buckets of > >> >> precision? > >> >> > >> > > >> > --mark > >> >> > >> > > >> > > >> > https://codereview.chromium.org/55193003/diff/110001/ > >> > tools/metrics/histograms/histograms.xml > >> > > >> >> File tools/metrics/histograms/histograms.xml (right): > >> >> > >> > > >> > > >> > https://codereview.chromium.org/55193003/diff/110001/ > >> > tools/metrics/histograms/histograms.xml#newcode1202 > >> > > >> >> tools/metrics/histograms/histograms.xml:1202: + entirely if it's > >> >> disabled, > >> >> > >> > so > >> > > >> >> it's possible for the false values to be 0. > >> >> On 2013/11/04 16:46:14, may wrote: > >> >> > So currently, the Chromecast button shows up as an overlay when the > >> user > >> >> > fullscreens a video. This histogram is intended to record if the user > >> >> can > >> >> > >> > see > >> > > >> >> > the button or not in that view. Given that the button is a regular > >> >> Android > >> >> > Button, it has the usual enabled/disabled state, which is something I > >> >> wanted > >> >> to > >> >> > record also since obviously a user can't click on a disabled button. > >> The > >> >> current > >> >> > UX (in beta, still subject to change) hides the button if it's not > >> >> enabled, > >> >> > hence the warning in the description that there might be no > >> "disabled" > >> >> > >> > values > >> > > >> >> > recorded. > >> >> > >> > > >> > Okay. In this case, no change in the description and histogram name is > >> >> necessary. (I would've preferred a better name in the first place, but > >> >> this > >> >> name is not as far wrong as I originally thought.) > >> >> > > >> >> > On 2013/11/01 16:16:09, Mark P wrote: > >> >> > > Wha? This histogram is inappropriately named then. Shouldn't it > >> be > >> >> > >> > called > >> > > >> >> > > something like > >> >> > > Cast.Sender.CastButtonEligibleToBeShown > >> >> > > ? > >> >> > > > >> >> > > analogous comment on histogram below > >> >> > > >> >> > >> > > >> > > >> > https://codereview.chromium.org/55193003/diff/160001/ > >> > tools/metrics/histograms/histograms.xml > >> > > >> >> File tools/metrics/histograms/histograms.xml (right): > >> >> > >> > > >> > > >> > https://codereview.chromium.org/55193003/diff/160001/ > >> > tools/metrics/histograms/histograms.xml#newcode1220 > >> > > >> >> tools/metrics/histograms/histograms.xml:1220: + exit fullscreen > >> >> session. > >> >> > >> > The > >> > > >> >> value will be true if the button is enabled, > >> >> On 2013/11/04 16:46:14, may wrote: > >> >> > No, recorded on enter fullscreen, but only once. So closing the > >> browser > >> >> > >> > while > >> > > >> >> in > >> >> > fullscreen would still be recorded. > >> >> > >> > > >> > In this case, perhaps you want to change: > >> >> The value will only be recorded once per fullscreen/exit fullscreen > >> >> session. > >> >> -> > >> >> The value is recorded upon entering fullscreen. > >> >> > >> > > >> > > On 2013/11/01 16:16:09, Mark P wrote: > >> >> > > recorded on exit fullscreen then, I assume? > >> >> > > (also, if I close the browser when fullscreeen, I assume it might > >> not > >> >> be > >> >> > > incremented.) > >> >> > > >> >> > >> > > >> > > >> > > >> > https://codereview.chromium.org/55193003/ > >> > > >> > > > > To unsubscribe from this group and stop receiving emails from it, send an > >> > > email > > > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> > > > > > > > > https://codereview.chromium.org/55193003/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
lgtm On Wed, Nov 6, 2013 at 4:38 AM, <maybelle@chromium.org> wrote: > I'd prefer the current name as is. Does the change look good otherwise? > > On 2013/11/04 19:13:40, Mark P wrote: > > On Mon, Nov 4, 2013 at 11:10 AM, <mailto:maybelle@chromium.org> wrote: >> > > > "CastButtonVisble"? I'm running low on name ideas today. >> > >> > > I liked inserting the word "Eligible" in there, as in my original >> comments, >> but if you don't like that you can leave it as is. >> > > --mark >> > > > > >> > On 2013/11/04 19:07:08, Mark P wrote: >> > >> > On Mon, Nov 4, 2013 at 11:03 AM, <mailto:maybelle@chromium.org> wrote: >> >> >> > >> > > Ok, this time adding you seemed to have worked. Buckets don't have >> to be >> >> > that >> >> > granular, I updated the other half of the UMA code (which you can >> see in >> >> > gerrit) >> >> > to use 10 buckets. >> >> >> > >> > >> > Okay, thanks. >> >> >> > >> > gerrit is acting funny for me. I don't have permission to see the >> issue >> >> you added me to (although I get sent the -emails containing the patch >> >> set). >> >> >> > >> > BTW, given that the original change hasn't been submitted, maybe you >> can >> >> clarify the "Shown" histogram names too? >> >> >> > >> > --mark >> >> >> > >> > >> > >> > >> > > >> >> > >> >> > On 2013/11/04 18:30:14, Mark P wrote: >> >> > >> >> >> I have logged into gerrit before (both under @google and @chromium >> >> >> accounts). >> >> >> >> >> > >> >> > I have one minor comment below. Other than that, things generally >> look >> >> >> good. >> >> >> >> >> > >> >> > Does the histogram I asked about (Cast.Sender. >> >> CastTimeRemainingPercentage) >> >> >> use >> >> >> the UMA_HISTOGRAM_PERCENTAGE macro? If so, I'd like like to >> encourage >> >> >> you to >> >> >> use a method that buckets more coarsely--do you really need 100 >> >> buckets of >> >> >> precision? >> >> >> >> >> > >> >> > --mark >> >> >> >> >> > >> >> > >> >> > https://codereview.chromium.org/55193003/diff/110001/ >> >> > tools/metrics/histograms/histograms.xml >> >> > >> >> >> File tools/metrics/histograms/histograms.xml (right): >> >> >> >> >> > >> >> > >> >> > https://codereview.chromium.org/55193003/diff/110001/ >> >> > tools/metrics/histograms/histograms.xml#newcode1202 >> >> > >> >> >> tools/metrics/histograms/histograms.xml:1202: + entirely if it's >> >> >> disabled, >> >> >> >> >> > so >> >> > >> >> >> it's possible for the false values to be 0. >> >> >> On 2013/11/04 16:46:14, may wrote: >> >> >> > So currently, the Chromecast button shows up as an overlay when >> the >> >> user >> >> >> > fullscreens a video. This histogram is intended to record if the >> user >> >> >> can >> >> >> >> >> > see >> >> > >> >> >> > the button or not in that view. Given that the button is a regular >> >> >> Android >> >> >> > Button, it has the usual enabled/disabled state, which is >> something I >> >> >> wanted >> >> >> to >> >> >> > record also since obviously a user can't click on a disabled >> button. >> >> The >> >> >> current >> >> >> > UX (in beta, still subject to change) hides the button if it's not >> >> >> enabled, >> >> >> > hence the warning in the description that there might be no >> >> "disabled" >> >> >> >> >> > values >> >> > >> >> >> > recorded. >> >> >> >> >> > >> >> > Okay. In this case, no change in the description and histogram >> name is >> >> >> necessary. (I would've preferred a better name in the first place, >> but >> >> >> this >> >> >> name is not as far wrong as I originally thought.) >> >> >> > >> >> >> > On 2013/11/01 16:16:09, Mark P wrote: >> >> >> > > Wha? This histogram is inappropriately named then. Shouldn't >> it >> >> be >> >> >> >> >> > called >> >> > >> >> >> > > something like >> >> >> > > Cast.Sender.CastButtonEligibleToBeShown >> >> >> > > ? >> >> >> > > >> >> >> > > analogous comment on histogram below >> >> >> > >> >> >> >> >> > >> >> > >> >> > https://codereview.chromium.org/55193003/diff/160001/ >> >> > tools/metrics/histograms/histograms.xml >> >> > >> >> >> File tools/metrics/histograms/histograms.xml (right): >> >> >> >> >> > >> >> > >> >> > https://codereview.chromium.org/55193003/diff/160001/ >> >> > tools/metrics/histograms/histograms.xml#newcode1220 >> >> > >> >> >> tools/metrics/histograms/histograms.xml:1220: + exit fullscreen >> >> >> session. >> >> >> >> >> > The >> >> > >> >> >> value will be true if the button is enabled, >> >> >> On 2013/11/04 16:46:14, may wrote: >> >> >> > No, recorded on enter fullscreen, but only once. So closing the >> >> browser >> >> >> >> >> > while >> >> > >> >> >> in >> >> >> > fullscreen would still be recorded. >> >> >> >> >> > >> >> > In this case, perhaps you want to change: >> >> >> The value will only be recorded once per fullscreen/exit fullscreen >> >> >> session. >> >> >> -> >> >> >> The value is recorded upon entering fullscreen. >> >> >> >> >> > >> >> > > On 2013/11/01 16:16:09, Mark P wrote: >> >> >> > > recorded on exit fullscreen then, I assume? >> >> >> > > (also, if I close the browser when fullscreeen, I assume it >> might >> >> not >> >> >> be >> >> >> > > incremented.) >> >> >> > >> >> >> >> >> > >> >> > >> >> > >> >> > https://codereview.chromium.org/55193003/ >> >> > >> >> >> > >> > To unsubscribe from this group and stop receiving emails from it, send >> an >> >> >> > email >> > >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> >> >> > >> > >> > >> > https://codereview.chromium.org/55193003/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > > https://codereview.chromium.org/55193003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maybelle@chromium.org/55193003/220001
Message was sent while issue was closed.
Change committed as 233301 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
