921112 - Expose MCC/MNC to payment providers in mozPay() on Android

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect)

Description

[Andy McKay]

This is very similar to bug 892068. We'd like to be able to get access to the MCC information so that we can find out what region the user is in.

Comment 1

[Kumar McMillan [:kumar]]

some basic API info is here https://wiki.mozilla.org/WebAPI/WebPaymentProvider#API

Comment 2

[Kumar McMillan [:kumar]]

FYI we're also asking for some of the mozPaymentProvider API in bug 924693

Comment 3

[Andy McKay]

Can we get some feedback on this, it would be an issue for payments on Android? Similar to bug 924693.

Comment 4

[Caitlin Galimidi [:c8o]]

Andy - you need feedback from Finkle on this? Who is your contact?

Comment 5

[Andy McKay]

Thanks Caitlin. Based on bug 840190, we are saying this isn't needed for v1 of Android payments. I'll still track it from bug 909896.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch

This uses JNI (and some string manipulation) to get the values out of the TelphonyManager. Values match what I exepct from https://en.wikipedia.org/wiki/Mobile_country_code

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8334892 [details] [diff] [review]
Patch

>diff --git a/mobile/android/base/GeckoNetworkManager.java b/mobile/android/base/GeckoNetworkManager.java

>+    private static int getNetworkOperator(String type) {

>+        String networkOperator = tel.getNetworkOperator();
>+        if ("mnc".equals(type)) {
>+            return Integer.parseInt(networkOperator.substring(3));
>+        } else if ("mcc".equals(type)) {
>+            return Integer.parseInt(networkOperator.substring(0, 3));

You will need to worry about this crash too: Bug 886921

>+    public static int getMCC() {
>+        return getNetworkOperator("mcc");
>+    }
>+
>+    public static int getMNC() {
>+        return getNetworkOperator("mnc");
>+    }

I'm not super thrilled about the strings. We could create static final ints for the types of data that can be pulled from getNetworkOperator:

public static final int NETOP_TYPE_MCC = 0
public static final int NETOP_TYPE_MNC = 1

>diff --git a/mobile/android/components/PaymentsUI.js b/mobile/android/components/PaymentsUI.js

>+          _getNetworkInfo: function(type) {
>+            let jni = new JNI();
>+            let cls = jni.findClass("org/mozilla/gecko/GeckoNetworkManager");
>+            let method = jni.getStaticMethodID(cls, "get" + type.toUpperCase(), "()I");

This seem a bit fragile. We should have the world's simple test to make sure the code does not break for some reason.

r- for dealing with the crash

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Now extra safe and with test for PaymentUIGlue. Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=d5ae8d7da5f2

They may have to be less strict about mcc/mnc values there (right now it checks that they're truthy, hoping that I'd catch crashes more than caring about the actual values).

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8336504 [details] [diff] [review]
Patch v2

>diff --git a/mobile/android/base/tests/roboextender/paymentsUI.html b/mobile/android/base/tests/roboextender/paymentsUI.html

>+function start() {
>+  if (!mozPaymentProvider)
>+    paymentFailed("no payment provider");
>+
>+  if (!mozPaymentProvider.mcc)
>+    paymentFailed("no mcc");
>+
>+  if (!mozPaymentProvider.mnc)
>+    paymentFailed("no mnc");
>+
>+  if (window.location.hash == "#pass")
>+    paymentSuccess("PAID CORRECTLY");
>+  else if (window.location.hash == "#fail")
>+    paymentFailed("FAILED CORRECTLY");
>+  else
>+    paymentFailed("invalid hash " + window.location.hash);
>+}

It would be nice to add some comments in here to let people know what the heck is happening. You are using paymentFailed and paymentSuccess, which we 

>\ No newline at end of file

Add newline

>diff --git a/mobile/android/base/tests/testMozPay.js b/mobile/android/base/tests/testMozPay.js

>+add_task(function test_get_set() {
>+  ppmm.addMessageListener("content-handler", this);
>+  let ui = Cc["@mozilla.org/payment/ui-glue;1"].getService(Ci.nsIPaymentsnsIPaymentUIGlue);

Can't be right

>+add_task(function test_default() {
>+  ppmm.addMessageListener("Payment:Success", this);
>+  ppmm.addMessageListener("Payment:Failed", this);
>+
>+  let ui = Cc["@mozilla.org/payment/ui-glue;1"].getService(Ci.nsIPaymentsnsIPaymentUIGlue);

Can't be right

>diff --git a/mobile/android/components/PaymentsUI.js b/mobile/android/components/PaymentsUI.js

> let paymentTabs = {};
>-
>+let cancelTabs = {};
>+function paymentCanceled(aRequestId) {
>+  return function() {
>+    paymentFailed(aRequestId)();
>+  }
>+}

UPDATE: I know see that cancelTabs holds a function, not a Tab. I think a new name is needed. Something to help us infer that this is a function:

cancelCallbacks ?

> function closePaymentTab(aId, aCallback) {
>   if (paymentTabs[aId]) {
>+    paymentTabs[aId].browser.removeEventListener("TabClose", cancelTabs[aId]);

Does cancelTabs hold references to Tabs? paymentTabs seems to. If cancelTabs holds references to Tabs, then this call to removeEventListener seems wrong.

UPDATE: I see that cancelTabs holds functions.

>     // Store a reference to the tab so that we can close it when the payment succeeds or fails.
>     paymentTabs[aRequestId] = tab;
>+    cancelTabs[aRequestId] = paymentCanceled(aRequestId);

If cancelTabs does not hold tabs perhaps a new name and comment are needed
UPDATE: I see cancelTabs holds functions. Please make a new comment or append to the existing comment to make this more clear.

>+    // fail the payment if the tab is closed on its own
>+    tab.browser.addEventListener("TabClose", cancelTabs[aRequestId]);

Same as the removeEventListener comment, but don't you need/want a 3rd param too?
UPDATE: I see that cancelTabs holds functions, but still wondering about the 3rd param.


r- for some cleanup in the test

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch v2.1

That patch wasn't the one I wrote yesterday... I don't know what hg is doing to me. This should be right.

Comment 11

[Wesley Johnston (:wesj)]

try run:
https://tbpl.mozilla.org/?tree=Try&rev=7ce07fa40178

I'm nervous that the try machines will not have mcc/mnc and will just throw/fail there. We can alter the test to be ok with that if we want (it can just make sure things don't crash when using the JNI.jsm stuff?).

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8336968 [details] [diff] [review]
Patch v2.1

>diff --git a/mobile/android/base/GeckoNetworkManager.java b/mobile/android/base/GeckoNetworkManager.java

>     }
> 
>+
>+    private enum InfoType {

No need for the extra line

>+        MCC,
>+        MNC

>diff --git a/mobile/android/base/tests/roboextender/paymentsUI.html b/mobile/android/base/tests/roboextender/paymentsUI.html

>+  // mcc and mcn will throw if they be found

Huh? :)

>+  try {
>+    var mcc = mozPaymentProvider.mcc;
>+    var mnc = mozPaymentProvider.mnc;
>+  } catch(ex) {
>+    mozPaymentProvider.paymentFailed("Error finding network info: " + ex);

Maybe we should silence the failure. Like you mention, we don't know if this will work in tests. We also need to worry about what happens on Wifi. Should the missing values kill any payments process?

Is there a way we can mock this?

>diff --git a/mobile/android/components/PaymentsUI.js b/mobile/android/components/PaymentsUI.js

>+          _getNetworkInfo: function(type) {
>+            let jni = new JNI();
>+            let cls = jni.findClass("org/mozilla/gecko/GeckoNetworkManager");
>+            let method = jni.getStaticMethodID(cls, "get" + type.toUpperCase(), "()I");
>+            let val = jni.callStaticIntMethod(cls, method);
>+            jni.close();
>+
>+            if (val < 0)
>+              throw "Could not get " + type + " data";
>+            return val;

We throw on failure here. In the getters, we delete the existing getter, then call _getNetworkInfo. If _getNetworkInfo throws, will we be left with a dead property? Should we hold the _getNetworkInfo value in a temporary and wait to delete the existing getter until we know _getNetworkInfo returns a real value?

>+    // fail the payment if the tab is closed on its own
>+    tab.browser.addEventListener("TabClose", cancelTabCallbacks[aRequestId]);

nit: // Fail

Am I being foolish in asking about the 3rd param to addEventListener?

Let's look at this one more time, r-

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Patch 3

Try actually didn't seem too unhappy, but we looked and gonk defaults to null here:

http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RILContentHelper.js#153

so I decided to just do that. The test just accesses them to make sure the world doesn't die. There was also some talk about multi-sim cards. I'm still digging into how android handles that.

Comment 14

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/3fdcc6922520

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3fdcc6922520

Comment 16

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/e512dcbdb0d9

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e512dcbdb0d9