Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2108403003:
Measure whether muted videos that started playing with play() become visible at some point (Closed)
|
Created:
4 years, 5 months ago by Zhiqiang Zhang (Slow) Modified:
4 years, 5 months ago CC:
asvitkine+watch_chromium.org, blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMeasure whether muted videos that started playing with play() become visible at some point
BUG=622720
Committed: https://crrev.com/9d961d0a083fe4ebd8c200c31b87d75e4727958b
Cr-Commit-Position: refs/heads/master@{#404710}
Patch Set 1 : NOT READY #Patch Set 2 : this one is ready #
Total comments: 7
Patch Set 3 : addressed mlamouri's comments #
Total comments: 14
Patch Set 4 : using EventListener #
Total comments: 22
Patch Set 5 : addressed comments #
Total comments: 6
Patch Set 6 : addressed comments #Patch Set 7 : rebasing #Patch Set 8 : fixed layout tests #
Total comments: 1
Patch Set 9 : clean up in a unload event listener #Messages
Total messages: 39 (14 generated)
Patchset #1 (id:1) has been deleted
zqzhang@chromium.org changed reviewers: + mlamouri@chromium.org
PTAL. Do I need to put other autoplay UMAs into the helper class in this CL?
Can you move recordAutoplaySourceMetric to the helper? I think it will be clearer :) Also, can you add you and me as OWNERS of the file? https://codereview.chromium.org/2108403003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2108403003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:18: } style: no indentation https://codereview.chromium.org/2108403003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:41: void AutoplayUmaHelper::onElementDestroyed() The issue with recording UMA in a dtor is that it might not work very well. I would make sure the uma description say that. https://codereview.chromium.org/2108403003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:44: recordMutedVideoOfSourceMethodFinallyVisibleUma(false); Maybe you could have a `m_shouldRecordVisibilityChange` boolean to keep track of this state? https://codereview.chromium.org/2108403003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2108403003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22539: +<histogram name="Media.Video.Autoplay.Muted.SourceMethod.FinallyVisible" I would rename "SourceMethod" to "PlayMethod" and maybe "FinallyVisible" to "BecomesVisible" (but not sure about that one).
PTAL. Addressed mlamouri's comments. +asvitkine@ for reviewing histograms.xml +haraken@ for reviewing OWNERS and core.gypi A question for the UMA: I'm currently recording a true count when Video becomes visible and false count when it is destructed. Since HTMLMediaElement is garbage-collected, the false count will not be recorded sometimes. Currently I'm subtracting the true count from a total count in another histogram. Is it OK? Or maybe we could listen to page destruction events and record the false count (which seems to require some plumbing)? https://codereview.chromium.org/2108403003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2108403003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:18: } On 2016/07/01 11:00:07, Mounir Lamouri wrote: > style: no indentation Done. https://codereview.chromium.org/2108403003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:41: void AutoplayUmaHelper::onElementDestroyed() On 2016/07/01 11:00:07, Mounir Lamouri wrote: > The issue with recording UMA in a dtor is that it might not work very well. I > would make sure the uma description say that. Done. https://codereview.chromium.org/2108403003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:44: recordMutedVideoOfSourceMethodFinallyVisibleUma(false); On 2016/07/01 11:00:07, Mounir Lamouri wrote: > Maybe you could have a `m_shouldRecordVisibilityChange` boolean to keep track of > this state? The logic here was wrong. I'm using a ElementVisibilityObserver to keep track of videos autoplaying muted by play() method. Doing a null check on the observer should be enough.
zqzhang@chromium.org changed reviewers: + haraken@chromium.org, jwd@chromium.org
PTAL. Addressed mlamouri's comments. +jwd@ for reviewing histograms.xml +haraken@ for reviewing OWNERS and core.gypi A question for the UMA: I'm currently recording a true count when Video becomes visible and false count when it is destructed. Since HTMLMediaElement is garbage-collected, the false count will not be recorded sometimes. Currently I'm subtracting the true count from a total count in another histogram. Is it OK? Or maybe we could listen to page destruction events and record the false count (which seems to require some plumbing)?
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/2108403003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.h (right): https://codereview.chromium.org/2108403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:23: enum AutoplayUnmuteActionStatus { nit: `enum class` https://codereview.chromium.org/2108403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:33: style: remove empty line https://codereview.chromium.org/2108403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:35: style: ditto https://codereview.chromium.org/2108403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:39: } nit: can you move the definition to the cpp ? https://codereview.chromium.org/2108403003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2108403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1623: m_autoplayUmaHelper->onAutoplayInitiated(AutoplaySource::Attribute); I wonder if we shouldn't move this check to the preload place. Technically, this is where we decide whether autoplay will be allowed. Though, it's something I've been looking into for that UMA CL I have pending so feel free to do nothing about it now :) https://codereview.chromium.org/2108403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2129: m_autoplayUmaHelper->onAutoplayStarted(); Maybe this should be called onPlaybackStart{,ed,ing}, it might not be at all autoplay related. There are also other places where playback could start. Crazy idea: can we listen to the playing event? ;) (more seriously, if you create a PlayingEventLister in autoplayUmaHelper and call addEventListerer(EventTypeNames::playing, playingEventLister, false), we might be able to have something very neat :) https://codereview.chromium.org/2108403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3894: } If we are here, the source is unlikely play() so it would be a no-op, right?
zqzhang@chromium.org changed reviewers: + rkaplow@chromium.org
PTAL, addressed mlamouri's comments (incl. using EventListener). +rkaplow may also take a look at histograms.xml https://codereview.chromium.org/2108403003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.h (right): https://codereview.chromium.org/2108403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:23: enum AutoplayUnmuteActionStatus { On 2016/07/04 15:18:19, Mounir Lamouri wrote: > nit: `enum class` Done. https://codereview.chromium.org/2108403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:33: On 2016/07/04 15:18:19, Mounir Lamouri wrote: > style: remove empty line Done. https://codereview.chromium.org/2108403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:35: On 2016/07/04 15:18:19, Mounir Lamouri wrote: > style: ditto Done. https://codereview.chromium.org/2108403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:39: } On 2016/07/04 15:18:19, Mounir Lamouri wrote: > nit: can you move the definition to the cpp ? Done. https://codereview.chromium.org/2108403003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2108403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1623: m_autoplayUmaHelper->onAutoplayInitiated(AutoplaySource::Attribute); On 2016/07/04 15:18:19, Mounir Lamouri wrote: > I wonder if we shouldn't move this check to the preload place. Technically, this > is where we decide whether autoplay will be allowed. Though, it's something I've > been looking into for that UMA CL I have pending so feel free to do nothing > about it now :) Acknowledged. https://codereview.chromium.org/2108403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2129: m_autoplayUmaHelper->onAutoplayStarted(); On 2016/07/04 15:18:19, Mounir Lamouri wrote: > Maybe this should be called onPlaybackStart{,ed,ing}, it might not be at all > autoplay related. There are also other places where playback could start. > > Crazy idea: can we listen to the playing event? ;) (more seriously, if you > create a PlayingEventLister in autoplayUmaHelper and call > addEventListerer(EventTypeNames::playing, playingEventLister, false), we might > be able to have something very neat :) Using EventListener now :) https://codereview.chromium.org/2108403003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3894: } On 2016/07/04 15:18:19, Mounir Lamouri wrote: > If we are here, the source is unlikely play() so it would be a no-op, right? Since we are using EventListener, there's no problem now. Removed this line.
lgtm
https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:57: m_element->addEventListener(EventTypeNames::playing, this, false); Nit: Add indent. https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:57: m_element->addEventListener(EventTypeNames::playing, this, false); Wouldn't this cause a flood of events? (i.e., how frequently will the event listeners get fired?)
https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:57: m_element->addEventListener(EventTypeNames::playing, this, false); On 2016/07/07 at 01:16:36, haraken wrote: > Wouldn't this cause a flood of events? (i.e., how frequently will the event listeners get fired?) The event is fired when the media element starts playback. For example, after `play()` is called, `playing` event will fire when the loading is done. If the users pauses then resume, the event will be fired again. I don't think it would be too much.
Thanks for using the event listener, it's quite nicer! :) I left a few comments below. https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:70: recordAutoplaySourceUma(m_element, m_source); That might be a silly question but why not move the content of the anonymous method in there? https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:71: } Also, would it make sense to do the addEventListener() here if m_source is method so we only listen when needed? https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:84: blink::recordAutoplayUnmuteStatus(status); That might be a silly question but why not move the content of the anonymous method in there? https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:99: if (event->type() == EventTypeNames::playing) What about: ``` switch (event->type()) { case EventTypeNames::playing: handlePlayingEvent(); break; default: ASSERT_NOT_REACHED(); } ``` https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:110: } Would it make sense to removeEventListener()? https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.h (right): https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:55: void handlePlayEvent(); handlePlayingEvent. There is an event named 'play' :) https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:59: WeakMember<HTMLMediaElement> m_element; nit: add comment saying that m_element owns |this|. https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:60: Member<ElementVisibilityObserver> m_videoMutedPlayMethodVisibilityObserver; nit: add comment? https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:47: enum class AutoplaySource; I don't think you need this
On 2016/07/07 09:23:40, Mounir Lamouri wrote: > https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): > > https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:57: > m_element->addEventListener(EventTypeNames::playing, this, false); > On 2016/07/07 at 01:16:36, haraken wrote: > > Wouldn't this cause a flood of events? (i.e., how frequently will the event > listeners get fired?) > > The event is fired when the media element starts playback. For example, after > `play()` is called, `playing` event will fire when the loading is done. If the > users pauses then resume, the event will be fired again. I don't think it would > be too much. Thanks, makes sense.
Patchset #5 (id:140001) has been deleted
PTAL. Addressed comments /w replies. https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:57: m_element->addEventListener(EventTypeNames::playing, this, false); On 2016/07/07 01:16:36, haraken wrote: > > Nit: Add indent. Done adding indent. https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:70: recordAutoplaySourceUma(m_element, m_source); On 2016/07/07 10:17:52, Mounir Lamouri wrote: > That might be a silly question but why not move the content of the anonymous > method in there? Done. I thought it would help if we record the same from different call sites. Since there's only one call site, it's good to move the body here :) https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:71: } On 2016/07/07 10:17:52, Mounir Lamouri wrote: > Also, would it make sense to do the addEventListener() here if m_source is > method so we only listen when needed? Done. https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:84: blink::recordAutoplayUnmuteStatus(status); On 2016/07/07 10:17:52, Mounir Lamouri wrote: > That might be a silly question but why not move the content of the anonymous > method in there? Done. https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:99: if (event->type() == EventTypeNames::playing) On 2016/07/07 10:17:52, Mounir Lamouri wrote: > What about: > ``` > switch (event->type()) { > case EventTypeNames::playing: > handlePlayingEvent(); > break; > default: > ASSERT_NOT_REACHED(); > } > ``` Actually event->type() is AtomicString XD https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:110: } On 2016/07/07 10:17:53, Mounir Lamouri wrote: > Would it make sense to removeEventListener()? Done. https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.h (right): https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:55: void handlePlayEvent(); On 2016/07/07 10:17:53, Mounir Lamouri wrote: > handlePlayingEvent. There is an event named 'play' :) Done. https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:59: WeakMember<HTMLMediaElement> m_element; On 2016/07/07 10:17:53, Mounir Lamouri wrote: > nit: add comment saying that m_element owns |this|. Done. https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:60: Member<ElementVisibilityObserver> m_videoMutedPlayMethodVisibilityObserver; On 2016/07/07 10:17:53, Mounir Lamouri wrote: > nit: add comment? Done. https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/2108403003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:47: enum class AutoplaySource; On 2016/07/07 10:17:53, Mounir Lamouri wrote: > I don't think you need this Done.
lgtm Looks great. Thanks! :) https://codereview.chromium.org/2108403003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2108403003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:91: handlePlayingEvent(); What about: ``` DCHECK(event->type() == EventTypeNames::playing); handlePlayingEvent(); ``` https://codereview.chromium.org/2108403003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.h (right): https://codereview.chromium.org/2108403003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:28: }; BTW, I think you can fwd declare these enums here and define them in the cpp file.
https://codereview.chromium.org/2108403003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2108403003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:59: m_element->addEventListener(EventTypeNames::playing, this, false); Shouldn't you only add the event listener if source is the method?
https://codereview.chromium.org/2108403003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2108403003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:59: m_element->addEventListener(EventTypeNames::playing, this, false); On 2016/07/08 17:58:29, Mounir Lamouri wrote: > Shouldn't you only add the event listener if source is the method? Oh yes! Nice catch. Fixed. https://codereview.chromium.org/2108403003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:91: handlePlayingEvent(); On 2016/07/07 12:45:47, Mounir Lamouri wrote: > What about: > ``` > DCHECK(event->type() == EventTypeNames::playing); > handlePlayingEvent(); > ``` Done. https://codereview.chromium.org/2108403003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.h (right): https://codereview.chromium.org/2108403003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:28: }; On 2016/07/07 12:45:47, Mounir Lamouri wrote: > BTW, I think you can fwd declare these enums here and define them in the cpp > file. These enums are referenced in HTMLMediaElement, so I think not.
Friendly ping to haraken@! Do you have any other concern about this CL? It's blocking some other UMA CLs which we want to land soon.
Sorry, I was not thinking that I'm blocking this CL. LGTM.
On 2016/07/11 13:29:07, haraken wrote: > Sorry, I was not thinking that I'm blocking this CL. LGTM. Thanks :)
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, mlamouri@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2108403003/#ps200001 (title: "rebasing")
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, rkaplow@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2108403003/#ps220001 (title: "fixed layout tests")
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 #8 (id:220001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Measure whether muted videos that started playing with play() become visible at some point BUG=622720 ========== to ========== Measure whether muted videos that started playing with play() become visible at some point BUG=622720 Committed: https://crrev.com/9d961d0a083fe4ebd8c200c31b87d75e4727958b Cr-Commit-Position: refs/heads/master@{#404710} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9d961d0a083fe4ebd8c200c31b87d75e4727958b Cr-Commit-Position: refs/heads/master@{#404710}
Message was sent while issue was closed.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2108403003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2108403003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:102: m_element->removeEventListener(EventTypeNames::playing, this, false); maybe removeEventListener() needs to be called even if the media element isn't played to prevent leaking an ActiveDOMObject(?) |
