Closed Bug 1345361 Opened 7 years ago Closed 7 years ago

[Payment Request API] PaymentRequest Constructor implementation

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: edenchuang, Assigned: edenchuang)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [WP-MVP][M3])

Attachments

(1 file, 10 obsolete files)

120.24 KB, patch
Details | Diff | Splinter Review
This bug is used for tracking PaymentRequest Constructor implementation, which is a part of Payment Request API.
Blocks: 1318987
Component: DOM → DOM: Device Interfaces
Depends on: 1345389, 1345390
Priority: -- → P3
Priority: P3 → P2
Mark P2 as this is planed for the next releases. Feel free to change it as you see fit, Marcos and Ben. :)
Component: DOM: Device Interfaces → DOM: Web Payments
Whiteboard: [WP-MVP][M3]
The attached file is the patch of PaymentRequest Constructor Implementation. This patch also include the implementation of bug 1318993.

And following is the try result with this patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27ae1661b5957c0aec2a5955cbdf89b5684e2075
Assignee: nobody → echuang
Status: NEW → ASSIGNED
Attachment #8864758 - Flags: review?(amarchesini)
Comment on attachment 8864758 [details] [diff] [review]
Bug1345361_PaymentRequest_Constructor_Implementation

Cancel review for fix build fail on some platform.
Attachment #8864758 - Flags: review?(amarchesini)
Comment on attachment 8864758 [details] [diff] [review]
Bug1345361_PaymentRequest_Constructor_Implementation

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

I was already reviewing this patch :) Here the comments.

1. Move all to PBrowser instead PContent. It seems that you need the TabId, so PBrowser is the right way to have it on the parent side.
2. localize the strings

In general it's good. It needs some more work!

::: dom/base/nsGkAtomList.h
@@ +923,5 @@
>  GK_ATOM(onsending, "onsending")
>  GK_ATOM(onsent, "onsent")
>  GK_ATOM(onset, "onset")
>  GK_ATOM(onshow, "onshow")
> +GK_ATOM(onshippingaddresschange, "onshippingaddresschange")

alphabetic order: it must go before onshow

::: dom/interfaces/payments/nsIPaymentActionRequest.idl
@@ +8,5 @@
> +#include "nsIPaymentRequest.idl"
> +
> +interface nsIArray;
> +
> +[uuid(7ddbe8be-beac-4952-96f6-619981dff7a6)]

builtinclass

@@ +29,5 @@
> +  void init(in AString aRequestId,
> +            in uint32_t aType);
> +};
> +
> +[uuid(1d38dce6-8bcd-441b-aa94-68e300b6e175)]

builtinclass

::: dom/interfaces/payments/nsIPaymentRequest.idl
@@ +7,5 @@
> +#include "nsIVariant.idl"
> +
> +interface nsIArray;
> +
> +[scriptable, uuid(2fe296cc-d917-4820-b492-aa42df23f9b4)]

builtinclass

@@ +14,5 @@
> +  readonly attribute nsIArray supportedMethods;
> +  readonly attribute AString data;
> +};
> +
> +[scriptable, uuid(d22a6f5f-767b-4fea-bf92-68b0b8003eba)]

builtinclass

@@ +21,5 @@
> +  readonly attribute AString currency;
> +  readonly attribute AString value;
> +};
> +
> +[scriptable, uuid(4f78a59f-b5ff-4fb5-ab48-3b37d0101b02)]

builtinclass

@@ +29,5 @@
> +  readonly attribute nsIPaymentCurrencyAmount amount;
> +  readonly attribute boolean pending;
> +};
> +
> +[scriptable, uuid(74259861-c318-40e8-b3d5-518e701bed80)]

builtinclass

@@ +38,5 @@
> +  readonly attribute nsIArray additionalDisplayItems;
> +  readonly attribute AString data;
> +};
> +
> +[scriptable, uuid(68341551-3605-4381-b936-41e830aa88fb)]

builtinclass

@@ +47,5 @@
> +  readonly attribute nsIPaymentCurrencyAmount amount;
> +  attribute boolean selected;
> +};
> +
> +[scriptable, uuid(73a5a3f1-45b9-4605-a6e6-7aa60daa9039)]

This cannot be a builtinclass, because this will be created by the UI. right?

@@ +58,5 @@
> +  readonly attribute nsIArray modifiers;
> +  readonly attribute AString error;
> +};
> +
> +[scriptable, uuid(d53f9f20-138e-47cc-9fd5-db16a3f6d301)]

builtinclass

@@ +69,5 @@
> +  readonly attribute AString shippingType;
> +};
> +
> +[scriptable, uuid(2fa36783-d684-4487-b7a8-9def6ae3128f)]
> +interface nsIPaymentRequest : nsISupports

builtinclass

::: dom/interfaces/payments/nsIPaymentRequestService.idl
@@ +13,5 @@
> + *  nsPaymentRequestService is used to manage the created PaymentRequest in the
> + *  chrome process. It is also the IPC agent for payment UI to communicate with
> + *  merchant side.
> + */
> +[scriptable, uuid(cccd665f-edf3-41fc-ab9b-fc55b37340aa)]

builtinclass

::: dom/ipc/ContentParent.cpp
@@ +3308,5 @@
> +  return true;
> +}
> +
> +mozilla::ipc::IPCResult
> +ContentParent::RecvPPaymentRequestConstructor(PPaymentRequestParent* aActor)

Don't implement this if you don't need it.

::: dom/payments/PaymentRequest.cpp
@@ +11,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +NS_IMPL_CYCLE_COLLECTION_CLASS(PaymentRequest)
> +

extra line.

@@ +52,5 @@
> +    aErrorMsg.AssignLiteral("The amount.value of \"");
> +    aErrorMsg.Append(aItem);
> +    aErrorMsg.AppendLiteral("\"(");
> +    aErrorMsg.Append(aValue);
> +    aErrorMsg.AppendLiteral(") must be a valid decimal monetary value.");

Well, this _must_ be localized.

@@ +71,5 @@
> +    aErrorMsg.AssignLiteral("The amount.value of \"");
> +    aErrorMsg.Append(aItem);
> +    aErrorMsg.AppendLiteral("\"(");
> +    aErrorMsg.Append(aValue);
> +    aErrorMsg.AppendLiteral(") must be a valid and positive decimal monetary value.");

same here.

@@ +155,5 @@
> +
> +  // Check payment methods is done by webidl
> +
> +  // Check payment details
> +  nsString message;

nsAutoString

@@ +170,5 @@
> +  RefPtr<PaymentRequestManager> manager = PaymentRequestManager::GetSingleton();
> +
> +  // Create PaymentRequest and set its |mId|
> +  RefPtr<PaymentRequest> request;
> +  nsresult rv = manager->CreatePayment(window, aMethodData, aDetails, aOptions, getter_AddRefs(request));

if (NS_WARN_IF(NS_FAILED(rv))) {
  return nullptr;
}

return request.forget();

@@ +178,5 @@
> +
> +PaymentRequest::PaymentRequest(nsPIDOMWindowInner* aWindow)
> +  : DOMEventTargetHelper(aWindow)
> +  , mUpdating(false)
> +  , mState(eUnknown)

eUnknown basically is not used. Remove it. Set it to eCreated immediately.

@@ +179,5 @@
> +PaymentRequest::PaymentRequest(nsPIDOMWindowInner* aWindow)
> +  : DOMEventTargetHelper(aWindow)
> +  , mUpdating(false)
> +  , mState(eUnknown)
> +{

MOZ_ASSERT(aWindow);

@@ +182,5 @@
> +  , mState(eUnknown)
> +{
> +  // Generate a unique id for identification
> +  nsID uuid;
> +  nsContentUtils::GenerateUUIDInPlace(uuid);

This can fail. You must make the CTOR private and expose a static Create() method. The error must be returned.

@@ +193,5 @@
> +
> +
> +already_AddRefed<Promise>
> +PaymentRequest::Show(ErrorResult& aRv)
> +{

return an error here.

@@ +200,5 @@
> +
> +already_AddRefed<Promise>
> +PaymentRequest::CanMakePayment(ErrorResult& aRv)
> +{
> +  return nullptr;

return an error here.

@@ +205,5 @@
> +}
> +
> +already_AddRefed<Promise>
> +PaymentRequest::Abort(ErrorResult& aRv)
> +{

return an error here.

::: dom/payments/PaymentRequest.h
@@ +31,5 @@
> +  NS_DECL_ISUPPORTS_INHERITED
> +
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(PaymentRequest, DOMEventTargetHelper)
> +
> +  PaymentRequest(nsPIDOMWindowInner* aWindow);

explicit. And make it private.

@@ +33,5 @@
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(PaymentRequest, DOMEventTargetHelper)
> +
> +  PaymentRequest(nsPIDOMWindowInner* aWindow);
> +
> +  nsPIDOMWindowInner* GetParentObject() const

Remove this. DOMEventTargetHelper does this for you.

@@ +42,5 @@
> +  virtual JSObject* WrapObject(JSContext* aCx,
> +                               JS::Handle<JSObject*> aGivenProto) override;
> +
> +  static bool
> +    PrefEnabled(JSContext* aCx, JSObject* aObj);

strange indentation.

::: dom/payments/PaymentRequestManager.cpp
@@ +21,5 @@
> + *  Following Convert* functions are used for convert PaymentRequest structs
> + *  to transferable structs for IPC.
> + */
> +nsString
> +SerializeFromJSObject(JSObject* aObject, nsresult& aRv)

do the other way around:

nsresult SerializeFromJSObject(JSObject* aObject, nsAString& aString);

@@ +24,5 @@
> +nsString
> +SerializeFromJSObject(JSObject* aObject, nsresult& aRv)
> +{
> +  nsCOMPtr<nsIJSON> serializer = do_CreateInstance("@mozilla.org/dom/json;1");
> +  if (!serializer) {

NS_WARN_IF

@@ +26,5 @@
> +{
> +  nsCOMPtr<nsIJSON> serializer = do_CreateInstance("@mozilla.org/dom/json;1");
> +  if (!serializer) {
> +    aRv = NS_ERROR_FAILURE;
> +    return EmptyString();

here just a return NS_ERROR_FAILURE;

@@ +31,5 @@
> +  }
> +  JS::Value value = JS::ObjectValue(*aObject);
> +  JSContext* cx = nsContentUtils::GetCurrentJSContext();
> +  MOZ_ASSERT(cx);
> +  nsString serializedObject;

nsAutoString

@@ +33,5 @@
> +  JSContext* cx = nsContentUtils::GetCurrentJSContext();
> +  MOZ_ASSERT(cx);
> +  nsString serializedObject;
> +  aRv = serializer->EncodeFromJSVal(&value, cx, serializedObject);
> +  if (NS_FAILED(aRv)) {

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

@@ +35,5 @@
> +  nsString serializedObject;
> +  aRv = serializer->EncodeFromJSVal(&value, cx, serializedObject);
> +  if (NS_FAILED(aRv)) {
> +    return EmptyString();
> +  }

aString = serializedObject;

@@ +40,5 @@
> +  return serializedObject;
> +}
> +
> +IPCPaymentMethodData
> +ConvertMethodData(const PaymentMethodData& aMethodData, nsresult& aRv)

Also here:

nsresult ConvertMethodData(const PaymenetMethodData& aMethodData, IPCPaymentMethodData& aData)

@@ +55,5 @@
> +  return IPCPaymentMethodData(supportedMethods, serializedData);
> +}
> +
> +IPCPaymentCurrencyAmount
> +ConvertCurrencyAmount(const PaymentCurrencyAmount& aAmount)

void
ConvertCurrentAmount(const PaymentCurrencyAmount& aAmount, IPCPaymentCurrencyAmount&)

@@ +62,5 @@
> +                                  aAmount.mValue);
> +}
> +
> +IPCPaymentItem
> +ConvertItem(const PaymentItem& item)

here as well.

@@ +69,5 @@
> +  return IPCPaymentItem(item.mLabel, amount, item.mPending);
> +}
> +
> +IPCPaymentDetailsModifier
> +ConvertModifier(const PaymentDetailsModifier& aModifier, nsresult& aRv)

here as well. In this way you avoid an extra copy.

@@ +96,5 @@
> +                                   aModifier.mAdditionalDisplayItems.WasPassed());
> +}
> +
> +IPCPaymentShippingOption
> +ConvertShippingOption(const PaymentShippingOption& aOption)

here as well.

@@ +107,5 @@
> +ConvertDetailsBase(const PaymentDetailsBase& aDetails,
> +                   nsTArray<IPCPaymentItem>& aDisplayItems,
> +                   nsTArray<IPCPaymentShippingOption>& aShippingOptions,
> +                   nsTArray<IPCPaymentDetailsModifier>& aModifiers,
> +                   nsresult& aRv)

nsresult ConvertDetailsBase(...

@@ +136,5 @@
> +  }
> +}
> +
> +IPCPaymentDetails
> +ConvertDetailsInit(const PaymentDetailsInit& aDetails, nsresult& aRv)

here as well.

@@ +165,5 @@
> +                           aDetails.mModifiers.WasPassed());
> +}
> +
> +IPCPaymentOptions
> +ConvertOptions(const PaymentOptions& aOptions)

here as well.

@@ +184,5 @@
> +
> +/* PaymentRequestManager */
> +
> +NS_IMPL_ISUPPORTS0(PaymentRequestManager);
> +StaticRefPtr<PaymentRequestManager> PaymentRequestManager::sSingleton;

Remove it from PaymenetRequestManager.

@@ +213,5 @@
> +void
> +PaymentRequestManager::ClearPaymentRequestChild()
> +{
> +  if (mPaymentRequestChild) {
> +    PPaymentRequestChild::Send__delete__(mPaymentRequestChild);

mPaymentRequestChild must be refcounted (or it must know the manager and nullify itself when ActorDestroyed() is called).
Otherwise, if the communication between child-parent is interrupted, mPayementRequestChild will receive an ActorDestoyed() and then it will be released. At that point, this will be an invalid pointer.

Here you should call:
mPaymenetRequestChild->MaybeDelete();
mPaymeentRequestChild = nullptr;


in MaybeDelete you should do:

if (mIPCActive) { Send__delete__.... }

where mIPCActive (or any better name) is set to false in ActorDestroyed and in MaybeDelete.

@@ +232,5 @@
> +already_AddRefed<PaymentRequest>
> +PaymentRequestManager::GetPaymentRequestById(const nsAString& aRequestId)
> +{
> +  for (const RefPtr<PaymentRequest>& request : mRequestQueue) {
> +    nsString requestId;

nsAutoString

@@ +233,5 @@
> +PaymentRequestManager::GetPaymentRequestById(const nsAString& aRequestId)
> +{
> +  for (const RefPtr<PaymentRequest>& request : mRequestQueue) {
> +    nsString requestId;
> +    request->GetInternalId(requestId);

Another approach would be to implement:

bool PaymentRequest::Equals(const nsAString& aId) const
{
  return mInternalId.Equals(aId);
}

@@ +259,5 @@
> +  nsresult rv;
> +  nsTArray<IPCPaymentMethodData> methodData;
> +  for (const PaymentMethodData& data : aMethodData) {
> +    IPCPaymentMethodData iData = ConvertMethodData(data, rv);
> +    if (NS_FAILED(rv)) {

NS_WARN_IF

@@ +266,5 @@
> +    methodData.AppendElement(iData);
> +  }
> +
> +  IPCPaymentDetails details = ConvertDetailsInit(aDetails, rv);
> +  if (NS_FAILED(rv)) {

NS_WARN_IF

::: dom/payments/PaymentRequestManager.h
@@ +21,5 @@
> +/*
> + *  PaymentRequestManager is a singleton used to manage the created PaymentRequests.
> + *  It is also the communication agent to chrome proces.
> + */
> +class PaymentRequestManager : public nsISupports

final?

@@ +24,5 @@
> + */
> +class PaymentRequestManager : public nsISupports
> +{
> +public:
> +  NS_DECL_ISUPPORTS

why nsISupports? NS_INLINE_DECL_REFCOUNTING is enough.

@@ +45,5 @@
> +                PaymentRequest** aRequest);
> +
> +protected:
> +  PaymentRequestManager();
> +  virtual ~PaymentRequestManager();

no virtual for final classes.

@@ +48,5 @@
> +  PaymentRequestManager();
> +  virtual ~PaymentRequestManager();
> +
> +  virtual void CreatePaymentRequestChild();
> +  virtual void ClearPaymentRequestChild();

same here.

@@ +52,5 @@
> +  virtual void ClearPaymentRequestChild();
> +
> +  static StaticRefPtr<PaymentRequestManager> sSingleton;
> +
> +  PaymentRequestChild* mPaymentRequestChild;

This is wrong, but I'll describe it in the cpp file.

@@ +56,5 @@
> +  PaymentRequestChild* mPaymentRequestChild;
> +
> +  // The container for the created PaymentRequests
> +  nsTArray<RefPtr<PaymentRequest>> mRequestQueue;
> +};

extra line here and before #endif

::: dom/payments/ipc/PPaymentRequest.ipdl
@@ +70,5 @@
> +};
> +
> +struct PaymentCreateActionRequest
> +{
> +  TabId tabId;

If you need the tabID, probably you want to move this protocol in PBrowser. In this way, TabParent already knows the correct TabId and you don't need to pass it here as param.

@@ +79,5 @@
> +};
> +
> +union PaymentActionRequest
> +{
> +  PaymentCreateActionRequest;

Why this empty union? Can you just use PaymentCreateActionRequest? I guess you are planning to expand it in the next patches.

::: dom/payments/ipc/PaymentRequestChild.cpp
@@ +17,5 @@
> +
> +PaymentRequestChild::~PaymentRequestChild()
> +{
> +  MOZ_COUNT_DTOR(PaymentRequestChild);
> +  if (!mActorDestroyed) {

This cannot happen. Remove this if (mActorDestroyed) ...
If it happens, there is a bug elsewhere.

@@ +26,5 @@
> +void
> +PaymentRequestChild::ActorDestroy(ActorDestroyReason aWhy)
> +{
> +  mActorDestroyed = true;
> +}

extra line after this.

::: dom/payments/ipc/PaymentRequestChild.h
@@ +16,5 @@
> +{
> +public:
> +  PaymentRequestChild();
> +
> +  virtual void ActorDestroy(ActorDestroyReason aWhy) override;

no virtual for final classes.

@@ +20,5 @@
> +  virtual void ActorDestroy(ActorDestroyReason aWhy) override;
> +private:
> +  ~PaymentRequestChild();
> +
> +  bool mActorDestroyed;

you don't use this. remove it.

::: dom/payments/ipc/PaymentRequestParent.cpp
@@ +22,5 @@
> +
> +NS_IMPL_ISUPPORTS0(PaymentRequestParent);
> +
> +PaymentRequestParent::PaymentRequestParent()
> +  : mActorDestroyed(false)

Instead calling mActorDestroyed, call it mIsAlive (or something similar). Then, set it to false when Recv__delete__ is received and in ActorDestroyed().
In RecvRequestPayment check that mIsAlive is true.

@@ +32,5 @@
> +}
> +
> +mozilla::ipc::IPCResult
> +PaymentRequestParent::RecvRequestPayment(const PaymentActionRequest& aRequest)
> +{

MOZ_ASSERT(mIsAlive);

@@ +33,5 @@
> +
> +mozilla::ipc::IPCResult
> +PaymentRequestParent::RecvRequestPayment(const PaymentActionRequest& aRequest)
> +{
> +  nsCOMPtr<nsIPaymentActionRequest> nsrequest;

call it 'request' or 'paymentActionRequest'

@@ +37,5 @@
> +  nsCOMPtr<nsIPaymentActionRequest> nsrequest;
> +  switch (aRequest.type()) {
> +    case PaymentActionRequest::TPaymentCreateActionRequest: {
> +      PaymentCreateActionRequest request = aRequest;
> +      nsCOMPtr<nsIMutableArray> nsMethodData =

methodData

@@ +41,5 @@
> +      nsCOMPtr<nsIMutableArray> nsMethodData =
> +        do_CreateInstance(NS_ARRAY_CONTRACTID);
> +      for (IPCPaymentMethodData data : request.methodData()) {
> +        nsCOMPtr<nsIPaymentMethodData> nsData =
> +          new nsPaymentMethodData(data);

don't use "ns" prefix. PaymentMethodData() is better.

@@ +42,5 @@
> +        do_CreateInstance(NS_ARRAY_CONTRACTID);
> +      for (IPCPaymentMethodData data : request.methodData()) {
> +        nsCOMPtr<nsIPaymentMethodData> nsData =
> +          new nsPaymentMethodData(data);
> +        nsMethodData->AppendElement(nsData, false);

1. methodData->AppendElement(...
2. this could fail. rv = ... if (NS_WARN_IF(NS_FAILED(rv).... IPC_FAIL..

@@ +46,5 @@
> +        nsMethodData->AppendElement(nsData, false);
> +      }
> +
> +      nsCOMPtr<nsIPaymentDetails> nsDetails =
> +        new nsPaymentDetails(request.details());

also here. Don't use 'ns' prefix.

@@ +49,5 @@
> +      nsCOMPtr<nsIPaymentDetails> nsDetails =
> +        new nsPaymentDetails(request.details());
> +
> +      nsCOMPtr<nsIPaymentOptions> nsOptions =
> +        new nsPaymentOptions(request.options());

here as well

@@ +58,5 @@
> +                                 request.tabId(),
> +                                 nsMethodData,
> +                                 nsDetails,
> +                                 nsOptions);
> +      nsrequest = do_QueryInterface(createRequest);

just to be sure everything is correctly implemented:
MOZ_ASSERT(nsrequest);

@@ +69,5 @@
> +  nsCOMPtr<nsIPaymentRequestService> service =
> +    do_GetService(NS_PAYMENT_REQUEST_SERVICE_CONTRACT_ID);
> +  MOZ_ASSERT(service);
> +  nsresult rv = service->RequestPayment(nsrequest);
> +  if (NS_FAILED(rv)) {

NS_WARN_IF

@@ +77,5 @@
> +}
> +
> +mozilla::ipc::IPCResult
> +PaymentRequestParent::Recv__delete__()
> +{

mIsAlive = false

@@ +84,5 @@
> +
> +void
> +PaymentRequestParent::ActorDestroy(ActorDestroyReason aWhy)
> +{
> +  mActorDestroyed = true;

mIsAlive = false;

::: dom/payments/ipc/PaymentRequestParent.h
@@ +16,5 @@
> +class PaymentRequestParent : public nsISupports
> +                           , public PPaymentRequestParent
> +{
> +public:
> +  NS_DECL_ISUPPORTS;

Why doesn't it need to be a nsISupports? Probably NS_INLINE_DECL_THREADSAFE_REFCOUNTING(PaymentRequestParent) is enough.

@@ +28,5 @@
> +  virtual mozilla::ipc::IPCResult Recv__delete__() override;
> +
> +  virtual void ActorDestroy(ActorDestroyReason aWhy) override;
> +private:
> +  virtual ~PaymentRequestParent();

= default - you are not actually implementing anything in the DTOR.

::: dom/payments/moz.build
@@ +10,5 @@
> +    'nsPaymentRequestService.h',
> +]
> +
> +EXPORTS.mozilla.dom += [
> +    'ipc/PaymentRequestChild.h',

1. create a ipc/moz.build with the all ipc stuff.
2. DIRS += ['ipc']

::: dom/payments/nsPaymentActionRequest.cpp
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsPaymentActionRequest.h"

rename all

@@ +15,5 @@
> +                  nsIPaymentActionRequest)
> +
> +NS_IMETHODIMP
> +nsPaymentActionRequest::Init(const nsAString& aRequestId,
> +                              const uint32_t aType)

indentation

@@ +45,5 @@
> +
> +NS_IMETHODIMP
> +nsPaymentCreateActionRequest::InitRequest(const nsAString& aRequestId,
> +                                           const uint64_t aTabId,
> +                                           nsIArray* aMethodData,

indentation

@@ +51,5 @@
> +                                           nsIPaymentOptions* aOptions)
> +{
> +  Init(aRequestId, nsIPaymentActionRequest::CREATE_ACTION);
> +  mTabId = aTabId;
> +  mMethodData = aMethodData;

This cannot be null. chcek the pointer.

@@ +53,5 @@
> +  Init(aRequestId, nsIPaymentActionRequest::CREATE_ACTION);
> +  mTabId = aTabId;
> +  mMethodData = aMethodData;
> +  mDetails = aDetails;
> +  mOptions = aOptions;

should you check the pointers here?

@@ +71,5 @@
> +  NS_ENSURE_ARG_POINTER(aMethodData);
> +  nsCOMPtr<nsIMutableArray> methodData = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +  uint32_t length;
> +  nsresult rv = mMethodData->GetLength(&length);
> +  if (NS_FAILED(rv)) {

NS_WARN_IF

@@ +77,5 @@
> +  }
> +  for (uint32_t idx = 0; idx < length; ++idx) {
> +    nsCOMPtr<nsIPaymentMethodData> method =
> +      do_QueryElementAt(mMethodData, idx);
> +    methodData->AppendElement(method, false);

this can fail.

@@ +100,5 @@
> +  mDetails->GetShippingOptions(getter_AddRefs(shippingOptions));
> +  mDetails->GetModifiers(getter_AddRefs(modifiers));
> +  mDetails->GetError(error);
> +  nsCOMPtr<nsIPaymentDetails> details =
> +    new nsPaymentDetails(id, totalItem, displayItems, shippingOptions, modifiers, error);

Clone()

@@ +120,5 @@
> +  mOptions->GetRequestPayerPhone(&requestPayerPhone);
> +  mOptions->GetRequestShipping(&requestShipping);
> +  mOptions->GetShippingType(shippingType);
> +  nsCOMPtr<nsIPaymentOptions> options =
> +    new nsPaymentOptions(requestPayerName,

Clone()

::: dom/payments/nsPaymentActionRequest.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_nsPaymentActionRequest_h

PaymentActionRequest

@@ +14,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class nsPaymentActionRequest : public nsIPaymentActionRequest

PaymentActionRequest

@@ +29,5 @@
> +  nsString mRequestId;
> +  uint32_t mType;
> +};
> +
> +class nsPaymentCreateActionRequest final : public nsIPaymentCreateActionRequest

PaymentCreateActionRequest

@@ +40,5 @@
> +
> +  nsPaymentCreateActionRequest() = default;
> +
> +private:
> +  ~nsPaymentCreateActionRequest() {};

= default here as well

::: dom/payments/nsPaymentRequest.cpp
@@ +20,5 @@
> +nsPaymentMethodData::nsPaymentMethodData(nsIArray* aSupportedMethods,
> +                                         const nsAString& aData)
> +  : mSupportedMethods(aSupportedMethods)
> +  , mData(aData)
> +{

MOZ_ASSERT(mSupportedMethods);

@@ +26,5 @@
> +
> +nsPaymentMethodData::nsPaymentMethodData(const IPCPaymentMethodData& aMethodData)
> +  : mData(aMethodData.data())
> +{
> +  ConvertStringstoISupportsStrings(aMethodData.supportedMethods(),

This can fail. You don't want to expose a CTOR that can fail. Move this private and make a:

PaymentMethodData* Create(const IPCPaymentMethodData& aMethodData)
{
  nsCOMPtr<nsIArray> supportedMethods;
  nsresult rv =
    ConvertStringstoISupportsStrings(aMethodData.supportedMethods(), supportedMethods);
  if (NS_WARN_IF(NS_FAILED(rv))) {
    return nullptr;
  }

  return new PaymentMethodData(supportedMethods, aMethodData.data())
}

and when you use it, check the return value.

@@ +27,5 @@
> +nsPaymentMethodData::nsPaymentMethodData(const IPCPaymentMethodData& aMethodData)
> +  : mData(aMethodData.data())
> +{
> +  ConvertStringstoISupportsStrings(aMethodData.supportedMethods(),
> +                                   getter_AddRefs(mSupportedMethods));

MOZ_ASSERT(mSupportedMethods);

@@ +30,5 @@
> +  ConvertStringstoISupportsStrings(aMethodData.supportedMethods(),
> +                                   getter_AddRefs(mSupportedMethods));
> +}
> +
> +nsPaymentMethodData::~nsPaymentMethodData()

default CTOR in the class declaration. Here and elsewhere.

@@ +38,5 @@
> +NS_IMETHODIMP
> +nsPaymentMethodData::GetSupportedMethods(nsIArray** aSupportedMethods)
> +{
> +  NS_ENSURE_ARG_POINTER(aSupportedMethods);
> +  MOZ_ASSERT(mSupportedMethods);

This can be removed.

@@ +69,5 @@
> +}
> +
> +nsPaymentCurrencyAmount::~nsPaymentCurrencyAmount()
> +{
> +}

= default;

@@ +106,5 @@
> +{
> +  mAmount = new nsPaymentCurrencyAmount(aItem.amount());
> +}
> +
> +nsPaymentItem::~nsPaymentItem()

same here.

@@ +123,5 @@
> +{
> +  NS_ENSURE_ARG_POINTER(aAmount);
> +  nsString system;
> +  nsString currency;
> +  nsString value;

all of these: nsAutoString and close to the getters.

@@ +127,5 @@
> +  nsString value;
> +  mAmount->GetCurrency(currency);
> +  mAmount->GetValue(value);
> +  RefPtr<nsPaymentCurrencyAmount> amount =
> +    new nsPaymentCurrencyAmount(currency, value);

just because mAmount is a PaymentCurrencyAmount class, you can have helper methods that allow you do to:

RefPtr<nsPaymentCurrencyAmount> amount = new PaymentCurrencyAmount(mAmount->Currency(), mAmount->Value());
In order to do that, instead having: nsCOMPtr<nsIPaymentCurrencyAmount> mAmount, you should have: RefPtr<PaymentCurrencyAmount> mAmount;

Probably, you can just have a already_AddRefed<PaymentCurrencyAmount> PaymentCurrecyAmount::Clone() const;

@@ +162,5 @@
> +  , mData(aModifier.data())
> +{
> +  mTotal = new nsPaymentItem(aModifier.total());
> +
> +  ConvertStringstoISupportsStrings(aModifier.supportedMethods(),

This can fail. Do a ::Create() as before.

@@ +169,5 @@
> +  if (aModifier.additionalDisplayItemsPassed()) {
> +    nsCOMPtr<nsIMutableArray> items = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +    for (const IPCPaymentItem& item : aModifier.additionalDisplayItems()) {
> +      nsCOMPtr<nsIPaymentItem> iitem = new nsPaymentItem(item);
> +      items->AppendElement(iitem, false);

check the return value.

@@ +194,5 @@
> +  NS_ENSURE_ARG_POINTER(aTotal);
> +  nsString label;
> +  nsCOMPtr<nsIPaymentCurrencyAmount> amount;
> +  bool pending;
> +  mTotal->GetLabel(label);

nsAutoString and value close to the getters.

@@ +197,5 @@
> +  bool pending;
> +  mTotal->GetLabel(label);
> +  mTotal->GetAmount(getter_AddRefs(amount));
> +  mTotal->GetPending(&pending);
> +  RefPtr<nsPaymentItem> total = new nsPaymentItem(label, amount, pending);

same here, if mTotal is a PaymentItem instead a nsIPaymenetItem, you can do:

RefPtr<PaymentItem> total = new PaymentItem(mTotal->Label(), mTotal->Amount(), mTotal->Pending());

Or, probably much better:

RefPtr<PaymentItem> total = mTotal->Clone();

@@ +205,5 @@
> +
> +NS_IMETHODIMP
> +nsPaymentDetailsModifier::GetAdditionalDisplayItems(nsIArray** aAdditionalDisplayItems)
> +{
> +  NS_ENSURE_ARG_POINTER(aAdditionalDisplayItems);

*aAdditionalDisplayItems = nullptr;

@@ +214,5 @@
> +  uint32_t length;
> +  mAdditionalDisplayItems->GetLength(&length);
> +  for (uint32_t index = 0; index < length; ++index) {
> +    nsCOMPtr<nsIPaymentItem> item = do_QueryElementAt(mAdditionalDisplayItems, index);
> +    items->AppendElement(item, false);

this can fail.

@@ +240,5 @@
> +  : mId(aId)
> +  , mLabel(aLabel)
> +  , mAmount(aAmount)
> +  , mSelected(aSelected)
> +{

MOZ_ASSERT(mAmout) ?

@@ +252,5 @@
> +  mAmount = new nsPaymentCurrencyAmount(aShippingOption.amount());
> +}
> +
> +nsPaymentShippingOption::~nsPaymentShippingOption()
> +{

= default;

@@ +276,5 @@
> +  NS_ENSURE_ARG_POINTER(aAmount);
> +  nsString system;
> +  nsString currency;
> +  nsString value;
> +  mAmount->GetCurrency(currency);

same comments as before.

@@ +279,5 @@
> +  nsString value;
> +  mAmount->GetCurrency(currency);
> +  mAmount->GetValue(value);
> +  RefPtr<nsPaymentCurrencyAmount> amount =
> +    new nsPaymentCurrencyAmount(currency, value);

->Clone()

@@ +332,5 @@
> +  if (aDetails.displayItemsPassed()) {
> +    nsCOMPtr<nsIMutableArray> items = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +    for (const IPCPaymentItem& displayItem : aDetails.displayItems()) {
> +      nsCOMPtr<nsIPaymentItem> item = new nsPaymentItem(displayItem);
> +      items->AppendElement(item, false);

This can fail. I want to see a ::Create() static method.

@@ +342,5 @@
> +    nsCOMPtr<nsIMutableArray> options = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +    for (const IPCPaymentShippingOption& shippingOption : aDetails.shippingOptions()) {
> +      nsCOMPtr<nsIPaymentShippingOption> option =
> +        new nsPaymentShippingOption(shippingOption);
> +      options->AppendElement(option, false);

return value

@@ +351,5 @@
> +  if (aDetails.modifiersPassed()) {
> +    nsCOMPtr<nsIMutableArray> modifiers = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +    for (const IPCPaymentDetailsModifier& modifier : aDetails.modifiers()) {
> +      nsCOMPtr<nsIPaymentDetailsModifier> imodifier = new nsPaymentDetailsModifier(modifier);
> +      modifiers->AppendElement(imodifier, false);

return value

@@ +357,5 @@
> +    mModifiers = modifiers.forget();
> +  }
> +}
> +
> +nsPaymentDetails::~nsPaymentDetails()

default

@@ +378,5 @@
> +  bool pending;
> +  mTotalItem->GetLabel(label);
> +  mTotalItem->GetAmount(getter_AddRefs(amount));
> +  mTotalItem->GetPending(&pending);
> +  RefPtr<nsPaymentItem> totalItem = new nsPaymentItem(label, amount, pending);

Clone()

@@ +386,5 @@
> +
> +NS_IMETHODIMP
> +nsPaymentDetails::GetDisplayItems(nsIArray** aDisplayItems)
> +{
> +  NS_ENSURE_ARG_POINTER(aDisplayItems);

*aDisplayItems = nullptr;

@@ +395,5 @@
> +  uint32_t length;
> +  mDisplayItems->GetLength(&length);
> +  for (uint32_t index = 0; index < length; ++index) {
> +    nsCOMPtr<nsIPaymentItem> item = do_QueryElementAt(mDisplayItems, index);
> +    items->AppendElement(item, false);

it can fail.

@@ +404,5 @@
> +
> +NS_IMETHODIMP
> +nsPaymentDetails::GetShippingOptions(nsIArray** aShippingOptions)
> +{
> +  NS_ENSURE_ARG_POINTER(aShippingOptions);

*aShippingOptions = nullptr;

@@ +413,5 @@
> +  uint32_t length;
> +  mShippingOptions->GetLength(&length);
> +  for (uint32_t index = 0; index < length; ++index) {
> +    nsCOMPtr<nsIPaymentShippingOption> option = do_QueryElementAt(mShippingOptions, index);
> +    options->AppendElement(option, false);

this can fail.

@@ +423,5 @@
> +NS_IMETHODIMP
> +nsPaymentDetails::GetModifiers(nsIArray** aModifiers)
> +{
> +  NS_ENSURE_ARG_POINTER(aModifiers);
> +  if (!mModifiers) {

*aModifiers = nullptr;

@@ +431,5 @@
> +  uint32_t length;
> +  mModifiers->GetLength(&length);
> +  for (uint32_t index = 0; index < length; ++index) {
> +    nsCOMPtr<nsIPaymentDetailsModifier> modifier = do_QueryElementAt(mModifiers, index);
> +    modifiers->AppendElement(modifier, false);

return value

@@ +473,5 @@
> +}
> +
> +nsPaymentOptions::~nsPaymentOptions()
> +{
> +}

default;

@@ +529,5 @@
> +  , mRequestId(aRequestId)
> +  , mPaymentMethods(aPaymentMethods)
> +  , mPaymentDetails(aPaymentDetails)
> +  , mPaymentOptions(aPaymentOptions)
> +{

MOZ_ASSERT for some of this pointers?

@@ +532,5 @@
> +  , mPaymentOptions(aPaymentOptions)
> +{
> +}
> +
> +nsPaymentRequest::~nsPaymentRequest()

default

@@ +580,5 @@
> +  mPaymentDetails->GetShippingOptions(getter_AddRefs(shippingOptions));
> +  mPaymentDetails->GetModifiers(getter_AddRefs(modifiers));
> +  mPaymentDetails->GetError(error);
> +  nsCOMPtr<nsIPaymentDetails> details =
> +    new nsPaymentDetails(id, totalItem, displayItems, shippingOptions, modifiers, error);

Clone()

@@ +587,5 @@
> +}
> +
> +NS_IMETHODIMP
> +nsPaymentRequest::UpdatePaymentDetails(nsIPaymentDetails* aPaymentDetails)
> +{

should you check aPaymenetDatails pointer?

@@ +588,5 @@
> +
> +NS_IMETHODIMP
> +nsPaymentRequest::UpdatePaymentDetails(nsIPaymentDetails* aPaymentDetails)
> +{
> +  nsString id;

nsAutoString

@@ +593,5 @@
> +  nsCOMPtr<nsIPaymentItem> totalItem;
> +  nsCOMPtr<nsIArray> displayItems;
> +  nsCOMPtr<nsIArray> shippingOptions;
> +  nsCOMPtr<nsIArray> modifiers;
> +  nsString error;

nsAutoString

@@ +602,5 @@
> +   * |error| only from new details.
> +   *
> +   *   [1] https://www.w3.org/TR/payment-request/#updatewith-method
> +   */
> +  mPaymentDetails->GetId(id);

move nsAutoString id here.

@@ +623,5 @@
> +    mPaymentDetails->GetModifiers(getter_AddRefs(modifiers));
> +  }
> +  aPaymentDetails->GetError(error);
> +
> +  mPaymentDetails = new nsPaymentDetails(id, totalItem, displayItems,

probably nsIPaymenetDatails cannot be a builtinclass. But it if could, just do a static_cast and do Clone()

@@ +643,5 @@
> +  mPaymentOptions->GetRequestPayerPhone(&requestPayerPhone);
> +  mPaymentOptions->GetRequestShipping(&requestShipping);
> +  mPaymentOptions->GetShippingType(shippingType);
> +  nsCOMPtr<nsIPaymentOptions> options =
> +    new nsPaymentOptions(requestPayerName,

Clone()

::: dom/payments/nsPaymentRequest.h
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_nsPaymentRequest_h
> +#define mozilla_dom_nsPaymentRequest_h

again, this will be PaymentRequest

@@ +12,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class nsPaymentMethodData final : public nsIPaymentMethodData

and this will be PaymentMethodData

@@ +20,5 @@
> +  NS_DECL_NSIPAYMENTMETHODDATA
> +
> +  nsPaymentMethodData(nsIArray* aSupportedMethods,
> +                      const nsAString& aData);
> +  nsPaymentMethodData(const IPCPaymentMethodData& aMethodData);

explicit

@@ +22,5 @@
> +  nsPaymentMethodData(nsIArray* aSupportedMethods,
> +                      const nsAString& aData);
> +  nsPaymentMethodData(const IPCPaymentMethodData& aMethodData);
> +
> +protected:

private: if this is final. Do the same for any other final class in any other file.

@@ +37,5 @@
> +  NS_DECL_NSIPAYMENTCURRENCYAMOUNT
> +
> +  nsPaymentCurrencyAmount(const nsAString& aCurrency,
> +                          const nsAString& aValue);
> +  nsPaymentCurrencyAmount(const IPCPaymentCurrencyAmount& aCurrencyAmount);

explicit

@@ +55,5 @@
> +
> +  nsPaymentItem(const nsAString& aLabel,
> +                nsIPaymentCurrencyAmount* aAmount,
> +                const bool aPending);
> +  nsPaymentItem(const IPCPaymentItem& aItem);

explicit

@@ +75,5 @@
> +  nsPaymentDetailsModifier(nsIArray* aSupportedMethods,
> +                           nsIPaymentItem* aTotal,
> +                           nsIArray* aAdditionalDisplayItems,
> +                           const nsAString& aData);
> +  nsPaymentDetailsModifier(const IPCPaymentDetailsModifier& modifier);

explicit :) Here and in the other CTORs.
Btw, do we need copy CTORs?

@@ +178,5 @@
> +  nsCOMPtr<nsIPaymentOptions> mPaymentOptions;
> +};
> +
> +} // end of namespace dom
> +} // end of namespace mozilla

extra line here.

::: dom/payments/nsPaymentRequestService.cpp
@@ +14,5 @@
> +/* nsPaymentRequestService */
> +
> +NS_IMPL_ISUPPORTS(nsPaymentRequestService,
> +                  nsIPaymentRequestService)
> +

namespace {
StaticRefPtr<nsPaymentRequestService> sSingleton;
}

@@ +24,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +  if (!sSingleton) {
> +    sSingleton = new nsPaymentRequestService();
> +  }
> +  RefPtr<nsPaymentRequestService> service = sSingleton.get();

you don't need to use .get() here.

@@ +32,5 @@
> +nsPaymentRequestService::nsPaymentRequestService()
> +{
> +}
> +
> +nsPaymentRequestService::~nsPaymentRequestService()

move it to the .h file and mark the DTOR with = default;
You can do the same for the CTOR.

@@ +40,5 @@
> +NS_IMETHODIMP
> +nsPaymentRequestService::GetPaymentRequestById(const nsAString& aRequestId,
> +                                               nsIPaymentRequest** aRequest)
> +{
> +  NS_ENSURE_ARG_POINTER(aRequest);

you must set *aRequest = nullptr; here

@@ +44,5 @@
> +  NS_ENSURE_ARG_POINTER(aRequest);
> +  uint32_t numRequests = mRequestQueue.Length();
> +  for (uint32_t index = 0; index < numRequests; ++index) {
> +    nsCOMPtr<nsIPaymentRequest> request = mRequestQueue[index];
> +    nsString requestId;

nsAutoString

@@ +64,5 @@
> +}
> +
> +NS_IMETHODIMP
> +nsPaymentRequestService::RequestPayment(nsIPaymentActionRequest* aRequest)
> +{

NS_ENSURE_ARG_POINTER(aRequest);

@@ +67,5 @@
> +nsPaymentRequestService::RequestPayment(nsIPaymentActionRequest* aRequest)
> +{
> +  nsCOMPtr<nsIPaymentRequest> payment;
> +  uint32_t type;
> +  aRequest->GetType(&type);

can this fail?

@@ +71,5 @@
> +  aRequest->GetType(&type);
> +  switch (type) {
> +    case nsIPaymentActionRequest::CREATE_ACTION: {
> +      nsCOMPtr<nsIPaymentCreateActionRequest> request =
> +        do_QueryInterface(aRequest);

MOZ_ASSERT(request);

@@ +77,5 @@
> +      nsString requestId;
> +      nsCOMPtr<nsIArray> methodData;
> +      nsCOMPtr<nsIPaymentDetails> details;
> +      nsCOMPtr<nsIPaymentOptions> options;
> +      request->GetTabId(&tabId);

variables close to the getters. Plus check the return value (if needed).

@@ +82,5 @@
> +      request->GetRequestId(requestId);
> +      request->GetMethodData(getter_AddRefs(methodData));
> +      request->GetDetails(getter_AddRefs(details));
> +      request->GetOptions(getter_AddRefs(options));
> +      payment = new nsPaymentRequest(tabId, requestId, methodData, details, options);

PaymentRequest...

@@ +83,5 @@
> +      request->GetMethodData(getter_AddRefs(methodData));
> +      request->GetDetails(getter_AddRefs(details));
> +      request->GetOptions(getter_AddRefs(options));
> +      payment = new nsPaymentRequest(tabId, requestId, methodData, details, options);
> +      mRequestQueue.AppendElement(payment);

Just make this array a FallibleTArray<> and here check the return value passing 'fallible' as argument. In case, return OOM.

::: dom/payments/nsPaymentRequestService.h
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_nsPaymentRequestService_h
> +#define mozilla_dom_nsPaymentRequestService_h

I don't like files called "nsFoo".  Just call them "Foo". Use nsSomething only for the interfaces.

@@ +16,5 @@
> +namespace dom {
> +
> +// The implmentation of nsIPaymentRequestService
> +
> +class nsPaymentRequestService final : public nsIPaymentRequestService

This will be PaymentRequestService

@@ +29,5 @@
> +
> +private:
> +  ~nsPaymentRequestService();
> +
> +  static StaticRefPtr<nsPaymentRequestService> sSingleton;

you don't need to have it here. Just move this in an anonymous namespace in the cpp file.

@@ +34,5 @@
> +  nsCOMArray<nsIPaymentRequest> mRequestQueue;
> +};
> +
> +} // end of namespace dom
> +} // end of namespace mozilla

extra line here.

::: dom/payments/nsPaymentRequestUtils.cpp
@@ +21,5 @@
> +  for (const nsString& string : aStrings) {
> +    nsCOMPtr<nsISupportsString> iString =
> +      do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID);
> +    iString->SetData(string);
> +    iStrings->AppendElement(iString, false);

checks the return value of this. It can happen that if fails if we have a huge number of strings

@@ +31,5 @@
> +nsresult
> +ConvertISupportsStringstoStrings(nsIArray* aIStrings,
> +                                 nsTArray<nsString>& aStrings)
> +{
> +  uint32_t length;

move it to the next line, close to when you use it.

@@ +33,5 @@
> +                                 nsTArray<nsString>& aStrings)
> +{
> +  uint32_t length;
> +  aStrings.Clear();
> +  aIStrings->GetLength(&length);

I guess this can fail. check the return value.

@@ +37,5 @@
> +  aIStrings->GetLength(&length);
> +  for (uint32_t index = 0; index < length; ++index) {
> +    nsCOMPtr<nsISupportsString> iString = do_QueryElementAt(aIStrings, index);
> +    MOZ_ASSERT(iString);
> +    nsString string;

nsAutoString ?

@@ +38,5 @@
> +  for (uint32_t index = 0; index < length; ++index) {
> +    nsCOMPtr<nsISupportsString> iString = do_QueryElementAt(aIStrings, index);
> +    MOZ_ASSERT(iString);
> +    nsString string;
> +    iString->GetData(string);

maybe this can fail as well.

@@ +39,5 @@
> +    nsCOMPtr<nsISupportsString> iString = do_QueryElementAt(aIStrings, index);
> +    MOZ_ASSERT(iString);
> +    nsString string;
> +    iString->GetData(string);
> +    aStrings.AppendElement(string);

same here.

@@ +53,5 @@
> +  uint32_t length;
> +  aSourceStrings->GetLength(&length);
> +  for (uint32_t index = 0; index < length; ++index) {
> +    nsCOMPtr<nsISupportsString> string = do_QueryElementAt(aSourceStrings, index);
> +    MOZ_ASSERT(string);

instead doing MOZ_ASSERT(), do: if (NS_WARN_IF(!string)) { return NS_ERROR_FAILURE; }

@@ +54,5 @@
> +  aSourceStrings->GetLength(&length);
> +  for (uint32_t index = 0; index < length; ++index) {
> +    nsCOMPtr<nsISupportsString> string = do_QueryElementAt(aSourceStrings, index);
> +    MOZ_ASSERT(string);
> +    strings->AppendElement(string, false);

check here.
Attachment #8864758 - Flags: review-
(In reply to Andrea Marchesini [:baku] from comment #7)
> Comment on attachment 8864758 [details] [diff] [review]
> Bug1345361_PaymentRequest_Constructor_Implementation
> 
> Review of attachment 8864758 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +52,5 @@
> > +    aErrorMsg.AssignLiteral("The amount.value of \"");
> > +    aErrorMsg.Append(aItem);
> > +    aErrorMsg.AppendLiteral("\"(");
> > +    aErrorMsg.Append(aValue);
> > +    aErrorMsg.AppendLiteral(") must be a valid decimal monetary value.");
> 
> Well, this _must_ be localized.
> 
> @@ +71,5 @@
> > +    aErrorMsg.AssignLiteral("The amount.value of \"");
> > +    aErrorMsg.Append(aItem);
> > +    aErrorMsg.AppendLiteral("\"(");
> > +    aErrorMsg.Append(aValue);
> > +    aErrorMsg.AppendLiteral(") must be a valid and positive decimal monetary value.");
> 
> same here.
> 

Hi Baku,
Thanks for your quick review. :)

These messages are for merchant site developer not for the user.
Do we still need to do the localization?

From the specification, it is optional to inform the developer the value is invalid.
So we can just throw a "TypeError" without the messages. What do you think?

(From specification:https://w3c.github.io/browser-payment-api/#constructor)
Process the total:
    If details.total.amount.value is not a valid decimal monetary value, then throw a TypeError; optionally informing the developer that the value is invalid.
Flags: needinfo?(amarchesini)
Blocks: 1318993
> These messages are for merchant site developer not for the user.
> Do we still need to do the localization?
Flags: needinfo?(amarchesini) → needinfo?(francesco.lodolo)
Can you clarify where these messages would be displayed? If we end up showing them to end-users, e.g. in console, it's still good to expose them properly for localization.
Flags: needinfo?(francesco.lodolo)
(In reply to Francesco Lodolo [:flod] from comment #10)
> Can you clarify where these messages would be displayed? If we end up
> showing them to end-users, e.g. in console, it's still good to expose them
> properly for localization.

If we find the invalid value in details when merchant site create a PaymentRequest,
we will throw this error with those messages.

[example]
  try {
    request = new PaymentRequest(supportedInstruments, details);
  } catch (e) {
    // merchant site catch this error with messages
  }

Since the details are provided by merchant site, the messages are not useful to end user.
The merchant site should re-create a new PaymentRequest with valid detail if they catch this error.
In my opinion, the user won't see these messages. Make sense?
Flags: needinfo?(francesco.lodolo)
(In reply to Alphan Chen [:alchen] from comment #11)
> In my opinion, the user won't see these messages. Make sense?

So, merchant site requests via API and get the error message via API as part of the response. If that's the case, hard-coding makes sense.
Flags: needinfo?(francesco.lodolo)
In this patch, I did following improvements according to the review on comment 7.

1. Moving IPC from PContent to PBrowser
2. Removing ns prefix in all files/classes.
   Since some XPCOM classes' name have conflicts with webidl codegen, I move these XPCOM classes to namespace mozilla::dom::payments and keep webidl codegen classes under mozilla::dom. 

and other some coding style improvements.

Following is the try result

https://treeherder.mozilla.org/#/jobs?repo=try&revision=03cde7ccdb5b02304f34191cc229a6daa5e9c9c3
Attachment #8864758 - Attachment is obsolete: true
Attachment #8866695 - Flags: review?(amarchesini)
> @@ +79,5 @@
> > +};
> > +
> > +union PaymentActionRequest
> > +{
> > +  PaymentCreateActionRequest;
> 
> Why this empty union? Can you just use PaymentCreateActionRequest? I guess
> you are planning to expand it in the next patches.
> 

Yes, I will expand this union when implementing other functions.
Currently, we only need PaymentCreateActionRequest here for payment request construction.
Comment on attachment 8866695 [details] [diff] [review]
Bug1345361_PaymentRequest_Constructor_Implementation

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

Good! I suspect that Clone() is not needed, in the end. Tell me why you want to duplicate data each time for getter methods.

::: dom/ipc/TabChild.cpp
@@ +3300,5 @@
> +bool
> +TabChild::DeallocPPaymentRequestChild(PPaymentRequestChild* aActor)
> +{
> +  PaymentRequestChild* child = static_cast<PaymentRequestChild*>(aActor);
> +  NS_RELEASE(child);

RefPtr<PaymentRequestChild> actor = dont_AddRef(static_cast<PaymentRequestChild*>(aActor));

don't use NS_RELEASE.

::: dom/payments/PaymentActionRequest.cpp
@@ +74,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PaymentCreateActionRequest::GetMethodData(nsIArray** aMethodData)
> +{

any reason why we cannot do:

nsCOMPtr<nsIArray> methodData = mMethodData;
methodData.forget(aMethodData);

@@ +103,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PaymentCreateActionRequest::GetDetails(nsIPaymentDetails** aDetails)
> +{

same here.

@@ +116,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PaymentCreateActionRequest::GetOptions(nsIPaymentOptions** aOptions)
> +{

same here.

::: dom/payments/PaymentActionRequest.h
@@ +31,5 @@
> +  uint32_t mType;
> +};
> +
> +class PaymentCreateActionRequest final : public nsIPaymentCreateActionRequest
> +                                         , public PaymentActionRequest

indentation

::: dom/payments/PaymentRequest.cpp
@@ +167,5 @@
> +  }
> +
> +  // Create PaymentRequest and set its |mId|
> +  RefPtr<PaymentRequest> request;
> +  nsresult rv = manager->CreatePayment(window, aMethodData, aDetails, aOptions, getter_AddRefs(request));

80chars max.

::: dom/payments/PaymentRequest.h
@@ +18,5 @@
> +
> +class EventHandlerNonNull;
> +class PaymentResponse;
> +
> +} // namespace dom

don't close these 2 namespaces. Just leave them open and remove line 25 and 26.

@@ +56,5 @@
> +  IsVaildDetailsBase(const PaymentDetailsBase& aDetails,
> +                     nsAString& aErrorMsg);
> +
> +  static already_AddRefed<PaymentRequest>
> +    Constructor(const GlobalObject& aGlobal,

don't indent Constructor in this way.

@@ +84,5 @@
> +
> +protected:
> +  ~PaymentRequest();
> +
> +  explicit PaymentRequest(nsPIDOMWindowInner* aWindow, const nsAString& aInternalId);

no explicit with more than 1 argument.

::: dom/payments/PaymentRequestData.cpp
@@ +89,5 @@
> +PaymentCurrencyAmount::Create(const IPCPaymentCurrencyAmount& aIPCAmount,
> +                              nsIPaymentCurrencyAmount** aAmount)
> +{
> +  NS_ENSURE_ARG_POINTER(aAmount);
> +  *aAmount = nullptr;

remove this

@@ +114,5 @@
> +NS_IMETHODIMP
> +PaymentCurrencyAmount::Clone(nsIPaymentCurrencyAmount** aAmount)
> +{
> +  NS_ENSURE_ARG_POINTER(aAmount);
> +  *aAmount = nullptr;

remove this.

@@ +138,5 @@
> +nsresult
> +PaymentItem::Create(const IPCPaymentItem& aIPCItem, nsIPaymentItem** aItem)
> +{
> +  NS_ENSURE_ARG_POINTER(aItem);
> +  *aItem = nullptr;

remove this.

@@ +141,5 @@
> +  NS_ENSURE_ARG_POINTER(aItem);
> +  *aItem = nullptr;
> +  nsCOMPtr<nsIPaymentCurrencyAmount> amount;
> +  nsresult rv = PaymentCurrencyAmount::Create(aIPCItem.amount(), getter_AddRefs(amount));
> +  if (NS_WARN_IF(NS_FAILED(rv) || !amount)) {

!amount cannot happen, right? remove it

@@ +161,5 @@
> +NS_IMETHODIMP
> +PaymentItem::GetAmount(nsIPaymentCurrencyAmount** aAmount)
> +{
> +  NS_ENSURE_ARG_POINTER(aAmount);
> +  *aAmount = nullptr;

remove

@@ +163,5 @@
> +{
> +  NS_ENSURE_ARG_POINTER(aAmount);
> +  *aAmount = nullptr;
> +  MOZ_ASSERT(mAmount);
> +  nsresult rv = mAmount->Clone(aAmount);

This should be just:

nsCOMPtr<nsIArray> amount = mAmount;
amount.forget(aAmount);

don't clone anything here. Otherwise: item.amount != item.amount

@@ +236,5 @@
> +    MOZ_ASSERT(items);
> +    for (const IPCPaymentItem& item : aIPCModifier.additionalDisplayItems()) {
> +      nsCOMPtr<nsIPaymentItem> additionalItem;
> +      rv = PaymentItem::Create(item, getter_AddRefs(additionalItem));
> +      if (NS_WARN_IF(NS_FAILED(rv) || !additionalItem)) {

additionalItem this cannot be null if rv is NS_OK. Remove this pattern everywhere in the code.

@@ +254,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PaymentDetailsModifier::GetSupportedMethods(nsIArray** aSupportedMethods)
> +{

return mSupportedMethods directly.

@@ +262,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PaymentDetailsModifier::GetTotal(nsIPaymentItem** aTotal)
> +{

don't clone mTotal.

@@ +264,5 @@
> +NS_IMETHODIMP
> +PaymentDetailsModifier::GetTotal(nsIPaymentItem** aTotal)
> +{
> +  NS_ENSURE_ARG_POINTER(aTotal);
> +  *aTotal = nullptr;

remove

@@ +275,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PaymentDetailsModifier::GetAdditionalDisplayItems(nsIArray** aAdditionalDisplayItems)
> +{

same here.

@@ +387,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PaymentShippingOption::GetAmount(nsIPaymentCurrencyAmount** aAmount)
> +{

ditto

@@ +536,5 @@
> +PaymentDetails::GetTotalItem(nsIPaymentItem** aTotalItem)
> +{
> +  NS_ENSURE_ARG_POINTER(aTotalItem);
> +  *aTotalItem = nullptr;
> +  nsresult rv = mTotalItem->Clone(aTotalItem);

why do you need to clone it? If you clone it, paymentaData.totalItem != paymentData.totalItem and this seems wrong.

@@ +545,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PaymentDetails::GetDisplayItems(nsIArray** aDisplayItems)
> +{

here as well.

@@ +577,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PaymentDetails::GetShippingOptions(nsIArray** aShippingOptions)
> +{

here as well.

@@ +609,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PaymentDetails::GetModifiers(nsIArray** aModifiers)
> +{

here as well.

@@ +851,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PaymentRequest::GetPaymentMethods(nsIArray** aPaymentMethods)
> +{

here as well.

@@ +881,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PaymentRequest::GetPaymentDetails(nsIPaymentDetails** aPaymentDetails)
> +{

here as well.

@@ +894,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PaymentRequest::GetPaymentOptions(nsIPaymentOptions** aPaymentOptions)
> +{

here as well.

::: dom/payments/PaymentRequestService.cpp
@@ +11,5 @@
> +namespace dom {
> +
> +StaticRefPtr<PaymentRequestService> gPaymentService;
> +
> +class PaymentRequestEnumerator final : public nsISimpleEnumerator

all this class can be in a anonymous namespace.

@@ +18,5 @@
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSISIMPLEENUMERATOR
> +
> +  PaymentRequestEnumerator()
> +    : mIndex(0) {}

{} in a new line.

@@ +50,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +  nsCOMPtr<nsIPaymentRequest> request =
> +    gPaymentService->GetPaymentRequestByIndex(mIndex);
> +  mIndex++;

increase this after the null-check for item.

@@ +82,5 @@
> +  return mRequestQueue.Length();
> +}
> +
> +already_AddRefed<nsIPaymentRequest>
> +PaymentRequestService::GetPaymentRequestByIndex(const uint32_t index)

aIndex

::: dom/payments/ipc/PaymentRequestChild.h
@@ +15,5 @@
> +class PaymentRequestChild final : public PPaymentRequestChild
> +{
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(PaymentRequestChild);
> +public:
> +  PaymentRequestChild()

move the CTOR in the cpp file.

@@ +16,5 @@
> +{
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(PaymentRequestChild);
> +public:
> +  PaymentRequestChild()
> +    : mActorAlive(true) {};

1. no ';' needed.
2. {} in a new line.

::: dom/payments/ipc/PaymentRequestParent.cpp
@@ +41,5 @@
> +      MOZ_ASSERT(methodData);
> +      for (IPCPaymentMethodData data : request.methodData()) {
> +        nsCOMPtr<nsIPaymentMethodData> method;
> +        rv = PaymentMethodData::Create(data, getter_AddRefs(method));
> +        if (NS_WARN_IF(NS_FAILED(rv) || !method)) {

It cannot be that rv is NS_OK but method is null. Just do:

if (NS_WARN_IF(NS_FAILED(rv)) {
  ...
}

MOZ_ASSERT(method);

@@ +52,5 @@
> +      }
> +
> +      nsCOMPtr<nsIPaymentDetails> details;
> +      rv = PaymentDetails::Create(request.details(), getter_AddRefs(details));
> +      if (NS_WARN_IF(NS_FAILED(rv) || !details)) {

same assertion here.

@@ +58,5 @@
> +      }
> +
> +      nsCOMPtr<nsIPaymentOptions> options;
> +      rv = PaymentOptions::Create(request.options(), getter_AddRefs(options));
> +      if (NS_WARN_IF(NS_FAILED(rv) || !options)) {

and here.

@@ +63,5 @@
> +        return IPC_FAIL_NO_REASON(this);
> +      }
> +
> +      nsCOMPtr<nsIPaymentCreateActionRequest> createRequest =
> +        do_CreateInstance(NS_PAYMENT_CREATE_ACTION_REQUEST_CONTRACT_ID);

this _could_ return null. Add a check: if (NS_WARN_IF(!createRequest)) ...
Attachment #8866695 - Flags: review?(amarchesini) → review-
(In reply to Andrea Marchesini [:baku] from comment #16)
> Comment on attachment 8866695 [details] [diff] [review]
> Bug1345361_PaymentRequest_Constructor_Implementation
> 
> Review of attachment 8866695 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good! I suspect that Clone() is not needed, in the end. Tell me why you want
> to duplicate data each time for getter methods.
> 


These data should be only sent and updated from merchant side. So, I duplicate them to make sure they are not modified even getting from C++ code.
However, except getter methods, we don't have other methods to access these data, we probably can return the pointer directly without duplication.
Attached patch bug1345361.patch (obsolete) — Splinter Review
The attached patch is modified according to the comment 16.

1. remove Clone form all PaymentRequestXXX.
2. add a mochitest for payment request construction to verify the payment is constructed with correct data.
   Most constructor error condition tests are in the newest web-payment-test for payment request
   https://github.com/w3c/web-platform-tests/tree/master/payment-request
Attachment #8866695 - Attachment is obsolete: true
Attachment #8868088 - Flags: review?(amarchesini)
Comment on attachment 8868088 [details] [diff] [review]
bug1345361.patch

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

This is ok but, before landing you need to add tests and the support for non-e10s. Separate patches for both tasks. Thanks!

::: dom/payments/PaymentRequestData.cpp
@@ +231,5 @@
> +NS_IMETHODIMP
> +PaymentDetailsModifier::GetAdditionalDisplayItems(nsIArray** aAdditionalDisplayItems)
> +{
> +  NS_ENSURE_ARG_POINTER(aAdditionalDisplayItems);
> +  if (!mAdditionalDisplayItems) {

remove this. nsCOMPtr supports nullptr.

@@ +432,5 @@
> +NS_IMETHODIMP
> +PaymentDetails::GetDisplayItems(nsIArray** aDisplayItems)
> +{
> +  NS_ENSURE_ARG_POINTER(aDisplayItems);
> +  if (!mDisplayItems) {

remove this as well

@@ +444,5 @@
> +NS_IMETHODIMP
> +PaymentDetails::GetShippingOptions(nsIArray** aShippingOptions)
> +{
> +  NS_ENSURE_ARG_POINTER(aShippingOptions);
> +  if (!mShippingOptions) {

remove this.

@@ +456,5 @@
> +NS_IMETHODIMP
> +PaymentDetails::GetModifiers(nsIArray** aModifiers)
> +{
> +  NS_ENSURE_ARG_POINTER(aModifiers);
> +  if (!mModifiers) {

remove

::: dom/payments/PaymentRequestData.h
@@ +25,5 @@
> +                         nsIPaymentMethodData** aMethodData);
> +
> +private:
> +  PaymentMethodData(nsIArray* aSupportedMethods,
> +                      const nsAString& aData);

indentation

@@ +44,5 @@
> +                         nsIPaymentCurrencyAmount** aAmount);
> +
> +private:
> +  PaymentCurrencyAmount(const nsAString& aCurrency,
> +                          const nsAString& aValue);

indentation

@@ +62,5 @@
> +  static nsresult Create(const IPCPaymentItem& aIPCItem, nsIPaymentItem** aItem);
> +
> +private:
> +  PaymentItem(const nsAString& aLabel,
> +                nsIPaymentCurrencyAmount* aAmount,

indentation

@@ +83,5 @@
> +                         nsIPaymentDetailsModifier** aModifier);
> +
> +private:
> +  PaymentDetailsModifier(nsIArray* aSupportedMethods,
> +                           nsIPaymentItem* aTotal,

indentation

@@ +106,5 @@
> +                         nsIPaymentShippingOption** aOption);
> +
> +private:
> +  PaymentShippingOption(const nsAString& aId,
> +                          const nsAString& aLabel,

indentation

@@ +129,5 @@
> +  static nsresult Create(const IPCPaymentDetails& aIPCDetails,
> +                         nsIPaymentDetails** aDetails);
> +private:
> +  PaymentDetails(const nsAString& aId,
> +                   nsIPaymentItem* aTotalItem,

indentation

@@ +156,5 @@
> +                         nsIPaymentOptions** aOptions);
> +
> +private:
> +  PaymentOptions(const bool aRequestPayerName,
> +                   const bool aRequestPayerEmail,

indentation

@@ +176,5 @@
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIPAYMENTREQUEST
> +
> +  PaymentRequest(const uint64_t aTabId,
> +                   const nsAString& aRequestId,

indentation

::: dom/payments/PaymentRequestManager.cpp
@@ +43,5 @@
> +  }
> +
> +  // Convert JSObject to a serialized string
> +  nsAutoString serializedData;
> +  nsresult rv;

rv is just used at line 49. move it there: nsresult rv = SerializeFromJSObject(...) ...

@@ +81,5 @@
> +  }
> +
> +  // Convert JSObject to a serialized string
> +  nsAutoString serializedData;
> +  nsresult rv;

same here.

@@ +224,5 @@
> +
> +  nsPIDOMWindowInner* win = aRequest->GetOwner();
> +  NS_ENSURE_TRUE(win, NS_ERROR_FAILURE);
> +  TabChild* tabChild = TabChild::GetFrom(win->GetDocShell());
> +  NS_ENSURE_TRUE(tabChild, NS_ERROR_FAILURE);

This means that this implementation doesn't work in non-e10s mode.
You need to support non-e10s. And we need tests.

::: dom/payments/PaymentRequestService.cpp
@@ +55,5 @@
> +    gPaymentService->GetPaymentRequestByIndex(mIndex);
> +  if (NS_WARN_IF(!request)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  mIndex++;

move this before ite.forget(aItem);

::: dom/payments/ipc/PaymentRequestParent.h
@@ +17,5 @@
> +{
> +public:
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(PaymentRequestParent)
> +
> +  explicit PaymentRequestParent(const uint64_t aTabId);

const here is not needed.
Attachment #8868088 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini [:baku] from comment #20)
> Comment on attachment 8868088 [details] [diff] [review]
> bug1345361.patch
> 
> Review of attachment 8868088 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is ok but, before landing you need to add tests and the support for
> non-e10s. Separate patches for both tasks. Thanks!

About non-e10s implementation, need we fire a new bug for non-e10s support? or we should work on the this bug.

if we need a Id that like tabId to identify which tab requests this PaymentRequest, do you have any suggestions for non-e10s situation? I guess DocshellId is probably a good candidate.
Flags: needinfo?(amarchesini)
File a separate bug. It's still unclear if we want to support non-e10s mode. I'll let you know. From my point of view, we should, but this can be done in a follow up.
Note that probably your tests will not work in non-e10s mode. Please disable them for non-e10s before landing.
Flags: needinfo?(amarchesini)
I fire a bug 1365964 for non-e10s support implementation of PaymentRequest API.
Improve patch according comment 20.

1. separating tests to another patch.
2. fire a bug for non-e10s support.
Attachment #8868088 - Attachment is obsolete: true
Attachment #8869313 - Flags: review+
Hello Baku

I found some IPC problems, so I did following improvements. 

1. PaymentRequestChild::ActorDestory should inform PaymentRequestManager to nullify the pointer.

2. Avoiding calling PaymentRequestChild::SendRequestPayment() directly, because PaymentRequestChild might not be valid. I use PaymentRequestChild::RequestPayment() to wrap SendRequestPayment() and check the mActorAlive first.

3. calling ClearOnShutdown for PaymentRequestManager creation to avoid leaks.

So before I set as checkin-needed, I want to send a review for these improvements.
And tests are attached in next attachment.
Attachment #8869313 - Attachment is obsolete: true
Attachment #8870350 - Flags: review?(amarchesini)
Mochitest for PaymentRequest constructor.

1. browser_payment_construction.js 
   testing the simplest payment request construction. Also testing the field generated by browser.

2. browser_multiple_construction.js
   testing multiple payment request construction in the same tab.

3. browser_payment_in_different_tabs.js
   testing the payment request construction from different tabs.
Attachment #8870352 - Flags: review?(amarchesini)
Comment on attachment 8870350 [details] [diff] [review]
Bug1345361 PaymentRequest constructor implementation. r?baku

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

Can you send me an interdiff please?
Attachment #8870350 - Flags: review?(amarchesini)
Comment on attachment 8870352 [details] [diff] [review]
Bug1345361 Mochitest for PaymentRequest constructor. r?baku

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

::: dom/payments/test/browser.ini
@@ +1,2 @@
> +[DEFAULT]
> +skip-if = !e10s

write a comment saying that this will be removed eventually.
Attachment #8870352 - Flags: review?(amarchesini) → review+
The attached patch is the interdiff with previous patch. Thanks.
Attachment #8870447 - Flags: review?(amarchesini)
Comment on attachment 8870447 [details] [diff] [review]
Bug1345361  Interdiff with previous patch. r?Baku

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

::: dom/interfaces/payments/nsIPaymentRequestService.idl
@@ +18,5 @@
>  interface nsIPaymentRequestService : nsISupports
>  {
>    nsIPaymentRequest getPaymentRequestById(in AString requestId);
>    nsISimpleEnumerator enumerate();
> +  void cleanup();

it seems you are not using this method. Remove it.

::: dom/ipc/TabChild.cpp
@@ +3227,5 @@
>  {
> +  /*
> +    No need to do anything here.
> +    PaymentRequestManager takes care the deallocation by reference counting
> +    mechanism. PaymentRequestChild::ActorDestory aslo calls PaymentRequestManager

also

::: dom/payments/PaymentRequestService.cpp
@@ +131,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +PaymentRequestService::Cleanup()

Remove this. Is not used.

::: dom/payments/ipc/PaymentRequestChild.cpp
@@ +20,5 @@
> +{
> +  if (!mActorAlive) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  SendRequestPayment(aAction);

in order to avoid a wrong use of SendRequestPaymenet. you could do this in the PaymenetRequestChild.h:

private:
  ... SendRequestPayment(const IPCPaymentActionRequest& aAction) {
    return PPaymenetRequestchild::SendRequestPaymenet(aAction);
  }

@@ +28,5 @@
>  void
>  PaymentRequestChild::ActorDestroy(ActorDestroyReason aWhy)
>  {
>    mActorAlive = false;
> +  RefPtr<PaymentRequestChild> actor = this;

why this?

@@ +31,5 @@
>    mActorAlive = false;
> +  RefPtr<PaymentRequestChild> actor = this;
> +  RefPtr<PaymentRequestManager> manager = PaymentRequestManager::GetSingleton();
> +  MOZ_ASSERT(manager);
> +  manager->ReleasePaymentChild(actor);

can you just do: ReleasePaymenetChild(this) ?
Attachment #8870447 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini [:baku] from comment #34)

> ::: dom/interfaces/payments/nsIPaymentRequestService.idl
> @@ +18,5 @@
> >  interface nsIPaymentRequestService : nsISupports
> >  {
> >    nsIPaymentRequest getPaymentRequestById(in AString requestId);
> >    nsISimpleEnumerator enumerate();
> > +  void cleanup();
> 
> it seems you are not using this method. Remove it.

Cleanup is only for testing. It is used for clear the created payment request to avoid influencing the test environment before running next tests. I will add comments for it.
Attached patch bug1345361.patch (obsolete) — Splinter Review
Attachment #8870350 - Attachment is obsolete: true
Attachment #8870352 - Attachment is obsolete: true
Attachment #8870447 - Attachment is obsolete: true
Attachment #8871365 - Attachment is obsolete: true
Attachment #8871366 - Attachment is obsolete: true
Keywords: checkin-needed
[Feature/regressing bug #]: Bug 1345361
[User impact if declined]: No impact. 
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=36f513bb7c3e76359147f62db2c024a74d4b0b88
[Risks and why]: No risk, need preference to turn on the feature. 
[String/UUID change made/needed]: No
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/437d6df1859c
PaymentRequest constructor implementation. r=baku
Keywords: checkin-needed
Also had a rooting hazard:

Function '_ZN7mozilla3dom12_GLOBAL__N_121SerializeFromJSObjectEP8JSObjectR9nsAString$PaymentRequestManager.cpp:uint32 mozilla::dom::{anonymous}::SerializeFromJSObject(JSObject*, nsAString*)' has unrooted 'aObject' of type 'JSObject*' live across GC call '_ZN8nsCOMPtrI7nsIJSONEC1ERK15nsCOMPtr_helper$nsCOMPtr<T>::nsCOMPtr(const nsCOMPtr_helper&) [with T = nsIJSON]' at dom/payments/PaymentRequestManager.cpp:26
    PaymentRequestManager.cpp:26: Call(1,2, __temp_2 := do_CreateInstance("@mozilla.org/dom/json;1",0))
    PaymentRequestManager.cpp:26: Call(2,3, __temp_1*.nsCOMPtr(0,__temp_2.field:0)) [[GC call]]
    PaymentRequestManager.cpp:26: Assign(3,4, serializer := __temp_1*)
    PaymentRequestManager.cpp:26: Call(4,5, __temp_1.~nsCOMPtr())
    PaymentRequestManager.cpp:27: Call(5,6, __temp_4 := serializer.operator 1())
    PaymentRequestManager.cpp:27: Call(6,7, __temp_3 := NS_warn_if_impl(!__temp_4*,"!serializer","/home/worker/checkouts/gecko/dom/payments/PaymentRequestManager.cpp",27))
    PaymentRequestManager.cpp:27: Assume(7,10, __temp_3*, false)
    PaymentRequestManager.cpp:30: Call(10,11, __temp_5 := ObjectValue(aObject*))
GC Function: _ZN8nsCOMPtrI7nsIJSONEC1ERK15nsCOMPtr_helper$nsCOMPtr<T>::nsCOMPtr(const nsCOMPtr_helper&) [with T = nsIJSON]
    nsCOMPtr<T>::nsCOMPtr(const nsCOMPtr_helper&) [with T = nsIJSON]
    void nsCOMPtr<T>::assign_from_helper(const nsCOMPtr_helper&, const nsIID&) [with T = nsIJSON; nsIID = nsID]
    nsresult nsQueryObject<T>::operator()(const nsIID&, void**) const [with T = TestNsRefPtr::Bar; nsIID = nsID]
    FieldCall: nsISupports.QueryInterface
Fix the root hazard and eslint issues.
Attachment #8871368 - Attachment is obsolete: true
[Feature/regressing bug #]: Bug 1345361
[User impact if declined]: No impact. 
[Describe test coverage new/current, TreeHerder]: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=26af411bfec34c4952cb1e97cef1fbf73da54393
https://treeherder.mozilla.org/#/jobs?repo=try&revision=255d754ebbac8540f2655492b7159b6dafd5696c (for root hazard checking)
[Risks and why]: No risk, need preference to turn on the feature. 
[String/UUID change made/needed]: No
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2370b6a03e8
PaymentRequest constructor implementation. r=baku
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d2370b6a03e8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Thanks for landing this first webpayment piece. :)
Depends on: 1369334
No longer depends on: 1369334
I've updated the browser support info on the ref doc:
https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/PaymentRequest

And added a note to the Fx55 rel notes:
https://developer.mozilla.org/en-US/Firefox/Releases/55#New_APIs

(fortunately, our friends from Google already documented most of it ;-) )
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: