diff --git a/chrome/browser/BUILD.gn b/chrome/browser/BUILD.gn index a8d16f4719..5cd3211 100644 --- a/chrome/browser/BUILD.gn +++ b/chrome/browser/BUILD.gn
@@ -627,6 +627,8 @@ "media/media_engagement_service.h", "media/media_engagement_service_factory.cc", "media/media_engagement_service_factory.h", + "media/media_engagement_session.cc", + "media/media_engagement_session.h", "media/media_storage_id_salt.cc", "media/media_storage_id_salt.h", "media/media_url_constants.cc",
diff --git a/chrome/browser/media/media_engagement_browsertest.cc b/chrome/browser/media/media_engagement_browsertest.cc index 2a8dc47..8db8b474 100644 --- a/chrome/browser/media/media_engagement_browsertest.cc +++ b/chrome/browser/media/media_engagement_browsertest.cc
@@ -92,27 +92,52 @@ injected_clock_ = false; }; - void LoadTestPage(const std::string& page) { + void LoadTestPage(const GURL& url) { // We can't do this in SetUp as the browser isn't ready yet and we // need it before the page navigates. InjectTimerTaskRunner(); - ui_test_utils::NavigateToURL(browser(), http_server_.GetURL("/" + page)); + ui_test_utils::NavigateToURL(browser(), url); }; - void LoadTestPageAndWaitForPlay(const std::string& page, - bool web_contents_muted) { - LoadTestPage(page); + void LoadTestPageAndWaitForPlay(const GURL& url, bool web_contents_muted) { + LoadTestPage(url); GetWebContents()->SetAudioMuted(web_contents_muted); WaitForPlay(); } + // TODO(beccahughes,mlamouri): update this to use GURL. void LoadTestPageAndWaitForPlayAndAudible(const std::string& page, bool web_contents_muted) { - LoadTestPageAndWaitForPlay(page, web_contents_muted); + LoadTestPageAndWaitForPlayAndAudible(http_server_.GetURL("/" + page), + web_contents_muted); + }; + + void LoadTestPageAndWaitForPlayAndAudible(const GURL& url, + bool web_contents_muted) { + LoadTestPageAndWaitForPlay(url, web_contents_muted); WaitForWasRecentlyAudible(); }; + void OpenTab(const GURL& url) { + chrome::NavigateParams params(browser(), url, ui::PAGE_TRANSITION_LINK); + params.disposition = WindowOpenDisposition::NEW_FOREGROUND_TAB; + // params.opener does not need to be set in the context of this test because + // it will use the current tab by default. + chrome::Navigate(¶ms); + + InjectTimerTaskRunner(); + params.target_contents->SetAudioMuted(false); + content::WaitForLoadStop(params.target_contents); + } + + void OpenTabAndwaitForPlayAndAudible(const GURL& url) { + OpenTab(url); + + WaitForPlay(); + WaitForWasRecentlyAudible(); + } + void Advance(base::TimeDelta time) { DCHECK(injected_clock_); task_runner_->FastForwardBy(time); @@ -129,7 +154,7 @@ int media_playbacks, int audible_playbacks, int significant_playbacks) { - ExpectScores(http_server_.GetURL("/"), visits, media_playbacks, + ExpectScores(http_server_.base_url(), visits, media_playbacks, audible_playbacks, significant_playbacks); } @@ -137,10 +162,22 @@ int media_playbacks, int audible_playbacks, int significant_playbacks) { - ExpectScores(http_server_origin2_.GetURL("/"), visits, media_playbacks, + ExpectScores(http_server_origin2_.base_url(), visits, media_playbacks, audible_playbacks, significant_playbacks); } + void ExpectScores(GURL url, + int visits, + int media_playbacks, + int audible_playbacks, + int significant_playbacks) { + MediaEngagementScore score = GetService()->CreateEngagementScore(url); + EXPECT_EQ(visits, score.visits()); + EXPECT_EQ(media_playbacks, score.media_playbacks()); + EXPECT_EQ(audible_playbacks, score.audible_playbacks()); + EXPECT_EQ(significant_playbacks, score.significant_playbacks()); + } + content::WebContents* GetWebContents() { return browser()->tab_strip_model()->GetActiveWebContents(); } @@ -160,10 +197,8 @@ EXPECT_TRUE(browser()->tab_strip_model()->CloseWebContentsAt(0, 0)); } - void LoadSubFrame(const std::string& page) { - ExecuteScript("window.open(\"" + - http_server_origin2_.GetURL("/" + page).spec() + - "\", \"subframe\")"); + void LoadSubFrame(const GURL& url) { + ExecuteScript("window.open(\"" + url.spec() + "\", \"subframe\")"); } void WaitForPlay() { @@ -190,40 +225,29 @@ ui_test_utils::NavigateToURL(browser(), http_server_origin2_.GetURL("/")); } + const net::EmbeddedTestServer& http_server() const { return http_server_; } + + const net::EmbeddedTestServer& http_server_origin2() const { + return http_server_origin2_; + } + void CloseBrowser() { CloseAllBrowsers(); } private: - void ExpectScores(GURL url, - int visits, - int media_playbacks, - int audible_playbacks, - int significant_playbacks) { - MediaEngagementScore score = GetService()->CreateEngagementScore(url); - EXPECT_EQ(visits, score.visits()); - EXPECT_EQ(media_playbacks, score.media_playbacks()); - EXPECT_EQ(audible_playbacks, score.audible_playbacks()); - EXPECT_EQ(significant_playbacks, score.significant_playbacks()); - } - void InjectTimerTaskRunner() { if (!injected_clock_) { GetService()->clock_ = std::move(test_clock_); injected_clock_ = true; } - contents_observer()->SetTaskRunnerForTest(task_runner_); + for (auto observer : GetService()->contents_observers_) + observer.second->SetTaskRunnerForTest(task_runner_); } MediaEngagementService* GetService() { return MediaEngagementService::Get(browser()->profile()); } - MediaEngagementContentsObserver* contents_observer() { - MediaEngagementService* mei_service = GetService(); - DCHECK(mei_service->contents_observers_.size() == 1); - return *mei_service->contents_observers_.begin(); - } - bool injected_clock_ = false; std::unique_ptr<base::SimpleTestClock> test_clock_; @@ -289,7 +313,8 @@ IN_PROC_BROWSER_TEST_F(MediaEngagementBrowserTest, DoNotRecordEngagement_PlayerMuted) { - LoadTestPageAndWaitForPlay("engagement_test_muted.html", false); + LoadTestPageAndWaitForPlay( + http_server().GetURL("/engagement_test_muted.html"), false); AdvanceMeaningfulPlaybackTime(); CloseTab(); ExpectScores(1, 0, 0, 0); @@ -297,7 +322,8 @@ IN_PROC_BROWSER_TEST_F(MediaEngagementBrowserTest, DoNotRecordEngagement_PlayerMuted_AudioOnly) { - LoadTestPageAndWaitForPlay("engagement_test_audio_muted.html", false); + LoadTestPageAndWaitForPlay( + http_server().GetURL("/engagement_test_muted.html"), false); AdvanceMeaningfulPlaybackTime(); CloseTab(); ExpectScores(1, 0, 0, 0); @@ -352,7 +378,8 @@ IN_PROC_BROWSER_TEST_F(MediaEngagementBrowserTest, DoNotRecordEngagement_NoAudioTrack) { - LoadTestPageAndWaitForPlay("engagement_test_no_audio_track.html", false); + LoadTestPageAndWaitForPlay( + http_server().GetURL("/engagement_test_no_audio_track.html"), false); AdvanceMeaningfulPlaybackTime(); CloseTab(); ExpectScores(1, 0, 0, 0); @@ -360,7 +387,8 @@ IN_PROC_BROWSER_TEST_F(MediaEngagementBrowserTest, DoNotRecordEngagement_SilentAudioTrack) { - LoadTestPageAndWaitForPlay("engagement_test_silent_audio_track.html", false); + LoadTestPageAndWaitForPlay( + http_server().GetURL("/engagement_test_silent_audio_track.html"), false); AdvanceMeaningfulPlaybackTime(); CloseTab(); ExpectScores(1, 0, 1, 0); @@ -400,16 +428,18 @@ IN_PROC_BROWSER_TEST_F(MediaEngagementBrowserTest, DoNotRecordEngagement_SilentAudioTrack_AudioOnly) { - LoadTestPageAndWaitForPlay("engagement_test_silent_audio_track_audio.html", - false); + LoadTestPageAndWaitForPlay( + http_server().GetURL("/engagement_test_silent_audio_track_audio.html"), + false); AdvanceMeaningfulPlaybackTime(); CloseTab(); ExpectScores(1, 0, 1, 0); } IN_PROC_BROWSER_TEST_F(MediaEngagementBrowserTest, IFrameDelegation) { - LoadTestPage("engagement_test_iframe.html"); - LoadSubFrame("engagement_test_iframe_child.html"); + LoadTestPage(http_server().GetURL("/engagement_test_iframe.html")); + LoadSubFrame( + http_server_origin2().GetURL("/engagement_test_iframe_child.html")); WaitForPlay(); WaitForWasRecentlyAudible(); @@ -421,8 +451,9 @@ } IN_PROC_BROWSER_TEST_F(MediaEngagementBrowserTest, IFrameDelegation_AudioOnly) { - LoadTestPage("engagement_test_iframe.html"); - LoadSubFrame("engagement_test_iframe_audio_child.html"); + LoadTestPage(http_server().GetURL("/engagement_test_iframe.html")); + LoadSubFrame( + http_server_origin2().GetURL("/engagement_test_iframe_audio_child.html")); WaitForPlay(); WaitForWasRecentlyAudible(); @@ -456,3 +487,86 @@ CloseTab(); ExpectScores(1, 0, 1, 0); } + +IN_PROC_BROWSER_TEST_F(MediaEngagementBrowserTest, + SessionNewTabNavigateSameURL) { + const GURL& url = http_server().GetURL("/engagement_test.html"); + + LoadTestPageAndWaitForPlayAndAudible(url, false); + AdvanceMeaningfulPlaybackTime(); + + OpenTab(GURL("about:blank")); + LoadTestPageAndWaitForPlayAndAudible(url, false); + AdvanceMeaningfulPlaybackTime(); + + browser()->tab_strip_model()->CloseAllTabs(); + + ExpectScores(2, 2, 2, 2); +} + +IN_PROC_BROWSER_TEST_F(MediaEngagementBrowserTest, SessionNewTabSameURL) { + const GURL& url = http_server().GetURL("/engagement_test.html"); + + LoadTestPageAndWaitForPlayAndAudible(url, false); + AdvanceMeaningfulPlaybackTime(); + + OpenTabAndwaitForPlayAndAudible(url); + AdvanceMeaningfulPlaybackTime(); + + browser()->tab_strip_model()->CloseAllTabs(); + + ExpectScores(1, 1, 2, 2); +} + +IN_PROC_BROWSER_TEST_F(MediaEngagementBrowserTest, SessionNewTabSameOrigin) { + const GURL& url = http_server().GetURL("/engagement_test.html"); + const GURL& other_url = http_server().GetURL("/engagement_test_audio.html"); + + LoadTestPageAndWaitForPlayAndAudible(url, false); + AdvanceMeaningfulPlaybackTime(); + + OpenTabAndwaitForPlayAndAudible(other_url); + AdvanceMeaningfulPlaybackTime(); + + browser()->tab_strip_model()->CloseAllTabs(); + + ExpectScores(1, 1, 2, 2); +} + +IN_PROC_BROWSER_TEST_F(MediaEngagementBrowserTest, SessionNewTabCrossOrigin) { + const GURL& url = http_server().GetURL("/engagement_test.html"); + const GURL& other_url = http_server_origin2().GetURL("/engagement_test.html"); + + LoadTestPageAndWaitForPlayAndAudible(url, false); + AdvanceMeaningfulPlaybackTime(); + + OpenTabAndwaitForPlayAndAudible(other_url); + AdvanceMeaningfulPlaybackTime(); + + browser()->tab_strip_model()->CloseAllTabs(); + + ExpectScores(http_server().base_url(), 1, 1, 1, 1); + ExpectScores(http_server_origin2().base_url(), 1, 1, 1, 1); +} + +IN_PROC_BROWSER_TEST_F(MediaEngagementBrowserTest, + SessionMultipleTabsClosingParent) { + const GURL& url = http_server().GetURL("/engagement_test.html"); + const GURL& other_url = http_server().GetURL("/engagement_test_audio.html"); + + LoadTestPageAndWaitForPlayAndAudible(url, false); + AdvanceMeaningfulPlaybackTime(); + + OpenTabAndwaitForPlayAndAudible(other_url); + AdvanceMeaningfulPlaybackTime(); + + CloseTab(); + ASSERT_EQ(other_url, GetWebContents()->GetLastCommittedURL()); + + OpenTabAndwaitForPlayAndAudible(url); + AdvanceMeaningfulPlaybackTime(); + + browser()->tab_strip_model()->CloseAllTabs(); + + ExpectScores(1, 1, 3, 3); +}
diff --git a/chrome/browser/media/media_engagement_contents_observer.cc b/chrome/browser/media/media_engagement_contents_observer.cc index da8beb2..0f974b4 100644 --- a/chrome/browser/media/media_engagement_contents_observer.cc +++ b/chrome/browser/media/media_engagement_contents_observer.cc
@@ -6,21 +6,23 @@ #include "base/metrics/histogram.h" #include "base/metrics/histogram_macros.h" +#include "build/build_config.h" #include "chrome/browser/media/media_engagement_service.h" +#include "chrome/browser/media/media_engagement_session.h" +#include "chrome/browser/profiles/profile.h" #include "content/public/browser/navigation_handle.h" #include "content/public/browser/render_frame_host.h" #include "content/public/browser/web_contents.h" -#include "services/metrics/public/cpp/ukm_builders.h" -#include "services/metrics/public/cpp/ukm_entry_builder.h" -#include "services/metrics/public/cpp/ukm_recorder.h" #include "third_party/WebKit/common/associated_interfaces/associated_interface_provider.h" #include "third_party/WebKit/public/platform/media_engagement.mojom.h" -namespace { +#if !defined(OS_ANDROID) +#include "chrome/browser/ui/browser.h" +#include "chrome/browser/ui/browser_list.h" +#include "chrome/browser/ui/tabs/tab_strip_model.h" +#endif // !defined(OS_ANDROID) -int ConvertScoreToPercentage(double score) { - return round(score * 100); -} +namespace { void SendEngagementLevelToFrame(const url::Origin& origin, content::RenderFrameHost* render_frame_host) { @@ -103,13 +105,11 @@ } void MediaEngagementContentsObserver::WebContentsDestroyed() { - StashAudiblePlayers(); - - // Commit a visit if we have not had a playback. - MaybeCommitPendingData(kVisitEnd); + RegisterAudiblePlayersWithSession(); + session_ = nullptr; ClearPlayerStates(); - service_->contents_observers_.erase(this); + service_->contents_observers_.erase(web_contents()); delete this; } @@ -119,141 +119,30 @@ significant_players_.clear(); } -void MediaEngagementContentsObserver::RecordUkmMetrics( - int audible_players_count, - int significant_players_count) { - ukm::UkmRecorder* recorder = GetUkmRecorder(); - if (!recorder) +void MediaEngagementContentsObserver::RegisterAudiblePlayersWithSession() { + if (!session_) return; - MediaEngagementScore score = - service_->CreateEngagementScore(committed_origin_.GetURL()); - ukm::builders::Media_Engagement_SessionFinished(ukm_source_id_) - .SetPlaybacks_Total(score.media_playbacks()) - .SetVisits_Total(score.visits()) - .SetEngagement_Score(ConvertScoreToPercentage(score.actual_score())) - .SetPlaybacks_Delta(significant_playback_recorded_) - .SetEngagement_IsHigh(score.high_score()) - .SetPlayer_Audible_Delta(audible_players_count) - .SetPlayer_Audible_Total(score.audible_playbacks()) - .SetPlayer_Significant_Delta(significant_players_count) - .SetPlayer_Significant_Total(score.significant_playbacks()) - .SetPlaybacks_SecondsSinceLast(time_since_playback_for_ukm_.InSeconds()) - .Record(recorder); - - time_since_playback_for_ukm_ = base::TimeDelta(); -} - -MediaEngagementContentsObserver::PendingCommitState& -MediaEngagementContentsObserver::GetPendingCommitState() { - if (!pending_data_to_commit_.has_value()) - pending_data_to_commit_ = PendingCommitState(); - return pending_data_to_commit_.value(); -} - -void MediaEngagementContentsObserver::RecordUkmIgnoredEvent(int length_msec) { - ukm::UkmRecorder* recorder = GetUkmRecorder(); - if (!recorder) - return; - - ukm::builders::Media_Engagement_ShortPlaybackIgnored(ukm_source_id_) - .SetLength(length_msec) - .Record(recorder); -} - -ukm::UkmRecorder* MediaEngagementContentsObserver::GetUkmRecorder() { - GURL url = committed_origin_.GetURL(); - if (!service_->ShouldRecordEngagement(url)) - return nullptr; - - ukm::UkmRecorder* ukm_recorder = ukm::UkmRecorder::Get(); - if (!ukm_recorder) - return nullptr; - - if (ukm_source_id_ == ukm::kInvalidSourceId) { - // TODO(beccahughes): Get from web contents. - ukm_source_id_ = ukm_recorder->GetNewSourceID(); - ukm_recorder->UpdateSourceURL(ukm_source_id_, url); - } - - return ukm_recorder; -} - -void MediaEngagementContentsObserver::StashAudiblePlayers() { - PendingCommitState& state = GetPendingCommitState(); + int32_t significant_players = 0; + int32_t audible_players = 0; for (const auto& row : audible_players_) { const PlayerState& player_state = GetPlayerState(row.first); const base::TimeDelta elapsed = player_state.playback_timer->Elapsed(); if (elapsed < kMaxShortPlaybackTime && player_state.reached_end_of_stream) { - RecordUkmIgnoredEvent(elapsed.InMilliseconds()); + session_->RecordShortPlaybackIgnored(elapsed.InMilliseconds()); continue; } - state.significant_players += row.second.first; - state.audible_players++; + significant_players += row.second.first; + ++audible_players; } + session_->RegisterAudiblePlayers(audible_players, significant_players); audible_players_.clear(); } -void MediaEngagementContentsObserver::MaybeCommitPendingData( - CommitTrigger trigger) { - // If the visit is over, make sure we have stashed all audible player data. - DCHECK(trigger != kVisitEnd || audible_players_.empty()); - - // If we don't have anything to commit then we can stop. - if (!pending_data_to_commit_.has_value()) { - // If we do not have anything to commit we should still record to UKM. - if (trigger == kVisitEnd) - RecordUkmMetrics(0, 0); - return; - } - PendingCommitState& state = pending_data_to_commit_.value(); - - // If the current origin is not a valid URL then we should just silently reset - // any pending data. - if (!service_->ShouldRecordEngagement(committed_origin_.GetURL())) { - pending_data_to_commit_.reset(); - audible_players_.clear(); - return; - } - - MediaEngagementScore score = - service_->CreateEngagementScore(committed_origin_.GetURL()); - - if (state.visit) - score.IncrementVisits(); - - if (state.media_playback) { - // Media playbacks trigger a commit so we should only increment media - // playbacks if a significant media playback has occured. - DCHECK_EQ(trigger, kSignificantMediaPlayback); - const base::Time old_time = score.last_media_playback_time(); - score.IncrementMediaPlaybacks(); - - if (!old_time.is_null()) { - // Calculate the time since the last playback and the first significant - // playback this visit. If there is no last playback time then we will - // record 0. - time_since_playback_for_ukm_ = - score.last_media_playback_time() - old_time; - } - } - - score.IncrementAudiblePlaybacks(state.audible_players); - score.IncrementSignificantPlaybacks(state.significant_players); - - score.Commit(); - - pending_data_to_commit_.reset(); - - // If the commit trigger was the end then we should record UKM metrics. - if (trigger == kVisitEnd) - RecordUkmMetrics(state.audible_players, state.significant_players); -} - void MediaEngagementContentsObserver::DidFinishNavigation( content::NavigationHandle* navigation_handle) { if (!navigation_handle->IsInMainFrame() || @@ -262,27 +151,14 @@ return; } - StashAudiblePlayers(); + RegisterAudiblePlayersWithSession(); ClearPlayerStates(); url::Origin new_origin = url::Origin::Create(navigation_handle->GetURL()); - if (committed_origin_.IsSameOriginWith(new_origin)) + if (session_ && session_->IsSameOriginWith(new_origin)) return; - // Commit a visit if we have not had a playback before the new origin is - // updated. - MaybeCommitPendingData(kVisitEnd); - - committed_origin_ = new_origin; - significant_playback_recorded_ = false; - ukm_source_id_ = ukm::kInvalidSourceId; - - // As any pending data would have been committed above, we should have no - // pending data and we should create a PendingData object. A visit will be - // automatically recorded if the PendingData object is present when - // MaybeCommitPendingData is called. - DCHECK(!pending_data_to_commit_.has_value()); - GetPendingCommitState().visit = true; + session_ = GetOrCreateSession(new_origin, GetOpener()); } MediaEngagementContentsObserver::PlayerState::PlayerState(base::Clock* clock) @@ -326,17 +202,18 @@ void MediaEngagementContentsObserver:: RecordEngagementScoreToHistogramAtPlayback(const MediaPlayerId& id) { - GURL url = committed_origin_.GetURL(); - if (!service_->ShouldRecordEngagement(url)) + if (!session_) return; PlayerState& state = GetPlayerState(id); if (!state.playing.value_or(false) || state.muted.value_or(true) || !state.has_audio.value_or(false) || !state.has_video.value_or(false) || - state.score_recorded) + state.score_recorded) { return; + } - int percentage = round(service_->GetEngagementScore(url) * 100); + int percentage = + round(service_->GetEngagementScore(session_->origin().GetURL()) * 100); UMA_HISTOGRAM_PERCENTAGE( MediaEngagementContentsObserver::kHistogramScoreAtPlaybackName, percentage); @@ -434,20 +311,17 @@ } void MediaEngagementContentsObserver::OnSignificantMediaPlaybackTimeForPage() { - DCHECK(!significant_playback_recorded_); + DCHECK(session_); + + if (session_->significant_playback_recorded()) + return; // Do not record significant playback if the tab did not make // a sound in the last two seconds. if (!web_contents()->WasRecentlyAudible()) return; - significant_playback_recorded_ = true; - - // A playback always comes after a visit so the visit should always be pending - // to commit. - DCHECK(pending_data_to_commit_.has_value()); - pending_data_to_commit_->media_playback = true; - MaybeCommitPendingData(kSignificantMediaPlayback); + session_->RecordSignificantPlayback(); } void MediaEngagementContentsObserver::RecordInsignificantReasons( @@ -584,7 +458,7 @@ } void MediaEngagementContentsObserver::UpdatePageTimer() { - if (significant_playback_recorded_) + if (!session_ || session_->significant_playback_recorded()) return; if (AreConditionsMet()) { @@ -621,3 +495,46 @@ handle->GetRenderFrameHost()); } } + +content::WebContents* MediaEngagementContentsObserver::GetOpener() const { +#if !defined(OS_ANDROID) + for (auto* browser : *BrowserList::GetInstance()) { + if (!browser->profile()->IsSameProfile(service_->profile()) || + browser->profile()->GetProfileType() != + service_->profile()->GetProfileType()) { + continue; + } + + int index = + browser->tab_strip_model()->GetIndexOfWebContents(web_contents()); + if (index == TabStripModel::kNoTab) + continue; + // Whether or not the |opener| is null, this is the right tab strip. + return browser->tab_strip_model()->GetOpenerOfWebContentsAt(index); + } +#endif // !defined(OS_ANDROID) + + return nullptr; +} + +scoped_refptr<MediaEngagementSession> +MediaEngagementContentsObserver::GetOrCreateSession( + const url::Origin& origin, + content::WebContents* opener) const { + GURL url = origin.GetURL(); + if (!url.is_valid()) + return nullptr; + + if (!service_->ShouldRecordEngagement(url)) + return nullptr; + + MediaEngagementContentsObserver* opener_observer = + service_->GetContentsObserverFor(opener); + + if (opener_observer && opener_observer->session_ && + opener_observer->session_->IsSameOriginWith(origin)) { + return opener_observer->session_; + } + + return new MediaEngagementSession(service_, origin); +}
diff --git a/chrome/browser/media/media_engagement_contents_observer.h b/chrome/browser/media/media_engagement_contents_observer.h index 46132a2..2e4de09 100644 --- a/chrome/browser/media/media_engagement_contents_observer.h +++ b/chrome/browser/media/media_engagement_contents_observer.h
@@ -6,7 +6,6 @@ #define CHROME_BROWSER_MEDIA_MEDIA_ENGAGEMENT_CONTENTS_OBSERVER_H_ #include "content/public/browser/web_contents_observer.h" -#include "services/metrics/public/cpp/ukm_source_id.h" namespace base { class Clock; @@ -16,12 +15,9 @@ class Size; } // namespace gfx -namespace ukm { -class UkmRecorder; -} // namespace ukm - class MediaEngagementContentsObserverTest; class MediaEngagementService; +class MediaEngagementSession; class MediaEngagementContentsObserver : public content::WebContentsObserver { public: @@ -193,50 +189,29 @@ const MediaPlayerId& id, MediaEngagementContentsObserver::InsignificantHistogram histogram); - // Commits any pending data to website settings. CommitTrigger is the event - // that is triggering the commit. - enum CommitTrigger { - // A significant media playback has occured. - kSignificantMediaPlayback, - - // A visit has ended either by navigating off the origin or when the - // observer is destroyed. - kVisitEnd, - }; - void MaybeCommitPendingData(CommitTrigger); - static const char* const kHistogramSignificantNotAddedAfterFirstTimeName; static const char* const kHistogramSignificantNotAddedFirstTimeName; static const char* const kHistogramSignificantRemovedName; static const int kMaxInsignificantPlaybackReason; - // Record the score and change in score to UKM. - void RecordUkmMetrics(int audible_players_count, - int significant_players_count); - - // Record the length of an ignored media playback. - void RecordUkmIgnoredEvent(int length_msec); - ukm::UkmRecorder* GetUkmRecorder(); - - bool significant_playback_recorded_ = false; + static const base::TimeDelta kSignificantMediaPlaybackTime; // Records the engagement score for the current origin to a histogram so we // can identify whether the playback would have been blocked. void RecordEngagementScoreToHistogramAtPlayback(const MediaPlayerId& id); - void StashAudiblePlayers(); + // Clears out players that are ignored because they are too short and register + // the result as significant/audible players with the `session_`. + void RegisterAudiblePlayersWithSession(); - // Stores pending media engagement data that needs to be committed either - // after a navigation to another domain, when the observer is destroyed or - // when we have had a media playback. - struct PendingCommitState { - bool visit = false; - bool media_playback = false; - int audible_players = 0; - int significant_players = 0; - }; - PendingCommitState& GetPendingCommitState(); - base::Optional<PendingCommitState> pending_data_to_commit_; + // Returns the opener of the current WebContents. Null if there is none. + content::WebContents* GetOpener() const; + + // Find the appropriate media engagement session if any or create a new one to + // be used. Will return nullptr if no session should be used. + scoped_refptr<MediaEngagementSession> GetOrCreateSession( + const url::Origin& origin, + content::WebContents* opener) const; // Stores the ids of the players that were audible. The boolean will be true // if the player was significant. @@ -246,15 +221,10 @@ // The task runner to use when creating timers. It is used only for testing. scoped_refptr<base::SequencedTaskRunner> task_runner_; - // The current UKM source id for |committed_origin_|. - ukm::SourceId ukm_source_id_ = ukm::kInvalidSourceId; - - // The time between significant playbacks to be recorded to UKM. - base::TimeDelta time_since_playback_for_ukm_; - - url::Origin committed_origin_; - - static const base::TimeDelta kSignificantMediaPlaybackTime; + // The MediaEngagementSession used by this MediaEngagementContentsObserver. It + // may be shared by other instances if they are part of the same session. It + // willl be null if it is not part of a session. + scoped_refptr<MediaEngagementSession> session_; DISALLOW_COPY_AND_ASSIGN(MediaEngagementContentsObserver); };
diff --git a/chrome/browser/media/media_engagement_contents_observer_unittest.cc b/chrome/browser/media/media_engagement_contents_observer_unittest.cc index 0c2481c0..86876b7e 100644 --- a/chrome/browser/media/media_engagement_contents_observer_unittest.cc +++ b/chrome/browser/media/media_engagement_contents_observer_unittest.cc
@@ -14,6 +14,7 @@ #include "chrome/browser/media/media_engagement_score.h" #include "chrome/browser/media/media_engagement_service.h" #include "chrome/browser/media/media_engagement_service_factory.h" +#include "chrome/browser/media/media_engagement_session.h" #include "chrome/test/base/chrome_render_view_host_test_harness.h" #include "chrome/test/base/testing_profile.h" #include "components/ukm/test_ukm_recorder.h" @@ -36,18 +37,30 @@ std::string()); ChromeRenderViewHostTestHarness::SetUp(); + SetContents(content::WebContentsTester::CreateTestWebContents( browser_context(), nullptr)); service_ = base::WrapUnique( new MediaEngagementService(profile(), base::WrapUnique(test_clock_))); - contents_observer_ = - new MediaEngagementContentsObserver(web_contents(), service_.get()); + contents_observer_ = CreateContentsObserverFor(web_contents()); + + // Navigate to an initial URL to setup the |session|. + content::WebContentsTester::For(web_contents()) + ->NavigateAndCommit(GURL("https://first.example.com")); contents_observer_->SetTaskRunnerForTest(task_runner_); SimulateInaudible(); } + MediaEngagementContentsObserver* CreateContentsObserverFor( + content::WebContents* web_contents) { + MediaEngagementContentsObserver* contents_observer = + new MediaEngagementContentsObserver(web_contents, service_.get()); + service_->contents_observers_.insert({web_contents, contents_observer}); + return contents_observer; + } + bool IsTimerRunning() const { return contents_observer_->playback_timer_->IsRunning(); } @@ -60,8 +73,10 @@ audible_row->second.second; } + bool HasSession() const { return !!contents_observer_->session_; } + bool WasSignificantPlaybackRecorded() const { - return contents_observer_->significant_playback_recorded_; + return contents_observer_->session_->significant_playback_recorded(); } size_t GetSignificantActivePlayersCount() const { @@ -137,7 +152,7 @@ } void SimulateSignificantPlaybackRecorded() { - contents_observer_->significant_playback_recorded_ = true; + contents_observer_->session_->RecordSignificantPlayback(); } void SimulateSignificantPlaybackTimeForPage() { @@ -194,7 +209,17 @@ content::NavigationHandle::CreateNavigationHandleForTesting( GURL(url), main_rfh(), true /** committed */); contents_observer_->DidFinishNavigation(test_handle.get()); - contents_observer_->committed_origin_ = url::Origin::Create(url); + } + + scoped_refptr<MediaEngagementSession> GetOrCreateSession( + const url::Origin& origin, + content::WebContents* opener) { + return contents_observer_->GetOrCreateSession(origin, opener); + } + + scoped_refptr<MediaEngagementSession> GetSessionFor( + MediaEngagementContentsObserver* contents_observer) { + return contents_observer->session_; } void SimulateAudible() { @@ -220,28 +245,36 @@ int seconds_since_playback) { using Entry = ukm::builders::Media_Engagement_SessionFinished; - std::vector<std::pair<const char*, int64_t>> metrics = { - {Entry::kPlaybacks_TotalName, playbacks_total}, - {Entry::kVisits_TotalName, visits_total}, - {Entry::kEngagement_ScoreName, score}, - {Entry::kPlaybacks_DeltaName, playbacks_delta}, - {Entry::kEngagement_IsHighName, high_score}, - {Entry::kPlayer_Audible_DeltaName, audible_players_delta}, - {Entry::kPlayer_Audible_TotalName, audible_players_total}, - {Entry::kPlayer_Significant_DeltaName, significant_players_delta}, - {Entry::kPlayer_Significant_TotalName, significant_players_total}, - {Entry::kPlaybacks_SecondsSinceLastName, seconds_since_playback}, - }; + auto ukm_entries = test_ukm_recorder_.GetEntriesByName(Entry::kEntryName); + ASSERT_NE(0u, ukm_entries.size()); - auto entries = test_ukm_recorder_.GetEntriesByName(Entry::kEntryName); - EXPECT_EQ(1u, entries.size()); - for (const auto* const entry : entries) { - test_ukm_recorder_.ExpectEntrySourceHasUrl(entry, url); - for (const auto& metric : metrics) { - test_ukm_recorder_.ExpectEntryMetric(entry, metric.first, - metric.second); - } - } + auto* ukm_entry = ukm_entries.back(); + test_ukm_recorder_.ExpectEntrySourceHasUrl(ukm_entry, url); + EXPECT_EQ(playbacks_total, *test_ukm_recorder_.GetEntryMetric( + ukm_entry, Entry::kPlaybacks_TotalName)); + EXPECT_EQ(visits_total, *test_ukm_recorder_.GetEntryMetric( + ukm_entry, Entry::kVisits_TotalName)); + EXPECT_EQ(score, *test_ukm_recorder_.GetEntryMetric( + ukm_entry, Entry::kEngagement_ScoreName)); + EXPECT_EQ(playbacks_delta, *test_ukm_recorder_.GetEntryMetric( + ukm_entry, Entry::kPlaybacks_DeltaName)); + EXPECT_EQ(high_score, !!*test_ukm_recorder_.GetEntryMetric( + ukm_entry, Entry::kEngagement_IsHighName)); + EXPECT_EQ(audible_players_delta, + *test_ukm_recorder_.GetEntryMetric( + ukm_entry, Entry::kPlayer_Audible_DeltaName)); + EXPECT_EQ(audible_players_total, + *test_ukm_recorder_.GetEntryMetric( + ukm_entry, Entry::kPlayer_Audible_TotalName)); + EXPECT_EQ(significant_players_delta, + *test_ukm_recorder_.GetEntryMetric( + ukm_entry, Entry::kPlayer_Significant_DeltaName)); + EXPECT_EQ(significant_players_total, + *test_ukm_recorder_.GetEntryMetric( + ukm_entry, Entry::kPlayer_Significant_TotalName)); + EXPECT_EQ(seconds_since_playback, + *test_ukm_recorder_.GetEntryMetric( + ukm_entry, Entry::kPlaybacks_SecondsSinceLastName)); } void ExpectUkmIgnoredEntries(GURL url, std::vector<int64_t> entries) { @@ -340,6 +373,10 @@ test_clock_->Advance(base::TimeDelta::FromMinutes(15)); } + ukm::TestAutoSetUkmRecorder& test_ukm_recorder() { + return test_ukm_recorder_; + } + private: // contents_observer_ auto-destroys when WebContents is destroyed. MediaEngagementContentsObserver* contents_observer_; @@ -740,18 +777,6 @@ MediaEngagementContentsObserver::kHistogramScoreAtPlaybackName, 0); } -TEST_F(MediaEngagementContentsObserverTest, - DoNotRecordScoreOnPlayback_InternalUrl) { - GURL url("chrome://about"); - SetScores(url, 6, 5); - - base::HistogramTester tester; - Navigate(url); - SimulateAudioVideoPlaybackStarted(0); - tester.ExpectTotalCount( - MediaEngagementContentsObserver::kHistogramScoreAtPlaybackName, 0); -} - TEST_F(MediaEngagementContentsObserverTest, VisibilityNotRequired) { EXPECT_FALSE(IsTimerRunning()); @@ -849,16 +874,20 @@ ExpectUkmEntry(url, 6, 7, 86, 1, true, 2, 5, 2, 3, 900); } -TEST_F(MediaEngagementContentsObserverTest, DoNotRecordMetricsOnInternalUrl) { +TEST_F(MediaEngagementContentsObserverTest, DoNotCreateSessionOnInternalUrl) { Navigate(GURL("chrome://about")); - EXPECT_FALSE(WasSignificantPlaybackRecorded()); - SimulateSignificantVideoPlayer(0); - SimulateSignificantPlaybackTimeForPage(); - EXPECT_TRUE(WasSignificantPlaybackRecorded()); + // Delete recorded UKM related to the previous navigation to not have to rely + // on how the SetUp() is made. + test_ukm_recorder().Purge(); + + EXPECT_FALSE(HasSession()); SimulateDestroy(); - ExpectNoUkmEntry(); + + // SessionFinished UKM isn't recorded for internal URLs. + using Entry = ukm::builders::Media_Engagement_SessionFinished; + EXPECT_EQ(0u, test_ukm_recorder().GetEntriesByName(Entry::kEntryName).size()); } TEST_F(MediaEngagementContentsObserverTest, RecordAudiblePlayers_OnDestroy) { @@ -1053,3 +1082,44 @@ ExpectScores(url, 0, 1, 0, 1, 0); ExpectNoUkmIgnoreEntry(url); } + +TEST_F(MediaEngagementContentsObserverTest, GetOrCreateSession_SpecialURLs) { + std::vector<GURL> urls = { + // chrome:// and about: URLs don't use MEI. + GURL("about:blank"), GURL("chrome://settings"), + // Only http/https URLs use MEI, ignoring other protocals. + GURL("file:///tmp/"), GURL("foobar://"), + }; + + for (const GURL& url : urls) + EXPECT_EQ(nullptr, GetOrCreateSession(url::Origin::Create(url), nullptr)); +} + +TEST_F(MediaEngagementContentsObserverTest, GetOrCreateSession_NoOpener) { + // Regular URLs with no |opener| have a new session (non-null). + EXPECT_NE(nullptr, + GetOrCreateSession(url::Origin::Create(GURL("https://example.com")), + nullptr)); +} + +TEST_F(MediaEngagementContentsObserverTest, GetOrCreateSession_WithOpener) { + const GURL& url = GURL("https://example.com"); + const GURL& cross_origin_url = GURL("https://second.example.com"); + + // Regular URLs with an |opener| from a different origin have a new session. + std::unique_ptr<content::WebContents> opener( + content::WebContentsTester::CreateTestWebContents(browser_context(), + nullptr)); + MediaEngagementContentsObserver* other_observer = + CreateContentsObserverFor(opener.get()); + content::WebContentsTester::For(opener.get()) + ->NavigateAndCommit(cross_origin_url); + EXPECT_NE(GetSessionFor(other_observer), + GetOrCreateSession(url::Origin::Create(url), opener.get())); + + // Same origin gets the session from the opener. + content::WebContentsTester::For(web_contents())->NavigateAndCommit(url); + content::WebContentsTester::For(opener.get())->NavigateAndCommit(url); + EXPECT_EQ(GetSessionFor(other_observer), + GetOrCreateSession(url::Origin::Create(url), opener.get())); +}
diff --git a/chrome/browser/media/media_engagement_service.cc b/chrome/browser/media/media_engagement_service.cc index a0ef8d042..cee4af53 100644 --- a/chrome/browser/media/media_engagement_service.cc +++ b/chrome/browser/media/media_engagement_service.cc
@@ -102,7 +102,8 @@ if (!service) return; service->contents_observers_.insert( - new MediaEngagementContentsObserver(web_contents, service)); + {web_contents, + new MediaEngagementContentsObserver(web_contents, service)}); } // static @@ -286,6 +287,16 @@ HostContentSettingsMapFactory::GetForProfile(profile_)); } +MediaEngagementContentsObserver* MediaEngagementService::GetContentsObserverFor( + content::WebContents* web_contents) const { + const auto& it = contents_observers_.find(web_contents); + return it == contents_observers_.end() ? nullptr : it->second; +} + +Profile* MediaEngagementService::profile() const { + return profile_; +} + bool MediaEngagementService::ShouldRecordEngagement(const GURL& url) const { return url.SchemeIsHTTPOrHTTPS(); }
diff --git a/chrome/browser/media/media_engagement_service.h b/chrome/browser/media/media_engagement_service.h index ad7e7108..33d01ec 100644 --- a/chrome/browser/media/media_engagement_service.h +++ b/chrome/browser/media/media_engagement_service.h
@@ -89,6 +89,11 @@ // Retrieves the MediaEngagementScore for |url|. MediaEngagementScore CreateEngagementScore(const GURL& url) const; + MediaEngagementContentsObserver* GetContentsObserverFor( + content::WebContents* web_contents) const; + + Profile* profile() const; + // The name of the histogram that scores are logged to on startup. static const char kHistogramScoreAtStartupName[]; @@ -98,9 +103,10 @@ private: friend class MediaEngagementBrowserTest; - friend class MediaEngagementServiceTest; friend class MediaEngagementContentsObserverTest; - friend MediaEngagementContentsObserver; + friend class MediaEngagementServiceTest; + friend class MediaEngagementSessionTest; + friend class MediaEngagementContentsObserver; MediaEngagementService(Profile* profile, std::unique_ptr<base::Clock> clock); @@ -108,7 +114,8 @@ // engagement is only earned for HTTP and HTTPS. bool ShouldRecordEngagement(const GURL& url) const; - std::set<MediaEngagementContentsObserver*> contents_observers_; + base::flat_map<content::WebContents*, MediaEngagementContentsObserver*> + contents_observers_; Profile* profile_;
diff --git a/chrome/browser/media/media_engagement_session.cc b/chrome/browser/media/media_engagement_session.cc new file mode 100644 index 0000000..feaebb46 --- /dev/null +++ b/chrome/browser/media/media_engagement_session.cc
@@ -0,0 +1,154 @@ +// Copyright 2017 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/media/media_engagement_session.h" + +#include "chrome/browser/media/media_engagement_score.h" +#include "chrome/browser/media/media_engagement_service.h" +#include "services/metrics/public/cpp/ukm_builders.h" +#include "services/metrics/public/cpp/ukm_entry_builder.h" +#include "services/metrics/public/cpp/ukm_recorder.h" + +MediaEngagementSession::MediaEngagementSession(MediaEngagementService* service, + const url::Origin& origin) + : service_(service), origin_(origin) {} + +bool MediaEngagementSession::IsSameOriginWith(const url::Origin& origin) const { + return origin_.IsSameOriginWith(origin); +} + +void MediaEngagementSession::RecordSignificantPlayback() { + DCHECK(!significant_playback_recorded_); + + significant_playback_recorded_ = true; + pending_data_to_commit_.playback = true; + + // When playback has happened, the visit can be recorded as there will be no + // further changes. + CommitPendingData(); +} + +void MediaEngagementSession::RecordShortPlaybackIgnored(int length_msec) { + ukm::UkmRecorder* ukm_recorder = GetUkmRecorder(); + if (!ukm_recorder) + return; + + ukm::builders::Media_Engagement_ShortPlaybackIgnored(ukm_source_id_) + .SetLength(length_msec) + .Record(ukm_recorder); +} + +void MediaEngagementSession::RegisterAudiblePlayers( + int32_t audible_players, + int32_t significant_players) { + DCHECK_GE(audible_players, significant_players); + + if (!audible_players && !significant_players) + return; + + pending_data_to_commit_.players = true; + audible_players_delta_ += audible_players; + significant_players_delta_ += significant_players; +} + +bool MediaEngagementSession::significant_playback_recorded() const { + return significant_playback_recorded_; +} + +const url::Origin& MediaEngagementSession::origin() const { + return origin_; +} + +MediaEngagementSession::~MediaEngagementSession() { + // The destructor is called when all the tabs associated te the MEI session + // are closed. Metrics and data related to "visits" need to be recorded now. + + if (HasPendingDataToCommit()) + CommitPendingData(); + + RecordUkmMetrics(); +} + +ukm::UkmRecorder* MediaEngagementSession::GetUkmRecorder() { + ukm::UkmRecorder* ukm_recorder = ukm::UkmRecorder::Get(); + if (!ukm_recorder) + return nullptr; + + if (ukm_source_id_ == ukm::kInvalidSourceId) { + ukm_source_id_ = ukm_recorder->GetNewSourceID(); + ukm_recorder->UpdateSourceURL(ukm_source_id_, origin_.GetURL()); + } + + return ukm_recorder; +} + +void MediaEngagementSession::RecordUkmMetrics() { + ukm::UkmRecorder* ukm_recorder = GetUkmRecorder(); + if (!ukm_recorder) + return; + + MediaEngagementScore score = + service_->CreateEngagementScore(origin_.GetURL()); + ukm::builders::Media_Engagement_SessionFinished(ukm_source_id_) + .SetPlaybacks_Total(score.media_playbacks()) + .SetVisits_Total(score.visits()) + .SetEngagement_Score(round(score.actual_score() * 100)) + .SetPlaybacks_Delta(significant_playback_recorded_) + .SetEngagement_IsHigh(score.high_score()) + .SetPlayer_Audible_Delta(audible_players_total_) + .SetPlayer_Audible_Total(score.audible_playbacks()) + .SetPlayer_Significant_Delta(significant_players_total_) + .SetPlayer_Significant_Total(score.significant_playbacks()) + .SetPlaybacks_SecondsSinceLast(time_since_playback_for_ukm_.InSeconds()) + .Record(ukm_recorder); +} + +bool MediaEngagementSession::HasPendingDataToCommit() const { + return pending_data_to_commit_.visit || pending_data_to_commit_.playback || + pending_data_to_commit_.players; +} + +void MediaEngagementSession::CommitPendingData() { + DCHECK(HasPendingDataToCommit()); + + MediaEngagementScore score = + service_->CreateEngagementScore(origin_.GetURL()); + + if (pending_data_to_commit_.visit) + score.IncrementVisits(); + + if (significant_playback_recorded_ && pending_data_to_commit_.playback) { + const base::Time old_time = score.last_media_playback_time(); + + score.IncrementMediaPlaybacks(); + + // This code should be reached once and |time_since_playback_for_ukm_| can't + // be set. + DCHECK(time_since_playback_for_ukm_.is_zero()); + + if (!old_time.is_null()) { + // Calculate the time since the last playback and the first significant + // playback this visit. If there is no last playback time then we will + // record 0. + time_since_playback_for_ukm_ = + score.last_media_playback_time() - old_time; + } + } + + if (pending_data_to_commit_.players) { + score.IncrementAudiblePlaybacks(audible_players_delta_); + score.IncrementSignificantPlaybacks(significant_players_delta_); + + audible_players_total_ += audible_players_delta_; + significant_players_total_ += significant_players_delta_; + + audible_players_delta_ = 0; + significant_players_delta_ = 0; + } + + score.Commit(); + + pending_data_to_commit_.visit = pending_data_to_commit_.playback = + pending_data_to_commit_.players = false; +}
diff --git a/chrome/browser/media/media_engagement_session.h b/chrome/browser/media/media_engagement_session.h new file mode 100644 index 0000000..f1e15f5 --- /dev/null +++ b/chrome/browser/media/media_engagement_session.h
@@ -0,0 +1,98 @@ +// Copyright 2017 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CHROME_BROWSER_MEDIA_MEDIA_ENGAGEMENT_SESSION_H_ +#define CHROME_BROWSER_MEDIA_MEDIA_ENGAGEMENT_SESSION_H_ + +#include "base/memory/ref_counted.h" +#include "services/metrics/public/cpp/ukm_source_id.h" +#include "url/origin.h" + +namespace ukm { +class UkmRecorder; +} // namespace ukm + +class MediaEngagementService; + +// Represents a session with regards to the Media Engagement. A session consists +// of a visit on a tab and all the tabs opened to the same origin originating +// from that tab or one of its children. +// +// Each tab will have a MediaEngagementContentsObserver that will hold a +// reference to a session. When all the references are released, the session +// will be destructed and record all the information. It guarantees that +// information are recorded only once. +class MediaEngagementSession : public base::RefCounted<MediaEngagementSession> { + public: + MediaEngagementSession(MediaEngagementService* service, + const url::Origin& origin); + + // Returns whether the session's origin is same origin with |origin|. + bool IsSameOriginWith(const url::Origin& origin) const; + + // Record that the session received a significant playback. + void RecordSignificantPlayback(); + + // Record the length of an ignored media playback. + void RecordShortPlaybackIgnored(int length_msec); + + // Register new audible/significant players. `significant_players` can't be + // greater than `audible_players`. + void RegisterAudiblePlayers(int32_t audible_players, + int32_t significant_players); + + bool significant_playback_recorded() const; + const url::Origin& origin() const; + + private: + friend class base::RefCounted<MediaEngagementSession>; + friend class MediaEngagementSessionTest; + + // Private because the class is RefCounted. + ~MediaEngagementSession(); + + // Returns the UkmRecorder with the right source id set. + ukm::UkmRecorder* GetUkmRecorder(); + + // Record the score and change in score to UKM. + void RecordUkmMetrics(); + + bool HasPendingDataToCommit() const; + + // Commits any pending data to website settings. + void CommitPendingData(); + + // Weak pointer because |this| has a lifetime shorter than it. + MediaEngagementService* service_; + + // Origin associated with the session. + url::Origin origin_; + + // UKM source ID associated with the session. It will be used by all the + // session clients. + ukm::SourceId ukm_source_id_ = ukm::kInvalidSourceId; + + // Delta counts are counts to be added to the score while total counts are + // the sum of all the changes that happened during the session lifetime. The + // total count is recorded as delta with regards to UKM. + int32_t audible_players_delta_ = 0; + int32_t significant_players_delta_ = 0; + int32_t audible_players_total_ = 0; + int32_t significant_players_total_ = 0; + + bool significant_playback_recorded_ = false; + struct PendingDataToCommit { + bool visit = true; + bool playback = false; + bool players = false; + }; + PendingDataToCommit pending_data_to_commit_; + + // The time between significant playbacks to be recorded to UKM. + base::TimeDelta time_since_playback_for_ukm_; + + DISALLOW_COPY_AND_ASSIGN(MediaEngagementSession); +}; + +#endif // CHROME_BROWSER_MEDIA_MEDIA_ENGAGEMENT_SESSION_H_
diff --git a/chrome/browser/media/media_engagement_session_unittest.cc b/chrome/browser/media/media_engagement_session_unittest.cc new file mode 100644 index 0000000..0dda1b0 --- /dev/null +++ b/chrome/browser/media/media_engagement_session_unittest.cc
@@ -0,0 +1,773 @@ +// Copyright 2017 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/media/media_engagement_session.h" + +#include "base/memory/scoped_refptr.h" +#include "base/test/simple_test_clock.h" +#include "chrome/browser/media/media_engagement_service.h" +#include "chrome/test/base/testing_profile.h" +#include "components/ukm/test_ukm_recorder.h" +#include "content/public/test/test_browser_thread_bundle.h" +#include "services/metrics/public/cpp/ukm_builders.h" +#include "testing/gtest/include/gtest/gtest.h" + +class MediaEngagementSessionTest : public testing::Test { + public: + // Helper methods to call non-public methods on the MediaEngagementSession + // class. + static ukm::SourceId GetUkmSourceIdForSession( + MediaEngagementSession* session) { + return session->ukm_source_id_; + } + + static ukm::UkmRecorder* GetUkmRecorderForSession( + MediaEngagementSession* session) { + return session->GetUkmRecorder(); + } + + static bool HasPendingVisitToCommitForSession( + MediaEngagementSession* session) { + return session->pending_data_to_commit_.visit; + } + + static bool HasPendingPlaybackToCommitForSession( + MediaEngagementSession* session) { + return session->pending_data_to_commit_.playback; + } + + static bool HasPendingPlayersToCommitForSession( + MediaEngagementSession* session) { + return session->pending_data_to_commit_.players; + } + + static bool HasPendingDataToCommitForSession( + MediaEngagementSession* session) { + return session->HasPendingDataToCommit(); + } + + static int32_t GetAudiblePlayersDeltaForSession( + MediaEngagementSession* session) { + return session->audible_players_delta_; + } + + static int32_t GetSignificantPlayersDeltaForSession( + MediaEngagementSession* session) { + return session->significant_players_delta_; + } + + static int32_t GetAudiblePlayersTotalForSession( + MediaEngagementSession* session) { + return session->audible_players_total_; + } + + static int32_t GetSignificantPlayersTotalForSession( + MediaEngagementSession* session) { + return session->significant_players_total_; + } + + static void SetPendingDataToCommitForSession(MediaEngagementSession* session, + bool visit, + bool playback, + bool players) { + session->pending_data_to_commit_ = {visit, playback, players}; + } + + static void SetSignificantPlaybackRecordedForSession( + MediaEngagementSession* session, + bool value) { + session->significant_playback_recorded_ = value; + } + + static void CommitPendingDataForSession(MediaEngagementSession* session) { + session->CommitPendingData(); + } + + static void RecordUkmMetricsForSession(MediaEngagementSession* session) { + session->RecordUkmMetrics(); + } + + static base::TimeDelta GetTimeSincePlaybackForSession( + MediaEngagementSession* session) { + return session->time_since_playback_for_ukm_; + } + + MediaEngagementSessionTest() + : origin_(url::Origin::Create(GURL("https://example.com"))), + test_clock_(new base::SimpleTestClock()) {} + + ~MediaEngagementSessionTest() override = default; + + void SetUp() override { + service_ = base::WrapUnique( + new MediaEngagementService(&profile_, base::WrapUnique(test_clock_))); + } + + MediaEngagementService* service() const { return service_.get(); } + + const url::Origin& origin() const { return origin_; } + + const ukm::TestAutoSetUkmRecorder& test_ukm_recorder() const { + return test_ukm_recorder_; + } + + base::SimpleTestClock* test_clock() { return test_clock_; } + + private: + const url::Origin origin_; + // Owned by |service_| but keeping a reference to it to manipulate. + base::SimpleTestClock* test_clock_; + + content::TestBrowserThreadBundle thread_bundle_; + TestingProfile profile_; + std::unique_ptr<MediaEngagementService> service_; + ukm::TestAutoSetUkmRecorder test_ukm_recorder_; +}; + +// SmokeTest checking that IsSameOrigin actually does a same origin check. +TEST_F(MediaEngagementSessionTest, IsSameOrigin) { + scoped_refptr<MediaEngagementSession> session = + new MediaEngagementSession(service(), origin()); + + std::vector<url::Origin> origins = { + origin(), + url::Origin::Create(GURL("http://example.com")), + url::Origin::Create(GURL("https://example.org")), + url::Origin(), + url::Origin::Create(GURL("http://google.com")), + url::Origin::Create(GURL("http://foo.example.com")), + }; + + for (const auto& orig : origins) { + EXPECT_EQ(origin().IsSameOriginWith(orig), session->IsSameOriginWith(orig)); + } +} + +// Checks that RecordShortPlaybackIgnored() records the right UKM. +TEST_F(MediaEngagementSessionTest, RecordShortPlaybackIgnored) { + using Entry = ukm::builders::Media_Engagement_ShortPlaybackIgnored; + const std::string url_string = origin().GetURL().spec(); + + scoped_refptr<MediaEngagementSession> session = + new MediaEngagementSession(service(), origin()); + + EXPECT_EQ(0u, test_ukm_recorder().GetEntriesByName(Entry::kEntryName).size()); + + session->RecordShortPlaybackIgnored(42); + + { + auto ukm_entries = test_ukm_recorder().GetEntriesByName(Entry::kEntryName); + + EXPECT_EQ(1u, ukm_entries.size()); + test_ukm_recorder().ExpectEntrySourceHasUrl(ukm_entries[0], + origin().GetURL()); + EXPECT_EQ(42, *test_ukm_recorder().GetEntryMetric(ukm_entries[0], + Entry::kLengthName)); + } + + session->RecordShortPlaybackIgnored(1337); + + { + auto ukm_entries = test_ukm_recorder().GetEntriesByName(Entry::kEntryName); + + EXPECT_EQ(2u, ukm_entries.size()); + test_ukm_recorder().ExpectEntrySourceHasUrl(ukm_entries[1], + origin().GetURL()); + EXPECT_EQ(1337, *test_ukm_recorder().GetEntryMetric(ukm_entries[1], + Entry::kLengthName)); + } +} + +// Set of tests for RegisterAudiblePlayers(). +TEST_F(MediaEngagementSessionTest, RegisterAudiblePlayers) { + scoped_refptr<MediaEngagementSession> session = + new MediaEngagementSession(service(), origin()); + + // Initial checks. + EXPECT_EQ(0, GetAudiblePlayersDeltaForSession(session.get())); + EXPECT_EQ(0, GetSignificantPlayersDeltaForSession(session.get())); + EXPECT_FALSE(HasPendingPlayersToCommitForSession(session.get())); + + // Registering (0,0) should be a no-op. + { + session->RegisterAudiblePlayers(0, 0); + EXPECT_EQ(0, GetAudiblePlayersDeltaForSession(session.get())); + EXPECT_EQ(0, GetSignificantPlayersDeltaForSession(session.get())); + EXPECT_FALSE(HasPendingPlayersToCommitForSession(session.get())); + } + + // Registering any value will trigger data to commit. + { + session->RegisterAudiblePlayers(1, 1); + EXPECT_EQ(1, GetAudiblePlayersDeltaForSession(session.get())); + EXPECT_EQ(1, GetSignificantPlayersDeltaForSession(session.get())); + EXPECT_TRUE(HasPendingPlayersToCommitForSession(session.get())); + } +} + +TEST_F(MediaEngagementSessionTest, TotalPlayers) { + using Entry = ukm::builders::Media_Engagement_SessionFinished; + scoped_refptr<MediaEngagementSession> session = + new MediaEngagementSession(service(), origin()); + + // Initial checks. + EXPECT_EQ(0, GetAudiblePlayersTotalForSession(session.get())); + EXPECT_EQ(0, GetSignificantPlayersTotalForSession(session.get())); + EXPECT_FALSE(HasPendingPlayersToCommitForSession(session.get())); + + // Registering players doesn't increment totals. + session->RegisterAudiblePlayers(1, 1); + EXPECT_EQ(0, GetAudiblePlayersTotalForSession(session.get())); + EXPECT_EQ(0, GetSignificantPlayersTotalForSession(session.get())); + + // Commiting data increment totals. + session->RegisterAudiblePlayers(1, 1); + CommitPendingDataForSession(session.get()); + EXPECT_EQ(2, GetAudiblePlayersTotalForSession(session.get())); + EXPECT_EQ(2, GetSignificantPlayersTotalForSession(session.get())); + + // Totals values have been saved to UKM as delta. + RecordUkmMetricsForSession(session.get()); + { + auto ukm_entries = test_ukm_recorder().GetEntriesByName(Entry::kEntryName); + EXPECT_EQ(1u, ukm_entries.size()); + + auto* ukm_entry = ukm_entries[0]; + test_ukm_recorder().ExpectEntrySourceHasUrl(ukm_entry, origin().GetURL()); + EXPECT_EQ(2, *test_ukm_recorder().GetEntryMetric( + ukm_entry, Entry::kPlayer_Audible_DeltaName)); + EXPECT_EQ(2, *test_ukm_recorder().GetEntryMetric( + ukm_entry, Entry::kPlayer_Significant_DeltaName)); + } +} + +// Checks that ukm_source_id_ is set when GetUkmRecorder is called. +TEST_F(MediaEngagementSessionTest, GetkmRecorder_SetsUkmSourceId) { + scoped_refptr<MediaEngagementSession> session = + new MediaEngagementSession(service(), origin()); + + EXPECT_EQ(ukm::kInvalidSourceId, GetUkmSourceIdForSession(session.get())); + + GetUkmRecorderForSession(session.get()); + EXPECT_NE(ukm::kInvalidSourceId, GetUkmSourceIdForSession(session.get())); +} + +// Checks that GetUkmRecorder() does not return nullptr. +TEST_F(MediaEngagementSessionTest, GetkmRecorder_NotNull) { + scoped_refptr<MediaEngagementSession> session = + new MediaEngagementSession(service(), origin()); + + EXPECT_NE(nullptr, GetUkmRecorderForSession(session.get())); +} + +// Test that RecordSignificantPlayback() sets the significant_playback_recorded_ +// boolean to true. +TEST_F(MediaEngagementSessionTest, RecordSignificantPlayback_SetsBoolean) { + scoped_refptr<MediaEngagementSession> session = + new MediaEngagementSession(service(), origin()); + + EXPECT_FALSE(session->significant_playback_recorded()); + + session->RecordSignificantPlayback(); + EXPECT_TRUE(session->significant_playback_recorded()); +} + +// Test that RecordSignificantPlayback() records playback. +TEST_F(MediaEngagementSessionTest, + RecordSignificantPlayback_SetsPendingPlayback) { + scoped_refptr<MediaEngagementSession> session = + new MediaEngagementSession(service(), origin()); + + int expected_visits = 0; + int expected_playbacks = 0; + + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + expected_visits = score.visits() + 1; + expected_playbacks = score.media_playbacks() + 1; + } + + EXPECT_FALSE(HasPendingPlaybackToCommitForSession(session.get())); + EXPECT_TRUE(HasPendingVisitToCommitForSession(session.get())); + + session->RecordSignificantPlayback(); + EXPECT_FALSE(HasPendingDataToCommitForSession(session.get())); + + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + + EXPECT_EQ(expected_visits, score.visits()); + EXPECT_EQ(expected_playbacks, score.media_playbacks()); + } +} + +// Test that CommitPendingData reset pending_data_to_commit_ after running. +TEST_F(MediaEngagementSessionTest, CommitPendingData_Reset) { + scoped_refptr<MediaEngagementSession> session = + new MediaEngagementSession(service(), origin()); + + EXPECT_TRUE(HasPendingDataToCommitForSession(session.get())); + + CommitPendingDataForSession(session.get()); + EXPECT_FALSE(HasPendingDataToCommitForSession(session.get())); + + SetSignificantPlaybackRecordedForSession(session.get(), true); + SetPendingDataToCommitForSession(session.get(), true, true, true); + EXPECT_TRUE(HasPendingDataToCommitForSession(session.get())); + + CommitPendingDataForSession(session.get()); + EXPECT_FALSE(HasPendingDataToCommitForSession(session.get())); +} + +// Test that CommitPendingData only update visits field when needed. +TEST_F(MediaEngagementSessionTest, CommitPendingData_UpdateVisitsAsNeeded) { + scoped_refptr<MediaEngagementSession> session = + new MediaEngagementSession(service(), origin()); + + int expected_visits = 0; + + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + expected_visits = score.visits(); + } + + EXPECT_TRUE(HasPendingVisitToCommitForSession(session.get())); + CommitPendingDataForSession(session.get()); + + ++expected_visits; + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + EXPECT_EQ(expected_visits, score.visits()); + } + + SetPendingDataToCommitForSession(session.get(), true, false, false); + EXPECT_TRUE(HasPendingVisitToCommitForSession(session.get())); + CommitPendingDataForSession(session.get()); + + ++expected_visits; + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + EXPECT_EQ(expected_visits, score.visits()); + } + + SetSignificantPlaybackRecordedForSession(session.get(), true); + SetPendingDataToCommitForSession(session.get(), false, true, true); + EXPECT_FALSE(HasPendingVisitToCommitForSession(session.get())); + CommitPendingDataForSession(session.get()); + + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + EXPECT_EQ(expected_visits, score.visits()); + } +} + +TEST_F(MediaEngagementSessionTest, CommitPendingData_UpdatePlaybackWhenNeeded) { + scoped_refptr<MediaEngagementSession> session = + new MediaEngagementSession(service(), origin()); + + int expected_playbacks = 0; + + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + expected_playbacks = score.media_playbacks(); + } + + EXPECT_FALSE(HasPendingPlaybackToCommitForSession(session.get())); + CommitPendingDataForSession(session.get()); + + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + EXPECT_EQ(expected_playbacks, score.media_playbacks()); + } + + SetSignificantPlaybackRecordedForSession(session.get(), true); + SetPendingDataToCommitForSession(session.get(), false, true, false); + EXPECT_TRUE(HasPendingPlaybackToCommitForSession(session.get())); + CommitPendingDataForSession(session.get()); + + ++expected_playbacks; + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + EXPECT_EQ(expected_playbacks, score.media_playbacks()); + } + + // Both significant_playback_recorded_ and pending data need to be true. + SetSignificantPlaybackRecordedForSession(session.get(), false); + SetPendingDataToCommitForSession(session.get(), false, true, false); + EXPECT_TRUE(HasPendingPlaybackToCommitForSession(session.get())); + CommitPendingDataForSession(session.get()); + + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + EXPECT_EQ(expected_playbacks, score.media_playbacks()); + } + + SetSignificantPlaybackRecordedForSession(session.get(), true); + SetPendingDataToCommitForSession(session.get(), true, false, true); + EXPECT_FALSE(HasPendingPlaybackToCommitForSession(session.get())); + CommitPendingDataForSession(session.get()); + + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + EXPECT_EQ(expected_playbacks, score.media_playbacks()); + } + + SetSignificantPlaybackRecordedForSession(session.get(), false); + SetPendingDataToCommitForSession(session.get(), true, false, true); + EXPECT_FALSE(HasPendingPlaybackToCommitForSession(session.get())); + CommitPendingDataForSession(session.get()); + + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + EXPECT_EQ(expected_playbacks, score.media_playbacks()); + } +} + +TEST_F(MediaEngagementSessionTest, CommitPendingData_UpdatePlayersWhenNeeded) { + scoped_refptr<MediaEngagementSession> session = + new MediaEngagementSession(service(), origin()); + + int expected_audible_playbacks = 0; + int expected_significant_playbacks = 0; + + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + expected_audible_playbacks = score.audible_playbacks(); + expected_significant_playbacks = score.significant_playbacks(); + } + + EXPECT_FALSE(HasPendingPlayersToCommitForSession(session.get())); + CommitPendingDataForSession(session.get()); + + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + EXPECT_EQ(expected_audible_playbacks, score.audible_playbacks()); + EXPECT_EQ(expected_significant_playbacks, score.significant_playbacks()); + } + + session->RegisterAudiblePlayers(0, 0); + SetPendingDataToCommitForSession(session.get(), true, true, false); + EXPECT_FALSE(HasPendingPlayersToCommitForSession(session.get())); + CommitPendingDataForSession(session.get()); + + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + EXPECT_EQ(expected_audible_playbacks, score.audible_playbacks()); + EXPECT_EQ(expected_significant_playbacks, score.significant_playbacks()); + } + + session->RegisterAudiblePlayers(0, 0); + SetPendingDataToCommitForSession(session.get(), false, false, true); + EXPECT_TRUE(HasPendingPlayersToCommitForSession(session.get())); + CommitPendingDataForSession(session.get()); + + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + EXPECT_EQ(expected_audible_playbacks, score.audible_playbacks()); + EXPECT_EQ(expected_significant_playbacks, score.significant_playbacks()); + } + + session->RegisterAudiblePlayers(1, 1); + SetPendingDataToCommitForSession(session.get(), true, true, false); + EXPECT_FALSE(HasPendingPlayersToCommitForSession(session.get())); + CommitPendingDataForSession(session.get()); + + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + EXPECT_EQ(expected_audible_playbacks, score.audible_playbacks()); + EXPECT_EQ(expected_significant_playbacks, score.significant_playbacks()); + } + + session->RegisterAudiblePlayers(0, 0); + SetPendingDataToCommitForSession(session.get(), false, false, true); + EXPECT_TRUE(HasPendingPlayersToCommitForSession(session.get())); + CommitPendingDataForSession(session.get()); + + ++expected_audible_playbacks; + ++expected_significant_playbacks; + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + EXPECT_EQ(expected_audible_playbacks, score.audible_playbacks()); + EXPECT_EQ(expected_significant_playbacks, score.significant_playbacks()); + } + + session->RegisterAudiblePlayers(1, 1); + SetPendingDataToCommitForSession(session.get(), false, false, true); + EXPECT_TRUE(HasPendingPlayersToCommitForSession(session.get())); + CommitPendingDataForSession(session.get()); + + ++expected_audible_playbacks; + ++expected_significant_playbacks; + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + EXPECT_EQ(expected_audible_playbacks, score.audible_playbacks()); + EXPECT_EQ(expected_significant_playbacks, score.significant_playbacks()); + } +} + +// SmokeTest that RecordUkmMetrics actually record UKM. The method has little to +// no logic. +TEST_F(MediaEngagementSessionTest, RecordUkmMetrics) { + const std::string url_string = origin().GetURL().spec(); + using Entry = ukm::builders::Media_Engagement_SessionFinished; + + scoped_refptr<MediaEngagementSession> session = + new MediaEngagementSession(service(), origin()); + + session->RecordSignificantPlayback(); + + EXPECT_EQ(0u, test_ukm_recorder().GetEntriesByName(Entry::kEntryName).size()); + + RecordUkmMetricsForSession(session.get()); + + { + auto ukm_entries = test_ukm_recorder().GetEntriesByName(Entry::kEntryName); + EXPECT_EQ(1u, ukm_entries.size()); + + auto* ukm_entry = ukm_entries[0]; + test_ukm_recorder().ExpectEntrySourceHasUrl(ukm_entry, origin().GetURL()); + EXPECT_EQ(1, *test_ukm_recorder().GetEntryMetric( + ukm_entry, Entry::kPlaybacks_TotalName)); + EXPECT_EQ(1, *test_ukm_recorder().GetEntryMetric(ukm_entry, + Entry::kVisits_TotalName)); + EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric( + ukm_entry, Entry::kEngagement_ScoreName)); + EXPECT_EQ(1, *test_ukm_recorder().GetEntryMetric( + ukm_entry, Entry::kPlaybacks_DeltaName)); + EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric( + ukm_entry, Entry::kEngagement_IsHighName)); + EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric( + ukm_entry, Entry::kPlayer_Audible_DeltaName)); + EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric( + ukm_entry, Entry::kPlayer_Audible_TotalName)); + EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric( + ukm_entry, Entry::kPlayer_Significant_DeltaName)); + EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric( + ukm_entry, Entry::kPlayer_Significant_TotalName)); + EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric( + ukm_entry, Entry::kPlaybacks_SecondsSinceLastName)); + } + + SetSignificantPlaybackRecordedForSession(session.get(), false); + session->RecordSignificantPlayback(); + + RecordUkmMetricsForSession(session.get()); + + { + auto ukm_entries = test_ukm_recorder().GetEntriesByName(Entry::kEntryName); + EXPECT_EQ(2u, ukm_entries.size()); + + auto* ukm_entry = ukm_entries[1]; + test_ukm_recorder().ExpectEntrySourceHasUrl(ukm_entry, origin().GetURL()); + EXPECT_EQ(2, *test_ukm_recorder().GetEntryMetric( + ukm_entry, Entry::kPlaybacks_TotalName)); + EXPECT_EQ(1, *test_ukm_recorder().GetEntryMetric(ukm_entry, + Entry::kVisits_TotalName)); + EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric( + ukm_entry, Entry::kEngagement_ScoreName)); + EXPECT_EQ(1, *test_ukm_recorder().GetEntryMetric( + ukm_entry, Entry::kPlaybacks_DeltaName)); + EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric( + ukm_entry, Entry::kEngagement_IsHighName)); + EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric( + ukm_entry, Entry::kPlayer_Audible_DeltaName)); + EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric( + ukm_entry, Entry::kPlayer_Audible_TotalName)); + EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric( + ukm_entry, Entry::kPlayer_Significant_DeltaName)); + EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric( + ukm_entry, Entry::kPlayer_Significant_TotalName)); + EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric( + ukm_entry, Entry::kPlaybacks_SecondsSinceLastName)); + } +} + +TEST_F(MediaEngagementSessionTest, DestructorRecordMetrics) { + using Entry = ukm::builders::Media_Engagement_SessionFinished; + + const url::Origin other_origin = + url::Origin::Create(GURL("https://example.org")); + + EXPECT_EQ(0u, test_ukm_recorder().GetEntriesByName(Entry::kEntryName).size()); + + { + scoped_refptr<MediaEngagementSession> session = + new MediaEngagementSession(service(), origin()); + + // |session| was destructed. + } + + { + auto ukm_entries = test_ukm_recorder().GetEntriesByName(Entry::kEntryName); + EXPECT_EQ(1u, ukm_entries.size()); + + test_ukm_recorder().ExpectEntrySourceHasUrl(ukm_entries[0], + origin().GetURL()); + } + + { + scoped_refptr<MediaEngagementSession> other_session = + new MediaEngagementSession(service(), other_origin); + // |other_session| was destructed. + } + + { + auto ukm_entries = test_ukm_recorder().GetEntriesByName(Entry::kEntryName); + EXPECT_EQ(2u, ukm_entries.size()); + + test_ukm_recorder().ExpectEntrySourceHasUrl(ukm_entries[0], + origin().GetURL()); + test_ukm_recorder().ExpectEntrySourceHasUrl(ukm_entries[1], + other_origin.GetURL()); + } +} + +TEST_F(MediaEngagementSessionTest, DestructorCommitDataIfNeeded) { + int expected_visits = 0; + int expected_playbacks = 0; + int expected_audible_playbacks = 0; + int expected_significant_playbacks = 0; + + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + + expected_visits = score.visits(); + expected_playbacks = score.media_playbacks(); + expected_audible_playbacks = score.audible_playbacks(); + expected_significant_playbacks = score.significant_playbacks(); + } + + { + scoped_refptr<MediaEngagementSession> session = + new MediaEngagementSession(service(), origin()); + + // |session| was destructed. + } + + ++expected_visits; + + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + EXPECT_EQ(expected_visits, score.visits()); + EXPECT_EQ(expected_playbacks, score.media_playbacks()); + EXPECT_EQ(expected_audible_playbacks, score.audible_playbacks()); + EXPECT_EQ(expected_significant_playbacks, score.significant_playbacks()); + } + + { + scoped_refptr<MediaEngagementSession> session = + new MediaEngagementSession(service(), origin()); + + session->RecordSignificantPlayback(); + + // |session| was destructed. + } + + ++expected_visits; + ++expected_playbacks; + + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + EXPECT_EQ(expected_visits, score.visits()); + EXPECT_EQ(expected_playbacks, score.media_playbacks()); + EXPECT_EQ(expected_audible_playbacks, score.audible_playbacks()); + EXPECT_EQ(expected_significant_playbacks, score.significant_playbacks()); + } + + { + scoped_refptr<MediaEngagementSession> session = + new MediaEngagementSession(service(), origin()); + + session->RegisterAudiblePlayers(2, 2); + + // |session| was destructed. + } + + ++expected_visits; + expected_audible_playbacks += 2; + expected_significant_playbacks += 2; + + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + EXPECT_EQ(expected_visits, score.visits()); + EXPECT_EQ(expected_playbacks, score.media_playbacks()); + EXPECT_EQ(expected_audible_playbacks, score.audible_playbacks()); + EXPECT_EQ(expected_significant_playbacks, score.significant_playbacks()); + } + + // Pretend there is nothing to commit, nothing should change. + { + scoped_refptr<MediaEngagementSession> session = + new MediaEngagementSession(service(), origin()); + + SetPendingDataToCommitForSession(session.get(), false, false, false); + + // |session| was destructed. + } + + { + MediaEngagementScore score = + service()->CreateEngagementScore(origin().GetURL()); + EXPECT_EQ(expected_visits, score.visits()); + EXPECT_EQ(expected_playbacks, score.media_playbacks()); + EXPECT_EQ(expected_audible_playbacks, score.audible_playbacks()); + EXPECT_EQ(expected_significant_playbacks, score.significant_playbacks()); + } +} + +// Tests that the TimeSinceLastPlayback is set to zero if there is no previous +// record. +TEST_F(MediaEngagementSessionTest, TimeSinceLastPlayback_NoPreviousRecord) { + scoped_refptr<MediaEngagementSession> session = + new MediaEngagementSession(service(), origin()); + + EXPECT_TRUE(GetTimeSincePlaybackForSession(session.get()).is_zero()); + + // Advance in time and play. + test_clock()->Advance(base::TimeDelta::FromSeconds(42)); + session->RecordSignificantPlayback(); + + EXPECT_TRUE(GetTimeSincePlaybackForSession(session.get()).is_zero()); +} + +// Tests that the TimeSinceLastPlayback is set to the delta when there is a +// previous record. +TEST_F(MediaEngagementSessionTest, TimeSinceLastPlayback_PreviousRecord) { + scoped_refptr<MediaEngagementSession> session = + new MediaEngagementSession(service(), origin()); + + EXPECT_TRUE(GetTimeSincePlaybackForSession(session.get()).is_zero()); + + // Advance in time and play. + test_clock()->Advance(base::TimeDelta::FromSeconds(42)); + service()->RecordPlayback(origin().GetURL()); + + test_clock()->Advance(base::TimeDelta::FromSeconds(42)); + session->RecordSignificantPlayback(); + + EXPECT_EQ(42, GetTimeSincePlaybackForSession(session.get()).InSeconds()); +}
diff --git a/chrome/browser/ui/cocoa/share_menu_controller.mm b/chrome/browser/ui/cocoa/share_menu_controller.mm index 508c69c4..31b0f39 100644 --- a/chrome/browser/ui/cocoa/share_menu_controller.mm +++ b/chrome/browser/ui/cocoa/share_menu_controller.mm
@@ -65,6 +65,21 @@ } // NSMenuDelegate + +- (BOOL)menuHasKeyEquivalent:(NSMenu*)menu + forEvent:(NSEvent*)event + target:(id*)target + action:(SEL*)action { + // Load the menu if it hasn't loaded already. + if ([menu numberOfItems] == 0) { + [self menuNeedsUpdate:menu]; + } + // Per tapted@'s comment in BookmarkMenuCocoaController, it's fine + // to return NO here if an item will handle this. This is why it's + // necessary to ensure the menu is loaded above. + return NO; +} + - (void)menuNeedsUpdate:(NSMenu*)menu { [menu removeAllItems]; [menu setAutoenablesItems:NO];
diff --git a/chrome/browser/ui/cocoa/share_menu_controller_browsertest.mm b/chrome/browser/ui/cocoa/share_menu_controller_browsertest.mm index 92c4ca6..0c9865d8 100644 --- a/chrome/browser/ui/cocoa/share_menu_controller_browsertest.mm +++ b/chrome/browser/ui/cocoa/share_menu_controller_browsertest.mm
@@ -20,6 +20,7 @@ #include "net/base/mac/url_conversions.h" #include "testing/gtest_mac.h" #include "ui/base/l10n/l10n_util_mac.h" +#include "ui/events/test/cocoa_test_event_utils.h" // Mock sharing service for sensing shared items. @interface MockSharingService : NSSharingService @@ -213,3 +214,28 @@ tester.ExpectTotalCount(histogram_name, 3); tester.ExpectBucketCount(histogram_name, false, 1); } + +IN_PROC_BROWSER_TEST_F(ShareMenuControllerTest, MenuHasKeyEquivalent) { + // If this method isn't implemented, |menuNeedsUpdate:| is called any time + // *any* hotkey is used + ASSERT_TRUE([controller_ respondsToSelector:@selector + (menuHasKeyEquivalent:forEvent:target:action:)]); + + // Ensure that calling |menuHasKeyEquivalent:...| the first time populates the + // menu. + base::scoped_nsobject<NSMenu> menu([[NSMenu alloc] initWithTitle:@"Share"]); + EXPECT_EQ([menu numberOfItems], 0); + NSEvent* event = cocoa_test_event_utils::KeyEventWithKeyCode( + 'i', 'i', NSKeyDown, NSCommandKeyMask | NSShiftKeyMask); + EXPECT_FALSE([controller_ menuHasKeyEquivalent:menu + forEvent:event + target:nil + action:nil]); + EXPECT_GT([menu numberOfItems], 0); + + NSMenuItem* item = [menu itemAtIndex:0]; + // |menuHasKeyEquivalent:....| shouldn't populate the menu after the first + // time. + [controller_ menuHasKeyEquivalent:menu forEvent:event target:nil action:nil]; + EXPECT_EQ(item, [menu itemAtIndex:0]); // Pointer equality intended. +}
diff --git a/chrome/browser/ui/views/payments/payment_request_browsertest_base.h b/chrome/browser/ui/views/payments/payment_request_browsertest_base.h index e001922..645263d 100644 --- a/chrome/browser/ui/views/payments/payment_request_browsertest_base.h +++ b/chrome/browser/ui/views/payments/payment_request_browsertest_base.h
@@ -63,7 +63,7 @@ public PaymentRequestDialogView::ObserverForTest, public content::WebContentsObserver { public: - // Various events that can be waited on by the DialogEventObserver. + // Various events that can be waited on by the DialogEventWaiter. enum DialogEvent : int { DIALOG_OPENED, DIALOG_CLOSED, @@ -244,7 +244,7 @@ // Resets the event waiter for a given |event| or |event_sequence|. void ResetEventWaiter(DialogEvent event); void ResetEventWaiterForSequence(std::list<DialogEvent> event_sequence); - // Wait for the event(s) passed to ResetEventObserver*() to occur. + // Wait for the event(s) passed to ResetEventWaiter*() to occur. void WaitForObservedEvent(); private:
diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn index 62c3726..ad5a5e1 100644 --- a/chrome/test/BUILD.gn +++ b/chrome/test/BUILD.gn
@@ -1652,7 +1652,9 @@ ] } if (toolkit_views) { - sources -= [ "../browser/ui/views/select_file_dialog_extension_browsertest.cc" ] + sources -= [ + "../browser/ui/views/select_file_dialog_extension_browsertest.cc", + ] } if (is_win || is_linux) { sources += @@ -1908,7 +1910,9 @@ } } if (enable_mdns) { - sources += [ "../browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc" ] + sources += [ + "../browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc", + ] } if (use_brlapi) { deps += [ "//build/linux/libbrlapi" ] @@ -2238,6 +2242,7 @@ "../browser/media/media_engagement_contents_observer_unittest.cc", "../browser/media/media_engagement_score_unittest.cc", "../browser/media/media_engagement_service_unittest.cc", + "../browser/media/media_engagement_session_unittest.cc", "../browser/media/media_storage_id_salt_unittest.cc", "../browser/media/midi_permission_context_unittest.cc", "../browser/media/midi_sysex_permission_context_unittest.cc", @@ -2973,7 +2978,9 @@ } if (is_win) { - sources += [ "../browser/notifications/notification_platform_bridge_win_unittest.cc" ] + sources += [ + "../browser/notifications/notification_platform_bridge_win_unittest.cc", + ] } } @@ -3395,7 +3402,9 @@ [ "../browser/extensions/external_registry_loader_win_unittest.cc" ] } if (is_chromeos) { - sources += [ "../browser/extensions/api/file_system/consent_provider_unittest.cc" ] + sources += [ + "../browser/extensions/api/file_system/consent_provider_unittest.cc", + ] } else { sources += [ "../browser/extensions/api/messaging/native_message_process_host_unittest.cc", @@ -3625,7 +3634,9 @@ ] if (is_mac && !mac_views_browser) { - sources += [ "../browser/ui/startup/session_crashed_infobar_delegate_unittest.cc" ] + sources += [ + "../browser/ui/startup/session_crashed_infobar_delegate_unittest.cc", + ] } } if (enable_webrtc) { @@ -4562,7 +4573,9 @@ "../browser/ui/views/keyboard_access_browsertest.cc", ] if (!use_ozone) { - sources += [ "../browser/ui/libgtkui/select_file_dialog_interactive_uitest.cc" ] + sources += [ + "../browser/ui/libgtkui/select_file_dialog_interactive_uitest.cc", + ] deps += [ "//build/config/linux/gtk" ] } }
diff --git a/content/browser/site_per_process_browsertest.cc b/content/browser/site_per_process_browsertest.cc index b1a3e03..1d3faaf 100644 --- a/content/browser/site_per_process_browsertest.cc +++ b/content/browser/site_per_process_browsertest.cc
@@ -19,6 +19,7 @@ #include "base/callback.h" #include "base/command_line.h" #include "base/feature_list.h" +#include "base/json/json_reader.h" #include "base/location.h" #include "base/memory/ptr_util.h" #include "base/path_service.h" @@ -80,6 +81,7 @@ #include "content/public/test/test_navigation_observer.h" #include "content/public/test/test_utils.h" #include "content/shell/browser/shell.h" +#include "content/shell/common/shell_switches.h" #include "content/test/content_browser_test_utils_internal.h" #include "content/test/mock_overscroll_observer.h" #include "ipc/constants.mojom.h" @@ -622,6 +624,25 @@ std::string GetHTMLContents() override { return "<p>Interstitial</p>"; } }; +#if defined(USE_AURA) || defined(OS_ANDROID) +bool ConvertJSONToPoint(const std::string& str, gfx::PointF* point) { + std::unique_ptr<base::Value> value = base::JSONReader::Read(str); + if (!value) + return false; + base::DictionaryValue* root; + if (!value->GetAsDictionary(&root)) + return false; + double x, y; + if (!root->GetDouble("x", &x)) + return false; + if (!root->GetDouble("y", &y)) + return false; + point->set_x(x); + point->set_y(y); + return true; +} +#endif // defined(USE_AURA) || defined (OS_ANDROID) + } // namespace // @@ -1115,6 +1136,159 @@ gesture_fling_start_ack_observer.Wait(); } +// Restrict to Aura to we can use routable MouseWheel event via +// RenderWidgetHostViewAura::OnScrollEvent(). +#if defined(USE_AURA) +class SitePerProcessInternalsBrowserTest : public SitePerProcessBrowserTest { + public: + SitePerProcessInternalsBrowserTest() {} + + protected: + void SetUpCommandLine(base::CommandLine* command_line) override { + SitePerProcessBrowserTest::SetUpCommandLine(command_line); + command_line->AppendSwitch(switches::kExposeInternalsForTesting); + // Needed to guarantee the scrollable div we're testing with is not given + // its own compositing layer. + command_line->AppendSwitch(switches::kDisablePreferCompositingToLCDText); + } +}; + +IN_PROC_BROWSER_TEST_F(SitePerProcessInternalsBrowserTest, + ScrollNestedLocalNonFastScrollableDiv) { + GURL main_url(embedded_test_server()->GetURL( + "a.com", "/cross_site_iframe_factory.html?a(b)")); + EXPECT_TRUE(NavigateToURL(shell(), main_url)); + + // It is safe to obtain the root frame tree node here, as it doesn't change. + FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents()) + ->GetFrameTree() + ->root(); + ASSERT_EQ(1U, root->child_count()); + + FrameTreeNode* parent_iframe_node = root->child_at(0); + + GURL site_url(embedded_test_server()->GetURL( + "b.com", "/tall_page_with_local_iframe.html")); + NavigateFrameToURL(parent_iframe_node, site_url); + + FrameTreeNode* nested_iframe_node = parent_iframe_node->child_at(0); + WaitForChildFrameSurfaceReady(nested_iframe_node->current_frame_host()); + + EXPECT_EQ( + " Site A ------------ proxies for B\n" + " +--Site B ------- proxies for A\n" + " +--Site B -- proxies for A\n" + "Where A = http://a.com/\n" + " B = http://b.com/", + DepictFrameTree(root)); + + const char* get_element_location_script_fmt = + "var rect = " + "document.getElementById('%s').getBoundingClientRect();\n" + "var point = {\n" + " x: rect.left,\n" + " y: rect.top\n" + "};\n" + "window.domAutomationController.send(JSON.stringify(point));"; + + // Since the nested local b-frame shares the RenderWidgetHostViewChildFrame + // with the parent frame, we need to query element offsets in both documents + // before converting to root space coordinates for the wheel event. + std::string str; + EXPECT_TRUE(ExecuteScriptAndExtractString( + nested_iframe_node->current_frame_host(), + base::StringPrintf(get_element_location_script_fmt, "scrollable_div"), + &str)); + gfx::PointF nested_point_f; + ConvertJSONToPoint(str, &nested_point_f); + + EXPECT_TRUE(ExecuteScriptAndExtractString( + parent_iframe_node->current_frame_host(), + base::StringPrintf(get_element_location_script_fmt, "nested_frame"), + &str)); + gfx::PointF parent_offset_f; + ConvertJSONToPoint(str, &parent_offset_f); + + // Compute location for wheel event. + gfx::PointF point_f(parent_offset_f.x() + nested_point_f.x() + 5.f, + parent_offset_f.y() + nested_point_f.y() + 5.f); + + RenderWidgetHostViewChildFrame* rwhv_nested = + static_cast<RenderWidgetHostViewChildFrame*>( + nested_iframe_node->current_frame_host() + ->GetRenderWidgetHost() + ->GetView()); + point_f = rwhv_nested->TransformPointToRootCoordSpaceF(point_f); + + RenderWidgetHostViewAura* rwhv_root = static_cast<RenderWidgetHostViewAura*>( + root->current_frame_host()->GetRenderWidgetHost()->GetView()); + + gfx::PointF nested_in_parent; + rwhv_root->TransformPointToCoordSpaceForView( + point_f, + parent_iframe_node->current_frame_host() + ->GetRenderWidgetHost() + ->GetView(), + &nested_in_parent); + + // Get original scroll position. + int div_scroll_top_start; + EXPECT_TRUE(ExecuteScriptAndExtractInt( + nested_iframe_node->current_frame_host(), + "window.domAutomationController.send(" + "document.getElementById('scrollable_div').scrollTop);", + &div_scroll_top_start)); + EXPECT_EQ(0, div_scroll_top_start); + + // Wait until renderer's compositor thread is synced. Otherwise the event + // handler won't be installed when the event arrives. + MainThreadFrameObserver observer(rwhv_root->GetRenderWidgetHost()); + observer.Wait(); + { + base::RunLoop run_loop; + base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( + FROM_HERE, run_loop.QuitClosure(), + // tiny_timeout() is too small to run without flakes, but + // action_timeout() is 100 times bigger, which is overkill. We use a + // custom delay here to achieve a balance. + base::TimeDelta::FromMilliseconds(1000)); + run_loop.Run(); + } + + // Send a wheel to scroll the div. + gfx::Point location(point_f.x(), point_f.y()); + ui::ScrollEvent scroll_event(ui::ET_SCROLL, location, ui::EventTimeForNow(), + 0, 0, -ui::MouseWheelEvent::kWheelDelta, 0, + ui::MouseWheelEvent::kWheelDelta, + 2); // This must be '2' or it gets silently + // dropped. + rwhv_root->OnScrollEvent(&scroll_event); + + InputEventAckWaiter ack_observer( + parent_iframe_node->current_frame_host()->GetRenderWidgetHost(), + blink::WebInputEvent::kGestureScrollUpdate); + ack_observer.Wait(); + + // Check compositor layers. + EXPECT_TRUE(ExecuteScriptAndExtractString( + nested_iframe_node->current_frame_host(), + "window.domAutomationController.send(" + "window.internals.layerTreeAsText(document));", + &str)); + // We expect the nested OOPIF to not have any compositor layers. + EXPECT_EQ(std::string(), str); + + // Verify the div scrolled. + int div_scroll_top = div_scroll_top_start; + EXPECT_TRUE(ExecuteScriptAndExtractInt( + nested_iframe_node->current_frame_host(), + "window.domAutomationController.send(" + "document.getElementById('scrollable_div').scrollTop);", + &div_scroll_top)); + EXPECT_NE(div_scroll_top_start, div_scroll_top); +} +#endif // defined(USE_AURA) + // Test that scrolling a nested out-of-process iframe bubbles unused scroll // delta to a parent frame. IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, ScrollBubblingFromOOPIFTest) { @@ -11964,27 +12138,6 @@ DISALLOW_COPY_AND_ASSIGN(TouchSelectionControllerClientTestWrapper); }; -namespace { - -bool ConvertJSONToPoint(const std::string& str, gfx::PointF* point) { - std::unique_ptr<base::Value> value = base::JSONReader::Read(str); - if (!value) - return false; - base::DictionaryValue* root; - if (!value->GetAsDictionary(&root)) - return false; - double x, y; - if (!root->GetDouble("x", &x)) - return false; - if (!root->GetDouble("y", &y)) - return false; - point->set_x(x); - point->set_y(y); - return true; -} - -} // namespace - IN_PROC_BROWSER_TEST_F(TouchSelectionControllerClientAndroidSiteIsolationTest, BasicSelectionIsolatedIframe) { GURL test_url(embedded_test_server()->GetURL(
diff --git a/content/renderer/gpu/render_widget_compositor.cc b/content/renderer/gpu/render_widget_compositor.cc index b7411f9..248835a 100644 --- a/content/renderer/gpu/render_widget_compositor.cc +++ b/content/renderer/gpu/render_widget_compositor.cc
@@ -972,12 +972,6 @@ touch_end_cancel_properties == WebEventListenerProperties::kBlockingAndPassive; - WebEventListenerProperties wheel_event_properties = - EventListenerProperties(WebEventListenerClass::kMouseWheel); - bool has_wheel_handlers = - wheel_event_properties == WebEventListenerProperties::kBlocking || - wheel_event_properties == WebEventListenerProperties::kBlockingAndPassive; - cc::Layer* root_layer = layer_tree_host_->root_layer(); cc::TouchActionRegion touch_event_handler; @@ -986,11 +980,6 @@ gfx::Rect(gfx::Point(), root_layer->bounds())); } root_layer->SetTouchActionRegion(std::move(touch_event_handler)); - - cc::Region wheel_handler_region; - if (has_wheel_handlers) - wheel_handler_region = gfx::Rect(gfx::Point(), root_layer->bounds()); - root_layer->SetNonFastScrollableRegion(wheel_handler_region); } blink::WebEventListenerProperties
diff --git a/content/test/data/tall_page_with_local_iframe.html b/content/test/data/tall_page_with_local_iframe.html new file mode 100644 index 0000000..133a84d --- /dev/null +++ b/content/test/data/tall_page_with_local_iframe.html
@@ -0,0 +1,9 @@ +<html> +<head></head> +<body> + <div>Tall w/ iframe.</div> + <iframe src="page_with_scrollable_div.html" + style="min-height: 800pxi; position: absolute; top: 28px; left: 8px" + id="nested_frame"></iframe> +</body> +</html>
diff --git a/media/audio/audio_manager.h b/media/audio/audio_manager.h index 77e9510..40c22b8 100644 --- a/media/audio/audio_manager.h +++ b/media/audio/audio_manager.h
@@ -91,7 +91,7 @@ // Log callback used for sending log messages from a stream to the object // that manages the stream. - using LogCallback = base::Callback<void(const std::string&)>; + using LogCallback = base::RepeatingCallback<void(const std::string&)>; // Factory for all the supported stream formats. |params| defines parameters // of the audio stream to be created.
diff --git a/media/audio/win/audio_low_latency_input_win.cc b/media/audio/win/audio_low_latency_input_win.cc index 89e530e0..d75ef650 100644 --- a/media/audio/win/audio_low_latency_input_win.cc +++ b/media/audio/win/audio_low_latency_input_win.cc
@@ -12,6 +12,7 @@ #include "base/logging.h" #include "base/metrics/histogram_macros.h" +#include "base/strings/stringprintf.h" #include "base/strings/utf_string_conversions.h" #include "base/trace_event/trace_event.h" #include "media/audio/audio_device_description.h" @@ -27,7 +28,9 @@ using base::win::ScopedCOMInitializer; namespace media { + namespace { + bool IsSupportedFormatForConversion(const WAVEFORMATEX& format) { if (format.nSamplesPerSec < limits::kMinSampleRate || format.nSamplesPerSec > limits::kMaxSampleRate) { @@ -50,14 +53,18 @@ return true; } + } // namespace -WASAPIAudioInputStream::WASAPIAudioInputStream(AudioManagerWin* manager, - const AudioParameters& params, - const std::string& device_id) - : manager_(manager), device_id_(device_id) { +WASAPIAudioInputStream::WASAPIAudioInputStream( + AudioManagerWin* manager, + const AudioParameters& params, + const std::string& device_id, + const AudioManager::LogCallback& log_callback) + : manager_(manager), device_id_(device_id), log_callback_(log_callback) { DCHECK(manager_); DCHECK(!device_id_.empty()); + DCHECK(!log_callback_.is_null()); // Load the Avrt DLL if not already loaded. Required to support MMCSS. bool avrt_init = avrt::Initialize(); @@ -102,15 +109,17 @@ DCHECK_EQ(OPEN_RESULT_OK, open_result_); // Verify that we are not already opened. - if (opened_) + if (opened_) { + log_callback_.Run("WASAPIAIS::Open: already open"); return false; + } // Obtain a reference to the IMMDevice interface of the capturing // device with the specified unique identifier or role which was // set at construction. HRESULT hr = SetCaptureDevice(); if (FAILED(hr)) { - ReportOpenResult(); + ReportOpenResult(hr); return false; } @@ -120,7 +129,7 @@ NULL, &audio_client_); if (FAILED(hr)) { open_result_ = OPEN_RESULT_ACTIVATION_FAILED; - ReportOpenResult(); + ReportOpenResult(hr); return false; } @@ -133,9 +142,10 @@ // Verify that the selected audio endpoint supports the specified format // set during construction. - if (!DesiredFormatIsSupported()) { + hr = S_OK; + if (!DesiredFormatIsSupported(&hr)) { open_result_ = OPEN_RESULT_FORMAT_NOT_SUPPORTED; - ReportOpenResult(); + ReportOpenResult(hr); return false; } @@ -144,7 +154,7 @@ hr = InitializeAudioEngine(); if (SUCCEEDED(hr) && converter_) open_result_ = OPEN_RESULT_OK_WITH_RESAMPLING; - ReportOpenResult(); // Report before we assign a value to |opened_|. + ReportOpenResult(hr); // Report before we assign a value to |opened_|. opened_ = SUCCEEDED(hr); return opened_; @@ -191,10 +201,20 @@ // Start streaming data between the endpoint buffer and the audio engine. HRESULT hr = audio_client_->Start(); - DLOG_IF(ERROR, FAILED(hr)) << "Failed to start input streaming."; + if (FAILED(hr)) { + DLOG(ERROR) << "Failed to start input streaming."; + log_callback_.Run(base::StringPrintf( + "WASAPIAIS::Start: Failed to start audio client, hresult = %#lx", hr)); + } - if (SUCCEEDED(hr) && audio_render_client_for_loopback_.Get()) + if (SUCCEEDED(hr) && audio_render_client_for_loopback_.Get()) { hr = audio_render_client_for_loopback_->Start(); + if (FAILED(hr)) + log_callback_.Run(base::StringPrintf( + "WASAPIAIS::Start: Failed to start render client for loopback, " + "hresult = %#lx", + hr)); + } started_ = SUCCEEDED(hr); } @@ -610,23 +630,24 @@ return hr; } -bool WASAPIAudioInputStream::DesiredFormatIsSupported() { +bool WASAPIAudioInputStream::DesiredFormatIsSupported(HRESULT* hr) { // An application that uses WASAPI to manage shared-mode streams can rely // on the audio engine to perform only limited format conversions. The audio // engine can convert between a standard PCM sample size used by the // application and the floating-point samples that the engine uses for its // internal processing. However, the format for an application stream // typically must have the same number of channels and the same sample - // rate as the stream format used byfCHANNEL_LAYOUT_UNSUPPORTED the device. + // rate as the stream format used by the device. // Many audio devices support both PCM and non-PCM stream formats. However, // the audio engine can mix only PCM streams. base::win::ScopedCoMem<WAVEFORMATEX> closest_match; - HRESULT hr = audio_client_->IsFormatSupported(AUDCLNT_SHAREMODE_SHARED, - &format_, &closest_match); - DLOG_IF(ERROR, hr == S_FALSE) + HRESULT hresult = audio_client_->IsFormatSupported(AUDCLNT_SHAREMODE_SHARED, + &format_, &closest_match); + DLOG_IF(ERROR, hresult == S_FALSE) << "Format is not supported but a closest match exists."; - if (hr == S_FALSE && IsSupportedFormatForConversion(*closest_match.get())) { + if (hresult == S_FALSE && + IsSupportedFormatForConversion(*closest_match.get())) { DVLOG(1) << "Audio capture data conversion needed."; // Ideally, we want a 1:1 ratio between the buffers we get and the buffers // we give to OnData so that each buffer we receive from the OS can be @@ -681,10 +702,15 @@ << "Audio capture data conversion: Need to inject fifo"; // Indicate that we're good to go with a close match. - hr = S_OK; + hresult = S_OK; } - return (hr == S_OK); + // At this point, |hresult| == S_OK if the desired format is supported. If + // |hresult| == S_FALSE, the OS supports a closest match but we don't support + // conversion to it. Thus, SUCCEEDED() or FAILED() can't be used to determine + // if the desired format is supported. + *hr = hresult; + return (hresult == S_OK); } HRESULT WASAPIAudioInputStream::InitializeAudioEngine() { @@ -820,10 +846,16 @@ return hr; } -void WASAPIAudioInputStream::ReportOpenResult() const { +void WASAPIAudioInputStream::ReportOpenResult(HRESULT hr) const { DCHECK(!opened_); // This method must be called before we set this flag. UMA_HISTOGRAM_ENUMERATION("Media.Audio.Capture.Win.Open", open_result_, OPEN_RESULT_MAX + 1); + if (open_result_ != OPEN_RESULT_OK && + open_result_ != OPEN_RESULT_OK_WITH_RESAMPLING) { + log_callback_.Run(base::StringPrintf( + "WASAPIAIS::Open: failed, result = %d, hresult = %#lx", open_result_, + hr)); + } } double WASAPIAudioInputStream::ProvideInput(AudioBus* audio_bus,
diff --git a/media/audio/win/audio_low_latency_input_win.h b/media/audio/win/audio_low_latency_input_win.h index 5b39ba3..4d199b5e 100644 --- a/media/audio/win/audio_low_latency_input_win.h +++ b/media/audio/win/audio_low_latency_input_win.h
@@ -75,6 +75,7 @@ #include "base/win/scoped_com_initializer.h" #include "base/win/scoped_handle.h" #include "media/audio/agc_audio_stream.h" +#include "media/audio/audio_manager.h" #include "media/base/audio_converter.h" #include "media/base/audio_parameters.h" #include "media/base/media_export.h" @@ -95,7 +96,8 @@ // the audio manager who is creating this object. WASAPIAudioInputStream(AudioManagerWin* manager, const AudioParameters& params, - const std::string& device_id); + const std::string& device_id, + const AudioManager::LogCallback& log_callback); // The dtor is typically called by the AudioManager only and it is usually // triggered by calling AudioInputStream::Close(). @@ -123,9 +125,13 @@ // The Open() method is divided into these sub methods. HRESULT SetCaptureDevice(); HRESULT GetAudioEngineStreamFormat(); - bool DesiredFormatIsSupported(); + // Returns whether the desired format is supported or not and writes the + // result of a failing system call to |*hr|, or S_OK if successful. If this + // function returns false with |*hr| == S_FALSE, the OS supports a closest + // match but we don't support conversion to it. + bool DesiredFormatIsSupported(HRESULT* hr); HRESULT InitializeAudioEngine(); - void ReportOpenResult() const; + void ReportOpenResult(HRESULT hr) const; // AudioConverter::InputCallback implementation. double ProvideInput(AudioBus* audio_bus, uint32_t frames_delayed) override; @@ -246,6 +252,9 @@ std::unique_ptr<AudioBus> convert_bus_; bool imperfect_buffer_size_conversion_ = false; + // Callback to send log messages. + AudioManager::LogCallback log_callback_; + SEQUENCE_CHECKER(sequence_checker_); DISALLOW_COPY_AND_ASSIGN(WASAPIAudioInputStream);
diff --git a/media/audio/win/audio_low_latency_input_win_unittest.cc b/media/audio/win/audio_low_latency_input_win_unittest.cc index fb18479..96a41fa 100644 --- a/media/audio/win/audio_low_latency_input_win_unittest.cc +++ b/media/audio/win/audio_low_latency_input_win_unittest.cc
@@ -11,6 +11,7 @@ #include <memory> +#include "base/bind.h" #include "base/environment.h" #include "base/files/file_util.h" #include "base/macros.h" @@ -41,6 +42,12 @@ namespace media { +namespace { + +void LogCallbackDummy(const std::string& /* message */) {} + +} // namespace + ACTION_P4(CheckCountAndPostQuitTask, count, limit, task_runner, quit_closure) { if (++*count >= limit) task_runner->PostTask(FROM_HERE, quit_closure); @@ -207,7 +214,7 @@ params.set_frames_per_buffer(frames_per_buffer_); AudioInputStream* ais = audio_man_->MakeAudioInputStream( params, AudioDeviceDescription::kDefaultDeviceId, - AudioManager::LogCallback()); + base::BindRepeating(&LogCallbackDummy)); EXPECT_TRUE(ais); return ais; } @@ -453,7 +460,7 @@ ScopedAudioInputStream stream(audio_manager_->MakeAudioInputStream( params, AudioDeviceDescription::kLoopbackInputDeviceId, - AudioManager::LogCallback())); + base::BindRepeating(&LogCallbackDummy))); ASSERT_TRUE(stream->Open()); FakeAudioInputCallback sink; stream->Start(&sink);
diff --git a/media/audio/win/audio_manager_win.cc b/media/audio/win/audio_manager_win.cc index 35a7489..83b28ac 100644 --- a/media/audio/win/audio_manager_win.cc +++ b/media/audio/win/audio_manager_win.cc
@@ -249,7 +249,7 @@ const LogCallback& log_callback) { // Used for both AUDIO_PCM_LOW_LATENCY and AUDIO_PCM_LINEAR. DVLOG(1) << "MakeLowLatencyInputStream: " << device_id; - return new WASAPIAudioInputStream(this, params, device_id); + return new WASAPIAudioInputStream(this, params, device_id, log_callback); } std::string AudioManagerWin::GetDefaultOutputDeviceID() {
diff --git a/testing/buildbot/filters/mus.content_browsertests.filter b/testing/buildbot/filters/mus.content_browsertests.filter index c3b607fc..b5300b3 100644 --- a/testing/buildbot/filters/mus.content_browsertests.filter +++ b/testing/buildbot/filters/mus.content_browsertests.filter
@@ -57,6 +57,7 @@ -SitePerProcessBrowserTest.NavigateCrashedSubframeToSameSite -SitePerProcessBrowserTest.SurfaceHitTestTest -SitePerProcessHighDPIBrowserTest.SurfaceHitTestTest +-SitePerProcessInternalsBrowserTest.ScrollNestedLocalNonFastScrollableDiv # TODO: reenable, uses RendererFrameNumber(), http://crbug.com/775030. -SitePerProcessBrowserTest.CompositorFrameSwapped
diff --git a/testing/buildbot/filters/viz.content_browsertests.filter b/testing/buildbot/filters/viz.content_browsertests.filter index 340d045..7b6707b 100644 --- a/testing/buildbot/filters/viz.content_browsertests.filter +++ b/testing/buildbot/filters/viz.content_browsertests.filter
@@ -28,6 +28,7 @@ -PointerLockBrowserTest.* -SitePerProcessGestureBrowserTest.* -SitePerProcessHighDPIBrowserTest.* +-SitePerProcessInternalsBrowserTest.* -SitePerProcessMouseWheelBrowserTest.* -TouchSelectionForCrossProcessFramesTests/TouchSelectionControllerClientAuraSiteIsolationTest.*
diff --git a/third_party/WebKit/Source/core/exported/WebPluginContainerImpl.cpp b/third_party/WebKit/Source/core/exported/WebPluginContainerImpl.cpp index c9c33bd3..6100456 100644 --- a/third_party/WebKit/Source/core/exported/WebPluginContainerImpl.cpp +++ b/third_party/WebKit/Source/core/exported/WebPluginContainerImpl.cpp
@@ -619,8 +619,10 @@ page->GetScrollingCoordinator()) { // Only call scrolling_coordinator if attached. SetWantsWheelEvents can // be called from Plugin Initialization when it is not yet attached. - if (is_attached_) - scrolling_coordinator->NotifyGeometryChanged(); + if (is_attached_) { + LocalFrameView* frame_view = element_->GetDocument().GetFrame()->View(); + scrolling_coordinator->NotifyGeometryChanged(frame_view); + } } } }
diff --git a/third_party/WebKit/Source/core/frame/LocalFrameView.cpp b/third_party/WebKit/Source/core/frame/LocalFrameView.cpp index 813954a..f643f26 100644 --- a/third_party/WebKit/Source/core/frame/LocalFrameView.cpp +++ b/third_party/WebKit/Source/core/frame/LocalFrameView.cpp
@@ -238,6 +238,7 @@ forcing_layout_parent_view_(false), needs_intersection_observation_(false), needs_forced_compositing_update_(false), + scroll_gesture_region_is_dirty_(false), main_thread_scrolling_reasons_(0), paint_frame_count_(0), unique_id_(NewUniqueObjectId()) { @@ -802,8 +803,9 @@ // Changing overflow should notify scrolling coordinator to ensures that it // updates non-fast scroll rects even if there is no layout. if (ScrollingCoordinator* scrolling_coordinator = - this->GetScrollingCoordinator()) - scrolling_coordinator->NotifyOverflowUpdated(); + this->GetScrollingCoordinator()) { + SetScrollGestureRegionIsDirty(true); + } IntRect document_rect = layout_view_item.DocumentRect(); if (ScrollOrigin() == -document_rect.Location() && @@ -2621,8 +2623,9 @@ ScheduleUpdatePluginsIfNecessary(); if (ScrollingCoordinator* scrolling_coordinator = - this->GetScrollingCoordinator()) - scrolling_coordinator->NotifyGeometryChanged(); + this->GetScrollingCoordinator()) { + scrolling_coordinator->NotifyGeometryChanged(this); + } ScrollToFragmentAnchor(); SendResizeEventIfNeeded(); @@ -3236,8 +3239,10 @@ if (target_state >= DocumentLifecycle::kPrePaintClean) { if (!RuntimeEnabledFeatures::SlimmingPaintV2Enabled()) { - if (view.Compositor()->InCompositingMode()) - GetScrollingCoordinator()->UpdateAfterCompositingChangeIfNeeded(); + if (view.Compositor()->InCompositingMode()) { + GetScrollingCoordinator()->UpdateAfterCompositingChangeIfNeeded( + this); + } } // This is needed since, at present, the ScrollingCoordinator doesn't @@ -3890,6 +3895,35 @@ resizer_areas_->erase(it); } +bool LocalFrameView::ScrollGestureRegionIsDirty() const { + DCHECK(GetFrame().IsLocalRoot()); + return scroll_gesture_region_is_dirty_; +} + +void LocalFrameView::SetScrollGestureRegionIsDirty(bool dirty) { + if (GetFrame().IsLocalRoot()) { + // TODO(wjmaclean): It would be nice to move the !NeedsLayout() check from + // ScrollableAreasDidChange() to here, but at present doing so breaks + // layout tests. This suggests that there is something that wants to set the + // dirty bit when layout is needed, and won't re-try setting the bit after + // layout has completed - it would be nice to find that and fix it. + scroll_gesture_region_is_dirty_ = dirty; + return; + } + + GetFrame().LocalFrameRoot().View()->SetScrollGestureRegionIsDirty(dirty); +} + +void LocalFrameView::ScrollableAreasDidChange() { + // Layout may update scrollable area bounding boxes. It also sets the same + // dirty flag making this one redundant (See + // |ScrollingCoordinator::notifyGeometryChanged|). + // So if layout is expected, ignore this call allowing scrolling coordinator + // to be notified post-layout to recompute gesture regions. + if (!NeedsLayout()) + SetScrollGestureRegionIsDirty(true); +} + void LocalFrameView::AddScrollableArea(ScrollableArea* scrollable_area) { DCHECK(scrollable_area); if (!scrollable_areas_) @@ -3897,8 +3931,9 @@ scrollable_areas_->insert(scrollable_area); if (ScrollingCoordinator* scrolling_coordinator = - this->GetScrollingCoordinator()) - scrolling_coordinator->ScrollableAreasDidChange(); + this->GetScrollingCoordinator()) { + ScrollableAreasDidChange(); + } } void LocalFrameView::RemoveScrollableArea(ScrollableArea* scrollable_area) { @@ -3907,8 +3942,9 @@ scrollable_areas_->erase(scrollable_area); if (ScrollingCoordinator* scrolling_coordinator = - this->GetScrollingCoordinator()) - scrolling_coordinator->ScrollableAreasDidChange(); + this->GetScrollingCoordinator()) { + ScrollableAreasDidChange(); + } } void LocalFrameView::AddAnimatingScrollableArea( @@ -5079,8 +5115,9 @@ if (!IsSelfVisible()) { SetSelfVisible(true); if (ScrollingCoordinator* scrolling_coordinator = - this->GetScrollingCoordinator()) - scrolling_coordinator->FrameViewVisibilityDidChange(); + this->GetScrollingCoordinator()) { + SetScrollGestureRegionIsDirty(true); + } SetNeedsCompositingUpdate(kCompositingUpdateRebuildTree); UpdateParentScrollableAreaSet(); if (!RuntimeEnabledFeatures::RootLayerScrollingEnabled()) { @@ -5107,8 +5144,9 @@ } SetSelfVisible(false); if (ScrollingCoordinator* scrolling_coordinator = - this->GetScrollingCoordinator()) - scrolling_coordinator->FrameViewVisibilityDidChange(); + this->GetScrollingCoordinator()) { + SetScrollGestureRegionIsDirty(true); + } SetNeedsCompositingUpdate(kCompositingUpdateRebuildTree); UpdateParentScrollableAreaSet(); if (!RuntimeEnabledFeatures::RootLayerScrollingEnabled()) { @@ -5261,7 +5299,7 @@ // ScrollingCoordinator needs to update according to the new throttling // status. if (scrolling_coordinator) - scrolling_coordinator->NotifyGeometryChanged(); + scrolling_coordinator->NotifyGeometryChanged(this); // Start ticking animation frames again if necessary. if (GetPage()) GetPage()->Animator().ScheduleVisualUpdate(frame_.Get());
diff --git a/third_party/WebKit/Source/core/frame/LocalFrameView.h b/third_party/WebKit/Source/core/frame/LocalFrameView.h index 2bd0fa0..c4877c6 100644 --- a/third_party/WebKit/Source/core/frame/LocalFrameView.h +++ b/third_party/WebKit/Source/core/frame/LocalFrameView.h
@@ -948,6 +948,24 @@ ForceThrottlingInvalidationBehavior = kDontForceThrottlingInvalidation, NotifyChildrenBehavior = kNotifyChildren); + // The following three functions may change when https://crbug.com/680606 is + // resolved. + + // Indicates that the local root's subtree needs to have its non-fast + // scrollable regions updated by ScrollingCoordinator. Should only ever be + // called on the local root's view. + bool ScrollGestureRegionIsDirty() const; + + // Updates a flag on the local root's LocalFrameView to mark the local + // subtree as needing to update its regions with ScrollingCoordinator. + // Only ScrollingCoordinator should ever call this function with |dirty| set + // to |false|. + void SetScrollGestureRegionIsDirty(bool dirty); + + // Should be called whenever this LocalFrameView adds or removes a + // scrollable area, or gains/loses a composited layer. + void ScrollableAreasDidChange(); + protected: // Scroll the content via the compositor. bool ScrollContentsFastPath(const IntSize& scroll_delta); @@ -1299,6 +1317,7 @@ bool forcing_layout_parent_view_; bool needs_intersection_observation_; bool needs_forced_compositing_update_; + bool scroll_gesture_region_is_dirty_; Member<ElementVisibilityObserver> visibility_observer_;
diff --git a/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp b/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp index 871ba0361..54f208a8 100644 --- a/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp +++ b/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
@@ -615,8 +615,10 @@ // account. if (Page* page = GetDocument().GetFrame()->GetPage()) { if (ScrollingCoordinator* scrolling_coordinator = - page->GetScrollingCoordinator()) - scrolling_coordinator->NotifyGeometryChanged(); + page->GetScrollingCoordinator()) { + LocalFrameView* frame_view = GetDocument().GetFrame()->View(); + scrolling_coordinator->NotifyGeometryChanged(frame_view); + } } return true; }
diff --git a/third_party/WebKit/Source/core/page/Page.cpp b/third_party/WebKit/Source/core/page/Page.cpp index c6071e8..45172950 100644 --- a/third_party/WebKit/Source/core/page/Page.cpp +++ b/third_party/WebKit/Source/core/page/Page.cpp
@@ -245,7 +245,7 @@ if (ScrollingCoordinator* scrolling_coordinator = this->GetScrollingCoordinator()) { // Hits in compositing/iframes/iframe-composited-scrolling.html - scrolling_coordinator->UpdateAfterCompositingChangeIfNeeded(); + scrolling_coordinator->UpdateAfterCompositingChangeIfNeeded(frame->View()); } GraphicsLayer* layer =
diff --git a/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp b/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp index 1cde853..706b248 100644 --- a/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp +++ b/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
@@ -92,7 +92,6 @@ ScrollingCoordinator::ScrollingCoordinator(Page* page) : page_(page), - scroll_gesture_region_is_dirty_(false), touch_event_target_rects_are_dirty_(false), should_scroll_on_main_thread_dirty_(false), was_frame_scrollable_(false), @@ -109,14 +108,10 @@ } void ScrollingCoordinator::SetShouldHandleScrollGestureOnMainThreadRegion( - const Region& region) { - if (!page_->MainFrame()->IsLocalFrame() || - !page_->DeprecatedLocalMainFrame()->View()) - return; - if (WebLayer* scroll_layer = toWebLayer(page_->DeprecatedLocalMainFrame() - ->View() - ->LayoutViewportScrollableArea() - ->LayerForScrolling())) { + const Region& region, + LocalFrameView* frame_view) { + if (WebLayer* scroll_layer = toWebLayer( + frame_view->LayoutViewportScrollableArea()->LayerForScrolling())) { Vector<IntRect> rects = region.Rects(); WebVector<WebRect> web_rects(rects.size()); for (size_t i = 0; i < rects.size(); ++i) @@ -125,8 +120,8 @@ } } -void ScrollingCoordinator::NotifyGeometryChanged() { - scroll_gesture_region_is_dirty_ = true; +void ScrollingCoordinator::NotifyGeometryChanged(LocalFrameView* frame_view) { + frame_view->SetScrollGestureRegionIsDirty(true); touch_event_target_rects_are_dirty_ = true; should_scroll_on_main_thread_dirty_ = true; } @@ -148,30 +143,6 @@ } } } -void ScrollingCoordinator::NotifyOverflowUpdated() { - scroll_gesture_region_is_dirty_ = true; -} - -void ScrollingCoordinator::FrameViewVisibilityDidChange() { - scroll_gesture_region_is_dirty_ = true; -} - -void ScrollingCoordinator::ScrollableAreasDidChange() { - DCHECK(page_); - if (!page_->MainFrame()->IsLocalFrame() || - !page_->DeprecatedLocalMainFrame()->View()) - return; - - // Layout may update scrollable area bounding boxes. It also sets the same - // dirty flag making this one redundant (See - // |ScrollingCoordinator::notifyGeometryChanged|). - // So if layout is expected, ignore this call allowing scrolling coordinator - // to be notified post-layout to recompute gesture regions. - if (page_->DeprecatedLocalMainFrame()->View()->NeedsLayout()) - return; - - scroll_gesture_region_is_dirty_ = true; -} void ScrollingCoordinator::DidScroll(const gfx::ScrollOffset& offset, const CompositorElementId& element_id) { @@ -196,17 +167,21 @@ // safely ignore the DidScroll callback. } -void ScrollingCoordinator::UpdateAfterCompositingChangeIfNeeded() { - if (!page_->MainFrame()->IsLocalFrame()) - return; +void ScrollingCoordinator::UpdateAfterCompositingChangeIfNeeded( + LocalFrameView* frame_view) { + LocalFrame* frame = &frame_view->GetFrame(); + DCHECK(frame->IsLocalRoot()); - if (!ShouldUpdateAfterCompositingChange()) + if (!(frame_view->ScrollGestureRegionIsDirty() || + touch_event_target_rects_are_dirty_ || + should_scroll_on_main_thread_dirty_ || FrameScrollerIsDirty())) { return; + } TRACE_EVENT0("input", "ScrollingCoordinator::updateAfterCompositingChangeIfNeeded"); - if (scroll_gesture_region_is_dirty_) { + if (frame_view->ScrollGestureRegionIsDirty()) { // Compute the region of the page where we can't handle scroll gestures and // mousewheel events // on the impl thread. This currently includes: @@ -217,19 +192,22 @@ // div/textarea/iframe when CSS property "resize" is enabled. // 3. Plugin areas. Region should_handle_scroll_gesture_on_main_thread_region = - ComputeShouldHandleScrollGestureOnMainThreadRegion( - page_->DeprecatedLocalMainFrame()); + ComputeShouldHandleScrollGestureOnMainThreadRegion(frame); SetShouldHandleScrollGestureOnMainThreadRegion( - should_handle_scroll_gesture_on_main_thread_region); - scroll_gesture_region_is_dirty_ = false; + should_handle_scroll_gesture_on_main_thread_region, frame_view); + frame_view->SetScrollGestureRegionIsDirty(false); } + // TODO(wjmaclean): Make the stuff below this point work for OOPIFs too. + // https://crbug.com/680606 + if (frame != frame_view->GetPage()->MainFrame()) + return; + if (touch_event_target_rects_are_dirty_) { UpdateTouchEventTargetRectsIfNeeded(); touch_event_target_rects_are_dirty_ = false; } - LocalFrameView* frame_view = ToLocalFrame(page_->MainFrame())->View(); bool frame_is_scrollable = frame_view && frame_view->LayoutViewportScrollableArea()->IsScrollable(); if (should_scroll_on_main_thread_dirty_ || @@ -240,7 +218,7 @@ // Need to update scroll on main thread reasons for subframe because // subframe (e.g. iframe with background-attachment:fixed) should // scroll on main thread while the main frame scrolls on impl. - frame_view->UpdateSubFrameScrollOnMainReason(*(page_->MainFrame()), 0); + frame_view->UpdateSubFrameScrollOnMainReason(*frame, 0); should_scroll_on_main_thread_dirty_ = false; } was_frame_scrollable_ = frame_is_scrollable; @@ -255,7 +233,7 @@ UpdateUserInputScrollable(&page_->GetVisualViewport()); if (!RuntimeEnabledFeatures::RootLayerScrollingEnabled()) { - const FrameTree& tree = page_->MainFrame()->Tree(); + const FrameTree& tree = frame_view->GetPage()->MainFrame()->Tree(); for (const Frame* child = tree.FirstChild(); child; child = child->Tree().NextSibling()) { if (!child->IsLocalFrame()) @@ -996,19 +974,23 @@ const LocalFrame* frame) const { Region should_handle_scroll_gesture_on_main_thread_region; LocalFrameView* frame_view = frame->View(); + if (!frame_view || frame_view->ShouldThrottleRendering() || - !frame_view->IsVisible()) + !frame_view->IsVisible()) { return should_handle_scroll_gesture_on_main_thread_region; + } if (const LocalFrameView::ScrollableAreaSet* scrollable_areas = frame_view->ScrollableAreas()) { for (const ScrollableArea* scrollable_area : *scrollable_areas) { if (scrollable_area->IsLocalFrameView() && - ToLocalFrameView(scrollable_area)->ShouldThrottleRendering()) + ToLocalFrameView(scrollable_area)->ShouldThrottleRendering()) { continue; + } // Composited scrollable areas can be scrolled off the main thread. if (scrollable_area->UsesCompositedScrolling()) continue; + IntRect box = scrollable_area->ScrollableAreaBoundingBox(); should_handle_scroll_gesture_on_main_thread_region.Unite(box); } @@ -1230,7 +1212,7 @@ if (!CoordinatesScrollingForFrameView(frame_view)) return; - NotifyGeometryChanged(); + NotifyGeometryChanged(frame_view); } bool ScrollingCoordinator::FrameScrollerIsDirty() const {
diff --git a/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h b/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h index cbc2c7a7..25e9286 100644 --- a/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h +++ b/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h
@@ -90,20 +90,11 @@ bool CoordinatesScrollingForFrameView(LocalFrameView*) const; // Called when any frame has done its layout or compositing has changed. - void NotifyGeometryChanged(); - // Called when any frame recalculates its overflows after style change. - void NotifyOverflowUpdated(); + void NotifyGeometryChanged(LocalFrameView*); // Called when any layoutBox has transform changed void NotifyTransformChanged(const LayoutBox&); - void UpdateAfterCompositingChangeIfNeeded(); - - // Should be called whenever a frameview visibility is changed. - void FrameViewVisibilityDidChange(); - - // Should be called whenever a scrollable area is added or removed, or - // gains/loses a composited layer. - void ScrollableAreasDidChange(); + void UpdateAfterCompositingChangeIfNeeded(LocalFrameView*); // Should be called whenever the slow repaint objects counter changes between // zero and one. @@ -172,21 +163,15 @@ // Dirty flags used to idenfity what really needs to be computed after // compositing is updated. - bool scroll_gesture_region_is_dirty_; bool touch_event_target_rects_are_dirty_; bool should_scroll_on_main_thread_dirty_; private: - bool ShouldUpdateAfterCompositingChange() const { - return scroll_gesture_region_is_dirty_ || - touch_event_target_rects_are_dirty_ || - should_scroll_on_main_thread_dirty_ || FrameScrollerIsDirty(); - } - void SetShouldUpdateScrollLayerPositionOnMainThread( MainThreadScrollingReasons); - void SetShouldHandleScrollGestureOnMainThreadRegion(const Region&); + void SetShouldHandleScrollGestureOnMainThreadRegion(const Region&, + LocalFrameView*); void SetTouchEventTargetRects(LayerHitTestRects&); void ComputeTouchEventTargetRects(LayerHitTestRects&);
diff --git a/third_party/WebKit/Source/core/paint/compositing/CompositedLayerMapping.cpp b/third_party/WebKit/Source/core/paint/compositing/CompositedLayerMapping.cpp index 4d14b12..f64a0083 100644 --- a/third_party/WebKit/Source/core/paint/compositing/CompositedLayerMapping.cpp +++ b/third_party/WebKit/Source/core/paint/compositing/CompositedLayerMapping.cpp
@@ -2510,7 +2510,9 @@ if (scrolling_coordinator && scrollable_area) { scrolling_coordinator->ScrollableAreaScrollLayerDidChange( scrollable_area); - scrolling_coordinator->ScrollableAreasDidChange(); + const auto& object = GetLayoutObject(); + if (object.IsLayoutView()) + ToLayoutView(object).GetFrameView()->ScrollableAreasDidChange(); } } } else if (scrolling_layer_) { @@ -2520,7 +2522,9 @@ if (scrolling_coordinator && scrollable_area) { scrolling_coordinator->ScrollableAreaScrollLayerDidChange( scrollable_area); - scrolling_coordinator->ScrollableAreasDidChange(); + const auto& object = GetLayoutObject(); + if (object.IsLayoutView()) + ToLayoutView(object).GetFrameView()->ScrollableAreasDidChange(); } }
diff --git a/third_party/WebKit/Source/core/paint/compositing/PaintLayerCompositor.cpp b/third_party/WebKit/Source/core/paint/compositing/PaintLayerCompositor.cpp index b868bc4..25b2495 100644 --- a/third_party/WebKit/Source/core/paint/compositing/PaintLayerCompositor.cpp +++ b/third_party/WebKit/Source/core/paint/compositing/PaintLayerCompositor.cpp
@@ -479,8 +479,10 @@ if (layers_changed) { update_type = std::max(update_type, kCompositingUpdateRebuildTree); if (ScrollingCoordinator* scrolling_coordinator = - GetScrollingCoordinator()) - scrolling_coordinator->NotifyGeometryChanged(); + GetScrollingCoordinator()) { + LocalFrameView* frame_view = layout_view_.GetFrameView(); + scrolling_coordinator->NotifyGeometryChanged(frame_view); + } } }
diff --git a/third_party/WebKit/Source/core/testing/Internals.cpp b/third_party/WebKit/Source/core/testing/Internals.cpp index 6fd09cf..da4de1ca 100644 --- a/third_party/WebKit/Source/core/testing/Internals.cpp +++ b/third_party/WebKit/Source/core/testing/Internals.cpp
@@ -2013,8 +2013,15 @@ } if (ScrollingCoordinator* scrolling_coordinator = - document->GetPage()->GetScrollingCoordinator()) - scrolling_coordinator->UpdateAfterCompositingChangeIfNeeded(); + document->GetPage()->GetScrollingCoordinator()) { + FrameView* view = document->GetPage()->MainFrame()->View(); + if (view->IsLocalFrameView()) { + scrolling_coordinator->UpdateAfterCompositingChangeIfNeeded( + static_cast<LocalFrameView*>(view)); + } else { + NOTREACHED(); + } + } LayoutViewItem view = document->GetLayoutViewItem(); if (!view.IsNull()) {
diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml index 0bb5cef..b7e6afd 100644 --- a/tools/metrics/histograms/enums.xml +++ b/tools/metrics/histograms/enums.xml
@@ -40289,7 +40289,7 @@ <int value="4" label="Failed to start"/> <int value="5" label="Registry exit code (deprecated)"/> <int value="6" label="Reset retries (deprecated)"/> - <int value="7" label="Begin SRT download"/> + <int value="7" label="Begin SRT download (deprecated)"/> <int value="8" label="No browser to show prompt"/> <int value="9" label="No local state (deprecated)"/> <int value="10" label="No prompt needed"/>