This patch fixes a problem where the fullscreen HTML5 video controls do not fade out after the desired 3 second timeout. Calls to HTMLMediaElement::mediaPlayerTimeChanged() happen at least once per second while playing, and each of these calls ends up in HTMLMediaElement::updatePlayState() where MediaControlRootElement::playbackStarted() gets called every time. This keeps repeatedly restarting the 3 second timer and it never has a chance to expire. The playbackStarted() call should be moved so it happens only on the play transition.
Created attachment 142535 [details] Patch
Comment on attachment 142535 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142535&action=review > Source/WebCore/ChangeLog:19 > + No new tests because the visibility of the controls is not > + available from Javascript. The visibility of the controls /is/ available from JavaScript. Look at the test in LayoutTests/fullscreen/video-controls-override.html. And a test is needed here. > Source/WebCore/html/HTMLMediaElement.cpp:-3596 > + if (hasMediaControls()) > + mediaControls()->playbackStarted(); > } > > - if (hasMediaControls()) > - mediaControls()->playbackStarted(); Why does this change only affect full-screen mode?
(In reply to comment #2) > (From update of attachment 142535 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142535&action=review > > Why does this change only affect full-screen mode? Ahh, to answer my own question, because the hideFullscreenControlsTimer is being reset every time the time updates.
(In reply to comment #2) > The visibility of the controls /is/ available from JavaScript. Look at the test in LayoutTests/fullscreen/video-controls-override.html. And a test is needed here. Thanks for that information. I'll work on a test...
Apart from the lack of a test, this looks like a good change. But has it been broken since r81283 <http://trac.webkit.org/changeset/81283>?
(In reply to comment #5) > Apart from the lack of a test, this looks like a good change. But has it been broken since r81283 <http://trac.webkit.org/changeset/81283>? Ah, to answer my own question again: this is probably MediaPlayerPrivate-specific behavior. Some platform media engines (namely, QTKit and AVFoundation) only emit a mediaPlayerTimeChanged callback during discontinuities (I.e. scrubbing). So on those platforms, this behavior will not occur. On what platform are you seeing this behavior?
(In reply to comment #6) > Ah, to answer my own question again: this is probably MediaPlayerPrivate-specific behavior. Some platform media engines (namely, QTKit and AVFoundation) only emit a mediaPlayerTimeChanged callback during discontinuities (I.e. scrubbing). So on those platforms, this behavior will not occur. On what platform are you seeing this behavior? This is on BlackBerry. We need to send frequent time updates to make sure that the currentTimeDisplay text and timeRemainingDisplay text stay up to date.
(In reply to comment #7) > (In reply to comment #6) > > Ah, to answer my own question again: this is probably MediaPlayerPrivate-specific behavior. Some platform media engines (namely, QTKit and AVFoundation) only emit a mediaPlayerTimeChanged callback during discontinuities (I.e. scrubbing). So on those platforms, this behavior will not occur. On what platform are you seeing this behavior? > > This is on BlackBerry. We need to send frequent time updates to make sure that the currentTimeDisplay text and timeRemainingDisplay text stay up to date. That should be handled independent of the MediaPlayerPrivate by HTMLMediaElement::playbackProgressTimerFired(Timer<HTMLMediaElement>*) and thus by mediaControls()->playbackProgressed(). This is a good fix, but I think you may be fixing the symptom of a larger issue. And perhaps we should make explicit the expectation that mediaPlayerTimeChanged() is only called during scrubbing / seeking.
(In reply to comment #8) > This is a good fix, but I think you may be fixing the symptom of a larger issue. And perhaps we should make explicit the expectation that mediaPlayerTimeChanged() is only called during scrubbing / seeking. Ah, yes. I see what you mean. I was not aware until now that mediaPlayerTimeChanged() needs to be called to indicate the end of seeking. I made sure this happens now in the BlackBerry platform media player code, and the playbackProgressTimer works reliably. I've stopped calling mediaPlayerTimeChanged() unnecessarily, and the patch attached to this PR is no longer needed. Or at least not needed to fix any perceptible problem. I'll have a hard time coming up with an automated test now ;)
(In reply to comment #9) > I've stopped calling mediaPlayerTimeChanged() unnecessarily, and the patch attached to this PR is no longer needed. Or at least not needed to fix any perceptible problem. I'll have a hard time coming up with an automated test now ;) Oh, I can imagine that a script that did: video.play() setTimeout(function() { video.currentTime = video.currentTime - 1.0; }, 1000); Would make the full screen controls reappear after seeking. But you're right, it's less of an important bug now. :)
Comment on attachment 142535 [details] Patch r- based on comment #9.
(In reply to comment #11) > (From update of attachment 142535 [details]) > r- based on comment #9. Rob, can you give more details on why you gave my patch an r-? It's my understanding that the only outstanding item is a test. If you have a problem with the code diff itself, please explain and I will withdraw this bug.
(In reply to comment #12) > (In reply to comment #11) > > (From update of attachment 142535 [details] [details]) > > r- based on comment #9. > > Rob, can you give more details on why you gave my patch an r-? It's my understanding that the only outstanding item is a test. If you have a problem with the code diff itself, please explain and I will withdraw this bug. "I've stopped calling mediaPlayerTimeChanged() unnecessarily, and the patch attached to this PR is no longer needed."
(In reply to comment #13): The fix is not needed to address the original synopsis of this bug: "HTML5 video controls are not automatically fading out when fullscreen". That is why I changed the synopsis to "mediaControls()->playbackStarted() may be called more than once". There is still an error in logic that was revealed by the original bug symptoms. Please let me know if you think this error in logic, which is fixed by my patch and was called "a good fix" by Jer Noble in comment #8, still needs to be fixed.
Hi Max, (In reply to comment #14) > (In reply to comment #13): > > The fix is not needed to address the original synopsis of this bug: "HTML5 video controls are not automatically fading out when fullscreen". That is why I changed the synopsis to "mediaControls()->playbackStarted() may be called more than once". > > There is still an error in logic that was revealed by the original bug symptoms. Please let me know if you think this error in logic, which is fixed by my patch and was called "a good fix" by Jer Noble in comment #8, still needs to be fixed. Do you feel the commit message is still accurate? Does a test make sense like Jer suggested? Cheers, Rob.
(In reply to comment #15) > Do you feel the commit message is still accurate? Does a test make sense like Jer suggested? I fully plan to update the patch with more accurate commit message and a test. But I do not plan to change the code itself.
Hi Max, (In reply to comment #16) > (In reply to comment #15) > > Do you feel the commit message is still accurate? Does a test make sense like Jer suggested? > > I fully plan to update the patch with more accurate commit message and a test. But I do not plan to change the code itself. Great! Please do that, I'll see it once it shows up in the queue and I can do a review then. Cheers, Rob.