Adding Rappor metrics for disabling cross-origin autoplay with sound experiment

third_party/WebKit/Source/core/html/HTMLMediaElement.cpp

Index: third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
diff --git a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
index db78202be84776c2d54247c0cb3323b16f52b7f8..af1292d0c04b7dea17053b6988e2fbf65253eb35 100644
--- a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
@@ -437,6 +437,7 @@ HTMLMediaElement::HTMLMediaElement(const QualifiedName& tagName,
       m_fragmentEndTime(std::numeric_limits<double>::quiet_NaN()),
       m_pendingActionFlags(0),
       m_lockedPendingUserGesture(false),
+      m_lockedPendingUserGestureIfCrossOriginExperimentEnabled(true),
       m_playing(false),
       m_shouldDelayLoadEvent(false),
       m_haveFiredLoadedData(false),
@@ -467,6 +468,8 @@ HTMLMediaElement::HTMLMediaElement(const QualifiedName& tagName,
   BLINK_MEDIA_LOG << "HTMLMediaElement(" << (void*)this << ")";
 
   m_lockedPendingUserGesture = computeLockedPendingUserGesture(document);
+  m_lockedPendingUserGestureIfCrossOriginExperimentEnabled =
+      isDocumentCrossOrigin(document);
 
   LocalFrame* frame = document.frame();
   if (frame) {
@@ -513,9 +516,8 @@ void HTMLMediaElement::didMoveToNewDocument(Document& oldDocument) {
       computeLockedPendingUserGesture(oldDocument);
   bool newDocumentRequiresUserGesture =
       computeLockedPendingUserGesture(document());
-  if (newDocumentRequiresUserGesture && !oldDocumentRequiresUserGesture) {
+  if (newDocumentRequiresUserGesture && !oldDocumentRequiresUserGesture)
     m_lockedPendingUserGesture = true;
-  }
 
   if (m_shouldDelayLoadEvent) {
     document().incrementLoadEventDelayCount();
@@ -528,6 +530,9 @@ void HTMLMediaElement::didMoveToNewDocument(Document& oldDocument) {
     oldDocument.incrementLoadEventDelayCount();
   }
 
+  if (isDocumentCrossOrigin(document()) && !isDocumentCrossOrigin(oldDocument))
+    m_lockedPendingUserGestureIfCrossOriginExperimentEnabled = true;

[whywhat, 2016/11/29 21:42:04]

should you make it false in the case the new document is not cross-origin?
what about cross-origin to cross-origin move where the element was unlocked in
the old document?

could we just do m_lockedPendingUserGesture =
computeLockedPendingUserGesture(document()) and check if the document is
cross-origin?

[Zhiqiang Zhang (Slow), 2016/11/30 17:30:52]

Please see the other reply.

+
   removeElementFromDocumentMap(this, &oldDocument);
   addElementToDocumentMap(this, &document());
 
@@ -1371,6 +1376,10 @@ bool HTMLMediaElement::isMediaDataCORSSameOrigin(SecurityOrigin* origin) const {
           !origin->taintsCanvas(currentSrc()));
 }
 
+bool HTMLMediaElement::isCrossOrigin() const {
+  return isDocumentCrossOrigin(document());
+}
+
 void HTMLMediaElement::startProgressEventTimer() {
   if (m_progressEventTimer.isActive())
     return;
@@ -1736,6 +1745,13 @@ void HTMLMediaElement::setReadyState(ReadyState state) {
       m_autoplayHelper->becameReadyToPlay();
 
       if (!isGestureNeededForPlayback()) {
+        if (isGestureNeededForPlaybackIfCrossOriginExperimentEnabled()) {
+          m_autoplayUmaHelper->recordCrossOriginExperimentMetric(
+              AutoplayCrossOriginExperimentMetric::AutoplayBlocked);
+        } else {
+          m_autoplayUmaHelper->recordCrossOriginExperimentMetric(
+              AutoplayCrossOriginExperimentMetric::AutoplayAllowed);
+        }
         if (isHTMLVideoElement() && muted() &&
             RuntimeEnabledFeatures::autoplayMutedVideosEnabled()) {
           // We might end up in a situation where the previous
@@ -1754,6 +1770,9 @@ void HTMLMediaElement::setReadyState(ReadyState state) {
           scheduleNotifyPlaying();
           m_autoplaying = false;
         }
+      } else {
+        m_autoplayUmaHelper->recordCrossOriginExperimentMetric(
+            AutoplayCrossOriginExperimentMetric::AutoplayBlocked);
       }
     }
 
@@ -2246,6 +2265,8 @@ Nullable<ExceptionCode> HTMLMediaElement::play() {
         return nullptr;
       }
 
+      m_autoplayUmaHelper->recordCrossOriginExperimentMetric(
+          AutoplayCrossOriginExperimentMetric::AutoplayBlocked);
       recordAutoplayMetric(PlayMethodFailed);
       String message = ExceptionMessages::failedToExecute(
           "play", "HTMLMediaElement",
@@ -2253,8 +2274,18 @@ Nullable<ExceptionCode> HTMLMediaElement::play() {
       document().addConsoleMessage(ConsoleMessage::create(
           JSMessageSource, WarningMessageLevel, message));
       return NotAllowedError;
+    } else {
+      if (isGestureNeededForPlaybackIfCrossOriginExperimentEnabled()) {
+        m_autoplayUmaHelper->recordCrossOriginExperimentMetric(
+            AutoplayCrossOriginExperimentMetric::AutoplayBlocked);
+      } else {
+        m_autoplayUmaHelper->recordCrossOriginExperimentMetric(

[whywhat, 2016/11/29 21:42:04]

seems like we'll record some metrics when the experiment is disabled, is that
intended?

[Zhiqiang Zhang (Slow), 2016/11/30 17:30:52]

Yes :)
Since the experiment might break many sites if it's enabled, we are recording
metrics "pretending" the experiment is enabled. This also answers why I added
"isLockedPendingUserGestureIfCrossOriginExperimentEnabled()" by side of
"isLockedPendingUserGesture()". Ditto for some other of your comments.

+            AutoplayCrossOriginExperimentMetric::AutoplayAllowed);
+      }
     }
   } else {
+    m_autoplayUmaHelper->recordCrossOriginExperimentMetric(
+        AutoplayCrossOriginExperimentMetric::PlayedWithGesture);
     UserGestureIndicator::utilizeUserGesture();
     // We ask the helper to remove the gesture requirement for us, so that
     // it can record the reason.
@@ -3904,6 +3935,7 @@ bool HTMLMediaElement::isLockedPendingUserGesture() const {
 
 void HTMLMediaElement::unlockUserGesture() {
   m_lockedPendingUserGesture = false;
+  m_lockedPendingUserGestureIfCrossOriginExperimentEnabled = false;
 }
 
 bool HTMLMediaElement::isGestureNeededForPlayback() const {
@@ -3933,6 +3965,23 @@ bool HTMLMediaElement::isGestureNeededForPlayback() const {
   return true;
 }
 
+bool HTMLMediaElement::

[whywhat, 2016/11/29 21:42:04]

how is this different from the isGestureNeededForPlayback from above?
add a check for MediaStream that you probably want and autoplay helper (which is
being removed) and it's identicall, no?

I thought you would implement it as something like

return isGestureNeededForPlayback() && isCrossOrigin() &&
isCrossOriginExperimentEnabled();

Or rather merge it to isGestureNeededForPlayback as suggested above.

[Zhiqiang Zhang (Slow), 2016/11/30 17:30:52]

Done. Added the missing checks.
The goal is to record metrics pretending that the flag is enabled even if it's
not. The best solution I can come up with is the current solution.

+    isGestureNeededForPlaybackIfCrossOriginExperimentEnabled() const {
+  if (!m_lockedPendingUserGestureIfCrossOriginExperimentEnabled)
+    return false;
+
+  if (isHTMLVideoElement() && muted() &&
+      RuntimeEnabledFeatures::autoplayMutedVideosEnabled() &&
+      !(document().settings() && document().settings()->dataSaverEnabled()) &&
+      !(document().settings() &&
+        document().settings()->forcePreloadNoneForMediaElements()) &&
+      isAutoplayAllowedPerSettings()) {
+    return false;
+  }
+
+  return true;
+}
+
 bool HTMLMediaElement::isAutoplayAllowedPerSettings() const {
   LocalFrame* frame = document().frame();
   if (!frame)