Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1700743003:
Record whether and why the media default controls are being shown. (Closed)
|
Created:
4 years, 10 months ago by mlamouri (slow - plz ping) Modified:
4 years, 10 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@media-controls-usecount Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRecord whether and why the media default controls are being shown.
Whenever the default media controls should become visible, an entry is
added to an histogram recording whether the default controls are shown
and if they are, why.
BUG=583709
Committed: https://crrev.com/e911caae47d8ed83b0b5ca28284d21a7091f49d6
Cr-Commit-Position: refs/heads/master@{#377265}
Patch Set 1 #
Total comments: 10
Patch Set 2 : review comments #
Total comments: 12
Patch Set 3 : review comments #
Messages
Total messages: 21 (7 generated)
mlamouri@chromium.org changed reviewers: + asvitkine@chromium.org, philipj@opera.com
https://codereview.chromium.org/1700743003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1700743003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:94: enum DefaultControlsShow { I'd call it MediaControls, because these are the only controls that Blink knows anything about. And enum class for less repetition? https://codereview.chromium.org/1700743003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:102: } // anonymous namespace Move the below static things into the anonymous namespace too? Nobody is ever going to do it after this change. ("This looks strange, but there's probably a good reason for it, never mind.") https://codereview.chromium.org/1700743003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2110: if (fastHasAttribute(controlsAttr)) { Duplicating the conditions of shouldShowControls() but in a different order makes it non-obvious that it's precisely correct, and that should be trivial. Is it separate for performance concerns? If so, make shouldShowControls() return an enum value which is just treated as bool everywhere except before mediaControls()->show(), where it's used to count? https://codereview.chromium.org/1700743003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1700743003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:285: mediaElement().recordShowControls(); This case is akin to the mouse moving over the video, which isn't counted separately. While it is possible for ensureMediaControls() to create the controls and start playing while the video is hidden such that the user never sees the controls until moving the mouse or tabbing it into focus, these are odd cases I don't think ought to be mixed in with the question "which condition causes the media controls to be potentially visible?"
https://codereview.chromium.org/1700743003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1700743003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:19114: + <summary>Whether default media controls were shown and why.</summary> Mention when this is logged - once for each video? Or multiple times if something changes (e.g. going fullscreen).
PTAL. https://codereview.chromium.org/1700743003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1700743003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:94: enum DefaultControlsShow { On 2016/02/16 at 06:20:27, philipj_UTC7 wrote: > I'd call it MediaControls, because these are the only controls that Blink knows anything about. Done. > And enum class for less repetition? enum class are not very friendly with macros so I would have to convert to int when passing to the macro which would be... unfortunate :) https://codereview.chromium.org/1700743003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:102: } // anonymous namespace On 2016/02/16 at 06:20:27, philipj_UTC7 wrote: > Move the below static things into the anonymous namespace too? Nobody is ever going to do it after this change. ("This looks strange, but there's probably a good reason for it, never mind.") I did that here: https://codereview.chromium.org/1710013002 This CL will be based on top of it. https://codereview.chromium.org/1700743003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2110: if (fastHasAttribute(controlsAttr)) { On 2016/02/16 at 06:20:27, philipj_UTC7 wrote: > Duplicating the conditions of shouldShowControls() but in a different order makes it non-obvious that it's precisely correct, and that should be trivial. Is it separate for performance concerns? If so, make shouldShowControls() return an enum value which is just treated as bool everywhere except before mediaControls()->show(), where it's used to count? Fixed. I know have an internal show controls that record the result. https://codereview.chromium.org/1700743003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1700743003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:285: mediaElement().recordShowControls(); On 2016/02/16 at 06:20:27, philipj_UTC7 wrote: > This case is akin to the mouse moving over the video, which isn't counted separately. While it is possible for ensureMediaControls() to create the controls and start playing while the video is hidden such that the user never sees the controls until moving the mouse or tabbing it into focus, these are odd cases I don't think ought to be mixed in with the question "which condition causes the media controls to be potentially visible?" Removed. https://codereview.chromium.org/1700743003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1700743003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:19114: + <summary>Whether default media controls were shown and why.</summary> On 2016/02/16 at 22:22:09, Alexei Svitkine wrote: > Mention when this is logged - once for each video? Or multiple times if something changes (e.g. going fullscreen). Done.
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
lgtm
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1700743003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1700743003/20001
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
https://codereview.chromium.org/1700743003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1700743003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:448: bool shouldShowControlsInternal(bool record) const; We prefer using enums rather bools in Blink.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1700743003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1700743003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:118: MediaControlsShowNone, Maybe MediaControlsNotShown? https://codereview.chromium.org/1700743003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:119: MediaControlsShowMax AutoplayExperimentHelper.h has NumberOfAutoplayMetrics, unless that's a weird exception being consistent with that would be nice. UseCounter's NumberOfFeatures is also familiar to me. https://codereview.chromium.org/1700743003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3581: // These checks are in order of conditions superseeding each other, which Well, this would be true of any order where all "return true" are before the "return false". The actual reason for the order is the expected frequency of each case, with the most frequent cause first. https://codereview.chromium.org/1700743003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1700743003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:448: bool shouldShowControlsInternal(bool record) const; On 2016/02/18 16:12:17, sof wrote: > We prefer using enums rather bools in Blink. Yes, see `enum class RecordMetricsBehavior { DoNotRecord, DoRecord }`. If you do that with a default argument, it should suffice to just keep shouldShowControls(), with one place that passes DoRecord. https://codereview.chromium.org/1700743003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1700743003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19190: +<histogram name="Media.Controls.Show" enum="MediaDefaultControlsReason"> I'd remove "default" everywhere in this file as well.
Comments applied. PTAL. https://codereview.chromium.org/1700743003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1700743003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:118: MediaControlsShowNone, On 2016/02/19 at 04:05:43, philipj_UTC7 wrote: > Maybe MediaControlsNotShown? Done. https://codereview.chromium.org/1700743003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:119: MediaControlsShowMax On 2016/02/19 at 04:05:43, philipj_UTC7 wrote: > AutoplayExperimentHelper.h has NumberOfAutoplayMetrics, unless that's a weird exception being consistent with that would be nice. UseCounter's NumberOfFeatures is also familiar to me. Yes, it is a weird exception, see: https://code.google.com/p/chromium/codesearch#search/&q=%22DEFINE_STATIC_LOCA... https://codereview.chromium.org/1700743003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3581: // These checks are in order of conditions superseeding each other, which On 2016/02/19 at 04:05:43, philipj_UTC7 wrote: > Well, this would be true of any order where all "return true" are before the "return false". The actual reason for the order is the expected frequency of each case, with the most frequent cause first. I understand what you mean. I guess my comment is confusing. I simply removed it. https://codereview.chromium.org/1700743003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1700743003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:448: bool shouldShowControlsInternal(bool record) const; On 2016/02/19 at 04:05:43, philipj_UTC7 wrote: > On 2016/02/18 16:12:17, sof wrote: > > We prefer using enums rather bools in Blink. > > Yes, see `enum class RecordMetricsBehavior { DoNotRecord, DoRecord }`. If you do that with a default argument, it should suffice to just keep shouldShowControls(), with one place that passes DoRecord. Re-used this enum. My understanding was that Chromium avoids default arguments but I can see Blink doing this in a few places. https://codereview.chromium.org/1700743003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1700743003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19190: +<histogram name="Media.Controls.Show" enum="MediaDefaultControlsReason"> On 2016/02/19 at 04:05:43, philipj_UTC7 wrote: > I'd remove "default" everywhere in this file as well. Done. Renamed to MediaControlsShowReason.
lgtm https://codereview.chromium.org/1700743003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1700743003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:119: MediaControlsShowMax On 2016/02/23 18:42:13, Mounir Lamouri wrote: > On 2016/02/19 at 04:05:43, philipj_UTC7 wrote: > > AutoplayExperimentHelper.h has NumberOfAutoplayMetrics, unless that's a weird > exception being consistent with that would be nice. UseCounter's > NumberOfFeatures is also familiar to me. > > Yes, it is a weird exception, see: > https://code.google.com/p/chromium/codesearch#search/&q=%22DEFINE_STATIC_LOCA... OK, there's a bit of both, and some *Count as well.
The CQ bit was checked by mlamouri@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1700743003/#ps40001 (title: "review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1700743003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1700743003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Record whether and why the media default controls are being shown. Whenever the default media controls should become visible, an entry is added to an histogram recording whether the default controls are shown and if they are, why. BUG=583709 ========== to ========== Record whether and why the media default controls are being shown. Whenever the default media controls should become visible, an entry is added to an histogram recording whether the default controls are shown and if they are, why. BUG=583709 Committed: https://crrev.com/e911caae47d8ed83b0b5ca28284d21a7091f49d6 Cr-Commit-Position: refs/heads/master@{#377265} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e911caae47d8ed83b0b5ca28284d21a7091f49d6 Cr-Commit-Position: refs/heads/master@{#377265} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
