Add promise-based addIceCandidate, setLocalDescription and setRemoteDescription to RTCPeerConnection

third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp

+// Copyright 2016 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 "modules/mediastream/RTCVoidRequestPromiseImpl.h"
+
+#include "bindings/core/v8/ScriptPromiseResolver.h"
+#include "core/dom/DOMException.h"
+#include "modules/mediastream/RTCPeerConnection.h"
+
+namespace blink {
+
+RTCVoidRequestPromiseImpl* RTCVoidRequestPromiseImpl::create(RTCPeerConnection* requester, ScriptPromiseResolver* resolver, const String& errorName)
+{
+    RTCVoidRequestPromiseImpl* request = new RTCVoidRequestPromiseImpl(requester, resolver, errorName);
+    return request;
+}
+
+RTCVoidRequestPromiseImpl::RTCVoidRequestPromiseImpl(RTCPeerConnection* requester, ScriptPromiseResolver* resolver, const String& errorName)
+    : m_resolver(resolver)
+    , m_errorName(errorName)
+    , m_requester(requester)
+{
+    ASSERT(m_resolver);
+    ASSERT(m_requester);
+    m_resolver->keepAliveWhilePending();
+}
+
+RTCVoidRequestPromiseImpl::~RTCVoidRequestPromiseImpl()
+{
+}
+
+void RTCVoidRequestPromiseImpl::requestSucceeded()
+{
+    bool shouldFireCallback = m_requester && m_requester->shouldFireDefaultCallbacks() && m_resolver->executionContext() && !m_resolver->executionContext()->activeDOMObjectsAreStopped();

[yhirano, 2016/02/11 01:58:17]

When keepAliveWhilePending is called, the resolver is kept alive until resolved,
rejected or ActiveDOMObjects are stopped. Why is it OK to not call resolve when
shouldFireDefaultCallbacks()?

[Guido Urdaneta, 2016/02/11 10:47:08]

With regards to the ActiveDOMObjectsAreStopped() check, is it OK? is it
unnecessary because I called keepAliveWhilePending()? should I instead not call
keepAliveWhilePending()?

With regards to shouldFireDefaultCallbacks(), that one returns false if the
RTCPeerConnection is closed/stopped. According to the spec, we should neither
resolve nor reject the promise in that case.

[yhirano, 2016/02/11 18:28:31]

I'd recommend calling resolve or reject unconditionally, because they are no-op
when already resolved or rejected, or active dom objects are stopped.
You don't need to clear the resolver pointer in clear() because all pointers
held in the resolver will be cleared when resolved, rejected or stopped.

[Guido Urdaneta, 2016/02/11 20:00:05]

Removed the unnecessary activeDOMObjects check.
However, did not remove the other check because the spec says the promise should
be left unresolved if the RTCPeerConnection is closed.

[yhirano, 2016/02/11 21:56:30]

It's problematic because the resolver continues to be kept alive in such a case.
Can you call m_resolver->stop() if shouldFireCallback is false? I would like
some comments here and the below, for example "This is needed to have the
resolver release its internal resources while leaving the associated promise
pending as specified".

[Guido Urdaneta, 2016/02/11 22:27:36]

Done.

[haraken, 2016/02/15 01:31:31]

It sounds weird that a promise is left without getting resolved or rejected. (I
believe that the contract of Promise is that it must be resolved or rejected.)
Maybe is it worth revisiting the spec?

+    if (shouldFireCallback)
+        m_resolver->resolve();
+
+    clear();
+}
+
+void RTCVoidRequestPromiseImpl::requestFailed(const String& error)
+{
+    bool shouldFireCallback = m_requester && m_requester->shouldFireDefaultCallbacks() && m_resolver->executionContext() && !m_resolver->executionContext()->activeDOMObjectsAreStopped();
+    if (shouldFireCallback)
+        m_resolver->reject(DOMException::create(error, m_errorName));
+
+    clear();
+}
+
+void RTCVoidRequestPromiseImpl::clear()
+{
+    m_resolver.clear();
+    m_requester.clear();
+}
+
+DEFINE_TRACE(RTCVoidRequestPromiseImpl)
+{
+    visitor->trace(m_resolver);
+    visitor->trace(m_requester);
+    RTCVoidRequest::trace(visitor);
+}
+
+} // namespace blink