Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years ago by tommyt Modified:
4 years ago Reviewers:
Ted C, please use gerrit instead CC:
chromium-reviews, rouslan+payments_chromium.org, sebsg+paymentswatch_chromium.org, gogerald+paymentswatch_chromium.org, agrieve+watch_chromium.org, zino, zino1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPaymentApp: Implement the JNI bridge
Add two native methods to ServiceWorkerPaymentAppBridge, and implement
them in service_worker_payment_app_bridge.cc. The two methods are
GetAllAppManifests and InvokePaymentApp. At the moment, they are just
stubs, although GetAllAppManifest contains a bit of code, mainly
for illustration and to avoid compile errors for unused @CalledFromNative
methods in ServiceWorkerPaymentAppBridge.
BUG=661608
Committed: https://crrev.com/48c76126b7ee062e0bcd470d9b47e0359f1f8db3
Cr-Commit-Position: refs/heads/master@{#438269}
Patch Set 1 #Patch Set 2 : Move the c++ bridging files into their own folder #
Total comments: 5
Patch Set 3 : Rebase onto async-refactoring change and address review comments #Patch Set 4 : Remove a rogue comment from a conflict resolution #
Total comments: 2
Patch Set 5 : Rebase and address Ted's comment #
Created: 4 years ago
Depends on Patchset: Messages
Total messages: 27 (17 generated)
The CQ bit was checked by tommyt@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== PaymentApp: Implement the JNI bridge Add two native methods to ServiceWorkerPaymentAppBridge, and implement them in service_worker_payment_app_bridge.cc. The two methods are GetAllAppManifests and InvokePaymentApp. At the moment, they are just stubs, although GetAllAppManifest contains a bit of code, mainly to for illustration and toavoid compile errors for unused @CalledFromNative methods in ServiceWorkerPaymentAppBridge. BUG=661608 ========== to ========== PaymentApp: Implement the JNI bridge Add two native methods to ServiceWorkerPaymentAppBridge, and implement them in service_worker_payment_app_bridge.cc. The two methods are GetAllAppManifests and InvokePaymentApp. At the moment, they are just stubs, although GetAllAppManifest contains a bit of code, mainly for illustration and toavoid compile errors for unused @CalledFromNative methods in ServiceWorkerPaymentAppBridge. BUG=661608 ==========
Description was changed from ========== PaymentApp: Implement the JNI bridge Add two native methods to ServiceWorkerPaymentAppBridge, and implement them in service_worker_payment_app_bridge.cc. The two methods are GetAllAppManifests and InvokePaymentApp. At the moment, they are just stubs, although GetAllAppManifest contains a bit of code, mainly for illustration and toavoid compile errors for unused @CalledFromNative methods in ServiceWorkerPaymentAppBridge. BUG=661608 ========== to ========== PaymentApp: Implement the JNI bridge Add two native methods to ServiceWorkerPaymentAppBridge, and implement them in service_worker_payment_app_bridge.cc. The two methods are GetAllAppManifests and InvokePaymentApp. At the moment, they are just stubs, although GetAllAppManifest contains a bit of code, mainly for illustration and to avoid compile errors for unused @CalledFromNative methods in ServiceWorkerPaymentAppBridge. BUG=661608 ==========
tommyt@opera.com changed reviewers: + rouslan@chromium.org
PTAL
lgtm % comments https://codereview.chromium.org/2556753002/diff/20001/chrome/browser/android/... File chrome/browser/android/payments/service_worker_payment_app_bridge.cc (right): https://codereview.chromium.org/2556753002/diff/20001/chrome/browser/android/... chrome/browser/android/payments/service_worker_payment_app_bridge.cc:22: NOTIMPLEMENTED(); I would advise against using NOTIMPLEMENTED() in a function that already has some code. https://codereview.chromium.org/2556753002/diff/20001/chrome/browser/android/... chrome/browser/android/payments/service_worker_payment_app_bridge.cc:65: // Register native methods Static methods should have a comment like this to match the style guide: // static Also, there's no need to add "Register native methods" comment. It's fairly obvious what this method does.
tommyt@opera.com changed reviewers: + tedchoc@chromium.org
Added tedchoc as reviewer for the files in chrome/browser/android/. PTAL https://codereview.chromium.org/2556753002/diff/20001/chrome/browser/android/... File chrome/browser/android/payments/service_worker_payment_app_bridge.cc (right): https://codereview.chromium.org/2556753002/diff/20001/chrome/browser/android/... chrome/browser/android/payments/service_worker_payment_app_bridge.cc:22: NOTIMPLEMENTED(); On 2016/12/07 13:43:58, rouslan wrote: > I would advise against using NOTIMPLEMENTED() in a function that already has > some code. I've removed the NOTIMPLEMENTED() https://codereview.chromium.org/2556753002/diff/20001/chrome/browser/android/... chrome/browser/android/payments/service_worker_payment_app_bridge.cc:65: // Register native methods On 2016/12/07 13:43:58, rouslan wrote: > Static methods should have a comment like this to match the style guide: > > // static > > Also, there's no need to add "Register native methods" comment. It's fairly > obvious what this method does. I've removed the obvious comment, but this is not a static method, so I don't think "// static" is correct here.
https://codereview.chromium.org/2556753002/diff/20001/chrome/browser/android/... File chrome/browser/android/payments/service_worker_payment_app_bridge.cc (right): https://codereview.chromium.org/2556753002/diff/20001/chrome/browser/android/... chrome/browser/android/payments/service_worker_payment_app_bridge.cc:65: // Register native methods On 2016/12/08 13:19:13, tommyt wrote: > On 2016/12/07 13:43:58, rouslan wrote: > > Static methods should have a comment like this to match the style guide: > > > > // static > > > > Also, there's no need to add "Register native methods" comment. It's fairly > > obvious what this method does. > > I've removed the obvious comment, but this is not a static method, so I don't > think "// static" is correct here. You're right, my bad.
https://codereview.chromium.org/2556753002/diff/60001/chrome/browser/android/... File chrome/browser/android/payments/service_worker_payment_app_bridge.cc (right): https://codereview.chromium.org/2556753002/diff/60001/chrome/browser/android/... chrome/browser/android/payments/service_worker_payment_app_bridge.cc:37: env, manifest->icon ? *manifest->icon : nullptr)); looking at the code for jni_string.cc, nullptr looks like it will crash can you investigate to see if I'm crazy here?
The CQ bit was checked by tommyt@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2556753002/diff/60001/chrome/browser/android/... File chrome/browser/android/payments/service_worker_payment_app_bridge.cc (right): https://codereview.chromium.org/2556753002/diff/60001/chrome/browser/android/... chrome/browser/android/payments/service_worker_payment_app_bridge.cc:37: env, manifest->icon ? *manifest->icon : nullptr)); On 2016/12/09 18:41:28, Ted C (OOO 12.8.16) wrote: > looking at the code for jni_string.cc, nullptr looks like it will crash > > can you investigate to see if I'm crazy here? No, you're absolutely right. The ConvertUTF8ToJavaString function does not like nullptr's very much, and will crash. The generated JNI functions, however, deal well with nullptr (passing them as a Java null), so I have moved the check on the manifest->icon optional member (and the option->icon member below) to the outside of the ConvertUTFToJavaString call.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm
The CQ bit was checked by tommyt@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org Link to the patchset: https://codereview.chromium.org/2556753002/#ps80001 (title: "Rebase and address Ted's comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481654023368900,
"parent_rev": "a1a3b3c0ad01952abf398cf9b7c83bcdededf055", "commit_rev":
"36ff153e12e3cb94cd5d1b8f09b96e36e9e97e3d"}
Message was sent while issue was closed.
Description was changed from ========== PaymentApp: Implement the JNI bridge Add two native methods to ServiceWorkerPaymentAppBridge, and implement them in service_worker_payment_app_bridge.cc. The two methods are GetAllAppManifests and InvokePaymentApp. At the moment, they are just stubs, although GetAllAppManifest contains a bit of code, mainly for illustration and to avoid compile errors for unused @CalledFromNative methods in ServiceWorkerPaymentAppBridge. BUG=661608 ========== to ========== PaymentApp: Implement the JNI bridge Add two native methods to ServiceWorkerPaymentAppBridge, and implement them in service_worker_payment_app_bridge.cc. The two methods are GetAllAppManifests and InvokePaymentApp. At the moment, they are just stubs, although GetAllAppManifest contains a bit of code, mainly for illustration and to avoid compile errors for unused @CalledFromNative methods in ServiceWorkerPaymentAppBridge. BUG=661608 Review-Url: https://codereview.chromium.org/2556753002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== PaymentApp: Implement the JNI bridge Add two native methods to ServiceWorkerPaymentAppBridge, and implement them in service_worker_payment_app_bridge.cc. The two methods are GetAllAppManifests and InvokePaymentApp. At the moment, they are just stubs, although GetAllAppManifest contains a bit of code, mainly for illustration and to avoid compile errors for unused @CalledFromNative methods in ServiceWorkerPaymentAppBridge. BUG=661608 Review-Url: https://codereview.chromium.org/2556753002 ========== to ========== PaymentApp: Implement the JNI bridge Add two native methods to ServiceWorkerPaymentAppBridge, and implement them in service_worker_payment_app_bridge.cc. The two methods are GetAllAppManifests and InvokePaymentApp. At the moment, they are just stubs, although GetAllAppManifest contains a bit of code, mainly for illustration and to avoid compile errors for unused @CalledFromNative methods in ServiceWorkerPaymentAppBridge. BUG=661608 Committed: https://crrev.com/48c76126b7ee062e0bcd470d9b47e0359f1f8db3 Cr-Commit-Position: refs/heads/master@{#438269} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/48c76126b7ee062e0bcd470d9b47e0359f1f8db3 Cr-Commit-Position: refs/heads/master@{#438269} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
