Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2101613002:
Record the offscreen playing duration of autoplaying muted videos (Closed)
|
Created:
4 years, 5 months ago by Zhiqiang Zhang (Slow) Modified:
4 years, 4 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. |
DescriptionRecord the offscreen playing duration of autoplaying muted videos
BUG=617681
Committed: https://crrev.com/cc74c3a6422be9a25c635021a57f7f0950c0381c
Cr-Commit-Position: refs/heads/master@{#413522}
Patch Set 1 #
Total comments: 4
Patch Set 2 : refactoring #Patch Set 3 : rebased and minor fixes #Patch Set 4 : fixed comments #
Total comments: 10
Patch Set 5 : rebased and addressed Mounir's comments #
Total comments: 13
Patch Set 6 : addressed more comments from Mounir #
Total comments: 11
Patch Set 7 : addressed more comments #
Total comments: 5
Patch Set 8 : addressed isherman's comments #Patch Set 9 : rebased #Patch Set 10 : added some nullchecks #Patch Set 11 : fix layout test expectation #
Messages
Total messages: 46 (20 generated)
zqzhang@chromium.org changed reviewers: + mlamouri@chromium.org
The CL is still rough, but I think we should take a initial round of review to see if I'm on the right track :) BTW, I'm worried about whether I'm handling all the MediaElement state changes correctly, especially when the element is buffering, looping, calling play() method during autoplay, destroyed, etc.
I think having a different class would help indeed. I wonder if you need to add all these m_autoplayFromAttribute assignments. We know when play() autoplays and when the autoplay attribute kicks in so we could only have two places. If we have a helper, it could be a method like `startAutoplay(AutoplaySource)`. I would recommend to do the metric that checks if play() is called for muted videos on off viewports videos first. It might be simpler and more self-contained than this one :) https://codereview.chromium.org/2101613002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2101613002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:22373: + <owner>zqzhang@chromium.org</owner> Can you do alphabetic ordering? :) https://codereview.chromium.org/2101613002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:22384: + <owner>zqzhang@chromium.org</owner> ditto
PTAL! Done lots of refactoring :) https://codereview.chromium.org/2101613002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2101613002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:22373: + <owner>zqzhang@chromium.org</owner> On 2016/06/30 11:14:18, Mounir Lamouri wrote: > Can you do alphabetic ordering? :) Done. https://codereview.chromium.org/2101613002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:22384: + <owner>zqzhang@chromium.org</owner> On 2016/06/30 11:14:18, Mounir Lamouri wrote: > ditto Done.
I'm leaving this on the side because IIRC it's depending on a CL that hasn't landed yet. Can you ping me when the dependencies are cleared?
Description was changed from ========== Record the offscreen playing duration of autoplaying muted videos BUG=617681 ========== to ========== Record the offscreen playing duration of autoplaying muted videos BUG=617681 ==========
Patchset #4 (id:60001) has been deleted
PTAL! Rebased onto the AutoplayUmaHelper patch. Did some refactoring on AutoplayUmaHelper. Hope it looks cleaner.
Thanks for the CL! See comments below. https://codereview.chromium.org/2101613002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2101613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:169: m_mutedVideoOffscreenDurationVisibilityObserver = new ElementVisibilityObserver(m_element, WTF::bind(&AutoplayUmaHelper::onVisibilityChangedForMutedVideoOffscreenDuration, wrapPersistent(this))); wrapWeakPersistent? https://codereview.chromium.org/2101613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:180: double timeDeltaMS = m_isVisible ? m_mutedVideoAutoplayOffscreenDurationMS : maxUmaDurationMS; I'm not sure I fully understand this logic. If we call this when the video is not visible, we would put max regardless. Why? https://codereview.chromium.org/2101613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:184: timeDeltaMS = maxUmaDurationMS; I think you can use clamp(): https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/MathExtras... (Coming to C++17 \o/) https://codereview.chromium.org/2101613002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.h (right): https://codereview.chromium.org/2101613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:87: double m_mutedVideoAutoplayStartTimeMS; Should this be specific to not visible playback? https://codereview.chromium.org/2101613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:92: // The observer is used to whether a autoplaying video autoplaying changes it's visibility, nit: "used to whether" -- a word is missing maybe?
Patchset #5 (id:100001) has been deleted
PTAL. Rebased and addressed Mounir's comments. https://codereview.chromium.org/2101613002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2101613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:169: m_mutedVideoOffscreenDurationVisibilityObserver = new ElementVisibilityObserver(m_element, WTF::bind(&AutoplayUmaHelper::onVisibilityChangedForMutedVideoOffscreenDuration, wrapPersistent(this))); On 2016/08/17 19:27:15, Mounir Lamouri wrote: > wrapWeakPersistent? Done. https://codereview.chromium.org/2101613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:180: double timeDeltaMS = m_isVisible ? m_mutedVideoAutoplayOffscreenDurationMS : maxUmaDurationMS; On 2016/08/17 19:27:15, Mounir Lamouri wrote: > I'm not sure I fully understand this logic. If we call this when the video is > not visible, we would put max regardless. Why? OK, not clamping the values now. Just found that histograms have buckets for underflow and overflow :) https://codereview.chromium.org/2101613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:184: timeDeltaMS = maxUmaDurationMS; On 2016/08/17 19:27:15, Mounir Lamouri wrote: > I think you can use clamp(): > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/MathExtras... > > (Coming to C++17 \o/) Not clamping the values as said in the other reply https://codereview.chromium.org/2101613002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.h (right): https://codereview.chromium.org/2101613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:87: double m_mutedVideoAutoplayStartTimeMS; On 2016/08/17 19:27:15, Mounir Lamouri wrote: > Should this be specific to not visible playback? Done. https://codereview.chromium.org/2101613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:92: // The observer is used to whether a autoplaying video autoplaying changes it's visibility, On 2016/08/17 19:27:15, Mounir Lamouri wrote: > nit: "used to whether" -- a word is missing maybe? Done. https://codereview.chromium.org/2101613002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2101613002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:132: m_mutedVideoAutoplayOffscreenDurationMS += monotonicallyIncreasingTimeMS() - m_mutedVideoAutoplayOffscreenStartTimeMS; This is new. Accumulating the offscreen time when pause() is called when video is offscreen. BTW, I'm not sure if pause event is fired when the frame is unloaded, in that case, we are still recording a finite value. Is this OK?
On 2016/08/18 at 10:52:43, zqzhang wrote: > BTW, I'm not sure if pause event is fired when the frame is unloaded, in that case, we are still recording a finite value. Is this OK? I don't think the pause event is fired when the page is unloaded.
https://codereview.chromium.org/2101613002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2101613002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:20: static const double umaBucketCount = 20; nit: no need for static if in anonymous namespace. You can also move this inside the function given that it's only used there. Might be easier for readability. https://codereview.chromium.org/2101613002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:132: m_mutedVideoAutoplayOffscreenDurationMS += monotonicallyIncreasingTimeMS() - m_mutedVideoAutoplayOffscreenStartTimeMS; On 2016/08/18 at 10:52:43, Zhiqiang Zhang wrote: > This is new. Accumulating the offscreen time when pause() is called when video is offscreen. > > BTW, I'm not sure if pause event is fired when the frame is unloaded, in that case, we are still recording a finite value. Is this OK? Could you increment on unload if !m_IsVisible? Maybe you would need to reset m_mutedVideoAutoplayOffscreenStartTimeMS to 0 here and check the value in the unload event? https://codereview.chromium.org/2101613002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:140: maybeStopRecordingMutedVideoOffscreenDuration(true); Instead of passing isInfinite=true here, could you compute how long it was offscreen? We have m_isVisible, that could help, right? https://codereview.chromium.org/2101613002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:157: if (!m_mutedVideoPlayMethodVisibilityObserver) nit: to make sure the metric is properly recorded, maybe you could wrap the observer stop around this check but not the rest of the method? https://codereview.chromium.org/2101613002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:183: return; ditto, see above https://codereview.chromium.org/2101613002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:185: double timeDeltaMS = isInfinite ? std::numeric_limits<int32_t>::max() : m_mutedVideoAutoplayOffscreenDurationMS; I'm really confused in how you use this. m_mutedVideoAutoplay... is a double. Can you save state in there? It can be set to max/infinity, etc.
zqzhang@chromium.org changed reviewers: + isherman@chromium.org
PTAL. Applied Mounir's comments. +isherman@ for histograms.xml https://codereview.chromium.org/2101613002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2101613002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:20: static const double umaBucketCount = 20; On 2016/08/18 14:15:14, Mounir Lamouri wrote: > nit: no need for static if in anonymous namespace. You can also move this inside > the function given that it's only used there. Might be easier for readability. Done. https://codereview.chromium.org/2101613002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:132: m_mutedVideoAutoplayOffscreenDurationMS += monotonicallyIncreasingTimeMS() - m_mutedVideoAutoplayOffscreenStartTimeMS; On 2016/08/18 14:15:14, Mounir Lamouri wrote: > On 2016/08/18 at 10:52:43, Zhiqiang Zhang wrote: > > This is new. Accumulating the offscreen time when pause() is called when video > is offscreen. > > > > BTW, I'm not sure if pause event is fired when the frame is unloaded, in that > case, we are still recording a finite value. Is this OK? > > Could you increment on unload if !m_IsVisible? Maybe you would need to reset > m_mutedVideoAutoplayOffscreenStartTimeMS to 0 here and check the value in the > unload event? Done, moved the final incrementing logic in maybeStopRecording...() https://codereview.chromium.org/2101613002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:140: maybeStopRecordingMutedVideoOffscreenDuration(true); On 2016/08/18 14:15:14, Mounir Lamouri wrote: > Instead of passing isInfinite=true here, could you compute how long it was > offscreen? We have m_isVisible, that could help, right? Done, moved the final incrementing logic in maybeStopRecording...() https://codereview.chromium.org/2101613002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:157: if (!m_mutedVideoPlayMethodVisibilityObserver) On 2016/08/18 14:15:14, Mounir Lamouri wrote: > nit: to make sure the metric is properly recorded, maybe you could wrap the > observer stop around this check but not the rest of the method? Won't fix as we discussed offline. https://codereview.chromium.org/2101613002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:183: return; On 2016/08/18 14:15:14, Mounir Lamouri wrote: > ditto, see above ditto. https://codereview.chromium.org/2101613002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:185: double timeDeltaMS = isInfinite ? std::numeric_limits<int32_t>::max() : m_mutedVideoAutoplayOffscreenDurationMS; On 2016/08/18 14:15:14, Mounir Lamouri wrote: > I'm really confused in how you use this. m_mutedVideoAutoplay... is a double. > Can you save state in there? It can be set to max/infinity, etc. Only using m_mutedVideoAutoplayOffscreenDurationMS now, removed infinity.
https://codereview.chromium.org/2101613002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2101613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:20: const double umaBucketCount = 20; Hmm, why are these variables of type double rather than int? https://codereview.chromium.org/2101613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:20: const double umaBucketCount = 20; How did you choose this bucket count? If 20 buckets is truly sufficient, then that's great! I just want to make sure that you thought about the granularity needed, and chose the bucket count accordingly. I think it's probably worth documenting this variable, and likely also giving it a slightly more specific name, to clarify which histograms it's intended to be used with.
lgtm with isherman's approval. https://codereview.chromium.org/2101613002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2101613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:20: const double umaBucketCount = 20; On 2016/08/18 at 19:45:03, Ilya Sherman wrote: > Hmm, why are these variables of type double rather than int? Maybe reuse https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?rcl=1471... ? I wonder if it would make sense to have something like that in platform/Histogram.h https://codereview.chromium.org/2101613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:152: DEFINE_STATIC_LOCAL(BooleanHistogram, histogram, ("Media.Video.Autoplay.Muted.PlayMethod.BecomesVisible")); nit: move after the early return? https://codereview.chromium.org/2101613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:177: void AutoplayUmaHelper::maybeStopRecordingMutedVideoOffscreenDuration(bool isInfinite) I don't think you still need `isInfinite` here, do you? https://codereview.chromium.org/2101613002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.h (right): https://codereview.chromium.org/2101613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:92: // The observer is used to observer an autoplaying muted video changing it's visibility, style: I think it would be better to leave an empty line between members.
Patchset #7 (id:160001) has been deleted
PTAL. Addressed comments from Ilya and Mounir. https://codereview.chromium.org/2101613002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2101613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:20: const double umaBucketCount = 20; On 2016/08/18 19:45:03, Ilya Sherman wrote: > Hmm, why are these variables of type double rather than int? Sorry, using int32_t now. https://codereview.chromium.org/2101613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:20: const double umaBucketCount = 20; On 2016/08/18 19:45:03, Ilya Sherman wrote: > How did you choose this bucket count? If 20 buckets is truly sufficient, then > that's great! I just want to make sure that you thought about the granularity > needed, and chose the bucket count accordingly. > > I think it's probably worth documenting this variable, and likely also giving it > a slightly more specific name, to clarify which histograms it's intended to be > used with. Using 50 as Mounir suggested, which is used by many duration-related UMAs. Also using 1 hour as the maximum time. https://codereview.chromium.org/2101613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:152: DEFINE_STATIC_LOCAL(BooleanHistogram, histogram, ("Media.Video.Autoplay.Muted.PlayMethod.BecomesVisible")); On 2016/08/19 10:00:08, Mounir Lamouri wrote: > nit: move after the early return? Done. https://codereview.chromium.org/2101613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:177: void AutoplayUmaHelper::maybeStopRecordingMutedVideoOffscreenDuration(bool isInfinite) On 2016/08/19 10:00:08, Mounir Lamouri wrote: > I don't think you still need `isInfinite` here, do you? Done. https://codereview.chromium.org/2101613002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.h (right): https://codereview.chromium.org/2101613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.h:92: // The observer is used to observer an autoplaying muted video changing it's visibility, On 2016/08/19 10:00:08, Mounir Lamouri wrote: > style: I think it would be better to leave an empty line between members. Done. https://codereview.chromium.org/2101613002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2101613002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:185: // Since histograms uses int32_t, the duration needs to be limited to std::numeric_limits<int32_t>::max(). Now the time is stored as int64_t inside AutoplayUmaHelper, but it needs to be bounded to int32_t when recording the UMA.
metrics lgtm % nits: https://codereview.chromium.org/2101613002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2101613002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:19: const int32_t maxOffscreenDurationUmaMS = 3600000; nit: This constant value is a bit hard to parse -- I'd write it as 60 * 60 * 1000; https://codereview.chromium.org/2101613002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:186: int32_t boundedTime = static_cast<int32_t>(std::min<int64_t>(m_mutedVideoAutoplayOffscreenDurationMS, std::numeric_limits<int32_t>::max())); Can you use base::saturated_cast?
https://codereview.chromium.org/2101613002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right): https://codereview.chromium.org/2101613002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:19: const int32_t maxOffscreenDurationUmaMS = 3600000; On 2016/08/19 23:15:33, Ilya Sherman wrote: > nit: This constant value is a bit hard to parse -- I'd write it as 60 * 60 * > 1000; Done. https://codereview.chromium.org/2101613002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:186: int32_t boundedTime = static_cast<int32_t>(std::min<int64_t>(m_mutedVideoAutoplayOffscreenDurationMS, std::numeric_limits<int32_t>::max())); On 2016/08/19 23:15:33, Ilya Sherman wrote: > Can you use base::saturated_cast? I guess not. Using base:: is not allowed in WebKit/Source/core :(
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2101613002/#ps200001 (title: "addressed isherman's comments")
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2101613002/#ps240001 (title: "added some nullchecks")
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 zqzhang@chromium.org
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2101613002/#ps260001 (title: "fix layout test expectation")
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zqzhang@chromium.org
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by zqzhang@chromium.org
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.
Description was changed from ========== Record the offscreen playing duration of autoplaying muted videos BUG=617681 ========== to ========== Record the offscreen playing duration of autoplaying muted videos BUG=617681 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Record the offscreen playing duration of autoplaying muted videos BUG=617681 ========== to ========== Record the offscreen playing duration of autoplaying muted videos BUG=617681 Committed: https://crrev.com/cc74c3a6422be9a25c635021a57f7f0950c0381c Cr-Commit-Position: refs/heads/master@{#413522} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/cc74c3a6422be9a25c635021a57f7f0950c0381c Cr-Commit-Position: refs/heads/master@{#413522}
Message was sent while issue was closed.
On 2016/08/22 20:32:34, commit-bot: I haz the power wrote: > Patchset 11 (id:??) landed as > https://crrev.com/cc74c3a6422be9a25c635021a57f7f0950c0381c > Cr-Commit-Position: refs/heads/master@{#413522} A manual revert has been created here: https://chromiumcodereview.appspot.com/2266253002/ ("Revert patchset" does not work due to histogram.xml being too large) Reason: Tests failing on https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak: media/autoplay-muted.html media/autoplay-unmute-offscreen.html media/autoplay-when-visible.html 15:30:48.155 15954 worker/3 media/autoplay-unmute-offscreen.html leaked 15:30:48.155 15954 Xlib: extension "RANDR" missing on display ":9". 15:30:48.155 15954 Xlib: extension "RANDR" missing on display ":9". 15:30:48.155 15954 [4:4:0822/153047:2028208541:WARNING:webmediaplayer_impl.cc(372)] Using MultibufferDataSource 15:30:48.155 15954 [4:4:0822/153047:2028209045:WARNING:webmediaplayer_impl.cc(372)] Using MultibufferDataSource 15:30:48.158 18115 [35/36] media/autoplay-unmute-offscreen.html failed unexpectedly (leak detected: ({"numberOfLiveActiveDOMObjects":[2,3]})) 15:30:48.157 15954 worker/3 media/autoplay-unmute-offscreen.html failed: 15:30:48.157 15954 worker/3 leak detected: ({"numberOfLiveActiveDOMObjects":[2,3]}) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
