Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2507223002:
Implement IsReadyToPay handling (Closed)
DescriptionImplement IsReadyToPay handling
Implement IsReadyToPay handling as defined in the specification:
https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJeE/edit?usp=sharing
In order to communicate with the IsReadyToPay Service AIDL is used.
Android payment apps are supposed to use the exact same AIDL
definitions, if not the payment app will not be accepted as
payment option.
BUG=620173
Review-Url: https://codereview.chromium.org/2507223002
Cr-Commit-Position: refs/heads/master@{#444140}
Committed: https://chromium.googlesource.com/chromium/src/+/b36b88ee87e3a4b5445cd6e865736a011541a714
Patch Set 1 #Patch Set 2 : Fix IsReadyToPay intent #Patch Set 3 : Use aidl #
Total comments: 1
Patch Set 4 : Rebase #Patch Set 5 : Fix sendInstrumentsReady abuse #Patch Set 6 : Make AIDL async and move to content #Patch Set 7 : Fix AIDLs #Patch Set 8 : Fix compile error #Patch Set 9 : Implement rouslans suggestion #Patch Set 10 : Rebase #Patch Set 11 : Rebase for the new year #Patch Set 12 : Rebase #
Total comments: 36
Patch Set 13 : Address most review comments #Patch Set 14 : Address remaining review comments #Patch Set 15 : More addressing of review comments #Patch Set 16 : Add separate exceptions + comment #
Total comments: 15
Patch Set 17 : Address some comments #Patch Set 18 : Address more review comments #Patch Set 19 : Fix oneway usage #Messages
Total messages: 38 (12 generated)
Looking pretty good! You're moving in the right direction here. https://codereview.chromium.org/2507223002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2507223002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1069: if (mPendingApps == null || mPendingInstruments == null) return false; Maybe check mWaitingForPaymentFactory here instead?
Description was changed from ========== IsReadyToPay WIP ========== to ========== IsReadyToPay WIP BUG=620173 ==========
Description was changed from ========== IsReadyToPay WIP BUG=620173 ========== to ========== Implement IsReadyToPay handling https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ... BUG=620173 ==========
Description was changed from ========== Implement IsReadyToPay handling https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ... BUG=620173 ========== to ========== Implement IsReadyToPay handling Implement IsReadyToPay handling as defined in the specification: https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ... BUG=620173 ==========
Description was changed from ========== Implement IsReadyToPay handling Implement IsReadyToPay handling as defined in the specification: https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ... BUG=620173 ========== to ========== Implement IsReadyToPay handling Implement IsReadyToPay handling as defined in the specification: https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ... In order to communicate with the IsReadyToPay Service AIDL is used. Android payment apps are supposed to use the exact same AIDL definitions, if not the payment app will not be accepted as payment option. BUG=620173 ==========
Description was changed from ========== Implement IsReadyToPay handling Implement IsReadyToPay handling as defined in the specification: https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ... In order to communicate with the IsReadyToPay Service AIDL is used. Android payment apps are supposed to use the exact same AIDL definitions, if not the payment app will not be accepted as payment option. BUG=620173 ========== to ========== Implement IsReadyToPay handling Implement IsReadyToPay handling as defined in the specification: https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ... In order to communicate with the IsReadyToPay Service AIDL is used. Android payment apps are supposed to use the exact same AIDL definitions, if not the payment app will not be accepted as payment option. BUG=620173 ==========
rob.buis@samsung.com changed reviewers: + rouslan@chromium.org
PTAL.
https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:24: No need for newline here. https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:46: private static final String READY_TO_PAY = "readyToPay"; Unused. https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:85: /** @param service The name of the "is ready to pay" service in the payment app. */ Update JavaDoc. https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:94: mHandler.post(new Runnable() { Use the handler only to make sendInstrumentsReady() asynchronous. Intents are already asynchronous, so there's no need for the handler in that case. https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:120: mInstrumentsCallback.onInstrumentsReady(AndroidPaymentApp.this, instruments); |this| is sufficient here, because you're not in an inner class. https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:148: } catch (SecurityException e) { Only bindService() can throw a SecurityException. Let's not check fromWebContents() and getWindowAndroid() for SecurityException as well. https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:154: private final ServiceConnection mServiceConnection = new ServiceConnection() { Variables should be above methods. https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:172: final IsReadyToPayServiceCallback.Stub callback = new IsReadyToPayServiceCallback.Stub() { The "final" keyword seems unnecessary. https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:184: } catch (Exception e) { We prefer explicit exception types. https://codereview.chromium.org/2507223002/diff/220001/content/public/android... File content/public/android/java/src/org/chromium/IsReadyToPayService.aidl (right): https://codereview.chromium.org/2507223002/diff/220001/content/public/android... content/public/android/java/src/org/chromium/IsReadyToPayService.aidl:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2507223002/diff/220001/content/public/android... content/public/android/java/src/org/chromium/IsReadyToPayService.aidl:7: oneway interface IsReadyToPayService { "public" and JavaDoc. https://codereview.chromium.org/2507223002/diff/220001/content/public/android... content/public/android/java/src/org/chromium/IsReadyToPayService.aidl:8: void isReadyToPay(IsReadyToPayServiceCallback callback); JavaDoc. https://codereview.chromium.org/2507223002/diff/220001/content/public/android... File content/public/android/java/src/org/chromium/IsReadyToPayServiceCallback.aidl (right): https://codereview.chromium.org/2507223002/diff/220001/content/public/android... content/public/android/java/src/org/chromium/IsReadyToPayServiceCallback.aidl:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2507223002/diff/220001/content/public/android... content/public/android/java/src/org/chromium/IsReadyToPayServiceCallback.aidl:7: interface IsReadyToPayServiceCallback { "public" and JavaDoc. Should this also have "oneway" keyword? https://codereview.chromium.org/2507223002/diff/220001/content/public/android... content/public/android/java/src/org/chromium/IsReadyToPayServiceCallback.aidl:8: void handleIsReadyToPay(boolean isReadyToPay); JavaDoc https://codereview.chromium.org/2507223002/diff/220001/content/public/android... File content/public/android/java/src/org/chromium/common.aidl (right): https://codereview.chromium.org/2507223002/diff/220001/content/public/android... content/public/android/java/src/org/chromium/common.aidl:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 Let's call this file payments_common.aidl
PTAL, I think only th Exception thing is still open. https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:24: On 2017/01/09 19:02:14, rouslan wrote: > No need for newline here. Done. https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:46: private static final String READY_TO_PAY = "readyToPay"; On 2017/01/09 19:02:14, rouslan wrote: > Unused. Done. https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:85: /** @param service The name of the "is ready to pay" service in the payment app. */ On 2017/01/09 19:02:14, rouslan wrote: > Update JavaDoc. Done. https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:94: mHandler.post(new Runnable() { On 2017/01/09 19:02:14, rouslan wrote: > Use the handler only to make sendInstrumentsReady() asynchronous. Intents are > already asynchronous, so there's no need for the handler in that case. Done. https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:120: mInstrumentsCallback.onInstrumentsReady(AndroidPaymentApp.this, instruments); On 2017/01/09 19:02:14, rouslan wrote: > |this| is sufficient here, because you're not in an inner class. Done. https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:148: } catch (SecurityException e) { On 2017/01/09 19:02:14, rouslan wrote: > Only bindService() can throw a SecurityException. Let's not check > fromWebContents() and getWindowAndroid() for SecurityException as well. Done. https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:154: private final ServiceConnection mServiceConnection = new ServiceConnection() { On 2017/01/09 19:02:14, rouslan wrote: > Variables should be above methods. Done. https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:172: final IsReadyToPayServiceCallback.Stub callback = new IsReadyToPayServiceCallback.Stub() { On 2017/01/09 19:02:14, rouslan wrote: > The "final" keyword seems unnecessary. Done. https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:184: } catch (Exception e) { On 2017/01/09 19:02:14, rouslan wrote: > We prefer explicit exception types. This is actually an important part of this patch. It is possible for the Service to misbehave (I caused an explicit null pointer crash in BobPay test app) and the browser will crash: 1-10 11:22:32.831 3792 3792 D AndroidRuntime: Shutting down VM^M 01-10 11:22:32.831 3792 3792 E AndroidRuntime: FATAL EXCEPTION: main^M 01-10 11:22:32.831 3792 3792 E AndroidRuntime: Process: org.chromium.chrome, PID: 3792^M 01-10 11:22:32.831 3792 3792 E AndroidRuntime: java.lang.NullPointerException: Attempt to invoke interface method 'void org.chromium.IsReadyToPayServiceCallback.handleIsReadyToPay(boolean)' on a null object reference^M 01-10 11:22:32.831 3792 3792 E AndroidRuntime: at android.os.Parcel.readException(Parcel.java:1626)^M 01-10 11:22:32.831 3792 3792 E AndroidRuntime: at android.os.Parcel.readException(Parcel.java:1573)^M 01-10 11:22:32.831 3792 3792 E AndroidRuntime: at org.chromium.IsReadyToPayService$Stub$Proxy.isReadyToPay(IsReadyToPayService.java:90)^M 01-10 11:22:32.831 3792 3792 E AndroidRuntime: at org.chromium.chrome.browser.payments.AndroidPaymentApp.sendIsReadyToPay(AndroidPaymentApp.java:182)^M 01-10 11:22:32.831 3792 3792 E AndroidRuntime: at org.chromium.chrome.browser.payments.AndroidPaymentApp.access$200(AndroidPaymentApp.java:39)^M 01-10 11:22:32.831 3792 3792 E AndroidRuntime: at org.chromium.chrome.browser.payments.AndroidPaymentApp$1.onServiceConnected(AndroidPaymentApp.java:64)^M 01-10 11:22:32.831 3792 3792 E AndroidRuntime: at android.app.LoadedApk$ServiceDispatcher.doConnected(LoadedApk.java:1335)^M 01-10 11:22:32.831 3792 3792 E AndroidRuntime: at android.app.LoadedApk$ServiceDispatcher$RunConnection.run(LoadedApk.java:1352)^M 01-10 11:22:32.831 3792 3792 E AndroidRuntime: at android.os.Handler.handleCallback(Handler.java:739)^M I think there are some things to consider. We could assume: 1) pay Services that go through stores will not make such mistakes 2) pay Services will catch any Exceptions they cause (but how to enforce?) I am quite sure there can be many more potential exceptions besides null pointer exceptions, so spelling them out may be impractical. Or should these Exceptions be caught at a higher level? https://codereview.chromium.org/2507223002/diff/220001/content/public/android... File content/public/android/java/src/org/chromium/IsReadyToPayService.aidl (right): https://codereview.chromium.org/2507223002/diff/220001/content/public/android... content/public/android/java/src/org/chromium/IsReadyToPayService.aidl:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/09 19:02:14, rouslan wrote: > 2017 Done. https://codereview.chromium.org/2507223002/diff/220001/content/public/android... content/public/android/java/src/org/chromium/IsReadyToPayService.aidl:7: oneway interface IsReadyToPayService { On 2017/01/09 19:02:14, rouslan wrote: > "public" and JavaDoc. Done. https://codereview.chromium.org/2507223002/diff/220001/content/public/android... content/public/android/java/src/org/chromium/IsReadyToPayService.aidl:8: void isReadyToPay(IsReadyToPayServiceCallback callback); On 2017/01/09 19:02:14, rouslan wrote: > JavaDoc. Done. https://codereview.chromium.org/2507223002/diff/220001/content/public/android... File content/public/android/java/src/org/chromium/IsReadyToPayServiceCallback.aidl (right): https://codereview.chromium.org/2507223002/diff/220001/content/public/android... content/public/android/java/src/org/chromium/IsReadyToPayServiceCallback.aidl:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/09 19:02:14, rouslan wrote: > 2017 Done. https://codereview.chromium.org/2507223002/diff/220001/content/public/android... content/public/android/java/src/org/chromium/IsReadyToPayServiceCallback.aidl:7: interface IsReadyToPayServiceCallback { On 2017/01/09 19:02:14, rouslan wrote: > "public" and JavaDoc. > > Should this also have "oneway" keyword? Done. https://codereview.chromium.org/2507223002/diff/220001/content/public/android... content/public/android/java/src/org/chromium/IsReadyToPayServiceCallback.aidl:8: void handleIsReadyToPay(boolean isReadyToPay); On 2017/01/09 19:02:14, rouslan wrote: > JavaDoc Done. https://codereview.chromium.org/2507223002/diff/220001/content/public/android... File content/public/android/java/src/org/chromium/common.aidl (right): https://codereview.chromium.org/2507223002/diff/220001/content/public/android... content/public/android/java/src/org/chromium/common.aidl:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/09 19:02:14, rouslan wrote: > 2017 > > Let's call this file payments_common.aidl Done.
https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:94: mHandler.post(new Runnable() { On 2017/01/10 16:40:00, rwlbuis wrote: > On 2017/01/09 19:02:14, rouslan wrote: > > Use the handler only to make sendInstrumentsReady() asynchronous. Intents are > > already asynchronous, so there's no need for the handler in that case. > > Done. You seem to have forgotten to update the code. https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:184: } catch (Exception e) { On 2017/01/10 16:40:00, rwlbuis wrote: > On 2017/01/09 19:02:14, rouslan wrote: > > We prefer explicit exception types. > > This is actually an important part of this patch. It is possible for the Service > to misbehave (I caused an explicit null pointer crash in BobPay test app) and > the browser will crash: Please make sure in your patch that the payment app is not able to crash the browser by checking every piece of data that you receive from it for validity before processing it. > I think there are some things to consider. We could assume: > 1) pay Services that go through stores will not make such mistakes > 2) pay Services will catch any Exceptions they cause (but how to enforce?) We cannot assume that the payment app will behave sanely. In fact, we should suspect malice. Therefore, validate all your inputs. > I am quite sure there can be many more potential exceptions besides null pointer > exceptions, so spelling them out may be impractical. Does Chrome rethrow exceptions from the payment app? > Or should these Exceptions > be caught at a higher level? Exceptions should be handled at the lowest possible level, as a rule of thumb.
https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:94: mHandler.post(new Runnable() { On 2017/01/10 16:49:22, rouslan wrote: > On 2017/01/10 16:40:00, rwlbuis wrote: > > On 2017/01/09 19:02:14, rouslan wrote: > > > Use the handler only to make sendInstrumentsReady() asynchronous. Intents > are > > > already asynchronous, so there's no need for the handler in that case. > > > > Done. > > You seem to have forgotten to update the code. Oops! Done. https://codereview.chromium.org/2507223002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:184: } catch (Exception e) { On 2017/01/10 16:49:22, rouslan wrote: > On 2017/01/10 16:40:00, rwlbuis wrote: > > On 2017/01/09 19:02:14, rouslan wrote: > > > We prefer explicit exception types. > > > > This is actually an important part of this patch. It is possible for the > Service > > to misbehave (I caused an explicit null pointer crash in BobPay test app) and > > the browser will crash: > > Please make sure in your patch that the payment app is not able to crash the > browser by checking every piece of data that you receive from it for validity > before processing it. For reference I found a description of the exception problems I saw while developing, here: https://blog.classycode.com/dealing-with-exceptions-in-aidl-9ba904c6d63#.qwqv... > > I think there are some things to consider. We could assume: > > 1) pay Services that go through stores will not make such mistakes > > 2) pay Services will catch any Exceptions they cause (but how to enforce?) > > We cannot assume that the payment app will behave sanely. In fact, we should > suspect malice. Therefore, validate all your inputs. I am not sure there are inputs in this case except the boolean? Let me know if I can check more. I agree that payment app could be malicious or do unexpected things like have incompatible AIDL files. > > I am quite sure there can be many more potential exceptions besides null > pointer > > exceptions, so spelling them out may be impractical. > > Does Chrome rethrow exceptions from the payment app? See the link above. *some* exceptions are passed on from he stub to the caller. Not sure what Chrome ends up doing but ultimately it will crash the client i.e. browser exactly as described in that link. In my last patch I added catching of RuntimeExceptions which would cover all 7 exceptions mentioned in the article. I verified that this catches my explicit null pointer crash in my BobPay test app.
This is really awful and makes me want to back away from using AIDL before it's too late. I believe that Activity intents do not necessarily show UI, so they can handle the use case of querying an app in the background. Let's switch to use window.showIntent() for IS_READY_TO_PAY in the patch. This will require some testing with BobPay.apk and updating the spec language, of which I can take care. WDYT?
On 2017/01/10 20:26:28, rouslan wrote: > This is really awful and makes me want to back away from using AIDL before it's > too late. I believe that Activity intents do not necessarily show UI, so they > can handle the use case of querying an app in the background. Let's switch to > use window.showIntent() for IS_READY_TO_PAY in the patch. This will require some > testing with BobPay.apk and updating the spec language, of which I can take > care. WDYT? A pity that after many man hours we run into Android undocumented behavior problems :( Anyway, I discussed it with colleagues, new approach seems reasonable especially since things should become more stable. Could you try updating the spec? After that maybe it is easier for us to determine needed effort to make those changes. Thanks!
The spec is updated: https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ... However, AliPay engineers pointed out that application lock apps can be configured to trigger when any activity starts up, including IS_READY_TO_PAY: https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ... Scouring the Google Play Store shows about 4 million users total that use application lock apps. That's less than 1% of Android users, AFAIK. Do you think this is a big problem?
I've reached the conclusion that we should continue using services. The spec has been updated. Please catch all exceptions with a comment as to why we must do that.
On 2017/01/13 15:15:10, rouslan wrote: > I've reached the conclusion that we should continue using services. The spec has > been updated. Please catch all exceptions with a comment as to why we must do > that. Great to hear, will do!
Patchset #16 (id:300001) has been deleted
On 2017/01/13 15:15:10, rouslan wrote: > I've reached the conclusion that we should continue using services. The spec has > been updated. Please catch all exceptions with a comment as to why we must do > that. Ok I adjusted the patch, PTAL!
Happy to see this moving along! Mostly minor comments. https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:61: private final ServiceConnection mServiceConnection = new ServiceConnection() { Please group all "private final" variables together. https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:68: sendIsReadyToPay(); These send* can get confusing. sendInstrumentsReady() talks to PaymentRequestImpl, but sendIsReadyToPay() talks to the payment app. Please specify the destination of the message in the method name for send* methods. For example, sendIsReadyToPayIntentToPaymentApp(). As for sendInstrumentsReady(), you may want to rename it into respondToGetInstrumentsQuery(). I think it should be clear that PaymentRequestImpl is querying for instruments. https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:111: public void getInstruments(final Map<String, PaymentMethodData> methodData, final String origin, No need for these three "final" keywords, because mHandler is not using these variables. https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:114: if (mIsReadyToPayIntent.getComponent() != null) { When you find an IS_READY_TO_PAY service, you call setClassName(), but here you check getComponent(). This asymmetry is jarring. Can you check getClassName() here as well? https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:143: private void isReadyToPay(String origin, PaymentMethodData data) { You can combine this function with getIntsruments(), because it's only called once: void getInstruments(methodData, origin, callback) { mInstrumentsCallback = callback; if (mIsReadyToPayIntent.getClassName() == null) { mHandler.post(new Runnable() { @Override public void run() { sendInstrumentsReady(AndroidPaymentApp.this); } }); return; } // Put contents of isReadyToPay(origin, data) here. } https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:190: | NetworkOnMainThreadException | UnsupportedOperationException e) { At this point I think it's OK to use Throwable. https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:191: /** The exceptions above are not caught in the remote Service but passed on to the "The exceptions above" --> "Many undocumented exceptions" https://codereview.chromium.org/2507223002/diff/320001/content/public/android... File content/public/android/java/src/org/chromium/IsReadyToPayServiceCallback.aidl (right): https://codereview.chromium.org/2507223002/diff/320001/content/public/android... content/public/android/java/src/org/chromium/IsReadyToPayServiceCallback.aidl:11: oneway interface IsReadyToPayServiceCallback { By the way, Android Studio is complaining about "oneway" on the interface definition. It thinks that "oneway" is allowed only on methods. Have you tested this to work? https://codereview.chromium.org/2507223002/diff/320001/content/public/android... content/public/android/java/src/org/chromium/IsReadyToPayServiceCallback.aidl:20: nit: no trailing newline https://codereview.chromium.org/2507223002/diff/320001/content/public/android... File content/public/android/java/src/org/chromium/payments_common.aidl (right): https://codereview.chromium.org/2507223002/diff/320001/content/public/android... content/public/android/java/src/org/chromium/payments_common.aidl:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017
Happy to see this moving along! Mostly minor comments. https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:61: private final ServiceConnection mServiceConnection = new ServiceConnection() { Please group all "private final" variables together. https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:68: sendIsReadyToPay(); These send* can get confusing. sendInstrumentsReady() talks to PaymentRequestImpl, but sendIsReadyToPay() talks to the payment app. Please specify the destination of the message in the method name for send* methods. For example, sendIsReadyToPayIntentToPaymentApp(). As for sendInstrumentsReady(), you may want to rename it into respondToGetInstrumentsQuery(). I think it should be clear that PaymentRequestImpl is querying for instruments. https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:111: public void getInstruments(final Map<String, PaymentMethodData> methodData, final String origin, No need for these three "final" keywords, because mHandler is not using these variables. https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:114: if (mIsReadyToPayIntent.getComponent() != null) { When you find an IS_READY_TO_PAY service, you call setClassName(), but here you check getComponent(). This asymmetry is jarring. Can you check getClassName() here as well? https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:143: private void isReadyToPay(String origin, PaymentMethodData data) { You can combine this function with getIntsruments(), because it's only called once: void getInstruments(methodData, origin, callback) { mInstrumentsCallback = callback; if (mIsReadyToPayIntent.getClassName() == null) { mHandler.post(new Runnable() { @Override public void run() { sendInstrumentsReady(AndroidPaymentApp.this); } }); return; } // Put contents of isReadyToPay(origin, data) here. } https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:190: | NetworkOnMainThreadException | UnsupportedOperationException e) { At this point I think it's OK to use Throwable. https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:191: /** The exceptions above are not caught in the remote Service but passed on to the "The exceptions above" --> "Many undocumented exceptions" https://codereview.chromium.org/2507223002/diff/320001/content/public/android... File content/public/android/java/src/org/chromium/IsReadyToPayServiceCallback.aidl (right): https://codereview.chromium.org/2507223002/diff/320001/content/public/android... content/public/android/java/src/org/chromium/IsReadyToPayServiceCallback.aidl:11: oneway interface IsReadyToPayServiceCallback { By the way, Android Studio is complaining about "oneway" on the interface definition. It thinks that "oneway" is allowed only on methods. Have you tested this to work? https://codereview.chromium.org/2507223002/diff/320001/content/public/android... content/public/android/java/src/org/chromium/IsReadyToPayServiceCallback.aidl:20: nit: no trailing newline https://codereview.chromium.org/2507223002/diff/320001/content/public/android... File content/public/android/java/src/org/chromium/payments_common.aidl (right): https://codereview.chromium.org/2507223002/diff/320001/content/public/android... content/public/android/java/src/org/chromium/payments_common.aidl:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017
https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:190: | NetworkOnMainThreadException | UnsupportedOperationException e) { On 2017/01/13 20:54:46, rouslan wrote: > At this point I think it's OK to use Throwable. Do you mean instead of last two or everyhing? Is this to make it future proof, i.e if more Exceptions gets thrown we'll cattch them here? https://codereview.chromium.org/2507223002/diff/320001/content/public/android... File content/public/android/java/src/org/chromium/IsReadyToPayServiceCallback.aidl (right): https://codereview.chromium.org/2507223002/diff/320001/content/public/android... content/public/android/java/src/org/chromium/IsReadyToPayServiceCallback.aidl:11: oneway interface IsReadyToPayServiceCallback { On 2017/01/13 20:54:46, rouslan wrote: > By the way, Android Studio is complaining about "oneway" on the interface > definition. It thinks that "oneway" is allowed only on methods. Have you tested > this to work? It is confusing, in an article I saw both ways are fine. But the actual documentation that I found is pretty vague. I get the same warning in Android Studio, so I'll verify first that putting oneway on the method works.
https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:190: | NetworkOnMainThreadException | UnsupportedOperationException e) { On 2017/01/13 22:12:25, rwlbuis wrote: > On 2017/01/13 20:54:46, rouslan wrote: > > At this point I think it's OK to use Throwable. > > Do you mean instead of last two or everyhing? Is this to make it future proof, > i.e if more Exceptions gets thrown we'll cattch them here? Instead of everything. It's to make the code more clear. If you have 8 undocumented exceptions to worry about, you might as well handle all of them.
I think I addressed all comments :) PTAL. https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2507223002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:190: | NetworkOnMainThreadException | UnsupportedOperationException e) { On 2017/01/16 17:41:18, rouslan wrote: > On 2017/01/13 22:12:25, rwlbuis wrote: > > On 2017/01/13 20:54:46, rouslan wrote: > > > At this point I think it's OK to use Throwable. > > > > Do you mean instead of last two or everyhing? Is this to make it future proof, > > i.e if more Exceptions gets thrown we'll cattch them here? > > Instead of everything. It's to make the code more clear. If you have 8 > undocumented exceptions to worry about, you might as well handle all of them. Done. https://codereview.chromium.org/2507223002/diff/320001/content/public/android... File content/public/android/java/src/org/chromium/IsReadyToPayServiceCallback.aidl (right): https://codereview.chromium.org/2507223002/diff/320001/content/public/android... content/public/android/java/src/org/chromium/IsReadyToPayServiceCallback.aidl:11: oneway interface IsReadyToPayServiceCallback { On 2017/01/13 22:12:25, rwlbuis wrote: > On 2017/01/13 20:54:46, rouslan wrote: > > By the way, Android Studio is complaining about "oneway" on the interface > > definition. It thinks that "oneway" is allowed only on methods. Have you > tested > > this to work? > > It is confusing, in an article I saw both ways are fine. But the actual > documentation that I found is pretty vague. > > I get the same warning in Android Studio, so I'll verify first that putting > oneway on the method works. Okay, basically adding it on the interface is as good as not adding it at all. At least, the generated java is the same. When putting the oneway on the method the generated code is not the same. I changed my local BobPay to also put the oneway on the methods and it works fine.
lgtm
The CQ bit was checked by rob.buis@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
You need a review from content/public/android/*aidl owner. That's palmer@chromium.org, I think.
rob.buis@samsung.com changed reviewers: + palmer@chromium.org
@palmer PTAL for review of aidl files.
CQ is committing da patch.
Bot data: {"patchset_id": 380001, "attempt_start_ts": 1484684140938180,
"parent_rev": "f07d2a7802da1348f732ec040306b4fc2a87d652", "commit_rev":
"b36b88ee87e3a4b5445cd6e865736a011541a714"}
Message was sent while issue was closed.
Description was changed from ========== Implement IsReadyToPay handling Implement IsReadyToPay handling as defined in the specification: https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ... In order to communicate with the IsReadyToPay Service AIDL is used. Android payment apps are supposed to use the exact same AIDL definitions, if not the payment app will not be accepted as payment option. BUG=620173 ========== to ========== Implement IsReadyToPay handling Implement IsReadyToPay handling as defined in the specification: https://docs.google.com/document/d/1izV4uC-tiRJG3JLooqY3YRLU22tYOsLTNq0P_InPJ... In order to communicate with the IsReadyToPay Service AIDL is used. Android payment apps are supposed to use the exact same AIDL definitions, if not the payment app will not be accepted as payment option. BUG=620173 Review-Url: https://codereview.chromium.org/2507223002 Cr-Commit-Position: refs/heads/master@{#444140} Committed: https://chromium.googlesource.com/chromium/src/+/b36b88ee87e3a4b5445cd6e86573... ==========
Message was sent while issue was closed.
Committed patchset #19 (id:380001) as https://chromium.googlesource.com/chromium/src/+/b36b88ee87e3a4b5445cd6e86573...
Message was sent while issue was closed.
Weird that presubmit did not block this. Either way, palmer@, would be great to have your review.
Message was sent while issue was closed.
> Weird that presubmit did not block this. Either way, palmer@, would be great to have your review. LGTM. Looking into the presubmit problem.
Message was sent while issue was closed.
wez@chromium.org changed reviewers: + aelias@chromium.org
Message was sent while issue was closed.
Hallo aelias@chromium.org! Due to a depot_tools patch which mistakenly removed the OWNERS check for non-source files (see crbug.com/684270), the following files landed in this CL and need a retrospective review from you: content/public/android/BUILD.gn Thanks, Wez
Message was sent while issue was closed.
lgtm |
