Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1470153004:
Autoplay experiment metric fixes and additions. (Closed)
|
Created:
5 years ago by liberato (no reviews please) Modified:
4 years, 8 months ago CC:
mlamouri (slow - plz ping), blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, oilpan-reviews, nessy, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAutoplay experiment metric fixes and additions.
This CL fixes the following issues in the autoplay experiment:
- "Any media playback" wasn't checking if the media was playing
already, so it over-counted quite a bit.
- "ifmuted" counting was broken, since the autoplay experiment
didn't call autoplayMediaEncountered in all cases. For mute,
this could actually allow the media to start playback without
preventing additional autoplay metrics for the next playback.
- made shouldAutoplay not call autoplayMediaEncountered, since
the experiment might not have overridden the gesture requirement
yet. This would generate incorrect metrics in SetReadyState.
- Adds a metric for "played to completion".
- Adds metrics for initial playback that capture whether any
user gesture was seen, and / or if one is active curently.
This will give us an idea about how often a gesture in
the inital load() call is responsible for a playback.
- Added counters for "total media elements seen" and "total video
elements seen".
BUG=561098
Committed: https://crrev.com/af2a826c3fa40d5e7e7895e860aa059ddab80b20
Cr-Commit-Position: refs/heads/master@{#383830}
Patch Set 1 #Patch Set 2 : added histograms.xml . #
Total comments: 28
Patch Set 3 : cl feedback. #
Total comments: 19
Patch Set 4 : refactored lots of autoplay metrics into AutoplayExperimentHelper #Patch Set 5 : cl feedback #
Total comments: 8
Patch Set 6 : refactored, unit tests #Patch Set 7 : EXPECT_EQ => EXPECT_TRUE/FALSE for some compilation issues. #Patch Set 8 : added CORE_EXPORT. #Patch Set 9 : removed unneeded const qualifier to try to make msvc happy. #Patch Set 10 : rebased. #
Total comments: 70
Patch Set 11 : cl feedback. #Patch Set 12 : removed reference for oilpan. #Patch Set 13 : yet more oilpan. #Patch Set 14 : yet more oilpan] #
Total comments: 2
Patch Set 15 : rawptrwillbeuntracedmember #Patch Set 16 : moved using directives in test. #Patch Set 17 : stopped using all of namespace testing. #Patch Set 18 : skipping using directive entirely like some other blink gmock tests.... #Patch Set 19 : back to using some of ::testing, since it works. #
Total comments: 28
Patch Set 20 : moved AutoplayExperimentHelper and client to the oilpan heap. #Patch Set 21 : OwnPtrWillBeMember => Persistent in the test. #Patch Set 22 : fixed trace. #
Total comments: 2
Patch Set 23 : moved RecordMetricsBehavior back into HTMLMediaElement #
Total comments: 9
Patch Set 24 : cl feedback. #Patch Set 25 : updated AutoplayClient::paused to replace media player state #Patch Set 26 : fixes for non-oilpan #Patch Set 27 : removed asymmetry between play and pause metrics #Patch Set 28 : more oilpan. #
Total comments: 3
Patch Set 29 : fix tests #Patch Set 30 : ...yet more test fixes for things that compile locally. #Patch Set 31 : comments. #Patch Set 32 : simplified paused logic #
Total comments: 1
Patch Set 33 : rebased. #Patch Set 34 : simplified ended() logic. #Patch Set 35 : ...because tests. #Dependent Patchsets: Messages
Total messages: 58 (14 generated)
Description was changed from ========== Autoplay experiment metric fixes and additions. This CL fixes the following issues in the autoplay experiment: - "Any media playback" wasn't checking if the media was playing already, so it over-counted quite a bit. - "ifmuted" counting was broken, since the autoplay experiment didn't call autoplayMediaEncountered in all cases. For mute, this could actually allow the media to start playback without preventing additional autoplay metrics for the next playback. - made shouldAutoplay not call autoplayMediaEncountered, since the experiment might not have overridden the gesture requirement yet. This would generate incorrect metrics in SetReadyState. - Adds a metric for "played to completion". - Adds metrics for initial playback that capture whether any user gesture was seen, and / or if one is active curently. This will give us an idea about how often a gesture in the inital load() call is responsible for a playback. - Added counters for "total media elements seen" and "total video elements seen". BUG=561098 ========== to ========== Autoplay experiment metric fixes and additions. This CL fixes the following issues in the autoplay experiment: - "Any media playback" wasn't checking if the media was playing already, so it over-counted quite a bit. - "ifmuted" counting was broken, since the autoplay experiment didn't call autoplayMediaEncountered in all cases. For mute, this could actually allow the media to start playback without preventing additional autoplay metrics for the next playback. - made shouldAutoplay not call autoplayMediaEncountered, since the experiment might not have overridden the gesture requirement yet. This would generate incorrect metrics in SetReadyState. - Adds a metric for "played to completion". - Adds metrics for initial playback that capture whether any user gesture was seen, and / or if one is active curently. This will give us an idea about how often a gesture in the inital load() call is responsible for a playback. - Added counters for "total media elements seen" and "total video elements seen". BUG=561098 ==========
liberato@chromium.org changed reviewers: + philipj@opera.com
more fun with autoplay! minor fixes, plus a few additional metrics coming from the first dry run. thanks -fl
Many questions :) https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h (right): https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:73: // User gesture was bypassed,and playback started, and media played to space before comma https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:74: // completion without a user-supplied pause. user-initiated maybe? https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:78: // element at or before. at or before... what? :) https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:365: recordAutoplayMetric(AnyMediaElement); It's likely that this and the AnyVideoElement will be poisoned entirely by feature detection, i.e. elements created just to call canPlayType() and similar. If you want an idea about media elements that could do something, I would instead add the metric before setNetworkState(NETWORK_LOADING) in HTMLMediaElement::loadResource, which is roughly the resource fetch algorithm in spec terms. That's before preload=none has any effect. With that change, you can also revert recordAutoplayMetric to being private, and it will probably make sense to measure audio+video than media+video. https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:675: void HTMLMediaElement::recordMetricsIfPausing() Maybe rename to recordMetricsBeforePause() if that was always the intended meaning? And then ASSERT(!m_paused) if that's actually true. https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:681: // Don't count seeking as a bailout. Why was this comment added? I see no changes related to seeking. https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:683: const bool ended = endedPlayback(); Have you tested that this is always true when recordMetricsIfPausing() is called from HTMLMediaElement::timeChanged()? HTMLMediaElement::endedPlayback() is rather convoluted, after all. I think this might all be clearer with a new recordMetricsAtPlaybackEnd() or similar, since it can't be a bailout and it doesn't really make sense to consider it a pause either. https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:713: bool gesture = UserGestureIndicator::processingUserGesture(); IIUC, this and a bunch of other changes are "will give us an idea about how often a gesture in the inital load() call is responsible for a playback." How about instead explicitly measuring this, by making all changes to m_userGestureRequiredForPlay go through removeUserGestureRequirement() and passing an enum for what it is that caused it? https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1549: recordAutoplayMetric(AnyPlaybackStarted); Rather than measuring the playing event, can you put this right after the webMediaPlayer()->play() call instead? It *should* be equivalent, but in https://github.com/whatwg/html/issues/309 we're considering changing when the playing event is fired, and also the other measurements are right after the play() call. https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1569: // Record that autoplay media was encountered, but wait until I don't quite understand this comment. Is it explaining why this code is here instead of just inside HTMLMediaElement::shouldAutoplay() before the `return true` statement? https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2022: recordAutoplayMetric(AnyPlaybackStarted); Can you put this right after the webMediaPlayer()->play() call instead? It *should* be equivalent, but in https://github.com/whatwg/html/issues/309 we're considering changing when the playing event is fired, and also the other measurements are right after the play() call. https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2036: // If no user gesture was required, then assume that playback will When you spell it out it's kind of weird. If you keep track of who cleared the user gesture requirement, can't this be counted at webMediaPlayer()->play() call site? If so that should also make it possible to call autoplayMediaEncountered() without worrying about whether the user gesture has been cleared yet, as is done above. https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2973: if (!m_initialPlaybackRecorded) { I believe this is also to "give us an idea about how often a gesture in the inital load() call is responsible for a playback." If you knew what it was that cleared the user gesture requirement to begin with, would you still have any use for these four metrics? In particular the During/NotDuring variants seem strange, since you can call pause() with a user gesture to clear the requirement and play much later while the gesture is no longer in effect, or even worse there could be another gesture in effect to confuse the data. https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:629: bool m_anyUserGestureEncountered; Can these be put on the AutoplayExperimentHelper instead?
thanks for the feedback. please notice the playInternal change, which should probably be a separate CL. -fl https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h (right): https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:73: // User gesture was bypassed,and playback started, and media played to On 2015/11/25 14:03:41, philipj wrote: > space before comma Done. https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:74: // completion without a user-supplied pause. On 2015/11/25 14:03:41, philipj wrote: > user-initiated maybe? Done. https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:78: // element at or before. On 2015/11/25 14:03:41, philipj wrote: > at or before... what? :) Done. https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:365: recordAutoplayMetric(AnyMediaElement); On 2015/11/25 14:03:42, philipj wrote: > It's likely that this and the AnyVideoElement will be poisoned entirely by > feature detection, i.e. elements created just to call canPlayType() and similar. > > If you want an idea about media elements that could do something, I would > instead add the metric before setNetworkState(NETWORK_LOADING) in > HTMLMediaElement::loadResource, which is roughly the resource fetch algorithm in > spec terms. That's before preload=none has any effect. > > With that change, you can also revert recordAutoplayMetric to being private, and > it will probably make sense to measure audio+video than media+video. very good point, thanks. https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:675: void HTMLMediaElement::recordMetricsIfPausing() On 2015/11/25 14:03:42, philipj wrote: > Maybe rename to recordMetricsBeforePause() if that was always the intended > meaning? And then ASSERT(!m_paused) if that's actually true. Done. https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:681: // Don't count seeking as a bailout. On 2015/11/25 14:03:42, philipj wrote: > Why was this comment added? I see no changes related to seeking. whoops, forgot to delete. https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:683: const bool ended = endedPlayback(); On 2015/11/25 14:03:42, philipj wrote: > Have you tested that this is always true when recordMetricsIfPausing() is called > from HTMLMediaElement::timeChanged()? HTMLMediaElement::endedPlayback() is > rather convoluted, after all. > > I think this might all be clearer with a new recordMetricsAtPlaybackEnd() or > similar, since it can't be a bailout and it doesn't really make sense to > consider it a pause either. it has worked in all of my tests. however, i rather like the idea of separating them and avoiding it. Done. https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:713: bool gesture = UserGestureIndicator::processingUserGesture(); On 2015/11/25 14:03:42, philipj wrote: > IIUC, this and a bunch of other changes are "will give us an idea about how > often a gesture in the inital load() call is responsible for a playback." > > How about instead explicitly measuring this, by making all changes to > m_userGestureRequiredForPlay go through removeUserGestureRequirement() and > passing an enum for what it is that caused it? i had done exactly that, but didn't like it. i'll try again. ... hrm, for some reason it looks much nicer the second time around. Done. Thanks. https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1549: recordAutoplayMetric(AnyPlaybackStarted); On 2015/11/25 14:03:41, philipj wrote: > Rather than measuring the playing event, can you put this right after the > webMediaPlayer()->play() call instead? It *should* be equivalent, but in > https://github.com/whatwg/html/issues/309 we're considering changing when the > playing event is fired, and also the other measurements are right after the > play() call. Done. https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1569: // Record that autoplay media was encountered, but wait until On 2015/11/25 14:03:41, philipj wrote: > I don't quite understand this comment. Is it explaining why this code is here > instead of just inside HTMLMediaElement::shouldAutoplay() before the `return > true` statement? yes. i'll update the comment to make it clearer. https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2022: recordAutoplayMetric(AnyPlaybackStarted); On 2015/11/25 14:03:42, philipj wrote: > Can you put this right after the webMediaPlayer()->play() call instead? It > *should* be equivalent, but in https://github.com/whatwg/html/issues/309 we're > considering changing when the playing event is fired, and also the other > measurements are right after the play() call. Done, and a third case as well. https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2036: // If no user gesture was required, then assume that playback will On 2015/11/25 14:03:42, philipj wrote: > When you spell it out it's kind of weird. If you keep track of who cleared the > user gesture requirement, can't this be counted at webMediaPlayer()->play() call > site? > > If so that should also make it possible to call autoplayMediaEncountered() > without worrying about whether the user gesture has been cleared yet, as is done > above. i'm not sure that it gets any better. wmp->play() might not be called (or might be called for some other reason later), and we'd then have to worry about whether it should record any metric at all. it really wants to record it only when we set m_autoplayMediaCounted here. https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2973: if (!m_initialPlaybackRecorded) { On 2015/11/25 14:03:42, philipj wrote: > I believe this is also to "give us an idea about how often a gesture in the > inital load() call is responsible for a playback." > > If you knew what it was that cleared the user gesture requirement to begin with, > would you still have any use for these four metrics? In particular the > During/NotDuring variants seem strange, since you can call pause() with a user > gesture to clear the requirement and play much later while the gesture is no > longer in effect, or even worse there could be another gesture in effect to > confuse the data. the load call is part of it. another part of the rationale behind these was to upper and lower bound the cases you describe. for example, the case with "no user gesture now or earlier" is almost certainly "autoplay" from the user's point of view. however, i'm removing these anyway. after further thought, it's unclear that it'll help. https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1470153004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:629: bool m_anyUserGestureEncountered; On 2015/11/25 14:03:42, philipj wrote: > Can these be put on the AutoplayExperimentHelper instead? Removed. https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2976: m_autoplaying = false; i moved this from playInternal(). not sure that this is per-spec (it only mentions removing autoplay on a play() call). otherwise, though, letting a video element with an autoplay attribute play to completion, then seeking to the middle, starts playback immediately when updating the network state. that seems really weird.
mlamouri@chromium.org changed reviewers: + mlamouri@chromium.org
Maybe you should tests the histograms to make sure the basic scenarios are actually working as expected?
https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h (right): https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:68: // Number of audio elements detected. Clarify that this is only elements that reached the resource fetch algorithm? https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:973: m_userGestureRequiredForPlay = false; Drop this line, it looks like it would make removeUserGestureRequirement() a no-op. https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1990: m_userGestureRequiredForPlay = false; Call recordAutoplayMetric(GesturelessPlaybackEnabledByPlay)? I don't think you should need an unknown case, we know exactly what removes the gesture if we just track it. https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2015: if (m_readyState <= HAVE_CURRENT_DATA) { Drop the added braces here and elsewhere: git checkout -p origin/master https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2035: recordAutoplayMetric(m_autoplayDeferredMetric); I'm still finding this a bit hard to follow. In general, I would like to see this: 1. All state is stored on AutoplayExperimentHelper, including the existing m_initialPlayWithoutUserGesture and m_autoplayMediaCounted. 2. HTMLMediaElement calls methods to set relevant state, like m_autoplayHelper.userGestureNotRequired(reason). 3. HTMLMediaElement calls the helper at points where it might want to record metrics based on that state, like the existing m_autoplayHelper.playMethodCalled() or m_autoplayHelper.playerPlaying()/m_autoplayHelper.playerPaused() in updatePlayState(). I think that would make the footprint in HTMLMediaElement a lot smaller, and hopefully reduce the amount of if (m_this) { m_that = true } that's hard to track, by just recording what events have occurred and laying the puzzle only the place where the metrics are actually recorded. If that makes it easier to write unit tests (for AutoplayExperimentHelper) that'd be a bonus. https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2976: m_autoplaying = false; On 2015/11/25 18:58:54, liberato wrote: > i moved this from playInternal(). not sure that this is per-spec (it only > mentions removing autoplay on a play() call). otherwise, though, letting a > video element with an autoplay attribute play to completion, then seeking to the > middle, starts playback immediately when updating the network state. that seems > really weird. Nice catch! Fixed in https://github.com/whatwg/html/pull/362 Can you fix this in a separate CL with a test suitable for upstreaming to web-platform-tests? hopefully there's some really short media resource you can use, and if not you could always use two samples of silence PCM as a data: URL. https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3098: recordMetricsBeforePause(); As I think I pointed out in an earlier review, this is ActiveDOMObject::stop() and it's probably not a good idea to consider it like a user-initiated pause. Maybe this will be reached if the tab was closed, but I don't think so. https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3623: if (m_networkState == NETWORK_LOADING && !m_recordedElement) { Why here instead of in HTMLMediaElement::loadResource at the beginning of the resource fetch algorithm? https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:629: // Reason that autoplay would be allowed to proceed without a user gesture. Can these be stored in AutoplayExperimentHelper?
i need to do more testing on this, so an in-depth review might not be needed. however, i want to see if you generally agree with the direction of the refactoring. thanks -fl https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h (right): https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:68: // Number of audio elements detected. On 2015/11/26 15:30:56, philipj wrote: > Clarify that this is only elements that reached the resource fetch algorithm? Done. https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:973: m_userGestureRequiredForPlay = false; On 2015/11/26 15:30:56, philipj wrote: > Drop this line, it looks like it would make removeUserGestureRequirement() a > no-op. Done. https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1990: m_userGestureRequiredForPlay = false; On 2015/11/26 15:30:56, philipj wrote: > Call recordAutoplayMetric(GesturelessPlaybackEnabledByPlay)? I don't think you > should need an unknown case, we know exactly what removes the gesture if we just > track it. updated to removeUserGestureRequirement. if gestureless playback starts later, then we'll record why it was allowed. i have to test this better tomorrow when i'm in the office. https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2015: if (m_readyState <= HAVE_CURRENT_DATA) { On 2015/11/26 15:30:56, philipj wrote: > Drop the added braces here and elsewhere: git checkout -p origin/master Done. https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2035: recordAutoplayMetric(m_autoplayDeferredMetric); On 2015/11/26 15:30:56, philipj wrote: > I'm still finding this a bit hard to follow. In general, I would like to see > this: > > 1. All state is stored on AutoplayExperimentHelper, including the existing > m_initialPlayWithoutUserGesture and m_autoplayMediaCounted. > 2. HTMLMediaElement calls methods to set relevant state, like > m_autoplayHelper.userGestureNotRequired(reason). > 3. HTMLMediaElement calls the helper at points where it might want to record > metrics based on that state, like the existing > m_autoplayHelper.playMethodCalled() or > m_autoplayHelper.playerPlaying()/m_autoplayHelper.playerPaused() in > updatePlayState(). > > I think that would make the footprint in HTMLMediaElement a lot smaller, and > hopefully reduce the amount of if (m_this) { m_that = true } that's hard to > track, by just recording what events have occurred and laying the puzzle only > the place where the metrics are actually recorded. > > If that makes it easier to write unit tests (for AutoplayExperimentHelper) > that'd be a bonus. oh, i didn't realize that you were suggesting to refactor all of it earlier. sure, that makes sense. https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2976: m_autoplaying = false; On 2015/11/26 15:30:56, philipj wrote: > On 2015/11/25 18:58:54, liberato wrote: > > i moved this from playInternal(). not sure that this is per-spec (it only > > mentions removing autoplay on a play() call). otherwise, though, letting a > > video element with an autoplay attribute play to completion, then seeking to > the > > middle, starts playback immediately when updating the network state. that > seems > > really weird. > > Nice catch! Fixed in https://github.com/whatwg/html/pull/362 > > Can you fix this in a separate CL with a test suitable for upstreaming to > web-platform-tests? hopefully there's some really short media resource you can > use, and if not you could always use two samples of silence PCM as a data: URL. https://codereview.chromium.org/1482393003/ https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3098: recordMetricsBeforePause(); On 2015/11/26 15:30:56, philipj wrote: > As I think I pointed out in an earlier review, this is ActiveDOMObject::stop() > and it's probably not a good idea to consider it like a user-initiated pause. > Maybe this will be reached if the tab was closed, but I don't think so. Done. https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3623: if (m_networkState == NETWORK_LOADING && !m_recordedElement) { On 2015/11/26 15:30:56, philipj wrote: > Why here instead of in HTMLMediaElement::loadResource at the beginning of the > resource fetch algorithm? Done. https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1470153004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.h:629: // Reason that autoplay would be allowed to proceed without a user gesture. On 2015/11/26 15:30:56, philipj wrote: > Can these be stored in AutoplayExperimentHelper? Done.
I've mostly skimmed the split between HTMLMediaElement and the AutoplayExperimentHelper, and it's going in precisely the direction I had hoped. I suppose that this should make it easy to write unit tests for AutoplayExperimentHelper, instead of trying to poke at every code path using LayoutTests. https://codereview.chromium.org/1470153004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/1470153004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1997: m_autoplaying = false; Did this spill over from the other CL, or did you mean to change it here? https://codereview.chromium.org/1470153004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1470153004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1500: if (isPotentiallyPlaying) { Some extra {} here and elsewhere. https://codereview.chromium.org/1470153004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1656: m_autoplayHelper.initialPlayWithUserGesture(); This is the seek method, can you remind me why `m_initialPlayWithoutUserGesture = false` was here? Renamed it doesn't make as much sense. https://codereview.chromium.org/1470153004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1926: } else if (m_userGestureRequiredForPlay) { You could now make this a plain else branch, since removeUserGestureRequirement itself will check m_userGestureRequiredForPlay.
On 2015/12/02 at 09:01:46, philipj wrote: > I've mostly skimmed the split between HTMLMediaElement and the AutoplayExperimentHelper, and it's going in precisely the direction I had hoped. > > I suppose that this should make it easy to write unit tests for AutoplayExperimentHelper, instead of trying to poke at every code path using LayoutTests. Is it possible to check what was counted from Blink? Maybe it would make sense to write higher level tests in order to check what was sent to histograms?
On 2015/12/02 10:26:31, Mounir Lamouri wrote: > On 2015/12/02 at 09:01:46, philipj wrote: > > I've mostly skimmed the split between HTMLMediaElement and the > AutoplayExperimentHelper, and it's going in precisely the direction I had hoped. > > > > I suppose that this should make it easy to write unit tests for > AutoplayExperimentHelper, instead of trying to poke at every code path using > LayoutTests. > > Is it possible to check what was counted from Blink? Maybe it would make sense > to write higher level tests in order to check what was sent to histograms? For use counters there's Internals.isUseCounted, but that only works because there's a bit array layer on top of histograms. It would be great if it were possible to test general histograms in the same way.
On 2015/12/02 at 12:25:48, philipj wrote: > On 2015/12/02 10:26:31, Mounir Lamouri wrote: > > On 2015/12/02 at 09:01:46, philipj wrote: > > > I've mostly skimmed the split between HTMLMediaElement and the > > AutoplayExperimentHelper, and it's going in precisely the direction I had hoped. > > > > > > I suppose that this should make it easy to write unit tests for > > AutoplayExperimentHelper, instead of trying to poke at every code path using > > LayoutTests. > > > > Is it possible to check what was counted from Blink? Maybe it would make sense > > to write higher level tests in order to check what was sent to histograms? > > For use counters there's Internals.isUseCounted, but that only works because there's a bit array layer on top of histograms. It would be great if it were possible to test general histograms in the same way. Using unit tests, one can override Platform to intercept histograms. Not sure what can be done from LayoutTests if we do not expose a new method in blink::Platform.
On 2015/12/07 17:52:05, Mounir Lamouri wrote: > On 2015/12/02 at 12:25:48, philipj wrote: > > On 2015/12/02 10:26:31, Mounir Lamouri wrote: > > > On 2015/12/02 at 09:01:46, philipj wrote: > > > > I've mostly skimmed the split between HTMLMediaElement and the > > > AutoplayExperimentHelper, and it's going in precisely the direction I had > hoped. > > > > > > > > I suppose that this should make it easy to write unit tests for > > > AutoplayExperimentHelper, instead of trying to poke at every code path using > > > LayoutTests. > > > > > > Is it possible to check what was counted from Blink? Maybe it would make > sense > > > to write higher level tests in order to check what was sent to histograms? > > > > For use counters there's Internals.isUseCounted, but that only works because > there's a bit array layer on top of histograms. It would be great if it were > possible to test general histograms in the same way. > > Using unit tests, one can override Platform to intercept histograms. Not sure > what can be done from LayoutTests if we do not expose a new method in > blink::Platform. Some kind of startCollectingHistogramData()/stopCollectingHistogramData() would presumably work. Or just unit tests :)
sorry for the delay. for unit tests, i opted to create a client interface for AutoplayExperimentHelper that's easier to mock. the metrics are recorded via calls to that. as a bonus, the client interface is unintrusive to HTMLMediaElement. thanks -fl https://codereview.chromium.org/1470153004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/1470153004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1997: m_autoplaying = false; On 2015/12/02 09:01:46, philipj wrote: > Did this spill over from the other CL, or did you mean to change it here? spilled over, fixed. https://codereview.chromium.org/1470153004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1470153004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1500: if (isPotentiallyPlaying) { On 2015/12/02 09:01:46, philipj wrote: > Some extra {} here and elsewhere. Done. https://codereview.chromium.org/1470153004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1656: m_autoplayHelper.initialPlayWithUserGesture(); On 2015/12/02 09:01:46, philipj wrote: > This is the seek method, can you remind me why `m_initialPlayWithoutUserGesture > = false` was here? Renamed it doesn't make as much sense. i'm not sure either. i think that was left over in the very first autoplay experiment from a year ago. i, too, do not see why we would want this. i'll remove it. https://codereview.chromium.org/1470153004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1926: } else if (m_userGestureRequiredForPlay) { On 2015/12/02 09:01:46, philipj wrote: > You could now make this a plain else branch, since removeUserGestureRequirement > itself will check m_userGestureRequiredForPlay. Done.
liberato@chromium.org changed reviewers: + isherman@chromium.org
isherman: PTAL at histograms.xml thanks -fl
histograms.xml lgtm
Happy new year! I skimmed the second half of the unit tests, if someone wants to take a closer look. I've also commented on some things which are in moved code which we may have discussed before, but the holidays flushed all of this out of cache for me. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp (right): https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:95: // Don't let future gestureless playbacks affect metrics. This doesn't seem true, AutoplayManualStart could still be counted again above, right? Simply calling play() twice inside a touch event handler should cause this. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:97: m_playbackStartedBefore = true; Shouldn't m_playbackStartedBefore be set only in playbackStarted()? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:116: recordMetricsBeforePause(); I wouldn't say that load() amounts to pausing. Something that calls load() usually isn't exposed to the user as a button or anything, so I don't think this will say anything about user behavior. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:119: recordAutoplayMetric(AutoplayEnabledThroughLoad); Should AutoplayEnabledThroughLoad simply be removed? Looks like it will be exactly the same as GesturelessPlaybackEnabledByLoad. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:121: // While usergesture-initiated load()s technically count as autoplayed, Why don't these plays feel like autoplay? And isn't measuring GesturelessPlaybackEnabledByLoad enough to get an idea of how common this case is? If further measuring really must be inhibited, an explicit way of doing it (like checking m_autoplayDeferredMetric == GesturelessPlaybackEnabledByLoad) seems better than pretending to have counted and played. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:290: // TODO(liberato): remove this. Remove it how? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:362: // we let autoplayMediaEncountered() do that later. It's actually recorded in playbackStarted(), right? Also, this comment would perhaps be better placed inside removeUserGestureRequirement(). https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:402: if (!m_autoplayMediaCounted) { Maybe rename to m_autoplayMediaEncountered? There isn't much counting going on. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:428: m_initialPlayWithoutUserGesture = false; Why is m_initialPlayWithoutUserGesture cleared here? It can't stop being true :) https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:447: recordAutoplayMetric(m_autoplayDeferredMetric); assert something about m_autoplayDeferredMetric? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:456: m_initialPlayWithoutUserGesture = false; And why here? Since it's still true that the initial play was without a user gesture, I assume that resetting this is in order to cause some other effect later, but what? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h (right): https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:89: GesturelessPlaybackUnknownReason = 20, As I said in an earlier comment, there should be no need to an "unknown" reason, the precise reason is knowable. If it's only needed as the initial value, maybe GesturelessPlaybackNotEnabled and assert in the appropriate places that it has changed to one of the other values? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:99: friend class blink::AutoplayExperimentTest; This is in namespace blink {}, is the prefix really needed? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:126: virtual WTF::String autoplayExperimentMode() const = 0; Is the WTF:: prefix really needed? It's not in HTMLMediaElement.h https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:155: // Missing comment? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:266: bool m_recordedElement : 1; The distinction between this and m_autoplayMediaCounted isn't very clear from the comments. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp (right): https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:19: AUDIO }; Enum on a single line or each entry on a separate line. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:21: MockAutoplayClient(const char* mode = "enabled-forvideo", ElementType type = VIDEO) The default value of at least mode doesn't make the tests below clearer. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:35: .WillRepeatedly(Return(type != VIDEO)); type == AUDIO? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:39: EXPECT_CALL(*this, duration()) Reading http://stackoverflow.com/questions/13933475/gmock-setting-default-actions-on-... I'm thinking maybe ON_CALL(*this, duration()).WillByDefault(Return(m_duration)) would be a clearer way to achieve these defaults? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:104: delete helper; Looks like you really want an auto pointer, so OwnPtr<>? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:127: MockAutoplayClient* client; https://www.chromium.org/blink/coding-style#TOC-Names says m_client and m_helper. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:155: EXPECT_CALL(*client, currentTime()) Is this how unit tests are normally written? I'm used to seeing EXPECT_CALL to actually expect that a method will be called (and fail otherwise), but here and many places in this test it's apparently used only for the side effect of causing m_duration to be returned. Maybe use ON_CALL? Or have the state on some object and directly update that state? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:192: void pausePlaybackNotExpectingBailout(bool expectingAutoplay) Doesn't this have to EXPECT_CALL *something* to fail if recordAutoplayMetric(AnyPlaybackBailout) is called anyway? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:360: delete m_autoplayHelper; If it can't be a reference, then OwnPtr. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2966: // Close the async event queue so that no events are enqueued by userCancelledLoad. I think this was a conflict resolution error? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3585: WTF::String HTMLMediaElement::AutoplayHelperClientImpl::autoplayExperimentMode() const Don't need WTF:: prefix. Please search for other cases too. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:136: bool shouldAutoplay(const AutoplayExperimentHelper::Client::RecordMetricsBehavior = AutoplayExperimentHelper::Client::RecordMetricsBehavior::DoNotRecord); Can using RecordMetricsBehavior = AutoplayExperimentHelper::Client::RecordMetricsBehavior make this simpler, or does RecordMetricsBehavior::DoNotRecord then not work? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:642: AutoplayExperimentHelper::Client* m_autoplayHelperClient; Why can't both of these be references still?
Happy new year! I skimmed the second half of the unit tests, if someone wants to take a closer look. I've also commented on some things which are in moved code which we may have discussed before, but the holidays flushed all of this out of cache for me. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp (right): https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:95: // Don't let future gestureless playbacks affect metrics. This doesn't seem true, AutoplayManualStart could still be counted again above, right? Simply calling play() twice inside a touch event handler should cause this. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:97: m_playbackStartedBefore = true; Shouldn't m_playbackStartedBefore be set only in playbackStarted()? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:116: recordMetricsBeforePause(); I wouldn't say that load() amounts to pausing. Something that calls load() usually isn't exposed to the user as a button or anything, so I don't think this will say anything about user behavior. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:119: recordAutoplayMetric(AutoplayEnabledThroughLoad); Should AutoplayEnabledThroughLoad simply be removed? Looks like it will be exactly the same as GesturelessPlaybackEnabledByLoad. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:121: // While usergesture-initiated load()s technically count as autoplayed, Why don't these plays feel like autoplay? And isn't measuring GesturelessPlaybackEnabledByLoad enough to get an idea of how common this case is? If further measuring really must be inhibited, an explicit way of doing it (like checking m_autoplayDeferredMetric == GesturelessPlaybackEnabledByLoad) seems better than pretending to have counted and played. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:290: // TODO(liberato): remove this. Remove it how? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:362: // we let autoplayMediaEncountered() do that later. It's actually recorded in playbackStarted(), right? Also, this comment would perhaps be better placed inside removeUserGestureRequirement(). https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:402: if (!m_autoplayMediaCounted) { Maybe rename to m_autoplayMediaEncountered? There isn't much counting going on. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:428: m_initialPlayWithoutUserGesture = false; Why is m_initialPlayWithoutUserGesture cleared here? It can't stop being true :) https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:447: recordAutoplayMetric(m_autoplayDeferredMetric); assert something about m_autoplayDeferredMetric? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:456: m_initialPlayWithoutUserGesture = false; And why here? Since it's still true that the initial play was without a user gesture, I assume that resetting this is in order to cause some other effect later, but what? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h (right): https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:89: GesturelessPlaybackUnknownReason = 20, As I said in an earlier comment, there should be no need to an "unknown" reason, the precise reason is knowable. If it's only needed as the initial value, maybe GesturelessPlaybackNotEnabled and assert in the appropriate places that it has changed to one of the other values? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:99: friend class blink::AutoplayExperimentTest; This is in namespace blink {}, is the prefix really needed? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:126: virtual WTF::String autoplayExperimentMode() const = 0; Is the WTF:: prefix really needed? It's not in HTMLMediaElement.h https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:155: // Missing comment? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:266: bool m_recordedElement : 1; The distinction between this and m_autoplayMediaCounted isn't very clear from the comments. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp (right): https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:19: AUDIO }; Enum on a single line or each entry on a separate line. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:21: MockAutoplayClient(const char* mode = "enabled-forvideo", ElementType type = VIDEO) The default value of at least mode doesn't make the tests below clearer. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:35: .WillRepeatedly(Return(type != VIDEO)); type == AUDIO? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:39: EXPECT_CALL(*this, duration()) Reading http://stackoverflow.com/questions/13933475/gmock-setting-default-actions-on-... I'm thinking maybe ON_CALL(*this, duration()).WillByDefault(Return(m_duration)) would be a clearer way to achieve these defaults? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:104: delete helper; Looks like you really want an auto pointer, so OwnPtr<>? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:127: MockAutoplayClient* client; https://www.chromium.org/blink/coding-style#TOC-Names says m_client and m_helper. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:155: EXPECT_CALL(*client, currentTime()) Is this how unit tests are normally written? I'm used to seeing EXPECT_CALL to actually expect that a method will be called (and fail otherwise), but here and many places in this test it's apparently used only for the side effect of causing m_duration to be returned. Maybe use ON_CALL? Or have the state on some object and directly update that state? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:192: void pausePlaybackNotExpectingBailout(bool expectingAutoplay) Doesn't this have to EXPECT_CALL *something* to fail if recordAutoplayMetric(AnyPlaybackBailout) is called anyway? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:360: delete m_autoplayHelper; If it can't be a reference, then OwnPtr. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2966: // Close the async event queue so that no events are enqueued by userCancelledLoad. I think this was a conflict resolution error? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3585: WTF::String HTMLMediaElement::AutoplayHelperClientImpl::autoplayExperimentMode() const Don't need WTF:: prefix. Please search for other cases too. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:136: bool shouldAutoplay(const AutoplayExperimentHelper::Client::RecordMetricsBehavior = AutoplayExperimentHelper::Client::RecordMetricsBehavior::DoNotRecord); Can using RecordMetricsBehavior = AutoplayExperimentHelper::Client::RecordMetricsBehavior make this simpler, or does RecordMetricsBehavior::DoNotRecord then not work? https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:642: AutoplayExperimentHelper::Client* m_autoplayHelperClient; Why can't both of these be references still?
i finally got back to this. i'm still trying to make oilpan happy, but i think that it works otherwise. wanted to put it out for review since there were a lot of comments and changes resulting from them. the oilpan issue will likely be a one or two line fix. thanks -fl https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp (right): https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:95: // Don't let future gestureless playbacks affect metrics. On 2016/01/12 15:12:16, philipj_OOO_til_Jan_18 wrote: > This doesn't seem true, AutoplayManualStart could still be counted again above, > right? Simply calling play() twice inside a touch event handler should cause > this. good catch. i think this was guarded before, and was unintentionally deleted. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:97: m_playbackStartedBefore = true; On 2016/01/12 15:12:17, philipj_OOO_til_Jan_18 wrote: > Shouldn't m_playbackStartedBefore be set only in playbackStarted()? i didn't just in case it doesn't get that far. i'd rather be sure that it records the metrics only once. i'll rename it to m_playbackStartMetricRecorded. then it's clear. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:116: recordMetricsBeforePause(); On 2016/01/12 15:12:17, philipj_UTC7 wrote: > I wouldn't say that load() amounts to pausing. Something that calls load() > usually isn't exposed to the user as a button or anything, so I don't think this > will say anything about user behavior. true, but not recording anything doesn't seem right either. unless you've got strong objections, i'd prefer to leave it this way to prevent us from missing a bailout. i didn't see any instances of this case actually happening, so hopefully it's not common. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:119: recordAutoplayMetric(AutoplayEnabledThroughLoad); On 2016/01/12 15:12:17, philipj_UTC7 wrote: > Should AutoplayEnabledThroughLoad simply be removed? Looks like it will be > exactly the same as GesturelessPlaybackEnabledByLoad. the former is recorded when the override happens, while the latter is recorded only if it tries to autoplay. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:121: // While usergesture-initiated load()s technically count as autoplayed, On 2016/01/12 15:12:17, philipj_UTC7 wrote: > Why don't these plays feel like autoplay? And isn't measuring > GesturelessPlaybackEnabledByLoad enough to get an idea of how common this case > is? > > If further measuring really must be inhibited, an explicit way of doing it (like > checking m_autoplayDeferredMetric == GesturelessPlaybackEnabledByLoad) seems > better than pretending to have counted and played. now that we're recording "GesturelessPlaybackEnabledByLoad" only when the thing actually starts playing, we probably don't need to treat it specially anymore. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:290: // TODO(liberato): remove this. On 2016/01/12 15:12:17, philipj_OOO_til_Jan_18 wrote: > Remove it how? i was planning to change how 'is muted' works, so that there's not a loophole. however, after i came back to planet earth, i realized that this would happen about zero times. i'll remove the comment. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:362: // we let autoplayMediaEncountered() do that later. On 2016/01/12 15:12:17, philipj_UTC7 wrote: > It's actually recorded in playbackStarted(), right? > > Also, this comment would perhaps be better placed inside > removeUserGestureRequirement(). it should say playbackStarted. however, i don't see how removeUserGestureRequirement is the right place for the comment. that does not know whether playback is going to start or not. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:402: if (!m_autoplayMediaCounted) { On 2016/01/12 15:12:17, philipj_UTC7 wrote: > Maybe rename to m_autoplayMediaEncountered? There isn't much counting going on. Done. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:428: m_initialPlayWithoutUserGesture = false; On 2016/01/12 15:12:17, philipj_UTC7 wrote: > Why is m_initialPlayWithoutUserGesture cleared here? It can't stop being true :) good point. it's really more like "need to record stop/pause metrics for autoplayed media". i'll rename. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:447: recordAutoplayMetric(m_autoplayDeferredMetric); On 2016/01/12 15:12:16, philipj_UTC7 wrote: > assert something about m_autoplayDeferredMetric? do you mean for the default (GesturelessPlaybackNotOverridden)? if so, then i think that it's not unexpected if run on desktop or with the user gesture requirement turned off in about://flags. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:456: m_initialPlayWithoutUserGesture = false; On 2016/01/12 15:12:16, philipj_UTC7 wrote: > And why here? Since it's still true that the initial play was without a user > gesture, I assume that resetting this is in order to cause some other effect > later, but what? it prevents pause/end metrics from being recorded again for autoplay. i've renamed it to make this more clear. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h (right): https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:89: GesturelessPlaybackUnknownReason = 20, On 2016/01/12 15:12:17, philipj_OOO_til_Jan_18 wrote: > As I said in an earlier comment, there should be no need to an "unknown" reason, > the precise reason is knowable. If it's only needed as the initial value, maybe > GesturelessPlaybackNotEnabled and assert in the appropriate places that it has > changed to one of the other values? yeah, i tried to comment around it. it is intended to catch cases in which the gesture requirement was not overridden, possibly because it was not required. for example, testing this on desktop produces exclusively this value. i don't expect to ever see this thing in practice, nor would i want to assert if it does somehow slip through -- there's no reason to impact somebody's playback experience. if it shows up in the metrics, then i'll investigate. i've updated the name to "GesturelessPlaybackNotOverridden", since that's actually what it means. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:99: friend class blink::AutoplayExperimentTest; On 2016/01/12 15:12:18, philipj_OOO_til_Jan_18 wrote: > This is in namespace blink {}, is the prefix really needed? Done. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:126: virtual WTF::String autoplayExperimentMode() const = 0; On 2016/01/12 15:12:18, philipj_OOO_til_Jan_18 wrote: > Is the WTF:: prefix really needed? It's not in HTMLMediaElement.h Done. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:155: // On 2016/01/12 15:12:18, philipj_OOO_til_Jan_18 wrote: > Missing comment? i meant to delete the method. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:266: bool m_recordedElement : 1; On 2016/01/12 15:12:18, philipj_OOO_til_Jan_18 wrote: > The distinction between this and m_autoplayMediaCounted isn't very clear from > the comments. Done. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp (right): https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:19: AUDIO }; On 2016/01/12 15:12:18, philipj_UTC7 wrote: > Enum on a single line or each entry on a separate line. Done. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:21: MockAutoplayClient(const char* mode = "enabled-forvideo", ElementType type = VIDEO) On 2016/01/12 15:12:18, philipj_UTC7 wrote: > The default value of at least mode doesn't make the tests below clearer. Done. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:35: .WillRepeatedly(Return(type != VIDEO)); On 2016/01/12 15:12:18, philipj_UTC7 wrote: > type == AUDIO? Done. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:39: EXPECT_CALL(*this, duration()) On 2016/01/12 15:12:18, philipj_UTC7 wrote: > Reading > http://stackoverflow.com/questions/13933475/gmock-setting-default-actions-on-... > I'm thinking maybe ON_CALL(*this, duration()).WillByDefault(Return(m_duration)) > would be a clearer way to achieve these defaults? Done, plus NiceMock to supress the warnings. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:104: delete helper; On 2016/01/12 15:12:18, philipj_UTC7 wrote: > Looks like you really want an auto pointer, so OwnPtr<>? Done. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:127: MockAutoplayClient* client; On 2016/01/12 15:12:18, philipj_UTC7 wrote: > https://www.chromium.org/blink/coding-style#TOC-Names says m_client and > m_helper. don't know why i did that. thanks. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:155: EXPECT_CALL(*client, currentTime()) On 2016/01/12 15:12:18, philipj_UTC7 wrote: > Is this how unit tests are normally written? I'm used to seeing EXPECT_CALL to > actually expect that a method will be called (and fail otherwise), but here and > many places in this test it's apparently used only for the side effect of > causing m_duration to be returned. > > Maybe use ON_CALL? Or have the state on some object and directly update that > state? moved to ON_CALL, and make a NiceMock out of it to supress the warnings. that was the only reason i used EXPECT. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:192: void pausePlaybackNotExpectingBailout(bool expectingAutoplay) On 2016/01/12 15:12:18, philipj_UTC7 wrote: > Doesn't this have to EXPECT_CALL *something* to fail if > recordAutoplayMetric(AnyPlaybackBailout) is called anyway? added Times(0) for both bailout cases. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:360: delete m_autoplayHelper; On 2016/01/12 15:12:19, philipj_UTC7 wrote: > If it can't be a reference, then OwnPtr. Done. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2966: // Close the async event queue so that no events are enqueued by userCancelledLoad. On 2016/01/12 15:12:19, philipj_UTC7 wrote: > I think this was a conflict resolution error? yeah, looks that way. i don't see 'userCancelledLoad' missing from anywhere in a diff against origin/master, so probably was an addition from my side of the merge. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3585: WTF::String HTMLMediaElement::AutoplayHelperClientImpl::autoplayExperimentMode() const On 2016/01/12 15:12:19, philipj_UTC7 wrote: > Don't need WTF:: prefix. Please search for other cases too. done. applying the same idea to WTF::currentTime is definitely not a good idea. :) https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:136: bool shouldAutoplay(const AutoplayExperimentHelper::Client::RecordMetricsBehavior = AutoplayExperimentHelper::Client::RecordMetricsBehavior::DoNotRecord); On 2016/01/12 15:12:19, philipj_UTC7 wrote: > Can using RecordMetricsBehavior = > AutoplayExperimentHelper::Client::RecordMetricsBehavior make this simpler, or > does RecordMetricsBehavior::DoNotRecord then not work? seems to work. good idea. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:642: AutoplayExperimentHelper::Client* m_autoplayHelperClient; On 2016/01/12 15:12:19, philipj_UTC7 wrote: > Why can't both of these be references still? i don't like relying on destruction order, since it's too easy to accidentally reorder the members. i'll update to OwnPtr.
https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp (right): https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:97: m_playbackStartedBefore = true; On 2016/01/29 08:25:24, liberato wrote: > On 2016/01/12 15:12:17, philipj_OOO_til_Jan_18 wrote: > > Shouldn't m_playbackStartedBefore be set only in playbackStarted()? > > i didn't just in case it doesn't get that far. i'd rather be sure that it > records the metrics only once. i'll rename it to m_playbackStartMetricRecorded. > then it's clear. Acknowledged. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:116: recordMetricsBeforePause(); On 2016/01/29 08:25:24, liberato wrote: > On 2016/01/12 15:12:17, philipj_UTC7 wrote: > > I wouldn't say that load() amounts to pausing. Something that calls load() > > usually isn't exposed to the user as a button or anything, so I don't think > this > > will say anything about user behavior. > > true, but not recording anything doesn't seem right either. unless you've got > strong objections, i'd prefer to leave it this way to prevent us from missing a > bailout. > > i didn't see any instances of this case actually happening, so hopefully it's > not common. If it were common it would taint the metric for pause, it seems, but I suppose that's an acceptable risk. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:119: recordAutoplayMetric(AutoplayEnabledThroughLoad); On 2016/01/29 08:25:24, liberato wrote: > On 2016/01/12 15:12:17, philipj_UTC7 wrote: > > Should AutoplayEnabledThroughLoad simply be removed? Looks like it will be > > exactly the same as GesturelessPlaybackEnabledByLoad. > > the former is recorded when the override happens, while the latter is recorded > only if it tries to autoplay. Oh, different function call there :) https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:290: // TODO(liberato): remove this. On 2016/01/29 08:25:24, liberato wrote: > On 2016/01/12 15:12:17, philipj_OOO_til_Jan_18 wrote: > > Remove it how? > > i was planning to change how 'is muted' works, so that there's not a loophole. > however, after i came back to planet earth, i realized that this would happen > about zero times. i'll remove the comment. Welcome back :) https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:362: // we let autoplayMediaEncountered() do that later. On 2016/01/29 08:25:24, liberato wrote: > On 2016/01/12 15:12:17, philipj_UTC7 wrote: > > It's actually recorded in playbackStarted(), right? > > > > Also, this comment would perhaps be better placed inside > > removeUserGestureRequirement(). > > it should say playbackStarted. however, i don't see how > removeUserGestureRequirement is the right place for the comment. that does not > know whether playback is going to start or not. Acknowledged. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:447: recordAutoplayMetric(m_autoplayDeferredMetric); On 2016/01/29 08:25:24, liberato wrote: > On 2016/01/12 15:12:16, philipj_UTC7 wrote: > > assert something about m_autoplayDeferredMetric? > > do you mean for the default (GesturelessPlaybackNotOverridden)? if so, then i > think that it's not unexpected if run on desktop or with the user gesture > requirement turned off in about://flags. Right, that other platform :) https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:456: m_initialPlayWithoutUserGesture = false; On 2016/01/29 08:25:24, liberato wrote: > On 2016/01/12 15:12:16, philipj_UTC7 wrote: > > And why here? Since it's still true that the initial play was without a user > > gesture, I assume that resetting this is in order to cause some other effect > > later, but what? > > it prevents pause/end metrics from being recorded again for autoplay. i've > renamed it to make this more clear. Acknowledged. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h (right): https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:89: GesturelessPlaybackUnknownReason = 20, On 2016/01/29 08:25:24, liberato wrote: > On 2016/01/12 15:12:17, philipj_OOO_til_Jan_18 wrote: > > As I said in an earlier comment, there should be no need to an "unknown" > reason, > > the precise reason is knowable. If it's only needed as the initial value, > maybe > > GesturelessPlaybackNotEnabled and assert in the appropriate places that it has > > changed to one of the other values? > > yeah, i tried to comment around it. > > it is intended to catch cases in which the gesture requirement was not > overridden, possibly because it was not required. for example, testing this on > desktop produces exclusively this value. > > i don't expect to ever see this thing in practice, nor would i want to assert if > it does somehow slip through -- there's no reason to impact somebody's playback > experience. if it shows up in the metrics, then i'll investigate. > > i've updated the name to "GesturelessPlaybackNotOverridden", since that's > actually what it means. Acknowledged. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp (right): https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:39: EXPECT_CALL(*this, duration()) On 2016/01/29 08:25:24, liberato wrote: > On 2016/01/12 15:12:18, philipj_UTC7 wrote: > > Reading > > > http://stackoverflow.com/questions/13933475/gmock-setting-default-actions-on-... > > I'm thinking maybe ON_CALL(*this, > duration()).WillByDefault(Return(m_duration)) > > would be a clearer way to achieve these defaults? > > Done, plus NiceMock to supress the warnings. It worked, yay! https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:136: bool shouldAutoplay(const AutoplayExperimentHelper::Client::RecordMetricsBehavior = AutoplayExperimentHelper::Client::RecordMetricsBehavior::DoNotRecord); On 2016/01/29 08:25:25, liberato wrote: > On 2016/01/12 15:12:19, philipj_UTC7 wrote: > > Can using RecordMetricsBehavior = > > AutoplayExperimentHelper::Client::RecordMetricsBehavior make this simpler, or > > does RecordMetricsBehavior::DoNotRecord then not work? > > seems to work. good idea. Isn't it possible to also replace AutoplayExperimentHelper::Client::RecordMetricsBehavior with just RecordMetricsBehavior? If the using keyword only helps a single instance, then I think it's not worth it. https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:642: AutoplayExperimentHelper::Client* m_autoplayHelperClient; On 2016/01/29 08:25:25, liberato wrote: > On 2016/01/12 15:12:19, philipj_UTC7 wrote: > > Why can't both of these be references still? > > i don't like relying on destruction order, since it's too easy to accidentally > reorder the members. > > i'll update to OwnPtr. Not sure why I said "references still" when it was originally an "AutoplayExperimentHelper m_autoplayHelper", but this still needs to be fixed in some way. https://codereview.chromium.org/1470153004/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1470153004/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:641: RawPtr<HTMLMediaElement> m_element; A plain RawPtr<T>, as opposed to an Oilpan transitional type like RefPtrWillBeRawPtr<T>, is pretty rare in Blink and in fact this doesn't compile for me: ../../third_party/WebKit/Source/core/html/HTMLMediaElement.h:611:5: error: [blink-gc] Class 'AutoplayHelperClientImpl' contains invalid fields. class AutoplayHelperClientImpl : public AutoplayExperimentHelper::Client { ^ ../../third_party/WebKit/Source/core/html/HTMLMediaElement.h:641:9: note: [blink-gc] Raw pointer field 'm_element' to a GC managed class declared here: RawPtr<HTMLMediaElement> m_element; ^ 1 error generated.
Oilpan reviewers, can you advise on the situation in AutoplayExperimentHelper.h and HTMLMediaElement.h? This started out with just a single part object AutoplayExperimentHelper (fixed in https://codereview.chromium.org/1436783002, so that nicely shows all the relevant plumbing) and now that helper need a client to abstract away the HTMLMediaElement reference. I see two options: 1. Make AutoplayExperimentHelper::Client a part object of HTMLMediaElement as well, traced by HTMLMediaElement and then give AutoplayExperimentHelper a Member<Client> m_client. 2. Make both AutoplayExperimentHelper and ::Client heap-allocated and handle as usual. I think option 1 is nice as it doesn't add unnecessary heap allocations, but are there other ways? For cases like these, is it permissible to have plain references to the embedding object and somehow suppress the warning? https://codereview.chromium.org/1470153004/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h (left): https://codereview.chromium.org/1470153004/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:157: RawPtrWillBeMember<HTMLMediaElement> m_element; I think you'll need to keep a RawPtrWillBeMember<T> here and perhaps move the Client class to the Oilpan heap as well. Will CC oilpan-reviews@ and ask for advice.
Patchset #19 (id:360001) has been deleted
@philipj: i believe that RawPtrWillBeUntracedMember is what we want for the oilpan problem. i found a similar usage of it in https://codereview.chromium.org/1487553002 . also, fixed some weirdness in msvc with the test. i'm wfh so have done only basic testing on these two small changes. are there any other issues that i should address, assuming that the manual tests are still successful? thanks -fl
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
> @philipj: i believe that RawPtrWillBeUntracedMember is what we want for the oilpan problem. i found a similar usage of it in https://codereview.chromium.org/1487553002 . UntracedMember<> is not something you want to be introducing..it essentially turns off Oilpan & is only intended used when we're in a really tight spot wrt object dependencies and lifetimes. Which is not the case here, the object relationships are regular & fine. I've added some comments..hopefully not too brief to be actionable, please ask if anything is unclear (or disagreeable :) ) https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp (right): https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:437: if (!m_playbackStartedMetricRecorded) { if (m_playbackStartedMetricRecorded) return; ... https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:451: if (m_waitingForAutoplayPlaybackEnd) { if (!m_waitingForAutoplayPlaybackEnd) return; .... https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h (left): https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:71: DISALLOW_NEW_EXCEPT_PLACEMENT_NEW(); I see, you can't keep this a part object because of the usage from a gtest unit test. Then this would be better off on the heap, so change this to class CORE_EXPORT AutoplayExperimentHelper final : public NoBaseWillBeGarbageCollectedFinalized<AutoplayExperimentHelper> { DEFINE_INLINE_TRACE() { visitor->trace(m_client); } https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h (right): https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:103: class Client { This is better off on the Oilpan heap as all neighboring objects are, so make this class Client : public GarbageCollectedFinalized<Client> { ..like..before DEFINE_INLINE_VIRTUAL_TRACE() { } }; https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:240: Client& m_client; RawPtrWillBeMember<Client> m_client; https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp (right): https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:121: OwnPtr<MockAutoplayClient> m_client; Persistent<MockAutoplayClient> https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:122: OwnPtr<AutoplayExperimentHelper> m_helper; Persistent<AutoplayExperimentHelper m_helper; https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:338: , m_autoplayHelperClient(adoptPtr(new AutoplayHelperClientImpl(this))) Could you add a static create() factory method & change this to , m_autoplayHelperClient(AutoplayHelperClientImpl::create(this)) https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:339: , m_autoplayHelper(adoptPtr(new AutoplayExperimentHelper(*m_autoplayHelperClient))) Could you add a static create() factory method & change this to , m_autoplayHelper(AutoplayExperimentHelper::create(*m_autoplayHelperClient)) https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:361: if (m_autoplayHelper) I haven't looked at the details, but is this explicit clearing needed to handle some finalization details? https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:608: class AutoplayHelperClientImpl : public AutoplayExperimentHelper::Client { You need to turn this into an Oilpan controlled object -- class AutoplayHelperClientImpl : public NoBaseWillBeGarbageCollectedFinalized<AutoplayHelperClientImpl>, public ... { .... }; Can we move this local class to the .cpp? HTMLMediaElement.h is too bulky as it is, so would be good if we can not extend it unnecessarily. https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:638: RawPtrWillBeUntracedMember<HTMLMediaElement> m_element; RawPtrWillBeMember<HTMLMediaElement> + it needs to be traced. https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:641: OwnPtr<AutoplayExperimentHelper::Client> m_autoplayHelperClient; OwnPtrWillBeMember<..> + it needs to be traced. https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:642: OwnPtr<AutoplayExperimentHelper> m_autoplayHelper; OwnPtrWillBeMemer<AutoplayExperimentHelper> m_autoplayHelper;
On 2016/02/16 07:59:42, liberato wrote: > @philipj: i believe that RawPtrWillBeUntracedMember is what we want for the > oilpan problem. i found a similar usage of it in > https://codereview.chromium.org/1487553002 . also, fixed some weirdness in msvc > with the test. > > i'm wfh so have done only basic testing on these two small changes. are there > any other issues that i should address, assuming that the manual tests are still > successful? There's still the AutoplayExperimentHelper::Client::RecordMetricsBehavior question. Leave no comment unresolved :) Other than that, I think it's just getting the ownership in order that's missing, and sof@ had some good feedback on that.
@sof: thanks for the detailed feedback. it looks like things are being cleaned up properly with oilpan, based on logging. i have no idea if i really got it right though. any feedback is appreciated. @philipj: sorry i missed that one. thanks -fl https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1470153004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:136: bool shouldAutoplay(const AutoplayExperimentHelper::Client::RecordMetricsBehavior = AutoplayExperimentHelper::Client::RecordMetricsBehavior::DoNotRecord); On 2016/02/02 08:08:15, philipj_UTC7 wrote: > On 2016/01/29 08:25:25, liberato wrote: > > On 2016/01/12 15:12:19, philipj_UTC7 wrote: > > > Can using RecordMetricsBehavior = > > > AutoplayExperimentHelper::Client::RecordMetricsBehavior make this simpler, > or > > > does RecordMetricsBehavior::DoNotRecord then not work? > > > > seems to work. good idea. > > Isn't it possible to also replace > AutoplayExperimentHelper::Client::RecordMetricsBehavior with just > RecordMetricsBehavior? If the using keyword only helps a single instance, then I > think it's not worth it. Done. in the interim, RecordMetricsBehavior was used elsewhere, so i also renamed '...SandboxFailure' back to just 'DoRecord'. there was no need to keep it separate. https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp (right): https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:437: if (!m_playbackStartedMetricRecorded) { On 2016/02/16 08:40:05, sof wrote: > if (m_playbackStartedMetricRecorded) > return; > ... Done. https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:451: if (m_waitingForAutoplayPlaybackEnd) { On 2016/02/16 08:40:05, sof wrote: > if (!m_waitingForAutoplayPlaybackEnd) > return; > .... Done. https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h (left): https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:71: DISALLOW_NEW_EXCEPT_PLACEMENT_NEW(); On 2016/02/16 08:40:05, sof wrote: > I see, you can't keep this a part object because of the usage from a gtest unit > test. > > Then this would be better off on the heap, so change this to > > class CORE_EXPORT AutoplayExperimentHelper final : public > NoBaseWillBeGarbageCollectedFinalized<AutoplayExperimentHelper> { > DEFINE_INLINE_TRACE() { visitor->trace(m_client); } > Done. https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h (right): https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:103: class Client { On 2016/02/16 08:40:05, sof wrote: > This is better off on the Oilpan heap as all neighboring objects are, so make > this > > class Client : public GarbageCollectedFinalized<Client> { > ..like..before > DEFINE_INLINE_VIRTUAL_TRACE() { } > }; Done. https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:240: Client& m_client; On 2016/02/16 08:40:05, sof wrote: > RawPtrWillBeMember<Client> m_client; Done. https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp (right): https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:121: OwnPtr<MockAutoplayClient> m_client; On 2016/02/16 08:40:06, sof wrote: > Persistent<MockAutoplayClient> Done. Before i saw this, i tried OwnPtrWillBeMember. why Persistent? https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:122: OwnPtr<AutoplayExperimentHelper> m_helper; On 2016/02/16 08:40:06, sof wrote: > Persistent<AutoplayExperimentHelper m_helper; Done. https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:338: , m_autoplayHelperClient(adoptPtr(new AutoplayHelperClientImpl(this))) On 2016/02/16 08:40:06, sof wrote: > Could you add a static create() factory method & change this to > > , m_autoplayHelperClient(AutoplayHelperClientImpl::create(this)) Done. https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:339: , m_autoplayHelper(adoptPtr(new AutoplayExperimentHelper(*m_autoplayHelperClient))) On 2016/02/16 08:40:06, sof wrote: > Could you add a static create() factory method & change this to > > , m_autoplayHelper(AutoplayExperimentHelper::create(*m_autoplayHelperClient)) Done. https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:361: if (m_autoplayHelper) On 2016/02/16 08:40:06, sof wrote: > I haven't looked at the details, but is this explicit clearing needed to handle > some finalization details? yes. destroying the helper calls into the client, with raw ptrs all around in the non-oilpan case. for oilpan, i've moved cleanup into dispose(), since it likes to unregister event listeners, for example. https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:608: class AutoplayHelperClientImpl : public AutoplayExperimentHelper::Client { On 2016/02/16 08:40:06, sof wrote: > You need to turn this into an Oilpan controlled object -- > > class AutoplayHelperClientImpl : public > NoBaseWillBeGarbageCollectedFinalized<AutoplayHelperClientImpl>, public ... { > .... > }; > > Can we move this local class to the .cpp? HTMLMediaElement.h is too bulky as it > is, so would be good if we can not extend it unnecessarily. declaring this to be a subclass of NoBase... causes an "ambiguous operator new" between this and Client::new. i just removed the NoBase... here. is that the right thing to do? https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:638: RawPtrWillBeUntracedMember<HTMLMediaElement> m_element; On 2016/02/16 08:40:06, sof wrote: > RawPtrWillBeMember<HTMLMediaElement> + it needs to be traced. Done. https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:641: OwnPtr<AutoplayExperimentHelper::Client> m_autoplayHelperClient; On 2016/02/16 08:40:06, sof wrote: > OwnPtrWillBeMember<..> + it needs to be traced. Done. https://codereview.chromium.org/1470153004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:642: OwnPtr<AutoplayExperimentHelper> m_autoplayHelper; On 2016/02/16 08:40:06, sof wrote: > OwnPtrWillBeMemer<AutoplayExperimentHelper> m_autoplayHelper; Done.
https://codereview.chromium.org/1470153004/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (left): https://codereview.chromium.org/1470153004/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:71: enum class RecordMetricsBehavior { Given that's it's used for other things as well now, can we leave it here and "using" it from AutoplayExperimentHelper instead?
thanks -fl https://codereview.chromium.org/1470153004/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (left): https://codereview.chromium.org/1470153004/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:71: enum class RecordMetricsBehavior { On 2016/02/25 10:53:49, philipj_UTC7 wrote: > Given that's it's used for other things as well now, can we leave it here and > "using" it from AutoplayExperimentHelper instead? Done. luckily, the autoplay helper only ever sent DoNotRecord, so i just removed it from Client::shouldAutoplay(). otherwise, the forward declaration would have been hard since it's nested in HTMLMediaElement.
I haven't done another full (or even partial) pass but this now LGTM assuming no comments have been left behind. I guess you want to wait for feedback on the Oilpan issue, though.
thanks for carefully following up - oilpan lgtm. https://codereview.chromium.org/1470153004/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp (right): https://codereview.chromium.org/1470153004/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 by now :) https://codereview.chromium.org/1470153004/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1470153004/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:257: class HTMLMediaElement::AutoplayHelperClientImpl : Move this up into the anon namespace and declare it as class AutplayHelperClientImpl : public ... { ... }; ? https://codereview.chromium.org/1470153004/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:295: DEFINE_INLINE_VIRTUAL_TRACE() { DEFINE_INLINE_VIRTUAL_TRACE() { ... } https://codereview.chromium.org/1470153004/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3652: HTMLMediaElement::AutoplayHelperClientImpl::~AutoplayHelperClientImpl() nit: it's ok this way, but defining these shorter methods inline along with its declaration would put it all in one place. https://codereview.chromium.org/1470153004/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1470153004/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:607: class AutoplayHelperClientImpl; Let's just leave this out (and make HTMLMediaElement a little bit shorter)?
thanks for the feedback everybody. philipj: the most recent CL requires that the web media player is actually playing, before it emits a paused metric. this makes the paused metrics symmetric with the playback started metrics. otherwise, play/pause/play/pause might emit two pauses and no plays, if the play doesn't actually start. do you agree with this change? https://codereview.chromium.org/1470153004/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp (right): https://codereview.chromium.org/1470153004/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/02/25 18:55:43, sof wrote: > 2016 by now :) i should try to plan further ahead. Done. https://codereview.chromium.org/1470153004/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1470153004/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:257: class HTMLMediaElement::AutoplayHelperClientImpl : On 2016/02/25 18:55:43, sof wrote: > Move this up into the anon namespace and declare it as > > class AutplayHelperClientImpl : public ... { ... }; > > ? can't without friending. https://codereview.chromium.org/1470153004/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:295: DEFINE_INLINE_VIRTUAL_TRACE() { On 2016/02/25 18:55:43, sof wrote: > DEFINE_INLINE_VIRTUAL_TRACE() > { > ... > } Done. https://codereview.chromium.org/1470153004/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1470153004/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:607: class AutoplayHelperClientImpl; On 2016/02/25 18:55:44, sof wrote: > Let's just leave this out (and make HTMLMediaElement a little bit shorter)? it accesses private members, so it would need a friend decl anyway.
On 2016/03/14 17:09:13, liberato wrote: > thanks for the feedback everybody. > > philipj: the most recent CL requires that the web media player is actually > playing, before it emits a paused metric. this makes the paused metrics > symmetric with the playback started metrics. otherwise, play/pause/play/pause > might emit two pauses and no plays, if the play doesn't actually start. > > do you agree with this change? I can't find the change in question with among the unrelated changes, not even looking at the difference between PS26 and PS27. Where is it? :)
https://codereview.chromium.org/1470153004/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1470153004/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:276: &&!m_element->webMediaPlayer()->paused(); here. sorry, i forgot that i rebased somewhere in there.
(Moving myself to CC. Let me know if I was expected to review something here.)
mlamouri@chromium.org changed reviewers: - mlamouri@chromium.org
https://codereview.chromium.org/1470153004/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1470153004/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:276: &&!m_element->webMediaPlayer()->paused(); On 2016/03/15 14:10:49, liberato wrote: > here. sorry, i forgot that i rebased somewhere in there. Looks a bit odd to check both the paused state (what we want to do) and the actual state. Would `return !m_element->webMediaPlayer() || m_element->webMediaPlayer()->paused()` suffice? A comment about why just the paused state isn't enough (because it can be out of sync with reality, by design) would be helpful too.
thanks -fl https://codereview.chromium.org/1470153004/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1470153004/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:276: &&!m_element->webMediaPlayer()->paused(); On 2016/03/16 11:37:43, philipj_UTC7 wrote: > On 2016/03/15 14:10:49, liberato wrote: > > here. sorry, i forgot that i rebased somewhere in there. > > Looks a bit odd to check both the paused state (what we want to do) and the > actual state. Would `return !m_element->webMediaPlayer() || > m_element->webMediaPlayer()->paused()` suffice? A comment about why just the > paused state isn't enough (because it can be out of sync with reality, by > design) would be helpful too. i added a comment. i also tried removing m_paused, but it turns out that checking the media player's state alone doesn't work. it requires that updatePlayState has notified the player before the helper finds out, else we miss pause() when we notify the helper before the media player. probably, pause metrics should be moved into updatePlayState as well, so that it's actually symmetric. then, paused() here can check just the media player, or (better yet) the helper won't need to check paused() at all. i'm going to look into how big of a change that is. if it's small, then i'll make it here. otherwise, it's probably left for future work, since i'd really rather like to avoid tearing it all up again, and possibly breaking it.
turns out that it was easy to move pause metrics reporting into updatePlayState. also fixed the tests, which weren't destroying the mock before the test, so test failures were being missed. DocumentTest does this already. thanks -fl
Just to double check, is this waiting for input from anyone? Me?
thanks for reminding me. do you have any objection to the simplified pause logic in the last ps? thanks -fl
https://codereview.chromium.org/1470153004/diff/640001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1470153004/diff/640001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:288: bool complete() const override Can this simply use HTMLMediaElement::ended()? There's a difference in readyState and playback direction, but do those differences matter?
On 2016/03/29 06:21:09, philipj_UTC7 wrote: > https://codereview.chromium.org/1470153004/diff/640001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1470153004/diff/640001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:288: bool complete() > const override > Can this simply use HTMLMediaElement::ended()? There's a difference in > readyState and playback direction, but do those differences matter? i think that you're right, thanks! either ended or endedPlayback seems like it'd work just as well. i'll give it a try when i'm back in the office in the (my) morning. assuming that it works okay, would you have any objection if i land it after that change and save a round trip? thanks -fl
On 2016/03/29 06:44:20, liberato wrote: > On 2016/03/29 06:21:09, philipj_UTC7 wrote: > > > https://codereview.chromium.org/1470153004/diff/640001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > > > > https://codereview.chromium.org/1470153004/diff/640001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:288: bool complete() > > const override > > Can this simply use HTMLMediaElement::ended()? There's a difference in > > readyState and playback direction, but do those differences matter? > > i think that you're right, thanks! either ended or endedPlayback seems like > it'd work just as well. i'll give it a try when i'm back in the office in the > (my) morning. assuming that it works okay, would you have any objection if i > land it after that change and save a round trip? > > thanks > -fl Sure, no problem, if there's anything worth for me to check you can let me know and I'll do it post-commit.
The CQ bit was checked by liberato@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, sigbjornf@opera.com, philipj@opera.com Link to the patchset: https://codereview.chromium.org/1470153004/#ps680001 (title: "simplified ended() logic.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1470153004/680001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1470153004/680001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...)
The CQ bit was checked by liberato@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com, isherman@chromium.org, philipj@opera.com Link to the patchset: https://codereview.chromium.org/1470153004/#ps700001 (title: "...because tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1470153004/700001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1470153004/700001
Message was sent while issue was closed.
Description was changed from ========== Autoplay experiment metric fixes and additions. This CL fixes the following issues in the autoplay experiment: - "Any media playback" wasn't checking if the media was playing already, so it over-counted quite a bit. - "ifmuted" counting was broken, since the autoplay experiment didn't call autoplayMediaEncountered in all cases. For mute, this could actually allow the media to start playback without preventing additional autoplay metrics for the next playback. - made shouldAutoplay not call autoplayMediaEncountered, since the experiment might not have overridden the gesture requirement yet. This would generate incorrect metrics in SetReadyState. - Adds a metric for "played to completion". - Adds metrics for initial playback that capture whether any user gesture was seen, and / or if one is active curently. This will give us an idea about how often a gesture in the inital load() call is responsible for a playback. - Added counters for "total media elements seen" and "total video elements seen". BUG=561098 ========== to ========== Autoplay experiment metric fixes and additions. This CL fixes the following issues in the autoplay experiment: - "Any media playback" wasn't checking if the media was playing already, so it over-counted quite a bit. - "ifmuted" counting was broken, since the autoplay experiment didn't call autoplayMediaEncountered in all cases. For mute, this could actually allow the media to start playback without preventing additional autoplay metrics for the next playback. - made shouldAutoplay not call autoplayMediaEncountered, since the experiment might not have overridden the gesture requirement yet. This would generate incorrect metrics in SetReadyState. - Adds a metric for "played to completion". - Adds metrics for initial playback that capture whether any user gesture was seen, and / or if one is active curently. This will give us an idea about how often a gesture in the inital load() call is responsible for a playback. - Added counters for "total media elements seen" and "total video elements seen". BUG=561098 ==========
Message was sent while issue was closed.
Committed patchset #35 (id:700001)
Message was sent while issue was closed.
Description was changed from ========== Autoplay experiment metric fixes and additions. This CL fixes the following issues in the autoplay experiment: - "Any media playback" wasn't checking if the media was playing already, so it over-counted quite a bit. - "ifmuted" counting was broken, since the autoplay experiment didn't call autoplayMediaEncountered in all cases. For mute, this could actually allow the media to start playback without preventing additional autoplay metrics for the next playback. - made shouldAutoplay not call autoplayMediaEncountered, since the experiment might not have overridden the gesture requirement yet. This would generate incorrect metrics in SetReadyState. - Adds a metric for "played to completion". - Adds metrics for initial playback that capture whether any user gesture was seen, and / or if one is active curently. This will give us an idea about how often a gesture in the inital load() call is responsible for a playback. - Added counters for "total media elements seen" and "total video elements seen". BUG=561098 ========== to ========== Autoplay experiment metric fixes and additions. This CL fixes the following issues in the autoplay experiment: - "Any media playback" wasn't checking if the media was playing already, so it over-counted quite a bit. - "ifmuted" counting was broken, since the autoplay experiment didn't call autoplayMediaEncountered in all cases. For mute, this could actually allow the media to start playback without preventing additional autoplay metrics for the next playback. - made shouldAutoplay not call autoplayMediaEncountered, since the experiment might not have overridden the gesture requirement yet. This would generate incorrect metrics in SetReadyState. - Adds a metric for "played to completion". - Adds metrics for initial playback that capture whether any user gesture was seen, and / or if one is active curently. This will give us an idea about how often a gesture in the inital load() call is responsible for a playback. - Added counters for "total media elements seen" and "total video elements seen". BUG=561098 Committed: https://crrev.com/af2a826c3fa40d5e7e7895e860aa059ddab80b20 Cr-Commit-Position: refs/heads/master@{#383830} ==========
Message was sent while issue was closed.
Patchset 35 (id:??) landed as https://crrev.com/af2a826c3fa40d5e7e7895e860aa059ddab80b20 Cr-Commit-Position: refs/heads/master@{#383830} |
