Commit 032986b1 authored by Ella Ge's avatar Ella Ge Committed by Commit Bot

Revert "Show controls and update time indicators during seeking"

This reverts commit f57001bc.

Reason for revert: suspect causing  media/controls-drag-timebar-rendering.html  failing on multiple builder

Original change's description:
> Show controls and update time indicators during seeking
> 
> This commit modifies the MediaControls to show the controls and update
> the current time indicators (current time text + timeline thumb)
> immediately during seek requests so the element behavior during rapid
> seek requests (e.g. user is holding down the FF button on their BT
> keyboard) look similar to when a user is scrubbing the timeline with
> their mouse.
> 
> Bug: 748749
> 
> TEST=MediaControlsImplTest, manual testing
>      out/Debug/bin/run_webkit_unit_tests -f *MediaControlsImplTest*
> 
> Change-Id: I6588aded2a93e081a6ca638ee1da534ea00abf4d
> Reviewed-on: https://chromium-review.googlesource.com/c/1274945
> Commit-Queue: Tommy Steimel <steimel@chromium.org>
> Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
> Reviewed-by: Tommy Steimel <steimel@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#636493}

TBR=mlamouri@chromium.org,steimel@chromium.org,agdoug@amazon.com

Change-Id: I37ebe9e972218afb164eead7c7f1142993bbb311
No-Presubmit: true
No-Try: true
Bug: 937029
Reviewed-on: https://chromium-review.googlesource.com/c/1495803Reviewed-by: default avatarElla Ge <eirage@chromium.org>
Commit-Queue: Ella Ge <eirage@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636571}
parent 17bd72be
...@@ -300,12 +300,8 @@ TEST_F(HTMLMediaElementWithMockSchedulerTest, OneTimeupdatePerSeek) { ...@@ -300,12 +300,8 @@ TEST_F(HTMLMediaElementWithMockSchedulerTest, OneTimeupdatePerSeek) {
EXPECT_CALL(*timeupdate_handler, Invoke(_, _)).Times(0); EXPECT_CALL(*timeupdate_handler, Invoke(_, _)).Times(0);
platform_->RunForPeriodSeconds(1); platform_->RunForPeriodSeconds(1);
EXPECT_CALL(*timeupdate_handler, Invoke(_, _)).Times(1); // Seek to some time in the past. A completed seek should trigger a *single*
Video()->pause(); // timeupdate.
platform_->RunUntilIdle();
// Seek to some time in the past. A completed seek while paused should trigger
// a *single* timeupdate.
EXPECT_CALL(*timeupdate_handler, Invoke(_, _)).Times(1); EXPECT_CALL(*timeupdate_handler, Invoke(_, _)).Times(1);
ASSERT_GE(WebMediaPlayer()->CurrentTime(), 1); ASSERT_GE(WebMediaPlayer()->CurrentTime(), 1);
Video()->setCurrentTime(WebMediaPlayer()->CurrentTime() - 1); Video()->setCurrentTime(WebMediaPlayer()->CurrentTime() - 1);
...@@ -358,14 +354,10 @@ TEST_F(HTMLMediaElementWithMockSchedulerTest, PeriodicTimeupdateAfterSeek) { ...@@ -358,14 +354,10 @@ TEST_F(HTMLMediaElementWithMockSchedulerTest, PeriodicTimeupdateAfterSeek) {
ASSERT_GE(WebMediaPlayer()->CurrentTime(), 1); ASSERT_GE(WebMediaPlayer()->CurrentTime(), 1);
Video()->setCurrentTime(WebMediaPlayer()->CurrentTime() - 1); Video()->setCurrentTime(WebMediaPlayer()->CurrentTime() - 1);
WebMediaPlayer()->FinishSeek(); WebMediaPlayer()->FinishSeek();
// Expect another timeupdate after FinishSeek due to
// seeking -> begin scrubbing -> pause -> timeupdate.
EXPECT_CALL(*timeupdate_handler, Invoke(_, _)).Times(1);
platform_->RunUntilIdle(); platform_->RunUntilIdle();
// Advancing the remainder of the last periodic timeupdate interval should be // Advancing the remainder of the last periodic timeupdate interval should be
// insufficient to trigger a new timeupdate event because the seek's // insufficient to triggger a new timeupdate event because the seek's
// timeupdate occurred only 125ms ago. We desire to fire periodic timeupdates // timeupdate occurred only 125ms ago. We desire to fire periodic timeupdates
// exactly every 250ms from the last timeupdate, and the seek's timeupdate // exactly every 250ms from the last timeupdate, and the seek's timeupdate
// should reset that 250ms ms countdown. // should reset that 250ms ms countdown.
......
...@@ -938,7 +938,9 @@ void MediaControlsImpl::Reset() { ...@@ -938,7 +938,9 @@ void MediaControlsImpl::Reset() {
UpdatePlayState(); UpdatePlayState();
UpdateTimeIndicators(); UpdateCurrentTimeDisplay();
timeline_->SetPosition(MediaElement().currentTime());
OnVolumeChange(); OnVolumeChange();
OnTextTracksAddedOrRemoved(); OnTextTracksAddedOrRemoved();
...@@ -953,11 +955,6 @@ void MediaControlsImpl::Reset() { ...@@ -953,11 +955,6 @@ void MediaControlsImpl::Reset() {
OnControlsListUpdated(); OnControlsListUpdated();
} }
void MediaControlsImpl::UpdateTimeIndicators() {
timeline_->SetPosition(MediaElement().currentTime());
UpdateCurrentTimeDisplay();
}
void MediaControlsImpl::OnControlsListUpdated() { void MediaControlsImpl::OnControlsListUpdated() {
BatchedControlUpdate batch(this); BatchedControlUpdate batch(this);
...@@ -1128,9 +1125,6 @@ bool MediaControlsImpl::ShouldHideMediaControls(unsigned behavior_flags) const { ...@@ -1128,9 +1125,6 @@ bool MediaControlsImpl::ShouldHideMediaControls(unsigned behavior_flags) const {
if (panel_->KeepDisplayedForAccessibility()) if (panel_->KeepDisplayedForAccessibility())
return false; return false;
if (MediaElement().seeking())
return false;
return true; return true;
} }
...@@ -1626,7 +1620,8 @@ void MediaControlsImpl::HandlePointerEvent(Event* event) { ...@@ -1626,7 +1620,8 @@ void MediaControlsImpl::HandlePointerEvent(Event* event) {
is_mouse_over_controls_ = true; is_mouse_over_controls_ = true;
if (!MediaElement().paused()) { if (!MediaElement().paused()) {
MakeOpaqueFromPointerEvent(); MakeOpaqueFromPointerEvent();
StartHideMediaControlsIfNecessary(); if (ShouldHideMediaControls())
StartHideMediaControlsTimer();
} }
} }
} else if (event->type() == event_type_names::kPointerout) { } else if (event->type() == event_type_names::kPointerout) {
...@@ -1809,11 +1804,6 @@ void MediaControlsImpl::HideMediaControlsTimerFired(TimerBase*) { ...@@ -1809,11 +1804,6 @@ void MediaControlsImpl::HideMediaControlsTimerFired(TimerBase*) {
overlay_cast_button_->SetIsWanted(false); overlay_cast_button_->SetIsWanted(false);
} }
void MediaControlsImpl::StartHideMediaControlsIfNecessary() {
if (ShouldHideMediaControls())
StartHideMediaControlsTimer();
}
void MediaControlsImpl::StartHideMediaControlsTimer() { void MediaControlsImpl::StartHideMediaControlsTimer() {
hide_media_controls_timer_.StartOneShot( hide_media_controls_timer_.StartOneShot(
GetTimeWithoutMouseMovementBeforeHidingMediaControls(), FROM_HERE); GetTimeWithoutMouseMovementBeforeHidingMediaControls(), FROM_HERE);
...@@ -1887,7 +1877,8 @@ void MediaControlsImpl::OnFocusIn() { ...@@ -1887,7 +1877,8 @@ void MediaControlsImpl::OnFocusIn() {
} }
void MediaControlsImpl::OnTimeUpdate() { void MediaControlsImpl::OnTimeUpdate() {
UpdateTimeIndicators(); timeline_->SetPosition(MediaElement().currentTime());
UpdateCurrentTimeDisplay();
// 'timeupdate' might be called in a paused state. The controls should not // 'timeupdate' might be called in a paused state. The controls should not
// become transparent in that case. // become transparent in that case.
...@@ -1929,7 +1920,8 @@ void MediaControlsImpl::OnDurationChange() { ...@@ -1929,7 +1920,8 @@ void MediaControlsImpl::OnDurationChange() {
void MediaControlsImpl::OnPlay() { void MediaControlsImpl::OnPlay() {
UpdatePlayState(); UpdatePlayState();
UpdateTimeIndicators(); timeline_->SetPosition(MediaElement().currentTime());
UpdateCurrentTimeDisplay();
UpdateCSSClassFromState(); UpdateCSSClassFromState();
} }
...@@ -1942,7 +1934,8 @@ void MediaControlsImpl::OnPlaying() { ...@@ -1942,7 +1934,8 @@ void MediaControlsImpl::OnPlaying() {
void MediaControlsImpl::OnPause() { void MediaControlsImpl::OnPause() {
UpdatePlayState(); UpdatePlayState();
UpdateTimeIndicators(); timeline_->SetPosition(MediaElement().currentTime());
UpdateCurrentTimeDisplay();
MakeOpaque(); MakeOpaque();
StopHideMediaControlsTimer(); StopHideMediaControlsTimer();
...@@ -1950,32 +1943,6 @@ void MediaControlsImpl::OnPause() { ...@@ -1950,32 +1943,6 @@ void MediaControlsImpl::OnPause() {
UpdateCSSClassFromState(); UpdateCSSClassFromState();
} }
void MediaControlsImpl::OnSeeking() {
UpdateTimeIndicators();
if (!is_scrubbing_) {
is_scrubbing_ = true;
UpdateCSSClassFromState();
}
// Don't try to show the controls if the seek was caused by the video being
// looped.
if (MediaElement().Loop() && MediaElement().currentTime() == 0)
return;
if (!MediaElement().ShouldShowControls())
return;
MaybeShow();
StopHideMediaControlsTimer();
}
void MediaControlsImpl::OnSeeked() {
StartHideMediaControlsIfNecessary();
is_scrubbing_ = false;
UpdateCSSClassFromState();
}
void MediaControlsImpl::OnTextTracksAddedOrRemoved() { void MediaControlsImpl::OnTextTracksAddedOrRemoved() {
toggle_closed_captions_button_->UpdateDisplayType(); toggle_closed_captions_button_->UpdateDisplayType();
toggle_closed_captions_button_->SetIsWanted( toggle_closed_captions_button_->SetIsWanted(
......
...@@ -282,9 +282,6 @@ class MODULES_EXPORT MediaControlsImpl final : public HTMLDivElement, ...@@ -282,9 +282,6 @@ class MODULES_EXPORT MediaControlsImpl final : public HTMLDivElement,
void ElementSizeChangedTimerFired(TimerBase*); void ElementSizeChangedTimerFired(TimerBase*);
// Update any visible indicators of the current time.
void UpdateTimeIndicators();
// Hide elements that don't fit, and show those things that we want which // Hide elements that don't fit, and show those things that we want which
// do fit. This requires that m_effectiveWidth and m_effectiveHeight are // do fit. This requires that m_effectiveWidth and m_effectiveHeight are
// current. // current.
...@@ -341,8 +338,6 @@ class MODULES_EXPORT MediaControlsImpl final : public HTMLDivElement, ...@@ -341,8 +338,6 @@ class MODULES_EXPORT MediaControlsImpl final : public HTMLDivElement,
void OnPlay(); void OnPlay();
void OnPlaying(); void OnPlaying();
void OnPause(); void OnPause();
void OnSeeking();
void OnSeeked();
void OnTextTracksAddedOrRemoved(); void OnTextTracksAddedOrRemoved();
void OnTextTracksChanged(); void OnTextTracksChanged();
void OnError(); void OnError();
......
...@@ -222,9 +222,6 @@ class MediaControlsImplTest : public PageTestBase, ...@@ -222,9 +222,6 @@ class MediaControlsImplTest : public PageTestBase,
void SimulateLoadedMetadata() { media_controls_->OnLoadedMetadata(); } void SimulateLoadedMetadata() { media_controls_->OnLoadedMetadata(); }
void SimulateOnSeeking() { media_controls_->OnSeeking(); }
void SimulateOnSeeked() { media_controls_->OnSeeked(); }
MediaControlsImpl& MediaControls() { return *media_controls_; } MediaControlsImpl& MediaControls() { return *media_controls_; }
MediaControlVolumeSliderElement* VolumeSliderElement() const { MediaControlVolumeSliderElement* VolumeSliderElement() const {
return media_controls_->volume_slider_; return media_controls_->volume_slider_;
...@@ -627,32 +624,6 @@ TEST_F(MediaControlsImplTest, TimelineImmediatelyUpdatesCurrentTime) { ...@@ -627,32 +624,6 @@ TEST_F(MediaControlsImplTest, TimelineImmediatelyUpdatesCurrentTime) {
EXPECT_EQ(duration / 2, current_time_display->CurrentValue()); EXPECT_EQ(duration / 2, current_time_display->CurrentValue());
} }
TEST_F(MediaControlsImplTest, TimeIndicatorsUpdatedOnSeeking) {
EnsureSizing();
MediaControlCurrentTimeDisplayElement* current_time_display =
GetCurrentTimeDisplayElement();
MediaControlTimelineElement* timeline = TimelineElement();
double duration = 1000;
LoadMediaWithDuration(duration);
EXPECT_EQ(0, current_time_display->CurrentValue());
EXPECT_EQ(0, timeline->valueAsNumber());
MediaControls().MediaElement().setCurrentTime(duration / 4);
// Time indicators are not yet updated.
EXPECT_EQ(0, current_time_display->CurrentValue());
EXPECT_EQ(0, timeline->valueAsNumber());
SimulateOnSeeking();
// The time indicators should be updated immediately when the 'seeking' event
// is fired.
EXPECT_EQ(duration / 4, current_time_display->CurrentValue());
EXPECT_EQ(duration / 4, timeline->valueAsNumber());
}
TEST_F(MediaControlsImplTest, TimelineMetricsWidth) { TEST_F(MediaControlsImplTest, TimelineMetricsWidth) {
MediaControls().MediaElement().SetSrc("https://example.com/foo.mp4"); MediaControls().MediaElement().SetSrc("https://example.com/foo.mp4");
test::RunPendingTasks(); test::RunPendingTasks();
...@@ -892,45 +863,6 @@ class MediaControlsImplTestWithMockScheduler ...@@ -892,45 +863,6 @@ class MediaControlsImplTestWithMockScheduler
} // namespace } // namespace
TEST_F(MediaControlsImplTestWithMockScheduler, SeekingShowsControls) {
Element* panel = GetElementByShadowPseudoId(MediaControls(),
"-webkit-media-controls-panel");
ASSERT_NE(nullptr, panel);
MediaControls().MediaElement().SetSrc("http://example.com");
MediaControls().MediaElement().Play();
// Hide the controls to start.
MediaControls().Hide();
EXPECT_FALSE(IsElementVisible(*panel));
// Seeking should cause the controls to become visible.
SimulateOnSeeking();
EXPECT_TRUE(IsElementVisible(*panel));
}
TEST_F(MediaControlsImplTestWithMockScheduler,
SeekingDoesNotShowControlsWhenNoControlsAttr) {
Element* panel = GetElementByShadowPseudoId(MediaControls(),
"-webkit-media-controls-panel");
ASSERT_NE(nullptr, panel);
MediaControls().MediaElement().SetBooleanAttribute(html_names::kControlsAttr,
false);
MediaControls().MediaElement().SetSrc("http://example.com");
MediaControls().MediaElement().Play();
// Hide the controls to start.
MediaControls().Hide();
EXPECT_FALSE(IsElementVisible(*panel));
// Seeking should not cause the controls to become visible because the
// controls attribute is not set.
SimulateOnSeeking();
EXPECT_FALSE(IsElementVisible(*panel));
}
TEST_F(MediaControlsImplTestWithMockScheduler, TEST_F(MediaControlsImplTestWithMockScheduler,
ControlsRemainVisibleDuringKeyboardInteraction) { ControlsRemainVisibleDuringKeyboardInteraction) {
EnsureSizing(); EnsureSizing();
......
...@@ -36,8 +36,6 @@ void MediaControlsMediaEventListener::Attach() { ...@@ -36,8 +36,6 @@ void MediaControlsMediaEventListener::Attach() {
GetMediaElement().addEventListener(event_type_names::kPause, this, false); GetMediaElement().addEventListener(event_type_names::kPause, this, false);
GetMediaElement().addEventListener(event_type_names::kDurationchange, this, GetMediaElement().addEventListener(event_type_names::kDurationchange, this,
false); false);
GetMediaElement().addEventListener(event_type_names::kSeeking, this, false);
GetMediaElement().addEventListener(event_type_names::kSeeked, this, false);
GetMediaElement().addEventListener(event_type_names::kError, this, false); GetMediaElement().addEventListener(event_type_names::kError, this, false);
GetMediaElement().addEventListener(event_type_names::kLoadedmetadata, this, GetMediaElement().addEventListener(event_type_names::kLoadedmetadata, this,
false); false);
...@@ -179,14 +177,6 @@ void MediaControlsMediaEventListener::Invoke( ...@@ -179,14 +177,6 @@ void MediaControlsMediaEventListener::Invoke(
media_controls_->OnPause(); media_controls_->OnPause();
return; return;
} }
if (event->type() == event_type_names::kSeeking) {
media_controls_->OnSeeking();
return;
}
if (event->type() == event_type_names::kSeeked) {
media_controls_->OnSeeked();
return;
}
if (event->type() == event_type_names::kError) { if (event->type() == event_type_names::kError) {
media_controls_->OnError(); media_controls_->OnError();
return; return;
......
...@@ -24,7 +24,7 @@ ...@@ -24,7 +24,7 @@
"drawsContent": false "drawsContent": false
}, },
{ {
"name": "LayoutFlexibleBox (relative positioned) DIV class='sizing-small test-mode phase-ready state-scrubbing'", "name": "LayoutFlexibleBox (relative positioned) DIV class='sizing-small test-mode phase-ready state-stopped'",
"position": [8, 8], "position": [8, 8],
"bounds": [320, 240] "bounds": [320, 240]
}, },
......
...@@ -76,7 +76,7 @@ Case: Displaying movie ...@@ -76,7 +76,7 @@ Case: Displaying movie
"drawsContent": false "drawsContent": false
}, },
{ {
"name": "LayoutFlexibleBox (relative positioned) DIV class='sizing-small phase-ready state-scrubbing'", "name": "LayoutFlexibleBox (relative positioned) DIV class='sizing-small phase-ready state-stopped'",
"position": [8, 8], "position": [8, 8],
"bounds": [352, 288] "bounds": [352, 288]
}, },
......
...@@ -76,7 +76,7 @@ Case: Displaying movie ...@@ -76,7 +76,7 @@ Case: Displaying movie
"drawsContent": false "drawsContent": false
}, },
{ {
"name": "LayoutFlexibleBox (relative positioned) DIV class='sizing-small phase-ready state-scrubbing'", "name": "LayoutFlexibleBox (relative positioned) DIV class='sizing-small phase-ready state-stopped'",
"position": [8, 8], "position": [8, 8],
"bounds": [352, 288] "bounds": [352, 288]
}, },
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment