PaymentApp: Implement the JNI bridge

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}

[tommyt, 2016-12-06 13:18:59]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 13:19:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 14:07:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 14:07:27]

Dry run: This issue passed the CQ dry run.

[tommyt, 2016-12-07 09:36:33]

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
==========

[tommyt, 2016-12-07 09:36:47]

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, 2016-12-07 09:38:03]

tommyt@opera.com changed reviewers:
+ rouslan@chromium.org

[tommyt, 2016-12-07 09:38:04]

PTAL

[please use gerrit instead, 2016-12-07 13:43:58]

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, 2016-12-08 13:19:13]

tommyt@opera.com changed reviewers:
+ tedchoc@chromium.org

[tommyt, 2016-12-08 13:19:14]

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.

[please use gerrit instead, 2016-12-08 13:47:01]

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.

[Ted C, 2016-12-09 18:41:28]

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?

[tommyt, 2016-12-12 10:05:47]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 10:06:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommyt, 2016-12-12 10:10:41]

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.

[commit-bot: I haz the power, 2016-12-12 11:40:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-12 11:40:22]

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...)

[Ted C, 2016-12-12 18:11:13]

lgtm

[tommyt, 2016-12-13 18:33:43]

The CQ bit was checked by tommyt@opera.com

[tommyt, 2016-12-13 18:33:43]

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")

[commit-bot: I haz the power, 2016-12-13 18:34:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 20:07:00]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481654023368900,
"parent_rev": "a1a3b3c0ad01952abf398cf9b7c83bcdededf055", "commit_rev":
"36ff153e12e3cb94cd5d1b8f09b96e36e9e97e3d"}

[commit-bot: I haz the power, 2016-12-13 20:07:34]

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
==========

[commit-bot: I haz the power, 2016-12-13 20:07:35]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-13 20:11:00]

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}
==========

[commit-bot: I haz the power, 2016-12-13 20:11:01]

Patchset 5 (id:??) landed as
https://crrev.com/48c76126b7ee062e0bcd470d9b47e0359f1f8db3
Cr-Commit-Position: refs/heads/master@{#438269}