Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1884573004:
Add user actions to record media controls interaction. (Closed)
|
Created:
4 years, 8 months ago by mlamouri (slow - plz ping) Modified:
4 years, 8 months ago CC:
asvitkine+watch_chromium.org, blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, philipj_slow, nessy, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd user actions to record media controls interaction.
This is recording interaction on:
- Play/Pause
- Play button overlay
- Fullscreen (enter/exit)
- Mute/Unmute
- Volume slider
- Scrubber
- Closed Caption (show/hide)
- Cast (built-in/overlay)
BUG=602714
R=isherman@chromium.org, philipj@opera.com
Committed: https://crrev.com/ea625d2ecbc66d8b47468c2662f399ed88e0eb22
Cr-Commit-Position: refs/heads/master@{#387342}
Patch Set 1 #Patch Set 2 : jochen@chromium.org #Patch Set 3 : #Patch Set 4 : fix #
Total comments: 23
Patch Set 5 : review comments #
Messages
Total messages: 25 (7 generated)
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1884573004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1884573004/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1884573004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1884573004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/04/13 at 13:17:00, commit-bot wrote: > Dry run: Try jobs failed on following builders: > linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ARGH... seems like there was a merge of CLs here.
back to normal, sorry for the noise
Thanks! Metrics LGTM % nits: https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:7850: <description>The user hide the closed caption from the controls.</description> nit: s/hide/hid, or possibly "chose to hide" or something like that. https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:7855: <description>Please enter the description of the metric.</description> Please update :) https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:7858: <action name="Media.Controls.ClosedCoptionShow"> nit: typo -- please merge this into the above. https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:7867: <description>The user entered fullscreen mode for the controls.</description> nit: Did you mean "from the controls"? https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:7872: <description>The user left fullscreen mode for the controls.</description> nit: Did you mean "from the controls"? https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:7877: <description>The user muted a media from the controls.</description> nit: "a media" -> "a media element" (here and throughout the file) https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:7904: <action name="Media.Controls.ScrubbingEnd"> I'm not sure that you'll really get much value from separating out scrubbing begin vs. end, but you're welcome to try if you think it'd be useful. https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:7923: <action name="Media.Controls.VolumeChangeEnd"> Ditto.
lgtm % nits and questions https://codereview.chromium.org/1884573004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/1884573004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:383: Platform::current()->recordAction(UserMetricsAction("Media.Controls.ClosedCaptionShow")); Isn't this the wrong way around? https://codereview.chromium.org/1884573004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:421: Platform::current()->recordAction(UserMetricsAction("Media.Controls.ScrubbingBegin")); Can anything much be learned from this? How about just record if a seek happens as a result? https://codereview.chromium.org/1884573004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:506: Platform::current()->recordAction(UserMetricsAction("Media.Controls.VolumeChangeBegin")); Same question here.
The CQ bit was checked by mlamouri@chromium.org
https://codereview.chromium.org/1884573004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/1884573004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:383: Platform::current()->recordAction(UserMetricsAction("Media.Controls.ClosedCaptionShow")); On 2016/04/14 at 13:46:15, philipj_slow wrote: > Isn't this the wrong way around? Oh boy... Thanks! :) https://codereview.chromium.org/1884573004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:421: Platform::current()->recordAction(UserMetricsAction("Media.Controls.ScrubbingBegin")); On 2016/04/14 at 13:46:15, philipj_slow wrote: > Can anything much be learned from this? How about just record if a seek happens as a result? I think the time spent seeking is an interesting metric. It might correlate with how hard it is to seek. For example, if the scrubber is shorter, people might seek as much but have a harder time. https://codereview.chromium.org/1884573004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:506: Platform::current()->recordAction(UserMetricsAction("Media.Controls.VolumeChangeBegin")); On 2016/04/14 at 13:46:15, philipj_slow wrote: > Same question here. Same here, though it's a bit weaker. I mostly did it for completeness and consistency in this case. I don't think we are going to change the size of the volume slider anytime soon because it's not a hot and expensive real estate ;) https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:7850: <description>The user hide the closed caption from the controls.</description> On 2016/04/13 at 19:12:12, Ilya Sherman wrote: > nit: s/hide/hid, or possibly "chose to hide" or something like that. Went with "hid". It's short and clear enough IMO. https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:7855: <description>Please enter the description of the metric.</description> On 2016/04/13 at 19:12:13, Ilya Sherman wrote: > Please update :) That's really odd. I wonder if it was auto-generated because of the typo below. https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:7858: <action name="Media.Controls.ClosedCoptionShow"> On 2016/04/13 at 19:12:13, Ilya Sherman wrote: > nit: typo -- please merge this into the above. Done. https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:7867: <description>The user entered fullscreen mode for the controls.</description> On 2016/04/13 at 19:12:12, Ilya Sherman wrote: > nit: Did you mean "from the controls"? Yes :) https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:7872: <description>The user left fullscreen mode for the controls.</description> On 2016/04/13 at 19:12:13, Ilya Sherman wrote: > nit: Did you mean "from the controls"? Did you mean I'm abusing copy-paste? :) https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:7877: <description>The user muted a media from the controls.</description> On 2016/04/13 at 19:12:13, Ilya Sherman wrote: > nit: "a media" -> "a media element" (here and throughout the file) Done. https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:7904: <action name="Media.Controls.ScrubbingEnd"> On 2016/04/13 at 19:12:12, Ilya Sherman wrote: > I'm not sure that you'll really get much value from separating out scrubbing begin vs. end, but you're welcome to try if you think it'd be useful. As said above, I think the time it takes to scrub might correlate with how hard it is to actually succeed. https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:7923: <action name="Media.Controls.VolumeChangeEnd"> On 2016/04/13 at 19:12:12, Ilya Sherman wrote: > Ditto. As said above, that one is mostly completeness and consistency.
The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1884573004/#ps80001 (title: "review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1884573004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1884573004/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add user actions to record media controls interaction. This is recording interaction on: - Play/Pause - Play button overlay - Fullscreen (enter/exit) - Mute/Unmute - Volume slider - Scrubber - Closed Caption (show/hide) - Cast (built-in/overlay) BUG=602714 R=isherman@chromium.org, philipj@opera.com ========== to ========== Add user actions to record media controls interaction. This is recording interaction on: - Play/Pause - Play button overlay - Fullscreen (enter/exit) - Mute/Unmute - Volume slider - Scrubber - Closed Caption (show/hide) - Cast (built-in/overlay) BUG=602714 R=isherman@chromium.org, philipj@opera.com Committed: https://crrev.com/ea625d2ecbc66d8b47468c2662f399ed88e0eb22 Cr-Commit-Position: refs/heads/master@{#387342} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ea625d2ecbc66d8b47468c2662f399ed88e0eb22 Cr-Commit-Position: refs/heads/master@{#387342}
Message was sent while issue was closed.
https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:7904: <action name="Media.Controls.ScrubbingEnd"> On 2016/04/14 15:46:30, Mounir Lamouri wrote: > On 2016/04/13 at 19:12:12, Ilya Sherman wrote: > > I'm not sure that you'll really get much value from separating out scrubbing > begin vs. end, but you're welcome to try if you think it'd be useful. > > As said above, I think the time it takes to scrub might correlate with how hard > it is to actually succeed. That might be true, but how do you plan to view that time? The dashboards don't make duration data very easy to view for user actions, though you could write custom Dremel queries. If you're not planning to write the Dremel queries, then I'd expect the Begin/End split won't give you much actionable data.
Message was sent while issue was closed.
On 2016/04/14 at 20:41:26, isherman wrote: > https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... > File tools/metrics/actions/actions.xml (right): > > https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... > tools/metrics/actions/actions.xml:7904: <action name="Media.Controls.ScrubbingEnd"> > On 2016/04/14 15:46:30, Mounir Lamouri wrote: > > On 2016/04/13 at 19:12:12, Ilya Sherman wrote: > > > I'm not sure that you'll really get much value from separating out scrubbing > > begin vs. end, but you're welcome to try if you think it'd be useful. > > > > As said above, I think the time it takes to scrub might correlate with how hard > > it is to actually succeed. > > That might be true, but how do you plan to view that time? The dashboards don't make duration data very easy to view for user actions, though you could write custom Dremel queries. If you're not planning to write the Dremel queries, then I'd expect the Begin/End split won't give you much actionable data. For the case of seek, there is ongoing work to change the size of the seekable area. The UI team has been looking for metrics to know if engagement would be impacted by such a change. I really expect them to use this information. I will definitely push them in this direction.
Message was sent while issue was closed.
On 2016/04/15 10:27:06, Mounir Lamouri wrote: > On 2016/04/14 at 20:41:26, isherman wrote: > > > https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... > > File tools/metrics/actions/actions.xml (right): > > > > > https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... > > tools/metrics/actions/actions.xml:7904: <action > name="Media.Controls.ScrubbingEnd"> > > On 2016/04/14 15:46:30, Mounir Lamouri wrote: > > > On 2016/04/13 at 19:12:12, Ilya Sherman wrote: > > > > I'm not sure that you'll really get much value from separating out > scrubbing > > > begin vs. end, but you're welcome to try if you think it'd be useful. > > > > > > As said above, I think the time it takes to scrub might correlate with how > hard > > > it is to actually succeed. > > > > That might be true, but how do you plan to view that time? The dashboards > don't make duration data very easy to view for user actions, though you could > write custom Dremel queries. If you're not planning to write the Dremel > queries, then I'd expect the Begin/End split won't give you much actionable > data. > > For the case of seek, there is ongoing work to change the size of the seekable > area. The UI team has been looking for metrics to know if engagement would be > impacted by such a change. I really expect them to use this information. I will > definitely push them in this direction. My point is not that I doubt the duration data would be useful. Rather, I'm saying that by choosing to record this data as a user action rather than as a histogram, you'll likely have a hard time using the UMA dashboards and tools to get at the duration data.
Message was sent while issue was closed.
On 2016/04/15 at 21:59:10, isherman wrote: > On 2016/04/15 10:27:06, Mounir Lamouri wrote: > > On 2016/04/14 at 20:41:26, isherman wrote: > > > > > https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... > > > File tools/metrics/actions/actions.xml (right): > > > > > > > > https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... > > > tools/metrics/actions/actions.xml:7904: <action > > name="Media.Controls.ScrubbingEnd"> > > > On 2016/04/14 15:46:30, Mounir Lamouri wrote: > > > > On 2016/04/13 at 19:12:12, Ilya Sherman wrote: > > > > > I'm not sure that you'll really get much value from separating out > > scrubbing > > > > begin vs. end, but you're welcome to try if you think it'd be useful. > > > > > > > > As said above, I think the time it takes to scrub might correlate with how > > hard > > > > it is to actually succeed. > > > > > > That might be true, but how do you plan to view that time? The dashboards > > don't make duration data very easy to view for user actions, though you could > > write custom Dremel queries. If you're not planning to write the Dremel > > queries, then I'd expect the Begin/End split won't give you much actionable > > data. > > > > For the case of seek, there is ongoing work to change the size of the seekable > > area. The UI team has been looking for metrics to know if engagement would be > > impacted by such a change. I really expect them to use this information. I will > > definitely push them in this direction. > > My point is not that I doubt the duration data would be useful. Rather, I'm saying that by choosing to record this data as a user action rather than as a histogram, you'll likely have a hard time using the UMA dashboards and tools to get at the duration data. Do you mean that you think I should compute the timing in the C++ code and save it as an histogram instead of saving two user actions and compute with a dremel query how long it did take?
Message was sent while issue was closed.
On 2016/04/16 10:34:16, Mounir Lamouri wrote: > On 2016/04/15 at 21:59:10, isherman wrote: > > On 2016/04/15 10:27:06, Mounir Lamouri wrote: > > > On 2016/04/14 at 20:41:26, isherman wrote: > > > > > > > > https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... > > > > File tools/metrics/actions/actions.xml (right): > > > > > > > > > > > > https://codereview.chromium.org/1884573004/diff/60001/tools/metrics/actions/a... > > > > tools/metrics/actions/actions.xml:7904: <action > > > name="Media.Controls.ScrubbingEnd"> > > > > On 2016/04/14 15:46:30, Mounir Lamouri wrote: > > > > > On 2016/04/13 at 19:12:12, Ilya Sherman wrote: > > > > > > I'm not sure that you'll really get much value from separating out > > > scrubbing > > > > > begin vs. end, but you're welcome to try if you think it'd be useful. > > > > > > > > > > As said above, I think the time it takes to scrub might correlate with > how > > > hard > > > > > it is to actually succeed. > > > > > > > > That might be true, but how do you plan to view that time? The dashboards > > > don't make duration data very easy to view for user actions, though you > could > > > write custom Dremel queries. If you're not planning to write the Dremel > > > queries, then I'd expect the Begin/End split won't give you much actionable > > > data. > > > > > > For the case of seek, there is ongoing work to change the size of the > seekable > > > area. The UI team has been looking for metrics to know if engagement would > be > > > impacted by such a change. I really expect them to use this information. I > will > > > definitely push them in this direction. > > > > My point is not that I doubt the duration data would be useful. Rather, I'm > saying that by choosing to record this data as a user action rather than as a > histogram, you'll likely have a hard time using the UMA dashboards and tools to > get at the duration data. > > Do you mean that you think I should compute the timing in the C++ code and save > it as an histogram instead of saving two user actions and compute with a dremel > query how long it did take? Right, I think that would be simpler for the analysis step. If you're comfortable writing and manually running the Dremel query, then it's fine either way. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
