PaymentRequest.abort() should return a promise.

third_party/WebKit/Source/modules/payments/PaymentRequest.cpp

Index: third_party/WebKit/Source/modules/payments/PaymentRequest.cpp
diff --git a/third_party/WebKit/Source/modules/payments/PaymentRequest.cpp b/third_party/WebKit/Source/modules/payments/PaymentRequest.cpp
index 707eb4766aca5803da6bdd8f08b71ee0bfa6bb34..bfc2522e318339ce44866e6e1c0fcfb053bff007 100644
--- a/third_party/WebKit/Source/modules/payments/PaymentRequest.cpp
+++ b/third_party/WebKit/Source/modules/payments/PaymentRequest.cpp
@@ -252,14 +252,14 @@ ScriptPromise PaymentRequest::show(ScriptState* scriptState)
     return m_showResolver->promise();
 }
 
-void PaymentRequest::abort(ExceptionState& exceptionState)
+ScriptPromise PaymentRequest::abort(ScriptState* scriptState)
 {
-    if (!m_showResolver) {
-        exceptionState.throwDOMException(InvalidStateError, "Never called show(), so nothing to abort");
-        return;
-    }
+    if (!m_showResolver)
+        return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(InvalidStateError, "Never called show(), so nothing to abort"));
 
+    m_abortResolver = ScriptPromiseResolver::create(scriptState);

[Marijn Kruisselbrink, 2016/06/08 22:44:12]

Presumably you want to do something special if m_abortResolver is already set?

The spec seems not quite meaningful if abort is called multiple times either
(it'll end up rejecting the show promise multiple times as well, if I'm reading
the spec correctly).

[please use gerrit instead, 2016/06/09 22:36:14]

Rejecting the second abort() promise is more consistent with how show() and
complete() promises behave.

     m_paymentProvider->Abort();
+    return m_abortResolver->promise();
 }
 
 const AtomicString& PaymentRequest::interfaceName() const
@@ -327,6 +327,7 @@ DEFINE_TRACE(PaymentRequest)
     visitor->trace(m_shippingAddress);
     visitor->trace(m_showResolver);
     visitor->trace(m_completeResolver);
+    visitor->trace(m_abortResolver);
     EventTargetWithInlineData::trace(visitor);
     ContextLifecycleObserver::trace(visitor);
 }
@@ -477,6 +478,8 @@ void PaymentRequest::OnError()
         m_completeResolver->reject(DOMException::create(SyntaxError, "Request cancelled"));
     if (m_showResolver)
         m_showResolver->reject(DOMException::create(SyntaxError, "Request cancelled"));
+    if (m_abortResolver)
+        m_abortResolver->reject(DOMException::create(SyntaxError, "Request cancelled"));
     clearResolversAndCloseMojoConnection();
 }
 
@@ -487,10 +490,26 @@ void PaymentRequest::OnComplete()
     clearResolversAndCloseMojoConnection();
 }
 
+void PaymentRequest::OnAbort(bool abortedSuccessfully)

[haraken, 2016/06/09 01:57:59]

OnAbort => onAbort

[please use gerrit instead, 2016/06/09 22:36:14]

This is an implementation of the interface defined mojom:

https://codereview.chromium.org/2048823004/diff/60001/third_party/WebKit/publ...

I thought mojom follows Chrome C++ standard of using the capital letters for the
first letter of a method name. Should I rename all of these into Blink style
instead?

+{
+    DCHECK(m_abortResolver);
+    DCHECK(m_showResolver);
+
+    if (!abortedSuccessfully) {
+        m_abortResolver->reject(DOMException::create(InvalidStateError));
+        return;
+    }
+
+    m_showResolver->reject(DOMException::create(AbortError));
+    m_abortResolver->resolve();
+    clearResolversAndCloseMojoConnection();
+}
+
 void PaymentRequest::clearResolversAndCloseMojoConnection()
 {
     m_completeResolver.clear();
     m_showResolver.clear();
+    m_abortResolver.clear();
     if (m_clientBinding.is_bound())
         m_clientBinding.Close();
     m_paymentProvider.reset();