872987 - [WebPayment] Expose ICC info to the payment provider

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

See bug 871591

Comment 1

[Jason Smith [:jsmith]]

How critical is this for 1.01? Can we workaround this for 1.01?

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

If I am not wrong, during the discussion about this functionality with product the decision was to nominate this for leo+.

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

This patch introduces:

* The injection of a new 'mozPaymentProvider' object within the payment flow that includes the old 'paymentSuccess' and 'paymentFailed' functions. So now the payment provider will need to call 'mozPaymentProvider.paymentSuccess' (or paymentFailed) to end the payment flow. The reason for injecting the 'mozPaymentProvider' object instead of the old plain 'paymentSuccess' and 'paymentFailed' is to not pollute the global scope too much and avoid possible collisions with functions already defined within the payment provider scope. Kumar, do you think that you can easily change this in the server side?

* It also adds the new 'mozPaymentProvider.getIccId()' function that returns the ICC ID if available.

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo instead of CC, please) from comment #3) 
> * It also adds the new 'mozPaymentProvider.getIccId()' function that returns
> the ICC ID if available.

Actually, it returns an *array* of ICC IDs, so we won't need to change the API once we support multiple SIMs.

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 751735 [details] [diff] [review]
v1

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

Let's make sure everyone likes mozPaymentProvider.iccIds. Also, this should probably go in 1.0.1 if we don't want users to have to support both versions of the API.

::: b2g/chrome/content/payment.js
@@ +32,3 @@
>  const kClosePaymentFlowEvent = "close-payment-flow-dialog";
>  
> +let _requestId = null;

Nit: it will be undefined by default, and this seems good enough.

@@ +102,2 @@
>  
> +  getIccId: function getIccId() {

I would rather have this be a property:
iccIds get() {
 ...
}

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

Thanks Fabrice!

I'll wait for Kumar's feedback before pushing this patch.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #5)
> Comment on attachment 751735 [details] [diff] [review]
> v1
> 
> Review of attachment 751735 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Let's make sure everyone likes mozPaymentProvider.iccIds. Also, this should
> probably go in 1.0.1 if we don't want users to have to support both versions
> of the API.

Sadly, we're out of time to take fixes like these to 1.01 unless it's absolutely ship stopping. We're probably going to have a make a breaking API change here across 1.01 and 1.1 or we need to continue to support the legacy fallback mechanism along with this new approach.

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 752099 [details] [diff] [review]
v2

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

I still think we should ship that in 1.0.1 if possible. Who is using the "old" api currently?

Comment 9

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #8)
> Comment on attachment 752099 [details] [diff] [review]
> v2
> 
> Review of attachment 752099 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I still think we should ship that in 1.0.1 if possible. Who is using the
> "old" api currently?

Marketplace is at the moment.

Still feels risky though IMO to flip this around this late in the game for 1.01. I do understand this poses a compatibility issue, but given only marketplace is dependent on it, we would just need marketplace to have the ability to be able to work with the old API vs. new API.

Comment 10

[[:fabrice] Fabrice Desré]

I don't think the change is risky, but I agree with the "late in the game" argument. :(

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't think it's a big deal to ask the marketplace to support two versions of the API. This API will be slowly evolving for some time to come I suspect.

Comment 12

[Kumar McMillan [:kumar]]

Comment on attachment 752099 [details] [diff] [review]
v2

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

Thanks ferjm, r+ from me. The iccIds array should be all we need for Bango to remember/forget mobile accounts for payment.

However, I was under the impression that the process running the Trusted UI (system app?) does not currently have permissions for the mobileConnection API. If so, does this need a Gaia patch?

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

Thanks Kumar!
(In reply to Kumar McMillan [:kumar] from comment #12) 
> However, I was under the impression that the process running the Trusted UI
> (system app?) does not currently have permissions for the mobileConnection
> API. If so, does this need a Gaia patch?

There is no need for a Gaia patch. The required usage of the mobileConnection API is done in the chrome side, so there is no need to request permissions in this case.

I'll wait for you to have the server side ready before landing this patch.

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

I've filed Bug 877103 and proposed a fix for the server side.

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

Bug 877103 landed, so I'm pushing this patch to birch.

http://hg.mozilla.org/projects/birch/rev/40bea9a5a19e

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/40bea9a5a19e

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/5ac88abdf901

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/5ac88abdf901