1474499 - Add support for "onmerchantvalidation" and MerchantValidationEvent

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, enhancement, P1)

Description

[Marcos Caceres [:marcosc]]

Adds:

 * interface MerchantValidationEvent
 * PaymentRequest.prototype.onmerchantvalidation

Allowing for merchant validation to be supported. We won't fire this event - it's just there for web-compat with Safari's Apple Pay when using the payment request API.

Comment 1

[Marcos Caceres [:marcosc]]

Attachment: Initial implementation

Made a reference implementation as I was working on this part of the spec. Passes web platform tests 🎉👌

Thanks for feedback on GitHub, Eden! I decided that to use URLs proper instead of nsIURI.

Comment 2

[Marcos Caceres [:marcosc]]

Eden, should I do the UI testing service for this too? I don't mind doing it, but we don't plan on supporting these events yet in Basic Card. 

This is just for Web Platform Tests conformance.

Comment 3

[Eden Chuang[:edenchuang]]

Comment on attachment 9005114 [details] [diff] [review]
Initial implementation

Review of attachment 9005114 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/payments/MerchantValidationEvent.cpp
@@ +25,5 @@
> +NS_IMPL_ADDREF_INHERITED(MerchantValidationEvent, Event)
> +NS_IMPL_RELEASE_INHERITED(MerchantValidationEvent, Event)
> +
> +class Promise;
> +class EventTarget;

Remove forward declarations, they can only be used in header file.
class Promise;
class EventTarget;

@@ +76,5 @@
> +  }
> +
> +  nsString href;
> +  url->GetHref(href);
> +  event->SetValidationURL(href);

Move code from line 55 to 80 into
  MerchantValidationEvent::Constructor(EventTarget* aOwner, const nsAString& aType, const MerchantValidationEventInit& aEventInitDict)
Or
  PaymentRequest::DispatchMerchantValidationEvent

In fact, you don't use this Constructor in PaymentRequest::DispatchMerchantValidationEvent
That means the logic would not be run when calling PaymentRequest::DispatchMerchantValidationEvent;

@@ +101,5 @@
> +  }
> +
> +  // TODO: If we eventually support merchant validation
> +  // we would verify `aValue` here.
> +  mRequest->AbortUpdate(NS_ERROR_DOM_NOT_SUPPORTED_ERR, false);

Do we really want this?
mRequest->AbortUpdate(NS_ERROR_DOM_NOT_SUPPORTED_ERR, false);
would make the PaymentRequest be aborted.

That means no matter what merchant passed into PaymentMerchantValidationEvent::complete().
PaymentRequest always is aborted with NotSupportedError.

::: dom/payments/moz.build
@@ +10,4 @@
>  
>  EXPORTS += [
>      'PaymentRequestData.h',
> +    'PaymentRequestService.h',\

nit: remove "\"

Comment 4

[Eden Chuang[:edenchuang]]

(In reply to Eden Chuang[:edenchuang] from comment #3)
> Comment on attachment 9005114 [details] [diff] [review]
> Initial implementation
> 
> Review of attachment 9005114 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +76,5 @@
> > +  }
> > +
> > +  nsString href;
> > +  url->GetHref(href);
> > +  event->SetValidationURL(href);
> 
> Move code from line 55 to 80 into
>   MerchantValidationEvent::Constructor(EventTarget* aOwner, const nsAString&
> aType, const MerchantValidationEventInit& aEventInitDict)
> Or
>   PaymentRequest::DispatchMerchantValidationEvent
> 
> In fact, you don't use this Constructor in
> PaymentRequest::DispatchMerchantValidationEvent
> That means the logic would not be run when calling
> PaymentRequest::DispatchMerchantValidationEvent;

Or you might do it like you did in PaymentMethodChangeEvent
https://searchfox.org/mozilla-central/source/dom/payments/PaymentMethodChangeEvent.cpp#47
Declare an init function for it and set up the validationURL.

Comment 5

[Marcos Caceres [:marcosc]]

(In reply to Eden Chuang[:edenchuang] from comment #4)
> Or you might do it like you did in PaymentMethodChangeEvent
> https://searchfox.org/mozilla-central/source/dom/payments/
> PaymentMethodChangeEvent.cpp#47
> Declare an init function for it and set up the validationURL.

I'm having problems figuring out how to derive the `GlobalObject` when I refactor the code. I need a global object to be able to construct the URL. 

I also need `aRV` so I can set the exception in case the URL construction results in failure.

Any suggestions?

Comment 6

[Eden Chuang[:edenchuang]]

(In reply to Marcos Caceres [:marcosc] from comment #5)
> (In reply to Eden Chuang[:edenchuang] from comment #4)
> > Or you might do it like you did in PaymentMethodChangeEvent
> > https://searchfox.org/mozilla-central/source/dom/payments/
> > PaymentMethodChangeEvent.cpp#47
> > Declare an init function for it and set up the validationURL.
> 
> I'm having problems figuring out how to derive the `GlobalObject` when I
> refactor the code. I need a global object to be able to construct the URL. 
> 
> I also need `aRV` so I can set the exception in case the URL construction
> results in failure.
> 
> Any suggestions?

Probably you needn't construct an URL, because the URL::GetHref() comes from URI::GetSpec()

see https://searchfox.org/mozilla-central/source/dom/url/URL.cpp#148

I think the correct behavior is you need to create a new URI by the EventInitDict.mValidationURL, like this

  nsCOMPtr<nsIURI> validationUri;
  nsresult rv = NS_NewURI(getter_AddRefs(validationUri), aEventInitDict.mValidationURL, nullptr, nullptr,
                          nsContentUtils::GetIOService());

Then you can get the href by calling validationUri.GetSpec() what we done in 

https://searchfox.org/mozilla-central/source/dom/url/URL.cpp#138

  nsAutoCString utf8href;
  nsresult rv = validationUri->GetSpec(utf8href);
  if (NS_SUCCEEDED(rv)) {
    CopyUTF8toUTF16(utf8href, mValidationURL);
  }

all above code should be in MerchantValidationEvent::Init(const MerchantValidationEventInit& init);
But in current attached patch, the method is not declared, I expect to see it in the next patch.

So we have two MerchantValidationEvent::Constructor(), one has the GlobalObject parameters, and one has not.
These two constructors have different purposes. It is a little complicated, but basically we can think the one with GlobalObject is for calling from JS code which the event is generated by users code, and the other one is for C++ gecko code that means the event is generated by gecko internally. 

Usually, we reuse the implementation of the version which has no GlobalObject in the other one. PaymentRequestUpdateEvent is an good example.

https://searchfox.org/mozilla-central/source/dom/payments/PaymentRequestUpdateEvent.cpp#37

Now what we want to implement is dispatching the MerchantValidationEvent when gecko need, and that is PaymentRequest::DispatchMerchantValidationEvent().

In PaymentRequest::DispatchMerchantValidationEvent(), we have followings to do

1. Create MerchantValidationEventInit and set up it.
2. Create MerchantValidationEvent with the MerchantValidationEventInit in step 1.
3. Dispatch the event.

So in the step 1, we need set up the EventInit with a ValidationURL. How it comes?
It should come from the window object, it is the thing in the patch line 55 to 64.
Then in the step 2, we call the Constructor which has no GlobalObject one with the EventInit.

Hope the explain make you more clear about the implementation part.

Comment 7

[Marcos Caceres [:marcosc]]

Attachment: Addressed Eden's feedback.

Baku, could you please check the Web IDL? 🙏

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 9008007 [details] [diff] [review]
Addressed Eden's feedback.

Review of attachment 9008007 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/payments/MerchantValidationEvent.cpp
@@ +64,5 @@
> +MerchantValidationEvent::init(const MerchantValidationEventInit& aEventInitDict,
> +                              ErrorResult& aRv)
> +{
> +  nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(GetParentObject());
> +  auto principal = window->GetExtantDoc()->NodePrincipal();

GetExtantDoc can be null. Add a check.

@@ +93,5 @@
> +
> +MerchantValidationEvent::MerchantValidationEvent(EventTarget* aOwner)
> +  : Event(aOwner, nullptr, nullptr)
> +  , mWaitForUpdate(false)
> +  , mRequest(nullptr)

remove this line. It's not needed for smart pointers.

@@ +109,5 @@
> +  if (!mWaitForUpdate) {
> +    return;
> +  }
> +
> +  // TODO: If we eventually support merchant validation

Do we have a follow up for this TODO? Can you write the bug ID here?

@@ +113,5 @@
> +  // TODO: If we eventually support merchant validation
> +  // we would verify `aValue` here.
> +  mRequest->AbortUpdate(NS_ERROR_DOM_NOT_SUPPORTED_ERR, false);
> +
> +  mWaitForUpdate = false;

Move it immediately after if(!mWaitForUpdate)

@@ +124,5 @@
> +{
> +  MOZ_ASSERT(mRequest);
> +
> +  mRequest->AbortUpdate(NS_ERROR_DOM_ABORT_ERR, false);
> +  mWaitForUpdate = false;

same here.
Don't we want to check mWaitForUpdate here? Probably yes.

::: dom/webidl/MerchantValidationEvent.webidl
@@ +1,1 @@
> +[Constructor(DOMString type, optional MerchantValidationEventInit eventInitDict),

Add an header here. CopyRight + a URL of the spec.

Comment 9

[Marcos Caceres [:marcosc]]

Attachment: Addressed Baku's feedback...

Comment 10

[Marcos Caceres [:marcosc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cbeed58a4375d5288dd8882e54dc5fda09c8f83

Comment 11

[Eden Chuang[:edenchuang]]

Comment on attachment 9008038 [details] [diff] [review]
Addressed Baku's feedback...

Review of attachment 9008038 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent. Nothing I can comment.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebe2c08a2454
Add support for onmerchantvalidation and MerchantValidationEvent. r=edenchuang,baku

Comment 13

[Arthur Iakab [arthur_iakab]]

Backed out changeset ebe2c08a2454 (bug 1474499) for failing web platform tests on ValidationEvent/constructor.https.html
Backout revision https://hg.mozilla.org/integration/mozilla-inbound/rev/1c3aeb221660f703493fbd8cb4c2f7caabbb8a47
Failed push https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ebe2c08a24548441f0682b8c048b21293d47aea3&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Link to the failure log https://treeherder.mozilla.org/logviewer.html#?job_id=198666122&repo=mozilla-inbound

:marcosc could you please take a look?

Comment 14

[Marcos Caceres [:marcosc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ad036655190d7b240c444666f93920742829bc7

Comment 15

[Marcos Caceres [:marcosc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0a2677ef868ad7a45624418c055e5d8a7d06c59

Comment 16

[Marcos Caceres [:marcosc]]

:sheppy, when you get to this, I can write the details of how merchant validations works (and provide an example) - but would appreciate help with documenting the MerchantValidationEvent interface.

Comment 17

[Marcos Caceres [:marcosc]]

I'm going to try assuming FAIL on Linux 32 for now. Talked to Eden and we will investigate what's happening later.

Comment 18

[Marcos Caceres [:marcosc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ed916e1576824757ece1361b587c544ae732d5a

Comment 19

[Marcos Caceres [:marcosc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=33a5877fb5fd57a9495d75c287fb0768dddfd270

Comment 20

[Marcos Caceres [:marcosc]]

Attachment: Removed WPT stuff

Talked to Eden about Linux 32 failures, and we are going to deal with that separately.

Comment 21

[Marcos Caceres [:marcosc]]

Attachment: Convert to hg patch.

Comment 22

[Marcos Caceres [:marcosc]]

Attachment: fixup

Comment 23

[Marcos Caceres [:marcosc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=05b0ad83e94142a5cd3fbfb8e34a5f1fc2727ffc

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72f155506bf6
Add support for onmerchantvalidation and MerchantValidationEvent. r=edenchuang,baku

Comment 25

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/72f155506bf6

Comment 26

[Marcos Caceres [:marcosc]]

Attachment: Bug 1474499 - Add support for onmerchantvalidation and MerchantValidationEvent. r=edenchuang,baku

Bug 1490599 - Implement MerchantValidationEvent.prototype.methodName attribute. r=baku

Comment 27

[Marcos Caceres [:marcosc]]

Ignore the Phabricator attachment, just me experimenting.

Comment 28

[Eric Shepherd [:sheppy]]

Underway. Current status below; what's still left to do is to add this to the guide docs.

Pages added:

* https://developer.mozilla.org/en-US/docs/Web/API/MerchantValidationEvent/MerchantValidationEvent
* https://developer.mozilla.org/en-US/docs/Web/API/MerchantValidationEvent/methodName
* https://developer.mozilla.org/en-US/docs/Web/API/MerchantValidationEvent/validationURL
* https://developer.mozilla.org/en-US/docs/Web/API/MerchantValidationEvent/complete

Pages updated:

* https://developer.mozilla.org/en-US/docs/Web/API/MerchantValidationEvent
* https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/onmerchantvalidation

Comment 29

[Marcos Caceres [:marcosc]]

These are great, :sheppy! Thank you! I'm giving them a once over now.

Comment 30

[Eric Shepherd [:sheppy]]

Added merchant validation info to the guide content:

https://developer.mozilla.org/en-US/docs/Web/API/Payment_Request_API/Concepts#Merchant_validation