Skip to content

GitLab

T
tangled
Commit ba01490b authored by Rouslan Solomakhin's avatar Rouslan Solomakhin Committed by Commit Bot
Browse files
Options

[Web Payment] CanMakePaymentCallback 

Before this patch, both "abort" and "can make payment" shared a
PaymentEventResultCallback type, which creates a lot of code churn when
only one of the event types needs an update.

This patch splits PaymentEventResultCallback into CanMakePaymentCallback
and AbortCallback.

After this patch, changing the parameters for "can make payment"
callback will be easier.

Bug: 1005076
Change-Id: I0df01a8a8b4551a16f1949d77ffd546df7ea5702
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2091646
Commit-Queue: Avi Drissman <avi@chromium.org>
Reviewed-by: default avatarAvi Drissman <avi@chromium.org>
Reviewed-by: default avatarSahel Sharify <sahel@chromium.org>
Auto-Submit: Rouslan Solomakhin <rouslan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749347}
parent 1ceff828
Hide whitespace changes
Inline Side-by-side
Showing with 71 additions and 48 deletions
content/browser/payments/payment_app_browsertest.cc
View file @ ba01490b
......@@ -41,9 +41,16 @@ void GetAllPaymentAppsCallback(base::OnceClosure done_callback,
std::move(done_callback).Run();
}
void PaymentEventResultCallback(base::OnceClosure done_callback,
bool* out_payment_event_result,
bool payment_event_result) {
void CaptureCanMakePaymentResult(base::OnceClosure done_callback,
bool* out_payment_event_result,
bool payment_event_result) {
*out_payment_event_result = payment_event_result;
std::move(done_callback).Run();
}
void CaptureAbortResult(base::OnceClosure done_callback,
bool* out_payment_event_result,
bool payment_event_result) {
*out_payment_event_result = payment_event_result;
std::move(done_callback).Run();
}
......@@ -124,7 +131,7 @@ class PaymentAppBrowserTest : public ContentBrowserTest {
PaymentAppProvider::GetInstance()->AbortPayment(
shell()->web_contents()->GetBrowserContext(), registration_id,
sw_origin, payment_request_id,
base::BindOnce(&PaymentEventResultCallback, run_loop.QuitClosure(),
base::BindOnce(&CaptureAbortResult, run_loop.QuitClosure(),
&payment_aborted));
run_loop.Run();
......@@ -143,7 +150,7 @@ class PaymentAppBrowserTest : public ContentBrowserTest {
PaymentAppProvider::GetInstance()->CanMakePayment(
shell()->web_contents()->GetBrowserContext(), registration_id,
sw_origin, payment_request_id, std::move(event_data),
base::BindOnce(&PaymentEventResultCallback, run_loop.QuitClosure(),
base::BindOnce(&CaptureCanMakePaymentResult, run_loop.QuitClosure(),
&can_make_payment));
run_loop.Run();
......
content/browser/payments/payment_app_provider_impl.cc
View file @ ba01490b
......@@ -120,16 +120,20 @@ class InvokePaymentAppCallbackRepository {
// Note that one and only one of the callbacks from this class must/should be
// called.
// TODO(crbug.com/1060298): Split RespondWithCallbacks into three classes with
// one callback each.
class RespondWithCallbacks : public PaymentHandlerResponseCallback {
public:
static RespondWithCallbacks* CreateForCanMakePayment(
BrowserContext* browser_context,
scoped_refptr<ServiceWorkerVersion> service_worker_version,
PaymentAppProvider::PaymentEventResultCallback callback) {
PaymentAppProvider::CanMakePaymentCallback callback) {
RespondWithCallbacks* callbacks = new RespondWithCallbacks(
browser_context, ServiceWorkerMetrics::EventType::CAN_MAKE_PAYMENT,
service_worker_version, PaymentAppProvider::InvokePaymentAppCallback(),
/*event_callback=*/std::move(callback));
service_worker_version,
/*can_make_payment_callback=*/std::move(callback),
PaymentAppProvider::InvokePaymentAppCallback(),
PaymentAppProvider::AbortCallback());
return callbacks;
}
......@@ -139,9 +143,9 @@ class RespondWithCallbacks : public PaymentHandlerResponseCallback {
PaymentAppProvider::InvokePaymentAppCallback callback) {
RespondWithCallbacks* callbacks = new RespondWithCallbacks(
browser_context, ServiceWorkerMetrics::EventType::PAYMENT_REQUEST,
service_worker_version,
service_worker_version, PaymentAppProvider::CanMakePaymentCallback(),
/*invoke_callback=*/std::move(callback),
PaymentAppProvider::PaymentEventResultCallback());
PaymentAppProvider::AbortCallback());
InvokePaymentAppCallbackRepository::GetInstance()->SetCallback(
browser_context, callbacks);
return callbacks;
......@@ -150,11 +154,12 @@ class RespondWithCallbacks : public PaymentHandlerResponseCallback {
static RespondWithCallbacks* CreateForAbort(
BrowserContext* browser_context,
scoped_refptr<ServiceWorkerVersion> service_worker_version,
PaymentAppProvider::PaymentEventResultCallback callback) {
PaymentAppProvider::AbortCallback callback) {
RespondWithCallbacks* callbacks = new RespondWithCallbacks(
browser_context, ServiceWorkerMetrics::EventType::ABORT_PAYMENT,
service_worker_version, PaymentAppProvider::InvokePaymentAppCallback(),
/*event_callback=*/std::move(callback));
service_worker_version, PaymentAppProvider::CanMakePaymentCallback(),
PaymentAppProvider::InvokePaymentAppCallback(),
/*abort_callback=*/std::move(callback));
return callbacks;
}
......@@ -179,13 +184,15 @@ class RespondWithCallbacks : public PaymentHandlerResponseCallback {
BrowserContext* browser_context,
ServiceWorkerMetrics::EventType event_type,
scoped_refptr<ServiceWorkerVersion> service_worker_version,
PaymentAppProvider::CanMakePaymentCallback can_make_payment_callback,
PaymentAppProvider::InvokePaymentAppCallback invoke_callback,
PaymentAppProvider::PaymentEventResultCallback event_callback)
PaymentAppProvider::AbortCallback abort_callback)
: browser_context_(browser_context),
event_type_(event_type),
service_worker_version_(service_worker_version),
can_make_payment_callback_(std::move(can_make_payment_callback)),
invoke_payment_app_callback_(std::move(invoke_callback)),
payment_event_result_callback_(std::move(event_callback)) {
abort_callback_(std::move(abort_callback)) {
request_id_ = service_worker_version->StartRequest(
event_type, base::BindOnce(&RespondWithCallbacks::OnErrorStatus,
weak_ptr_factory_.GetWeakPtr()));
......@@ -210,10 +217,9 @@ class RespondWithCallbacks : public PaymentHandlerResponseCallback {
CanMakePaymentResponsePtr response) override {
DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
service_worker_version_->FinishRequest(request_id_, false);
RunOrPostTaskOnThread(
FROM_HERE, BrowserThread::UI,
base::BindOnce(std::move(payment_event_result_callback_),
response->can_make_payment));
RunOrPostTaskOnThread(FROM_HERE, BrowserThread::UI,
base::BindOnce(std::move(can_make_payment_callback_),
response->can_make_payment));
delete this;
}
......@@ -222,8 +228,7 @@ class RespondWithCallbacks : public PaymentHandlerResponseCallback {
service_worker_version_->FinishRequest(request_id_, false);
RunOrPostTaskOnThread(
FROM_HERE, BrowserThread::UI,
base::BindOnce(std::move(payment_event_result_callback_),
payment_aborted));
base::BindOnce(std::move(abort_callback_), payment_aborted));
ClearCallbackRepositoryAndCloseWindow();
delete this;
......@@ -232,17 +237,19 @@ class RespondWithCallbacks : public PaymentHandlerResponseCallback {
void RespondWithErrorAndDeleteSelf(PaymentEventResponseType response_type) {
DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
if (event_type_ == ServiceWorkerMetrics::EventType::PAYMENT_REQUEST) {
if (event_type_ == ServiceWorkerMetrics::EventType::CAN_MAKE_PAYMENT) {
RunOrPostTaskOnThread(
FROM_HERE, BrowserThread::UI,
base::BindOnce(std::move(invoke_payment_app_callback_),
CreateBlankPaymentHandlerResponse(response_type)));
base::BindOnce(std::move(can_make_payment_callback_), false));
} else if (event_type_ ==
ServiceWorkerMetrics::EventType::CAN_MAKE_PAYMENT ||
event_type_ == ServiceWorkerMetrics::EventType::ABORT_PAYMENT) {
ServiceWorkerMetrics::EventType::PAYMENT_REQUEST) {
RunOrPostTaskOnThread(
FROM_HERE, BrowserThread::UI,
base::BindOnce(std::move(payment_event_result_callback_), false));
base::BindOnce(std::move(invoke_payment_app_callback_),
CreateBlankPaymentHandlerResponse(response_type)));
} else if (event_type_ == ServiceWorkerMetrics::EventType::ABORT_PAYMENT) {
RunOrPostTaskOnThread(FROM_HERE, BrowserThread::UI,
base::BindOnce(std::move(abort_callback_), false));
}
if (event_type_ == ServiceWorkerMetrics::EventType::PAYMENT_REQUEST ||
......@@ -291,8 +298,9 @@ class RespondWithCallbacks : public PaymentHandlerResponseCallback {
BrowserContext* browser_context_;
ServiceWorkerMetrics::EventType event_type_;
scoped_refptr<ServiceWorkerVersion> service_worker_version_;
PaymentAppProvider::CanMakePaymentCallback can_make_payment_callback_;
PaymentAppProvider::InvokePaymentAppCallback invoke_payment_app_callback_;
PaymentAppProvider::PaymentEventResultCallback payment_event_result_callback_;
PaymentAppProvider::AbortCallback abort_callback_;
mojo::Receiver<PaymentHandlerResponseCallback> receiver_{this};
base::WeakPtrFactory<RespondWithCallbacks> weak_ptr_factory_{this};
......@@ -316,7 +324,7 @@ void GetAllPaymentAppsOnCoreThread(
void DispatchAbortPaymentEvent(
BrowserContext* browser_context,
PaymentAppProvider::PaymentEventResultCallback callback,
PaymentAppProvider::AbortCallback callback,
scoped_refptr<ServiceWorkerVersion> active_version,
blink::ServiceWorkerStatusCode service_worker_status) {
DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
......@@ -346,7 +354,7 @@ void DispatchAbortPaymentEvent(
void DispatchCanMakePaymentEvent(
BrowserContext* browser_context,
CanMakePaymentEventDataPtr event_data,
PaymentAppProvider::PaymentEventResultCallback callback,
PaymentAppProvider::CanMakePaymentCallback callback,
scoped_refptr<ServiceWorkerVersion> active_version,
blink::ServiceWorkerStatusCode service_worker_status) {
DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
......@@ -565,7 +573,7 @@ void OnResponseForCanMakePaymentOnUiThread(
int64_t registration_id,
const url::Origin& sw_origin,
const std::string& payment_request_id,
PaymentAppProvider::PaymentEventResultCallback callback,
PaymentAppProvider::CanMakePaymentCallback callback,
bool can_make_payment) {
auto* dev_tools = GetDevToolsForInstanceGroup(instance_group, sw_origin);
if (dev_tools) {
......@@ -584,7 +592,7 @@ void OnResponseForAbortPaymentOnUiThread(
int64_t registration_id,
const url::Origin& sw_origin,
const std::string& payment_request_id,
PaymentAppProvider::PaymentEventResultCallback callback,
PaymentAppProvider::AbortCallback callback,
bool payment_aborted) {
auto* dev_tools = GetDevToolsForInstanceGroup(instance_group, sw_origin);
if (dev_tools) {
......@@ -740,7 +748,7 @@ void PaymentAppProviderImpl::CanMakePayment(
const url::Origin& sw_origin,
const std::string& payment_request_id,
CanMakePaymentEventDataPtr event_data,
PaymentEventResultCallback callback) {
CanMakePaymentCallback callback) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
auto* dev_tools = GetDevTools(browser_context, sw_origin);
......@@ -776,7 +784,7 @@ void PaymentAppProviderImpl::AbortPayment(BrowserContext* browser_context,
int64_t registration_id,
const url::Origin& sw_origin,
const std::string& payment_request_id,
PaymentEventResultCallback callback) {
AbortCallback callback) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
auto* dev_tools = GetDevTools(browser_context, sw_origin);
......
content/browser/payments/payment_app_provider_impl.h
View file @ ba01490b
......@@ -47,12 +47,12 @@ class CONTENT_EXPORT PaymentAppProviderImpl : public PaymentAppProvider {
const url::Origin& sw_origin,
const std::string& payment_request_id,
payments::mojom::CanMakePaymentEventDataPtr event_data,
PaymentEventResultCallback callback) override;
CanMakePaymentCallback callback) override;
void AbortPayment(BrowserContext* browser_context,
int64_t registration_id,
const url::Origin& sw_origin,
const std::string& payment_request_id,
PaymentEventResultCallback callback) override;
AbortCallback callback) override;
void SetOpenedWindow(WebContents* web_contents) override;
void CloseOpenedWindow(BrowserContext* browser_context) override;
void OnClosingOpenedWindow(
......
content/browser/payments/payment_app_provider_impl_unittest.cc
View file @ ba01490b
......@@ -40,15 +40,22 @@ void GetAllPaymentAppsCallback(PaymentAppProvider::PaymentApps* out_apps,
*out_apps = std::move(apps);
}
void CaptureCanMakePaymentResult(base::OnceClosure callback,
bool* out_payment_event_result,
bool payment_event_result) {
*out_payment_event_result = payment_event_result;
std::move(callback).Run();
}
void InvokePaymentAppCallback(
bool* called,
payments::mojom::PaymentHandlerResponsePtr response) {
*called = true;
}
void PaymentEventResultCallback(base::OnceClosure callback,
bool* out_payment_event_result,
bool payment_event_result) {
void CaptureAbortResult(base::OnceClosure callback,
bool* out_payment_event_result,
bool payment_event_result) {
*out_payment_event_result = payment_event_result;
std::move(callback).Run();
}
......@@ -102,7 +109,7 @@ class PaymentAppProviderTest : public PaymentAppContentUnitTestBase {
const url::Origin& sw_origin,
const std::string& payment_request_id,
payments::mojom::CanMakePaymentEventDataPtr event_data,
PaymentAppProvider::PaymentEventResultCallback callback) {
PaymentAppProvider::CanMakePaymentCallback callback) {
PaymentAppProviderImpl::GetInstance()->CanMakePayment(
browser_context(), registration_id, sw_origin, payment_request_id,
std::move(event_data), std::move(callback));
......@@ -111,7 +118,7 @@ class PaymentAppProviderTest : public PaymentAppContentUnitTestBase {
void AbortPayment(int64_t registration_id,
const url::Origin& sw_origin,
const std::string& payment_request_id,
PaymentAppProvider::PaymentEventResultCallback callback) {
PaymentAppProvider::AbortCallback callback) {
PaymentAppProviderImpl::GetInstance()->AbortPayment(
browser_context(), registration_id, sw_origin, payment_request_id,
std::move(callback));
......@@ -145,7 +152,7 @@ TEST_F(PaymentAppProviderTest, AbortPaymentTest) {
base::RunLoop loop;
AbortPayment(last_sw_registration_id(), url::Origin::Create(apps[0]->scope),
"id",
base::BindOnce(&PaymentEventResultCallback, loop.QuitClosure(),
base::BindOnce(&CaptureAbortResult, loop.QuitClosure(),
&payment_aborted));
loop.Run();
ASSERT_TRUE(payment_aborted);
......@@ -176,8 +183,8 @@ TEST_F(PaymentAppProviderTest, CanMakePaymentTest) {
CanMakePayment(last_sw_registration_id(),
url::Origin::Create(GURL("https://example.com")), "id",
std::move(event_data),
base::BindOnce(&PaymentEventResultCallback, loop.QuitClosure(),
&can_make_payment));
base::BindOnce(&CaptureCanMakePaymentResult,
loop.QuitClosure(), &can_make_payment));
loop.Run();
ASSERT_TRUE(can_make_payment);
}
......
content/public/browser/payment_app_provider.h
View file @ ba01490b
......@@ -45,7 +45,8 @@ class CONTENT_EXPORT PaymentAppProvider {
base::OnceCallback<void(int64_t registration_id)>;
using InvokePaymentAppCallback =
base::OnceCallback<void(payments::mojom::PaymentHandlerResponsePtr)>;
using PaymentEventResultCallback = base::OnceCallback<void(bool)>;
using CanMakePaymentCallback = base::OnceCallback<void(bool)>;
using AbortCallback = base::OnceCallback<void(bool)>;
// Should be accessed only on the UI thread.
virtual void GetAllPaymentApps(BrowserContext* browser_context,
......@@ -74,12 +75,12 @@ class CONTENT_EXPORT PaymentAppProvider {
const url::Origin& sw_origin,
const std::string& payment_request_id,
payments::mojom::CanMakePaymentEventDataPtr event_data,
PaymentEventResultCallback callback) = 0;
CanMakePaymentCallback callback) = 0;
virtual void AbortPayment(BrowserContext* browser_context,
int64_t registration_id,
const url::Origin& sw_origin,
const std::string& payment_request_id,
PaymentEventResultCallback callback) = 0;
AbortCallback callback) = 0;
// Set opened window for payment handler. Note that we maintain at most one
// opened window for payment handler at any moment in a browser context. The
......@@ -102,7 +103,7 @@ class CONTENT_EXPORT PaymentAppProvider {
std::string* error_message) = 0;
protected:
virtual ~PaymentAppProvider() {}
virtual ~PaymentAppProvider() = default;
};
} // namespace content
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment