[Payment Request] Update the CurrencyStringFormatter to call the native impl.

[Payment Request] Update the CurrencyStringFormatter to call the native impl.

Uses components/payments/currency_formatter.h by way of a JNI bridge.

Had to get rid of the junit tests because the implementation now require the
native libraries to be loaded.

BUG=679797
TEST=CurrencyStringFormatterTest, manual

Review-Url: https://codereview.chromium.org/2629883004
Cr-Commit-Position: refs/heads/master@{#443797}
Committed: https://chromium.googlesource.com/chromium/src/+/94af7a6d022fc5368503a8c47480d012617376f3

[Mathieu, 2017-01-13 21:04:06]

Description was changed from

==========
[Payment Request] Update the CurrencyStringFormatter to call the native impl.

Uses components/payments/currency_formatter.h by way of a JNI bridge.

Had to get rid of the junit tests because the implementation now require the
native libraries to be loaded.

BUG=679797
TEST=CurrencyStringFormatterTest, manual
==========

to

==========
[Payment Request] Update the CurrencyStringFormatter to call the native impl.

Uses components/payments/currency_formatter.h by way of a JNI bridge.

Had to get rid of the junit tests because the implementation now require the
native libraries to be loaded.

BUG=679797
TEST=CurrencyStringFormatterTest, manual
==========

[Mathieu, 2017-01-13 21:04:06]

mathp@chromium.org changed reviewers:
+ rouslan@chromium.org

[Mathieu, 2017-01-13 21:04:15]

Hi Rouslan, please take a look!

[please use gerrit instead, 2017-01-13 21:16:25]

lgtm % comments

https://codereview.chromium.org/2629883004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java
(right):

https://codereview.chromium.org/2629883004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:42:
private final Pattern mAmountValuePattern;
No longer used in this file, so can be deleted.

https://codereview.chromium.org/2629883004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:55:
public CurrencyStringFormatter(String currencyCode, String currencySystem,
Locale userLocale) {
@Nullable String currencySystem.

https://codereview.chromium.org/2629883004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:59:
// Note that this technically leaks the native object.
Is there a remedy for the leak?

https://codereview.chromium.org/2629883004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:64:
mFormattedCurrencyCode = currencyCode.length() <= MAX_CURRENCY_CHARS
Can we similarly format the currency code in C++? IMHO this file should be a
thin bridge that always calls into C++ instead of doing anything itself.

https://codereview.chromium.org/2629883004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:75:
public boolean isValidAmountValue(String amountValue) {
No longer used in production code. Can be deleted.

https://codereview.chromium.org/2629883004/diff/1/components/payments/android...
File components/payments/android/currency_formatter_android.h (right):

https://codereview.chromium.org/2629883004/diff/1/components/payments/android...
components/payments/android/currency_formatter_android.h:14: using
base::android::JavaParamRef;
"using" should not be in header files, as a rule of thumb. I know this results
in bigger header files for us, but those who use this header will thank us for
not polluting the global namespace.

[Mathieu, 2017-01-13 22:27:00]

mathp@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[Mathieu, 2017-01-13 22:27:01]

Thanks!

+Dan for 1-line JNI registration.

https://codereview.chromium.org/2629883004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java
(right):

https://codereview.chromium.org/2629883004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:42:
private final Pattern mAmountValuePattern;
On 2017/01/13 21:16:25, rouslan wrote:
> No longer used in this file, so can be deleted.

Done.

https://codereview.chromium.org/2629883004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:55:
public CurrencyStringFormatter(String currencyCode, String currencySystem,
Locale userLocale) {
On 2017/01/13 21:16:24, rouslan wrote:
> @Nullable String currencySystem.

Done.

https://codereview.chromium.org/2629883004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:59:
// Note that this technically leaks the native object.
On 2017/01/13 21:16:25, rouslan wrote:
> Is there a remedy for the leak?

added destroy() (Calling nativeDestroy() which calls "delete this" on the C++
object -- a common pattern in this code). 

Since we don't know when we'll stop using the mFormatter in PaymentRequestImpl,
I've added a finalize() method which is called just before the GC.

https://codereview.chromium.org/2629883004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:64:
mFormattedCurrencyCode = currencyCode.length() <= MAX_CURRENCY_CHARS
On 2017/01/13 21:16:25, rouslan wrote:
> Can we similarly format the currency code in C++? IMHO this file should be a
> thin bridge that always calls into C++ instead of doing anything itself.

Sure thing, done

https://codereview.chromium.org/2629883004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:75:
public boolean isValidAmountValue(String amountValue) {
On 2017/01/13 21:16:25, rouslan wrote:
> No longer used in production code. Can be deleted.

Done.

https://codereview.chromium.org/2629883004/diff/1/components/payments/android...
File components/payments/android/currency_formatter_android.h (right):

https://codereview.chromium.org/2629883004/diff/1/components/payments/android...
components/payments/android/currency_formatter_android.h:14: using
base::android::JavaParamRef;
On 2017/01/13 21:16:25, rouslan wrote:
> "using" should not be in header files, as a rule of thumb. I know this results
> in bigger header files for us, but those who use this header will thank us for
> not polluting the global namespace.

Done.

[Mathieu, 2017-01-13 22:30:00]

The CQ bit was checked by mathp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 22:30:51]

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, 2017-01-14 00:04:09]

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

[commit-bot: I haz the power, 2017-01-14 00:04:10]

Dry run: This issue passed the CQ dry run.

[gone, 2017-01-14 00:55:41]

lgtm for the single line

https://codereview.chromium.org/2629883004/diff/20001/chrome/browser/android/...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/2629883004/diff/20001/chrome/browser/android/...
chrome/browser/android/chrome_jni_registrar.cc:275: {"CurrencyStringFormatter",
payments::CurrencyFormatterAndroid::Register},
Make the string match and say "CurrencyFormatterAndroid" instead?

[Mathieu, 2017-01-14 09:57:14]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2017-01-14 09:57:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2629883004/#ps40001
(title: "CurrencyStringFormatter -> CurrencyFormatter")

[commit-bot: I haz the power, 2017-01-14 09:57:23]

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

[Mathieu, 2017-01-14 09:57:23]

https://codereview.chromium.org/2629883004/diff/20001/chrome/browser/android/...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/2629883004/diff/20001/chrome/browser/android/...
chrome/browser/android/chrome_jni_registrar.cc:275: {"CurrencyStringFormatter",
payments::CurrencyFormatterAndroid::Register},
On 2017/01/14 00:55:41, dfalcantara (load balance plz) wrote:
> Make the string match and say "CurrencyFormatterAndroid" instead?

Ah, good suggestion! I changed it to CurrencyFormatter.java to be more
consistent..

[commit-bot: I haz the power, 2017-01-14 11:10:08]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484387834956080,
"parent_rev": "8c47492479edb7e7f9c1519e6366ca846f166dac", "commit_rev":
"94af7a6d022fc5368503a8c47480d012617376f3"}

[commit-bot: I haz the power, 2017-01-14 11:11:19]

Description was changed from

==========
[Payment Request] Update the CurrencyStringFormatter to call the native impl.

Uses components/payments/currency_formatter.h by way of a JNI bridge.

Had to get rid of the junit tests because the implementation now require the
native libraries to be loaded.

BUG=679797
TEST=CurrencyStringFormatterTest, manual
==========

to

==========
[Payment Request] Update the CurrencyStringFormatter to call the native impl.

Uses components/payments/currency_formatter.h by way of a JNI bridge.

Had to get rid of the junit tests because the implementation now require the
native libraries to be loaded.

BUG=679797
TEST=CurrencyStringFormatterTest, manual

Review-Url: https://codereview.chromium.org/2629883004
Cr-Commit-Position: refs/heads/master@{#443797}
Committed:
https://chromium.googlesource.com/chromium/src/+/94af7a6d022fc5368503a8c47480...
==========

[commit-bot: I haz the power, 2017-01-14 11:11:20]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/94af7a6d022fc5368503a8c47480...