From ad90070a3f3ce95f4dd9881a5c91965828e377f9 Mon Sep 17 00:00:00 2001 From: Mounir Lamouri Date: Tue, 17 Oct 2017 14:41:02 +0000 Subject: [PATCH] Autoplay: records metrics in UKM entries. This is using the UKM service to record the autoplay attempts and the unmute results. Bug: 744659 Change-Id: Ice9a259a4006beff1044e5ed62981619841895e5 Reviewed-on: https://chromium-review.googlesource.com/525692 Commit-Queue: Mounir Lamouri Reviewed-by: Chris Palmer Reviewed-by: Steven Holte Reviewed-by: Kentaro Hara Reviewed-by: Mike West Cr-Commit-Position: refs/heads/master@{#509384} --- .../app/mojo/content_renderer_manifest.json | 2 +- .../metrics/public/cpp/mojo_ukm_recorder.h | 4 +- services/metrics/public/cpp/ukm_recorder.h | 5 ++ third_party/WebKit/PRESUBMIT.py | 8 +-- third_party/WebKit/Source/core/DEPS | 1 + third_party/WebKit/Source/core/dom/BUILD.gn | 5 ++ .../WebKit/Source/core/dom/Document.cpp | 24 ++++++++ third_party/WebKit/Source/core/dom/Document.h | 15 ++++- third_party/WebKit/Source/core/html/BUILD.gn | 4 ++ .../core/html/media/AutoplayUmaHelper.cpp | 58 +++++++++++++++++++ .../core/html/media/AutoplayUmaHelper.h | 8 +++ .../WebKit/Source/platform/weborigin/KURL.cpp | 5 ++ .../WebKit/Source/platform/weborigin/KURL.h | 7 +++ tools/metrics/ukm/ukm.xml | 58 +++++++++++++++++++ 14 files changed, 197 insertions(+), 7 deletions(-) diff --git a/content/public/app/mojo/content_renderer_manifest.json b/content/public/app/mojo/content_renderer_manifest.json index 1e773f67149a4c..ab135e15269f44 100644 --- a/content/public/app/mojo/content_renderer_manifest.json +++ b/content/public/app/mojo/content_renderer_manifest.json @@ -24,7 +24,7 @@ }, "requires": { "*": [ "app" ], - "content_browser": [ "renderer" ], + "content_browser": [ "renderer", "url_keyed_metrics" ], "device": [ "device:power_monitor", "device:screen_orientation", diff --git a/services/metrics/public/cpp/mojo_ukm_recorder.h b/services/metrics/public/cpp/mojo_ukm_recorder.h index 514be3f5656317..032a105faf6149 100644 --- a/services/metrics/public/cpp/mojo_ukm_recorder.h +++ b/services/metrics/public/cpp/mojo_ukm_recorder.h @@ -30,9 +30,11 @@ class METRICS_EXPORT MojoUkmRecorder : public UkmRecorder { explicit MojoUkmRecorder(mojom::UkmRecorderInterfacePtr interface); ~MojoUkmRecorder() override; - private: // UkmRecorder: void UpdateSourceURL(SourceId source_id, const GURL& url) override; + + private: + // UkmRecorder: void AddEntry(mojom::UkmEntryPtr entry) override; mojom::UkmRecorderInterfacePtr interface_; diff --git a/services/metrics/public/cpp/ukm_recorder.h b/services/metrics/public/cpp/ukm_recorder.h index 13bef79aee911b..d0ecdfab406b99 100644 --- a/services/metrics/public/cpp/ukm_recorder.h +++ b/services/metrics/public/cpp/ukm_recorder.h @@ -26,6 +26,10 @@ class SubresourceFilterMetricsObserver; class UkmPageLoadMetricsObserver; class LocalNetworkRequestsPageLoadMetricsObserver; +namespace blink { +class AutoplayUmaHelper; +} + namespace content { class RenderWidgetHostLatencyTracker; } // namespace content @@ -76,6 +80,7 @@ class METRICS_EXPORT UkmRecorder { virtual void UpdateSourceURL(SourceId source_id, const GURL& url) = 0; private: + friend blink::AutoplayUmaHelper; friend ContextualSearchRankerLoggerImpl; friend PluginInfoMessageFilter; friend UkmPageLoadMetricsObserver; diff --git a/third_party/WebKit/PRESUBMIT.py b/third_party/WebKit/PRESUBMIT.py index f5a67a7ffc1f57..d1c997baeeaa17 100644 --- a/third_party/WebKit/PRESUBMIT.py +++ b/third_party/WebKit/PRESUBMIT.py @@ -102,11 +102,11 @@ def _CheckForPrintfDebugging(input_api, output_api): return [] -def _CheckForForbiddenNamespace(input_api, output_api): - """Checks that Blink uses Chromium namespaces only in permitted code.""" +def _CheckForForbiddenChromiumCode(input_api, output_api): + """Checks that Blink uses Chromium classes and namespaces only in permitted code.""" # This list is not exhaustive, but covers likely ones. chromium_namespaces = ["base", "cc", "content", "gfx", "net", "ui"] - chromium_forbidden_classes = [] + chromium_forbidden_classes = ["GURL"] chromium_allowed_classes = [ "base::make_span", "base::span", @@ -161,7 +161,7 @@ def CheckChangeOnUpload(input_api, output_api): results.extend(_CommonChecks(input_api, output_api)) results.extend(_CheckStyle(input_api, output_api)) results.extend(_CheckForPrintfDebugging(input_api, output_api)) - results.extend(_CheckForForbiddenNamespace(input_api, output_api)) + results.extend(_CheckForForbiddenChromiumCode(input_api, output_api)) return results diff --git a/third_party/WebKit/Source/core/DEPS b/third_party/WebKit/Source/core/DEPS index 40411c32e92162..689b9c7533a75e 100644 --- a/third_party/WebKit/Source/core/DEPS +++ b/third_party/WebKit/Source/core/DEPS @@ -14,6 +14,7 @@ include_rules = [ "+platform", "+public/platform", "+public/web", + "+services/metrics/public", "+services/network/public/interfaces", "+services/service_manager/public/interfaces/interface_provider.mojom-blink.h", "+third_party/skia/include", diff --git a/third_party/WebKit/Source/core/dom/BUILD.gn b/third_party/WebKit/Source/core/dom/BUILD.gn index 9de0fd42cdea7e..c37d6be1385d6b 100644 --- a/third_party/WebKit/Source/core/dom/BUILD.gn +++ b/third_party/WebKit/Source/core/dom/BUILD.gn @@ -361,4 +361,9 @@ blink_core_sources("dom") { public_deps = [ "//third_party/WebKit/common:mojo_bindings_blink", ] + + deps = [ + "//services/metrics/public/cpp:metrics_cpp", + "//services/metrics/public/interfaces", + ] } diff --git a/third_party/WebKit/Source/core/dom/Document.cpp b/third_party/WebKit/Source/core/dom/Document.cpp index 13e2fc04613ec5..13450a75df441f 100644 --- a/third_party/WebKit/Source/core/dom/Document.cpp +++ b/third_party/WebKit/Source/core/dom/Document.cpp @@ -256,11 +256,14 @@ #include "platform/wtf/text/CharacterNames.h" #include "platform/wtf/text/StringBuffer.h" #include "platform/wtf/text/TextEncodingRegistry.h" +#include "public/platform/InterfaceProvider.h" #include "public/platform/Platform.h" #include "public/platform/WebAddressSpace.h" #include "public/platform/WebPrerenderingSupport.h" #include "public/platform/modules/insecure_input/insecure_input_service.mojom-blink.h" #include "public/platform/site_engagement.mojom-blink.h" +#include "services/metrics/public/cpp/mojo_ukm_recorder.h" +#include "services/metrics/public/interfaces/ukm_interface.mojom-shared.h" #include "services/service_manager/public/cpp/interface_provider.h" #ifndef NDEBUG @@ -3696,6 +3699,9 @@ void Document::SetURL(const KURL& url) { access_entry_from_url_ = nullptr; UpdateBaseURL(); GetContextFeatures().UrlDidChange(this); + + if (ukm_recorder_) + ukm_recorder_->UpdateSourceURL(ukm_source_id_, url_); } KURL Document::ValidBaseElementURL() const { @@ -5960,6 +5966,24 @@ void Document::SetFeaturePolicy(const String& feature_policy_header) { frame_->Client()->DidSetFeaturePolicyHeader(parsed_header); } +ukm::UkmRecorder* Document::UkmRecorder() { + if (ukm_recorder_) + return ukm_recorder_.get(); + + ukm::mojom::UkmRecorderInterfacePtr interface; + Platform::Current()->GetInterfaceProvider()->GetInterface( + mojo::MakeRequest(&interface)); + ukm_recorder_.reset(new ukm::MojoUkmRecorder(std::move(interface))); + ukm_source_id_ = ukm_recorder_->GetNewSourceID(); + ukm_recorder_->UpdateSourceURL(ukm_source_id_, url_); + return ukm_recorder_.get(); +} + +int64_t Document::UkmSourceID() const { + DCHECK(ukm_recorder_); + return ukm_source_id_; +} + void Document::InitSecurityContext(const DocumentInit& initializer) { DCHECK(!GetSecurityOrigin()); diff --git a/third_party/WebKit/Source/core/dom/Document.h b/third_party/WebKit/Source/core/dom/Document.h index de783532f4d014..1e2cfe78840f25 100644 --- a/third_party/WebKit/Source/core/dom/Document.h +++ b/third_party/WebKit/Source/core/dom/Document.h @@ -75,11 +75,15 @@ #include "public/platform/WebFocusType.h" #include "public/platform/WebInsecureRequestPolicy.h" +namespace ukm { +class UkmRecorder; +} // namespace ukm + namespace blink { namespace mojom { enum class EngagementLevel : int32_t; -} +} // namespace mojom class AnimationClock; class AXObjectCache; @@ -1385,6 +1389,9 @@ class CORE_EXPORT Document : public ContainerNode, void captureEvents() {} void releaseEvents() {} + ukm::UkmRecorder* UkmRecorder(); + int64_t UkmSourceID() const; + protected: Document(const DocumentInit&, DocumentClassFlags = kDefaultDocumentClass); @@ -1757,6 +1764,12 @@ class CORE_EXPORT Document : public ContainerNode, bool has_high_media_engagement_; std::unique_ptr document_outlive_time_reporter_; + + // |mojo_ukm_recorder_| and |source_id_| will allow objects that are part of + // the |ukm_recorder_| and |source_id_| will allow objects that are part of + // the document to recorde UKM. + std::unique_ptr ukm_recorder_; + int64_t ukm_source_id_; }; extern template class CORE_EXTERN_TEMPLATE_EXPORT Supplement; diff --git a/third_party/WebKit/Source/core/html/BUILD.gn b/third_party/WebKit/Source/core/html/BUILD.gn index 03e9e33f4d6d6b..e034fc7987a463 100644 --- a/third_party/WebKit/Source/core/html/BUILD.gn +++ b/third_party/WebKit/Source/core/html/BUILD.gn @@ -633,6 +633,10 @@ blink_core_sources("html") { # TODO(jschuh): crbug.com/167187 fix size_t to int truncations. "//build/config/compiler:no_size_t_to_int_warning", ] + + deps = [ + "//services/metrics/public/cpp:metrics_cpp", + ] } fuzzer_test("blink_html_tokenizer_fuzzer") { diff --git a/third_party/WebKit/Source/core/html/media/AutoplayUmaHelper.cpp b/third_party/WebKit/Source/core/html/media/AutoplayUmaHelper.cpp index 4181c8f8b6886d..766e83fb5a2b9c 100644 --- a/third_party/WebKit/Source/core/html/media/AutoplayUmaHelper.cpp +++ b/third_party/WebKit/Source/core/html/media/AutoplayUmaHelper.cpp @@ -13,7 +13,10 @@ #include "core/html/media/HTMLMediaElement.h" #include "platform/Histogram.h" #include "platform/wtf/CurrentTime.h" +#include "public/platform/InterfaceProvider.h" #include "public/platform/Platform.h" +#include "services/metrics/public/cpp/ukm_entry_builder.h" +#include "services/metrics/public/cpp/ukm_recorder.h" namespace blink { @@ -24,6 +27,18 @@ const int32_t kOffscreenDurationUmaBucketCount = 50; const int32_t kMaxWaitTimeUmaMS = 30 * 1000; const int32_t kWaitTimeBucketCount = 50; +const char kAutoplayAttemptUkmEvent[] = "Media.Autoplay.Attempt"; +const char kAutoplayAttemptUkmSourceMetric[] = "Source"; +const char kAutoplayAttemptUkmAudioTrackMetric[] = "AudioTrack"; +const char kAutoplayAttemptUkmVideoTrackMetric[] = "VideoTrack"; +const char kAutoplayAttemptUkmUserGestureRequiredMetric[] = + "UserGestureRequired"; +const char kAutoplayAttemptUkmMutedMetric[] = "Muted"; + +const char kAutoplayMutedUnmuteUkmEvent[] = "Media.Autoplay.Muted.UnmuteAction"; +const char kAutoplayMutedUnmuteUkmSourceMetric[] = "Source"; +const char kAutoplayMutedUnmuteUkmResultMetric[] = "Result"; + } // namespace AutoplayUmaHelper* AutoplayUmaHelper::Create(HTMLMediaElement* element) { @@ -168,6 +183,22 @@ void AutoplayUmaHelper::OnAutoplayInitiated(AutoplaySource source) { } } + // Record UKM autoplay event. + { + std::unique_ptr builder = + CreateUkmBuilder(kAutoplayAttemptUkmEvent); + builder->AddMetric(kAutoplayAttemptUkmSourceMetric, + source == AutoplaySource::kMethod); + builder->AddMetric(kAutoplayAttemptUkmAudioTrackMetric, + element_->HasAudio()); + builder->AddMetric(kAutoplayAttemptUkmVideoTrackMetric, + element_->HasVideo()); + builder->AddMetric( + kAutoplayAttemptUkmUserGestureRequiredMetric, + element_->GetAutoplayPolicy().IsGestureNeededForPlayback()); + builder->AddMetric(kAutoplayAttemptUkmMutedMetric, element_->muted()); + } + element_->addEventListener(EventTypeNames::playing, this, false); } @@ -257,6 +288,24 @@ void AutoplayUmaHelper::RecordAutoplayUnmuteStatus( static_cast(AutoplayUnmuteActionStatus::kNumberOfStatus))); autoplay_unmute_histogram.Count(static_cast(status)); + + // Record UKM event for unmute muted autoplay. + { + std::unique_ptr builder = + CreateUkmBuilder(kAutoplayMutedUnmuteUkmEvent); + + int source = static_cast(AutoplaySource::kAttribute); + if (sources_.size() == + static_cast(AutoplaySource::kNumberOfSources)) { + source = static_cast(AutoplaySource::kDualSource); + } else if (sources_.count(AutoplaySource::kMethod)) { + source = static_cast(AutoplaySource::kAttribute); + } + + builder->AddMetric(kAutoplayMutedUnmuteUkmSourceMetric, source); + builder->AddMetric(kAutoplayMutedUnmuteUkmResultMetric, + status == AutoplayUnmuteActionStatus::kSuccess); + } } void AutoplayUmaHelper::VideoWillBeDrawnToCanvas() { @@ -442,6 +491,15 @@ bool AutoplayUmaHelper::ShouldRecordUserPausedAutoplayingCrossOriginVideo() CrossOriginAutoplayResult::kUserPaused); } +std::unique_ptr AutoplayUmaHelper::CreateUkmBuilder( + const char* event) { + ukm::UkmRecorder* ukm_recorder = element_->GetDocument().UkmRecorder(); + DCHECK(ukm_recorder); + + return ukm_recorder->GetEntryBuilder(element_->GetDocument().UkmSourceID(), + event); +} + DEFINE_TRACE(AutoplayUmaHelper) { EventListener::Trace(visitor); ContextLifecycleObserver::Trace(visitor); diff --git a/third_party/WebKit/Source/core/html/media/AutoplayUmaHelper.h b/third_party/WebKit/Source/core/html/media/AutoplayUmaHelper.h index 0b3c6cf9ab03d1..888890252ce59b 100644 --- a/third_party/WebKit/Source/core/html/media/AutoplayUmaHelper.h +++ b/third_party/WebKit/Source/core/html/media/AutoplayUmaHelper.h @@ -13,6 +13,10 @@ #include +namespace ukm { +class UkmEntryBuilder; +} // namespace ukm + namespace blink { // These values are used for histograms. Do not reorder. @@ -114,6 +118,10 @@ class CORE_EXPORT AutoplayUmaHelper : public EventListener, bool ShouldListenToContextDestroyed() const; bool ShouldRecordUserPausedAutoplayingCrossOriginVideo() const; + // Returns a ukm::UkmEntryBuilder created from the UkmRecorder associated with + // the Document. + std::unique_ptr CreateUkmBuilder(const char*); + // The autoplay sources. std::set sources_; diff --git a/third_party/WebKit/Source/platform/weborigin/KURL.cpp b/third_party/WebKit/Source/platform/weborigin/KURL.cpp index 5df888887150ef..94715cd233c32b 100644 --- a/third_party/WebKit/Source/platform/weborigin/KURL.cpp +++ b/third_party/WebKit/Source/platform/weborigin/KURL.cpp @@ -37,6 +37,7 @@ #include "platform/wtf/text/StringStatics.h" #include "platform/wtf/text/StringUTF8Adaptor.h" #include "platform/wtf/text/TextEncoding.h" +#include "url/gurl.h" #include "url/url_util.h" #ifndef NDEBUG #include @@ -881,4 +882,8 @@ bool KURL::IsSafeToSendToAnotherThread() const { (!inner_url_ || inner_url_->IsSafeToSendToAnotherThread()); } +KURL::operator GURL() const { + return GURL(string_.Utf8().data(), parsed_, is_valid_); +} + } // namespace blink diff --git a/third_party/WebKit/Source/platform/weborigin/KURL.h b/third_party/WebKit/Source/platform/weborigin/KURL.h index 3905842aaaf62e..265bfaabefadb2 100644 --- a/third_party/WebKit/Source/platform/weborigin/KURL.h +++ b/third_party/WebKit/Source/platform/weborigin/KURL.h @@ -61,6 +61,8 @@ namespace WTF { class TextEncoding; } +class GURL; + namespace blink { struct KURLHash; @@ -212,6 +214,11 @@ class PLATFORM_EXPORT KURL { return parsed_.potentially_dangling_markup; } + // Returns a GURL with the same properties. This can be used in platform/ and + // web/. However, in core/ and modules/, this should only be used to pass + // a GURL to a layer that is expecting one instead of a KURL or a WebURL. + operator GURL() const; + private: friend struct WTF::HashTraits; diff --git a/tools/metrics/ukm/ukm.xml b/tools/metrics/ukm/ukm.xml index 67a47129793239..3d9075491c530f 100644 --- a/tools/metrics/ukm/ukm.xml +++ b/tools/metrics/ukm/ukm.xml @@ -514,6 +514,64 @@ be describing additional metrics about the same event. + + mlamouri@chromium.org + media-dev@chromium.org + + Event recorded when there is an attempt to autoplay (ie. no user gesture). + It will be recorded regardless of the result of this attempt. + + + + Whether the element had an audio track when autoplay was attempted. + + + + + Whether the element was muted when autoplay was attempted. + + + + + Source of the autoplay attempt: 0 for attribute; 1 for play(). + + + + + Whether a user gesture was required per autoplay rules at the time of + attempt. By definition there is no user gesture on the stack when the + attempt is registered. + + + + + Whether the element had a video track when autoplay was attempted. + + + + + + mlamouri@chromium.org + media-dev@chromium.org + + Event recorded when there is an attempt to unmute an media that was + autoplaying following the rules of autoplay muted. + + + + 0 means that the unmute failed because it happened without a user gesture. + 1 means that it succeeded because it had a user gesture. + + + + + Similar to "Source" in "Media.Autoplay.Attempt" with + the addition of a value for both sources being used: 0 for attribute; 1 + for play(); 2 for both attempted. + + + + beccahughes@chromium.org media-dev@chromium.org