Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

Commit

Permalink
Remove clientWidth call during MediaControls::reset.
Browse files Browse the repository at this point in the history
This is a merge to M48 of f003b7d .

Avoid calling clientWidth early to get the control panel width.
It can cause a crash when the HitTestCache is cleared, if the
cache was the last holder to the media element.  Normally, this
can't happen, but a callback from the WebMediaPlayer can be
scheduled between the cache becoming the last reference and the
next layout.

BUG=531259
NOTRY=true
NOPRESUBMIT=true

Review URL: https://codereview.chromium.org/1578853002

Cr-Commit-Position: refs/branch-heads/2564@{#542}
Cr-Branched-From: 1283eca-refs/heads/master@{#359700}
  • Loading branch information
liberato-at-chromium authored and Commit bot committed Jan 12, 2016
1 parent eb4903f commit 5f2f77f
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 11 deletions.
22 changes: 12 additions & 10 deletions third_party/WebKit/Source/core/html/shadow/MediaControls.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -260,13 +260,6 @@ void MediaControls::reset()
m_fullScreenButton->setIsWanted(shouldShowFullscreenButton(mediaElement()));

refreshCastButtonVisibilityWithoutUpdate();

// Set the panel width here, and force a layout, before the controls update.
// This would be harmless for the !useNewUi case too, but it causes
// compositing/geometry/video-fixed-scrolling.html to fail with two extra
// 0 height nodes in the render tree.
if (useNewUi)
m_panelWidth = m_panel->clientWidth();
}

LayoutObject* MediaControls::layoutObjectForTextTrackLayout()
Expand Down Expand Up @@ -652,9 +645,6 @@ void MediaControls::computeWhichControlsFit()
if (!RuntimeEnabledFeatures::newMediaPlaybackUiEnabled())
return;

if (!m_panelWidth)
return;

// Controls that we'll hide / show, in order of decreasing priority.
MediaControlElement* elements[] = {
m_playButton.get(),
Expand All @@ -668,6 +658,18 @@ void MediaControls::computeWhichControlsFit()
m_durationDisplay.get(),
};

if (!m_panelWidth) {
// No layout yet -- hide everything, then make them show up later.
// This prevents the wrong controls from being shown briefly
// immediately after the first layout and paint, but before we have
// a chance to revise them.
for (MediaControlElement* element : elements) {
if (element)
element->setDoesFit(false);
}
return;
}

int usedWidth = 0;
bool droppedCastButton = false;
// Assume that all controls require 48px. Ideally, we could get this
Expand Down
14 changes: 13 additions & 1 deletion third_party/WebKit/Source/core/html/shadow/MediaControlsTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,14 @@ class MediaControlsTest : public testing::Test {
virtual void SetUp()
{
m_pageHolder = DummyPageHolder::create(IntSize(800, 600));
Document& document = m_pageHolder->document();
Document& document = this->document();
document.write("<video controls>");
HTMLVideoElement& video = toHTMLVideoElement(*document.querySelector("video", ASSERT_NO_EXCEPTION));
m_mediaControls = video.mediaControls();
}

MediaControls& mediaControls() { return *m_mediaControls; }
Document& document() { return m_pageHolder->document(); }

private:
OwnPtr<DummyPageHolder> m_pageHolder;
Expand Down Expand Up @@ -103,4 +104,15 @@ TEST_F(MediaControlsTest, HideAndReset)
ASSERT_FALSE(isElementVisible(*panel));
}

TEST_F(MediaControlsTest, ResetDoesNotTriggerInitialLayout)
{
Document& document = this->document();
int oldResolverCount = document.styleEngine().resolverAccessCount();
// Also assert that there are no layouts yet.
ASSERT_EQ(0, oldResolverCount);
mediaControls().reset();
int newResolverCount = document.styleEngine().resolverAccessCount();
ASSERT_EQ(oldResolverCount, newResolverCount);
}

} // namespace blink

0 comments on commit 5f2f77f

Please sign in to comment.