1423124 - Add comments on nsIPaymentUIService.idl

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, enhancement)

Description

[Eden Chuang[:edenchuang]]

Need some comments on nsIPaymentUIService.idl to explain the interface.

Comment 1

[Eden Chuang[:edenchuang]]

Attachment: Bug 1423124 - Add comments on nsIPaymentUIService.idl. r?MattN

Add comments to explain the purpose of nsIPaymentUIService. And details each interface.

Comment 2

[Eden Chuang[:edenchuang]]

Attachment: Bug 1423124 - Add comments on nsIPaymentUIService.idl. r?MattN

Update the commit message.

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8934450 [details] [diff] [review]
Bug 1423124 - Add comments on nsIPaymentUIService.idl. r?MattN

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

I'm not sure if you realized but on IRC I was asking for comments on the `*_ACTION` consts: https://dxr.mozilla.org/mozilla-central/search?q=const+_ACTION+path%3Apayment+ext%3Aidl&redirect=false
This is helpful too though.

Thanks :)

::: dom/interfaces/payments/nsIPaymentUIService.idl
@@ +11,5 @@
> + * payment UI.
> + * In general, the implementation of this interface should be a service that
> + * manages all payment UI components and receives the requested payment actions
> + * from Gecko by the providing APIs. Then accroding to different APIs called, it
> + * should perform the corresponding behavior to user.

I think you can replace "by the providing APIs" and later by: "and perform the corresponding UI behavior" to make it a bit easier to read.

@@ +17,5 @@
>  [scriptable, uuid(01f8bd55-9017-438b-85ec-7c15d2b35cdc)]
>  interface nsIPaymentUIService : nsISupports
>  {
> +  /**
> +   *  Gecko calls this API to show the payment UI to users.

Remove "Gecko calls this API to" throughout as it's not necessary

@@ +22,5 @@
> +   *  The implementation gets the payment data through nsIPaymentRequestService
> +   *  by the passed in requestId, then shows the payment UI and start to interact
> +   *  with users.
> +   *  According to user's action, nsIPaymentRequestService's APIs respondPayment,
> +   *  changeShippingAddress , or changeShippingOtpion is possible to  called in

Nit: remove the space before the comma and the double space

@@ +24,5 @@
> +   *  with users.
> +   *  According to user's action, nsIPaymentRequestService's APIs respondPayment,
> +   *  changeShippingAddress , or changeShippingOtpion is possible to  called in
> +   *  the implementation.
> +   *  requestId: the request identify of the payment request. Notice that this

parameters should use @param syntax

@@ +25,5 @@
> +   *  According to user's action, nsIPaymentRequestService's APIs respondPayment,
> +   *  changeShippingAddress , or changeShippingOtpion is possible to  called in
> +   *  the implementation.
> +   *  requestId: the request identify of the payment request. Notice that this
> +   *             requestId is an internal request Id which generated by Gecko

s/which // here and for the other requestId params

@@ +31,5 @@
>    void showPayment(in AString requestId);
> +
> +  /**
> +   *  Gecko calls this API to abort the payment.
> +   *  The implementation must abort and close the showing payment UI. then call

remove the period?

@@ +43,5 @@
> +  /**
> +   *  Gecko calls this API to complete the payment.
> +   *  The implementation should close the showing payment UI, then call
> +   *  nsIPaymentRequestService respondPayment with nsIPaymentCompleteActionResponse
> +   *  to inform Gecko the UI status.

s/the/of the/

@@ +51,4 @@
>    void completePayment(in AString requestId);
> +
> +  /**
> +   *  Gecko calls this API to update the payment data to the payment UI.

s/to the/in the/

@@ +51,5 @@
>    void completePayment(in AString requestId);
> +
> +  /**
> +   *  Gecko calls this API to update the payment data to the payment UI.
> +   *  The implementation should get the updated payment data through

s/through/through the/

@@ +53,5 @@
> +  /**
> +   *  Gecko calls this API to update the payment data to the payment UI.
> +   *  The implementation should get the updated payment data through
> +   *  nsIPaymentRequestService again, then update the UI according to the updated
> +   *  data to user.

Replace: "then update the UI according to the updated…user." with "and update the UI"

Comment 4

[Eden Chuang[:edenchuang]]

Attachment: Bug 1423124 - Add comments on nsIPaymentUIService.idl. r=MattN

update patch according to comment 3

Comment 5

[Eden Chuang[:edenchuang]]

Attachment: Bug 1423124 - Add comments on nsIPaymentActionResponse.idl and nsIPaymentRequestService.idl. r?MattN

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8935674 [details] [diff] [review]
Bug 1423124 - Add comments on nsIPaymentActionResponse.idl and nsIPaymentRequestService.idl. r?MattN

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

Thanks

::: dom/interfaces/payments/nsIPaymentActionResponse.idl
@@ +29,1 @@
>    readonly attribute uint32_t type;

This uses the above consts? Can you mention that please.

@@ +172,1 @@
>     *  Accpet status of the payment.

Nit: typo: Accept

@@ +172,3 @@
>     *  Accpet status of the payment.
>     */
>    readonly attribute uint32_t acceptStatus;

What values does this get? Any consts that are relevant?

@@ +177,3 @@
>     *  The decided payment method name.
>     */
>    readonly attribute AString methodName;

Giving "basic-card" as an example may be useful for readers.

@@ +228,2 @@
>     */
>    readonly attribute uint32_t abortStatus;

Is this one of the ABORT_ consts? If so, can you refer to them here.

@@ +249,2 @@
>     */
>    readonly attribute uint32_t completeStatus;

Refer to nsIPaymentActionResponse.COMPLETE_* here (IIUC)

::: dom/interfaces/payments/nsIPaymentRequestService.idl
@@ +21,5 @@
>  interface nsIPaymentRequestService : nsISupports
>  {
> +  /**
> +   *  Get the nsIPaymentRequest through the given payment request identify.
> +   *  @param aRequestId - the payment request identify.

s/identify/identifier/g (2x)

@@ +41,5 @@
> +   */
> +  void respondPayment(in nsIPaymentActionResponse aResponse);
> +
> +  /**
> +   *  Inform the merchant the shipping addres is changed.

s/addres/address/
s/ is / has /

@@ +42,5 @@
> +  void respondPayment(in nsIPaymentActionResponse aResponse);
> +
> +  /**
> +   *  Inform the merchant the shipping addres is changed.
> +   *  @param requestId - the request identify of the payment request.

s/identify/identifier/g

(in other places too maybe)

@@ +48,5 @@
> +   */
> +  void changeShippingAddress(in AString requestId, in nsIPaymentAddress aAddress);
> +
> +  /**
> +   *  Inform the merchant the shipping addres is changed.

s/shipping addres is/shipping option has/

@@ +50,5 @@
> +
> +  /**
> +   *  Inform the merchant the shipping addres is changed.
> +   *  @param requestId - the request identify of the payment request.
> +   *  @param option - the shipping option string.

This should just be the ID string from the option, right?

@@ +87,2 @@
>     */
> +  void removeActionCallback(in nsIPaymentActionCallback aCallback);

I still don't understand when to use this? Is it only for testing?

Comment 7

[Eden Chuang[:edenchuang]]

> 
> @@ +87,2 @@
> >     */
> > +  void removeActionCallback(in nsIPaymentActionCallback aCallback);
> 
> I still don't understand when to use this? Is it only for testing?

No, this is used when destroying the IPC connection.
requestPayment and removeActionCallback are used by IPC parent. To not expose the PaymentRequestService to the IPC parent, I created these two interfaces on nsIPaymentRequestService. However, we probably should move them out of nsIPaymentRequestService, let nsIPaymentRequestService serve for UI code only.

Comment 8

[Eden Chuang[:edenchuang]]

Attachment: Bug 1423124 - Add comments on nsIPaymentActionResponse.idl and nsIPaymentRequestService.idl. r=MattN

Update the patch according to comment 6.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b033526ed3d8
add comments on nsIPaymentUIService.idl. r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/5076095da2ac
add comments on nsIPaymentActionResponse.idl and nsIPaymentRequestService.idl. r=MattN DONTBUILD

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/b033526ed3d8
https://hg.mozilla.org/mozilla-central/rev/5076095da2ac