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

RESOLVED FIXED in Firefox 29

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: andy+bugzilla, Assigned: wesj)

Tracking

unspecified
Firefox 29
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Summary: Expose MCC/MNC to payment providers in mozPay() → Expose MCC/MNC to payment providers in mozPay() on Android
Blocks: 909896
FYI we're also asking for some of the mozPaymentProvider API in bug 924693
Can we get some feedback on this, it would be an issue for payments on Android? Similar to bug 924693.
Andy - you need feedback from Finkle on this? Who is your contact?
Flags: needinfo?(amckay)
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.
Flags: needinfo?(amckay)
Assignee: nobody → wjohnston
Posted patch Patch (obsolete) — Splinter Review
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
Attachment #8334892 - Flags: review?(mark.finkle)
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
Attachment #8334892 - Flags: review?(mark.finkle) → review-
Posted patch Patch v2 (obsolete) — Splinter Review
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).
Attachment #8334892 - Attachment is obsolete: true
Attachment #8336504 - Flags: review?(mark.finkle)
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
Attachment #8336504 - Flags: review?(mark.finkle) → review-
Posted patch Patch v2.1 (obsolete) — Splinter Review
That patch wasn't the one I wrote yesterday... I don't know what hg is doing to me. This should be right.
Attachment #8336504 - Attachment is obsolete: true
Attachment #8336968 - Flags: review?(mark.finkle)
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 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-
Attachment #8336968 - Flags: review?(mark.finkle) → review-
Posted patch Patch 3Splinter Review
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.
Attachment #8336968 - Attachment is obsolete: true
Attachment #8337062 - Flags: review?(mark.finkle)
Attachment #8337062 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/3fdcc6922520
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.