PaymentRequest.abort() should return a promise.

third_party/WebKit/Source/modules/payments/PaymentRequest.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/payments/PaymentRequest.h"
 
 #include "bindings/core/v8/ExceptionState.h"
 #include "bindings/core/v8/JSONValuesForV8.h"
 #include "bindings/core/v8/ScriptPromiseResolver.h"
 #include "bindings/core/v8/ScriptState.h"
@@ skipping 234 matching lines @@
     DCHECK(!m_paymentProvider.is_bound());
     scriptState->domWindow()->frame()->serviceRegistry()->connectToRemoteService(mojo::GetProxy(&m_paymentProvider));
     m_paymentProvider.set_connection_error_handler(createBaseCallback(bind(&PaymentRequest::OnError, WeakPersistentThisPointer<PaymentRequest>(this))));
     m_paymentProvider->SetClient(m_clientBinding.CreateInterfacePtrAndBind());
     m_paymentProvider->Show(std::move(m_supportedMethods), mojom::blink::PaymentDetails::From(m_details), mojom::blink::PaymentOptions::From(m_options), m_stringifiedData.isNull() ? "" : m_stringifiedData);
 
     m_showResolver = ScriptPromiseResolver::create(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
 {
     return EventTargetNames::PaymentRequest;
 }
 
 ExecutionContext* PaymentRequest::getExecutionContext() const
 {
     return ContextLifecycleObserver::getExecutionContext();
@@ skipping 47 matching lines @@
     clearResolversAndCloseMojoConnection();
 }
 
 DEFINE_TRACE(PaymentRequest)
 {
     visitor->trace(m_details);
     visitor->trace(m_options);
     visitor->trace(m_shippingAddress);
     visitor->trace(m_showResolver);
     visitor->trace(m_completeResolver);
+    visitor->trace(m_abortResolver);
     EventTargetWithInlineData::trace(visitor);
     ContextLifecycleObserver::trace(visitor);
 }
 
 PaymentRequest::PaymentRequest(ScriptState* scriptState, const Vector<String>& supportedMethods, const PaymentDetails& details, const PaymentOptions& options, const ScriptValue& data, ExceptionState& exceptionState)
     : ContextLifecycleObserver(scriptState->getExecutionContext())
     , ActiveScriptWrappable(this)
     , m_options(options)
     , m_clientBinding(this)
 {
@@ skipping 130 matching lines @@
     // connection to display a success or failure message to the user.
     m_showResolver.clear();
 }
 
 void PaymentRequest::OnError()
 {
     if (m_completeResolver)
         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();
 }
 
 void PaymentRequest::OnComplete()
 {
     DCHECK(m_completeResolver);
     m_completeResolver->resolve();
     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();
 }
 
 } // namespace blink