Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2091253002:
When autoplaying muted video, record when unmute happen and the result. (Closed)
|
Created:
4 years, 6 months ago by mlamouri (slow - plz ping) Modified:
4 years, 5 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, nessy, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWhen autoplaying muted video, record when unmute happen and the result.
Unmuting might fail if the element was not unlocked. This is record the
unmute action only if the element is muted and was playing because of
the autoplay muted video feature. It will be recorded once per element.
BUG=622711
Committed: https://crrev.com/d26c8d5d38d8613d61dfe02af9451275868ecb98
Cr-Commit-Position: refs/heads/master@{#402937}
Patch Set 1 #
Total comments: 5
Patch Set 2 : review comments and rebase #
Total comments: 2
Messages
Total messages: 24 (9 generated)
mlamouri@chromium.org changed reviewers: + avayvod@chromium.org, jwd@chromium.org
jwd@, PTAL at histograms changes. avayvod@, PTAL at HTMLMediaElement changes.
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/2091253002/1
mlamouri@chromium.org changed reviewers: + holte@chromium.org - jwd@chromium.org
-jwd@ (he is OOO) +holte@ for histograms
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
I mainly don't like duplicating the autoplay muted conditions, left some nits with suggestions. https://codereview.chromium.org/2091253002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2091253002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2317: bool wasLocked = isGestureNeededForPlayback(); nit: I think you either want: bool wasLocked = m_lockedPendingUserGesture; or bool wasGestureNeededForPlayback = isGestureNeededForPlayback(); https://codereview.chromium.org/2091253002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2322: if (wasLocked && !muted && isHTMLVideoElement() && RuntimeEnabledFeatures::autoplayMutedVideosEnabled()) this condition seems to be very hard to read and maintain. Should've you checked for data saver as well? https://codereview.chromium.org/2091253002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2326: if (!muted && isGestureNeededForPlayback()) // this implicitly relies on m_muted set to false beforehand, so I don't like it much // bool wasGestureNeededForPlayback = isGestureNeededForPlayback(); // this needs to be called at the top but at least the dependency is explicit bool wasAutoplayingMuted = !paused() && m_muted && m_lockedPendingUserGesture; // do other stuff, unlock the gesture if processing it // ... if (wasAutoplayingMuted) { if (isGestureNeededForPlayback()) pause(); recordAutoplayUnmutePausedState(paused()); } wdyt? https://codereview.chromium.org/2091253002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/2091253002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.h:301: enum class AutoplayUnmuteStatus { Failure, Success }; nit: do we really need two enums for this? this one could be a bool with the method named recordAutoplayUnmutePausedState(bool paused) ? Then the recordXX method would look like: autoplayUnmuteHistogram.count(paused ? AutoplayUnmuteActionFailure : AutoplayUnmuteActionSuccess);
Anton, PTAL. I've applied your comments. I need to double-check that using isLockedPendingUserGesture() is fine but it should be. https://codereview.chromium.org/2091253002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/2091253002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.h:301: enum class AutoplayUnmuteStatus { Failure, Success }; On 2016/06/28 at 10:20:59, whywhat wrote: > nit: do we really need two enums for this? this one could be a bool with the method named recordAutoplayUnmutePausedState(bool paused) ? > > Then the recordXX method would look like: > > autoplayUnmuteHistogram.count(paused ? AutoplayUnmuteActionFailure : AutoplayUnmuteActionSuccess); I moved to one enum in the header used for histograms too.
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm https://codereview.chromium.org/2091253002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2091253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2298: if (UserGestureIndicator::processingUserGesture()) nit: you could avoid moving that if calculating wasAutoplayingMuted above it. Not sure though if it can happen that muted is changed with user gesture to the same value and whether we should unlock the element in that case :) So perhaps this move is okay.
The CQ bit was checked by mlamouri@chromium.org
https://codereview.chromium.org/2091253002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2091253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2298: if (UserGestureIndicator::processingUserGesture()) On 2016/06/29 at 19:44:18, whywhat wrote: > nit: you could avoid moving that if calculating wasAutoplayingMuted above it. Not sure though if it can happen that muted is changed with user gesture to the same value and whether we should unlock the element in that case :) So perhaps this move is okay. ACK :)
The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org Link to the patchset: https://codereview.chromium.org/2091253002/#ps20001 (title: "review comments and rebase")
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.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== When autoplaying muted video, record when unmute happen and the result. Unmuting might fail if the element was not unlocked. This is record the unmute action only if the element is muted and was playing because of the autoplay muted video feature. It will be recorded once per element. BUG=622711 ========== to ========== When autoplaying muted video, record when unmute happen and the result. Unmuting might fail if the element was not unlocked. This is record the unmute action only if the element is muted and was playing because of the autoplay muted video feature. It will be recorded once per element. BUG=622711 Committed: https://crrev.com/d26c8d5d38d8613d61dfe02af9451275868ecb98 Cr-Commit-Position: refs/heads/master@{#402937} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d26c8d5d38d8613d61dfe02af9451275868ecb98 Cr-Commit-Position: refs/heads/master@{#402937} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
