Commit 8f6ba357 authored by Becca Hughes's avatar Becca Hughes Committed by Commit Bot

Media Engagement: Fix muted status propagation and timer bug.

The playbacks were not be recoreded correctly because muted state
was not propagating through to the WebContentsObserver correctly.
There was also a bug where the timer didn't have the time-delta
set correctly so it was never being called.

BUG=731750

Change-Id: I2944679adbb88b9f37c1a88efa97652ff032d9b1
Reviewed-on: https://chromium-review.googlesource.com/563668
Commit-Queue: Becca Hughes <beccahughes@chromium.org>
Reviewed-by: default avatarMounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485599}
parent c6395714
...@@ -8,8 +8,12 @@ ...@@ -8,8 +8,12 @@
#include "content/public/browser/navigation_handle.h" #include "content/public/browser/navigation_handle.h"
#include "content/public/browser/web_contents.h" #include "content/public/browser/web_contents.h"
constexpr base::TimeDelta namespace {
MediaEngagementContentsObserver::kSignificantMediaPlaybackTime;
constexpr base::TimeDelta kSignificantMediaPlaybackTime =
base::TimeDelta::FromSeconds(7);
} // namespace.
// This is the minimum size (in px) of each dimension that a media // This is the minimum size (in px) of each dimension that a media
// element has to be in order to be determined significant. // element has to be in order to be determined significant.
...@@ -86,8 +90,6 @@ MediaEngagementContentsObserver::GetPlayerState(const MediaPlayerId& id) { ...@@ -86,8 +90,6 @@ MediaEngagementContentsObserver::GetPlayerState(const MediaPlayerId& id) {
void MediaEngagementContentsObserver::MediaStartedPlaying( void MediaEngagementContentsObserver::MediaStartedPlaying(
const MediaPlayerInfo& media_player_info, const MediaPlayerInfo& media_player_info,
const MediaPlayerId& media_player_id) { const MediaPlayerId& media_player_id) {
// TODO(mlamouri): check if:
// - the playback has the minimum size requirements;
if (!media_player_info.has_audio) if (!media_player_info.has_audio)
return; return;
...@@ -96,12 +98,12 @@ void MediaEngagementContentsObserver::MediaStartedPlaying( ...@@ -96,12 +98,12 @@ void MediaEngagementContentsObserver::MediaStartedPlaying(
UpdateTimer(); UpdateTimer();
} }
void MediaEngagementContentsObserver::MediaMutedStateChanged( void MediaEngagementContentsObserver::MediaMutedStatusChanged(
const MediaPlayerId& id, const MediaPlayerId& id,
bool muted_state) { bool muted) {
GetPlayerState(id)->muted = muted_state; GetPlayerState(id)->muted = muted;
if (muted_state) if (muted)
MaybeRemoveSignificantPlayer(id); MaybeRemoveSignificantPlayer(id);
else else
MaybeInsertSignificantPlayer(id); MaybeInsertSignificantPlayer(id);
...@@ -188,6 +190,7 @@ void MediaEngagementContentsObserver::UpdateTimer() { ...@@ -188,6 +190,7 @@ void MediaEngagementContentsObserver::UpdateTimer() {
if (AreConditionsMet()) { if (AreConditionsMet()) {
if (playback_timer_->IsRunning()) if (playback_timer_->IsRunning())
return; return;
playback_timer_->Start( playback_timer_->Start(
FROM_HERE, kSignificantMediaPlaybackTime, FROM_HERE, kSignificantMediaPlaybackTime,
base::Bind( base::Bind(
......
...@@ -29,7 +29,7 @@ class MediaEngagementContentsObserver : public content::WebContentsObserver { ...@@ -29,7 +29,7 @@ class MediaEngagementContentsObserver : public content::WebContentsObserver {
void MediaStoppedPlaying(const MediaPlayerInfo& media_player_info, void MediaStoppedPlaying(const MediaPlayerInfo& media_player_info,
const MediaPlayerId& media_player_id) override; const MediaPlayerId& media_player_id) override;
void DidUpdateAudioMutingState(bool muted) override; void DidUpdateAudioMutingState(bool muted) override;
void MediaMutedStateChanged(const MediaPlayerId& id, bool muted_state); void MediaMutedStatusChanged(const MediaPlayerId& id, bool muted) override;
void MediaResized(const gfx::Size& size, const MediaPlayerId& id) override; void MediaResized(const gfx::Size& size, const MediaPlayerId& id) override;
static const int kSignificantSize; static const int kSignificantSize;
...@@ -39,9 +39,6 @@ class MediaEngagementContentsObserver : public content::WebContentsObserver { ...@@ -39,9 +39,6 @@ class MediaEngagementContentsObserver : public content::WebContentsObserver {
friend MediaEngagementService; friend MediaEngagementService;
friend MediaEngagementContentsObserverTest; friend MediaEngagementContentsObserverTest;
static constexpr base::TimeDelta kSignificantMediaPlaybackTime =
base::TimeDelta::FromSeconds(7);
MediaEngagementContentsObserver(content::WebContents* web_contents, MediaEngagementContentsObserver(content::WebContents* web_contents,
MediaEngagementService* service); MediaEngagementService* service);
......
...@@ -78,10 +78,10 @@ class MediaEngagementContentsObserverTest ...@@ -78,10 +78,10 @@ class MediaEngagementContentsObserverTest
contents_observer_->MediaStoppedPlaying(player_info, player_id); contents_observer_->MediaStoppedPlaying(player_info, player_id);
} }
void SimulateMutedStateChange(int id, bool muted_state) { void SimulateMutedStateChange(int id, bool muted) {
content::WebContentsObserver::MediaPlayerId player_id = content::WebContentsObserver::MediaPlayerId player_id =
std::make_pair(nullptr /* RenderFrameHost */, id); std::make_pair(nullptr /* RenderFrameHost */, id);
contents_observer_->MediaMutedStateChanged(player_id, muted_state); contents_observer_->MediaMutedStatusChanged(player_id, muted);
} }
void SimulateIsVisible() { contents_observer_->WasShown(); } void SimulateIsVisible() { contents_observer_->WasShown(); }
......
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