Web Audio: when media playback requires a user gesture, apply rule to cross origin iframes.

third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp

Index: third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp
diff --git a/third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp b/third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp
index 2d1e489aa55875d48d66576e229779f104f79649..cd2a3939e8a81c50f177a0bb4b57b7d00cf127db 100644
--- a/third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp
+++ b/third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp
@@ -33,9 +33,10 @@
 #include "core/dom/Document.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/dom/ExecutionContextTask.h"
-#include "core/frame/Deprecation.h"
 #include "core/frame/Settings.h"
 #include "core/html/HTMLMediaElement.h"
+#include "core/inspector/ConsoleMessage.h"
+#include "core/inspector/ConsoleTypes.h"
 #include "modules/mediastream/MediaStream.h"
 #include "modules/webaudio/AnalyserNode.h"
 #include "modules/webaudio/AudioBuffer.h"
@@ -76,18 +77,6 @@
 
 namespace blink {
 
-namespace {
-
-enum UserGestureRecord {
-    UserGestureRequiredAndAvailable = 0,
-    UserGestureRequiredAndNotAvailable,
-    UserGestureNotRequiredAndAvailable,
-    UserGestureNotRequiredAndNotAvailable,
-    UserGestureRecordMax
-};
-
-} // anonymous namespace
-
 BaseAudioContext* BaseAudioContext::create(Document& document, ExceptionState& exceptionState)
 {
     return AudioContext::create(document, exceptionState);
@@ -113,11 +102,12 @@ BaseAudioContext::BaseAudioContext(Document* document)
     , m_periodicWaveSawtooth(nullptr)
     , m_periodicWaveTriangle(nullptr)
 {
-    // TODO(mlamouri): we might want to use other ways of checking for this but
-    // in order to record metrics, re-using the HTMLMediaElement setting is
-    // probably the simplest solution.
-    if (document->settings() && document->settings()->mediaPlaybackRequiresUserGesture())
+    // If mediaPlaybackRequiresUserGesture is enabled, cross origin iframes will

[Raymond Toy, 2016/09/26 16:09:54]

Is mediaPlaybackRequiresUserGesture enabled for all platforms?  Or is this
currently intended only for Android?

[mlamouri (slow - plz ping), 2016/09/26 17:01:24]

Only enabled on Android.

[Raymond Toy, 2016/09/26 17:24:39]

Can you just a note that this is for Android?  Without digging through lots of
code, it's not obvious.

[mlamouri (slow - plz ping), 2016/09/27 14:11:15]

I don't think that this would be a good idea. It is a recipe to end up with
outdated comments.

[Raymond Toy, 2016/09/28 16:13:47]

Fair enough.  But its only outdated if, in the future, you want the gesture to
apply on platforms other than Android.  I think there would be strong opposition
to that.

[mlamouri (slow - plz ping), 2016/09/28 16:17:25]

Indeed. But I don't want to close this door :) 

+    // require user gesture for the AudioContext to produce sound.
+    if (document->settings() && document->settings()->mediaPlaybackRequiresUserGesture()
+        && document->frame() && document->frame()->isCrossOriginSubframe()) {
         m_userGestureRequired = true;
+    }
 
     m_destinationNode = DefaultAudioDestinationNode::create(this);
 
@@ -141,11 +131,6 @@ BaseAudioContext::BaseAudioContext(Document* document, unsigned numberOfChannels
     , m_periodicWaveSawtooth(nullptr)
     , m_periodicWaveTriangle(nullptr)
 {
-    // TODO(mlamouri): we might want to use other ways of checking for this but
-    // in order to record metrics, re-using the HTMLMediaElement setting is
-    // probably the simplest solution.
-    if (document->settings() && document->settings()->mediaPlaybackRequiresUserGesture())
-        m_userGestureRequired = true;
 }
 
 BaseAudioContext::~BaseAudioContext()
@@ -543,32 +528,6 @@ PeriodicWave* BaseAudioContext::periodicWave(int type)
     }
 }
 
-void BaseAudioContext::recordUserGestureState()
-{
-    DEFINE_STATIC_LOCAL(EnumerationHistogram, userGestureHistogram, ("WebAudio.UserGesture", UserGestureRecordMax));
-
-    if (!m_userGestureRequired) {
-        if (UserGestureIndicator::processingUserGesture())
-            userGestureHistogram.count(UserGestureNotRequiredAndAvailable);
-        else
-            userGestureHistogram.count(UserGestureNotRequiredAndNotAvailable);
-        return;
-    }
-
-    DCHECK(m_userGestureRequired);
-    if (!UserGestureIndicator::processingUserGesture()) {
-        userGestureHistogram.count(UserGestureRequiredAndNotAvailable);
-
-        Document* document = toDocument(getExecutionContext());
-        if (document)
-            Deprecation::countDeprecationCrossOriginIframe(*document, UseCounter::WebAudioAutoplayCrossOriginIframe);
-
-        return;
-    }
-    userGestureHistogram.count(UserGestureRequiredAndAvailable);
-    m_userGestureRequired = false;
-}
-
 String BaseAudioContext::state() const
 {
     // These strings had better match the strings for AudioContextState in AudioContext.idl.
@@ -813,7 +772,13 @@ void BaseAudioContext::startRendering()
     DCHECK(isMainThread());
     DCHECK(m_destinationNode);
 
-    recordUserGestureState();
+    if (m_userGestureRequired) {
+        if (!UserGestureIndicator::processingUserGesture()) {
+            toDocument(getExecutionContext())->addConsoleMessage(ConsoleMessage::create(JSMessageSource, WarningMessageLevel, "An AudioContext in a cross origin iframe must be created from a user gesture handler or have resume() called from one."));

[Raymond Toy, 2016/09/26 16:09:54]

The message says the context must be created from a user gesture handler.  This
is not correct is it? I thought you could create the context, but you must
resume it from a gesture handler.

[mlamouri (slow - plz ping), 2016/09/26 17:01:24]

Both are valid: you can create it from a user gesture or call resume() from a
user gesture. You need to do one or the other. This is what I tried to express
in the message. I'm glad to rephrase if you have a suggestion (I spent a
ridiculous amount of time rewriting this message already :))

[Raymond Toy, 2016/09/26 17:24:38]

Yeah, concise messages are really hard.  Oh, you already mentioned created or
resumed. (My window was small enough that the continuation to the next line made
sense so I didn't see the resume part.)

I think you can make the message shorter by saying "created or resumed from a
user gesture to enable audio output", and get rid of the "or have resume()
called...". I think this is a bit clearer on what you need to do to get audio
output.

[mlamouri (slow - plz ping), 2016/09/27 14:11:15]

Done.

+            return;
+        }
+        m_userGestureRequired = false;
+    }

[Raymond Toy, 2016/09/26 16:09:54]

I think this block of code should be in the caller. My expectation is that
startRendering will start rendering.  I think it should be in
AudioContext::create(Document&, ExceptionState&).

[mlamouri (slow - plz ping), 2016/09/26 17:01:24]

Sure. I will add a DCHECK though to make sure we don't miss a call.

[Raymond Toy, 2016/09/26 17:24:39]

Yeah, that's fine with me.

 
     if (m_contextState == Suspended) {
         destination()->audioDestinationHandler().startRendering();