Java implementation of PaymentRequest mojo service

Java implementation of PaymentRequest mojo service

BUG=602657
Committed: https://crrev.com/82735e00e98486689f2d6bca470b0ad18d05a985
Cr-Commit-Position: refs/heads/master@{#389638}

[please use gerrit instead, 2016-04-20 05:11:23]

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

[please use gerrit instead, 2016-04-20 05:11:23]

tedchoc@, this is the meat and potatoes of the PaymentRequest mojo service. Am I
moving in the right direction here?

[Ted C, 2016-04-22 03:59:38]

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java
(right):

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:45:
result.languageCode = languageScriptCode.length > 0 ? languageScriptCode[0] :
null;
will try to see where this get used, but just an FYI android and chrome do not
have perfectly matching usages of language codes:
https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/...

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java
(right):

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java:36:
for (CreditCard card : PersonalDataManager.getInstance().getCreditCards()) {
if getCreditCards doesn't return an array (instead a list), use the for (int i =
0; i < ...) instead of the foreach version.  This will cause an iterator to be
allocated and that should be avoided if possible.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java:39:
callback.onInstrumentsReady(this, instruments);
Maybe we should be posting this (new Handler().post<...>) since this is supposed
to be async.  should make the calling sites more predictable.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java:52:
return new HashSet<String>(Arrays.asList(new String[] {
this is an allocation of a string array, arraylist, and finally a set for the
same strings. 

better to make this more verbose and add them one at a time.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java:58:
return "org.chromium.chrome.browser.payments.AutofillPaymentApp";
do you want this to stay consistent over time (across file moves) or would you
be happy if this just reflected the current class name.

Just wondering if we should just use getClass().getName()?

ah, I see it is stored in prefs.  I wonder if we should use something other than
class name here (maybe make it a constant).  Chrome_Autofill_Payment_App or
something.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java
(right):

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:23:
private CreditCard mCard;
final?

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

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:33:
mCurrencySymbol = Currency.getInstance(currencyCode).getSymbol();
any chance there is no Currency associated with currencyCode?

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:34:
DecimalFormatSymbols symbols = new DecimalFormatSymbols();
this looks to use the current user's locale.  what if the currency isn't in
their locale?  If you are paying in USD, should we use the . as the decimal
separator if in France, or because we are in France should we use the,?

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:49:
int decimalSeparator = value.indexOf(".");
what happens if there are more than one .?

Should we just use regex w/ groups to match here instead?

Something like ^(-?)([0-9]+)(\.[0-9]+)?$

Then for inserting the decimal separator, it would be a reverse for loop -= 3
for the first group instead of adding one char at a time (using a substring of
the region).

"might" make this a bit more compliant with the spec.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentApp.java
(right):

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentApp.java:40:
void getInstruments(ArrayList<LineItem> items, InstrumentsCallback callback);
any reason we don't use vanilla List here?

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentApp.java:49:
HashSet<String> getSupportedMethodNames();
same thing...should use a vanilla Set if we can.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java
(right):

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:25:
public interface Addition {
In general, I like to keep interface names qualified beyond the containing class
name (Addition w/o the paymentappfactory isn't sufficient).  Maybe
SupplementalPaymentAppsProvider <ahh...good ol' java>

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java
(right):

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java:57:
public abstract void getDetails(String merchantName, String origin,
ArrayList<LineItem> items,
List vs ArrayList comment as before

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:47:
PaymentApp.InstrumentsCallback,
indent 8 from the start of the previous line

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:60:
private HashSet<String> mSupportedMethods;
Set/List generic versions plz

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:88:
if (mContext == null) return;
personal aside...we should really write a helper function to not make this so
gross for you.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:118:
mClient = client;
can this be called more than once?

should we assert that mClient == null first?

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:132:
mClient.onError();
for debugging, we might want to log each of these Log.d(TAG, ...)

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:161:
for (AutofillProfile profile : PersonalDataManager.getInstance().getProfiles())
{
same comment about iterator vs for int i = 0...

applies multiple places in this file

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:188:
if (mFavicon != null) mUI.setTitleBitmap(mFavicon);
should we set mFavicon = null after this?

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:191:
private HashSet<String> getValidatedSupportedMethods(String[] methods) {
s/HashSet/Set

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:194:
HashSet<String> result = new HashSet<String>();
when creating like this, you can do:

Set<String> result = new HashSet<>();

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:196:
if (TextUtils.isEmpty(method)) return null;
continue or return?

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:203:
private ArrayList<LineItem> getValidatedLineItems(PaymentDetails details) {
List

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:204:
if (null == details || details.items == null || details.items.length == 0)
return null;
details == null is the more consistent thing to do

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:219:
for (PaymentItem item : details.items) {
i would do i < length - 1 and skip the == total check below

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:225:
result.add(new LineItem(item.label, currencyCode,
yikes...we support multiple currencies in a single transaction...yikes :-P

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:246:
return null;
return or continue?

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:268:
if (!supportedMethods.contains(name)) return null;
same...return or continue?

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:282:
provideDefaultPaymentInformation();
in general I would avoid this indirection for now.  I would also avoid stashing
the callback.  In general, I would rather have it be passed along to the
necessary callers than held here for the return.

I think we can inline the provideDefault bits below, but we should post it to
keep the async-ness of the api correct

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:294:
callback.onResult(mLineItems);
same async-ness for all the callbacks

[please use gerrit instead, 2016-04-25 19:22:32]

tedchoc@, ptal patch 5.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java
(right):

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:45:
result.languageCode = languageScriptCode.length > 0 ? languageScriptCode[0] :
null;
On 2016/04/22 03:59:36, Ted C wrote:
> will try to see where this get used, but just an FYI android and chrome do not
> have perfectly matching usages of language codes:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/...

Thank you for the heads up. I think this language code both comes from Chrome
and is used in Chrome. Android language codes should not be mixed in here.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java
(right):

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java:36:
for (CreditCard card : PersonalDataManager.getInstance().getCreditCards()) {
On 2016/04/22 03:59:36, Ted C wrote:
> if getCreditCards doesn't return an array (instead a list), use the for (int i
=
> 0; i < ...) instead of the foreach version.  This will cause an iterator to be
> allocated and that should be avoided if possible.

Done.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java:39:
callback.onInstrumentsReady(this, instruments);
On 2016/04/22 03:59:36, Ted C wrote:
> Maybe we should be posting this (new Handler().post<...>) since this is
supposed
> to be async.  should make the calling sites more predictable.

Made AutofillPaymentApp implement the Runnable interface so that we can post via
"new Handler().post(this);". This should be more efficient, right?

PRO: Eliminate the need for the anonymous Runnable instance. You did not want to
allocate an iterator on line 36, so I assume that allocating new objects is
expensive.

CON: Need to store "callback" and "instruments" as member variables.

getInstruments() is called only once, so there's no worry about overwriting the
member variables.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java:52:
return new HashSet<String>(Arrays.asList(new String[] {
On 2016/04/22 03:59:36, Ted C wrote:
> this is an allocation of a string array, arraylist, and finally a set for the
> same strings. 
> 
> better to make this more verbose and add them one at a time.

Done.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java:58:
return "org.chromium.chrome.browser.payments.AutofillPaymentApp";
On 2016/04/22 03:59:36, Ted C wrote:
> I wonder if we should use something other than
> class name here (maybe make it a constant).  Chrome_Autofill_Payment_App or
> something.

Using "Chrome_Autofill_Payment_App".

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java
(right):

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:23:
private CreditCard mCard;
On 2016/04/22 03:59:36, Ted C wrote:
> final?

Done.

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

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:33:
mCurrencySymbol = Currency.getInstance(currencyCode).getSymbol();
On 2016/04/22 03:59:37, Ted C wrote:
> any chance there is no Currency associated with currencyCode?

Yes, the spec does not limit to only valid ISO 4217 currencies. I've changed the
code to handle this case as well.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:34:
DecimalFormatSymbols symbols = new DecimalFormatSymbols();
On 2016/04/22 03:59:36, Ted C wrote:
> this looks to use the current user's locale.  what if the currency isn't in
> their locale?  If you are paying in USD, should we use the . as the decimal
> separator if in France, or because we are in France should we use the,?

Added the following comment:

// Use the current locale's symbols. For example, use ","
// for decimal separator in France, even if paying in "USD".

This is because we don't want to confuse the user by switching from "." to ","
and vice versa based on the merchant's specified currency. A user would be more
likely to buy things from abroad than to travel abroad, because shipping small
goods costs less than travel. This feature's behavior should reduce user
confusion.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:49:
int decimalSeparator = value.indexOf(".");
On 2016/04/22 03:59:36, Ted C wrote:
> what happens if there are more than one .?

More than one "." would be rejected in validation stage in
PaymentRequestImpl.show().

> Should we just use regex w/ groups to match here instead?
> 
> Something like ^(-?)([0-9]+)(\.[0-9]+)?$
> 
> Then for inserting the decimal separator, it would be a reverse for loop -= 3
> for the first group instead of adding one char at a time (using a substring of
> the region).

Using regex with groups. Would be nice to use group names when we start using
jdk 1.8.

> "might" make this a bit more compliant with the spec.

Done. (s/can be too large/might be too large/g)

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentApp.java
(right):

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentApp.java:40:
void getInstruments(ArrayList<LineItem> items, InstrumentsCallback callback);
On 2016/04/22 03:59:37, Ted C wrote:
> any reason we don't use vanilla List here?

Was ensuring O(1) random access, until you assured me that our code already uses
ArrayList everywhere as a rule of thumb.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentApp.java:49:
HashSet<String> getSupportedMethodNames();
On 2016/04/22 03:59:37, Ted C wrote:
> same thing...should use a vanilla Set if we can.

Done.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java
(right):

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:25:
public interface Addition {
On 2016/04/22 03:59:37, Ted C wrote:
> In general, I like to keep interface names qualified beyond the containing
class
> name (Addition w/o the paymentappfactory isn't sufficient).  Maybe
> SupplementalPaymentAppsProvider <ahh...good ol' java>

Done.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java
(right):

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java:57:
public abstract void getDetails(String merchantName, String origin,
ArrayList<LineItem> items,
On 2016/04/22 03:59:37, Ted C wrote:
> List vs ArrayList comment as before

Done.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:47:
PaymentApp.InstrumentsCallback,
On 2016/04/22 03:59:37, Ted C wrote:
> indent 8 from the start of the previous line

Done.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:60:
private HashSet<String> mSupportedMethods;
On 2016/04/22 03:59:38, Ted C wrote:
> Set/List generic versions plz

Done.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:88:
if (mContext == null) return;
On 2016/04/22 03:59:38, Ted C wrote:
> personal aside...we should really write a helper function to not make this so
> gross for you.

Acknowledged.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:118:
mClient = client;
On 2016/04/22 03:59:37, Ted C wrote:
> can this be called more than once?
> 
> should we assert that mClient == null first?

Should not be called more than once. Added the assert.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:132:
mClient.onError();
On 2016/04/22 03:59:37, Ted C wrote:
> for debugging, we might want to log each of these Log.d(TAG, ...)

Done.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:161:
for (AutofillProfile profile : PersonalDataManager.getInstance().getProfiles())
{
On 2016/04/22 03:59:37, Ted C wrote:
> same comment about iterator vs for int i = 0...
> 
> applies multiple places in this file

Done.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:188:
if (mFavicon != null) mUI.setTitleBitmap(mFavicon);
On 2016/04/22 03:59:37, Ted C wrote:
> should we set mFavicon = null after this?

Done.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:191:
private HashSet<String> getValidatedSupportedMethods(String[] methods) {
On 2016/04/22 03:59:37, Ted C wrote:
> s/HashSet/Set

Done.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:194:
HashSet<String> result = new HashSet<String>();
On 2016/04/22 03:59:37, Ted C wrote:
> when creating like this, you can do:
> 
> Set<String> result = new HashSet<>();

Done.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:196:
if (TextUtils.isEmpty(method)) return null;
On 2016/04/22 03:59:37, Ted C wrote:
> continue or return?

"return null" to indicate invalid method name.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:203:
private ArrayList<LineItem> getValidatedLineItems(PaymentDetails details) {
On 2016/04/22 03:59:37, Ted C wrote:
> List

Done.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:204:
if (null == details || details.items == null || details.items.length == 0)
return null;
On 2016/04/22 03:59:37, Ted C wrote:
> details == null is the more consistent thing to do

Done.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:219:
for (PaymentItem item : details.items) {
On 2016/04/22 03:59:37, Ted C wrote:
> i would do i < length - 1 and skip the == total check below

Done.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:225:
result.add(new LineItem(item.label, currencyCode,
On 2016/04/22 03:59:37, Ted C wrote:
> yikes...we support multiple currencies in a single transaction...yikes :-P

Although the spec does not explicitly prohibit this, I've double-checked with
zkoch@ (a spec editor), who says that indeed we should support only one currency
at a time. Fixed.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:246:
return null;
On 2016/04/22 03:59:37, Ted C wrote:
> return or continue?

"return null" to indicate invalid shipping options.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:268:
if (!supportedMethods.contains(name)) return null;
On 2016/04/22 03:59:37, Ted C wrote:
> same...return or continue?

"return null" to indicate invalid data. I'll add a comment :-)

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:282:
provideDefaultPaymentInformation();
On 2016/04/22 03:59:37, Ted C wrote:
> in general I would avoid this indirection for now.  I would also avoid
stashing
> the callback.  In general, I would rather have it be passed along to the
> necessary callers than held here for the return. I think we can inline the
provideDefault bits below.

I call provideDefault() also when I've finished loading the payment apps. For
example, Android Pay will have slight latency when I check whether it's
installed and has a suitable payment instrument. Since I need to invoke the
callback from two different locations, the callback needs to be a member
variable and invoking the callback should be in a method. Do you see a better
alternative?

> but we should post it to
> keep the async-ness of the api correct

Posting.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:294:
callback.onResult(mLineItems);
On 2016/04/22 03:59:37, Ted C wrote:
> same async-ness for all the callbacks

Done.

[please use gerrit instead, 2016-04-25 22:08:43]

Description was changed from

==========
Java implementation of PaymentRequest mojo service

BUG=587995
==========

to

==========
Java implementation of PaymentRequest mojo service

BUG=602657
==========

[Ted C, 2016-04-25 22:49:03]

lgtm w/ a few smaller comments

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java
(right):

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java:39:
callback.onInstrumentsReady(this, instruments);
On 2016/04/25 19:22:29, Rouslan wrote:
> On 2016/04/22 03:59:36, Ted C wrote:
> > Maybe we should be posting this (new Handler().post<...>) since this is
> supposed
> > to be async.  should make the calling sites more predictable.
> 
> Made AutofillPaymentApp implement the Runnable interface so that we can post
via
> "new Handler().post(this);". This should be more efficient, right?
> 
> PRO: Eliminate the need for the anonymous Runnable instance. You did not want
to
> allocate an iterator on line 36, so I assume that allocating new objects is
> expensive.
> 
> CON: Need to store "callback" and "instruments" as member variables.
> 
> getInstruments() is called only once, so there's no worry about overwriting
the
> member variables.

it is more efficient, but I would argue that readability is worse of here.

the iterator thing is again more of a convention.  It really only matters when
you are doing something on the draw path that could push android over the java
limit and force a GC.  I tell people to do it that way as a means of consistency
more than anything else.  If all places follow it then it's more likely that new
people will do that. etc, etc..  I would just define a runnable here.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:196:
if (TextUtils.isEmpty(method)) return null;
On 2016/04/25 19:22:31, Rouslan wrote:
> On 2016/04/22 03:59:37, Ted C wrote:
> > continue or return?
> 
> "return null" to indicate invalid method name.

I'm still somewhat surprised we don't just skip over the invalid ones instead of
aborting everything if we hit one, but I defer to you.

https://codereview.chromium.org/1904553003/diff/150001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java
(right):

https://codereview.chromium.org/1904553003/diff/150001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:37:
: "Country code should be in valid format";
+4 indent

https://codereview.chromium.org/1904553003/diff/150001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java
(right):

https://codereview.chromium.org/1904553003/diff/150001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:53:
mAmountValuePattern = Pattern.compile("^(-?)([0-9]+)(\\.([0-9]+))?$");
I would make these private static final constants (for the just the regex
pattern strings), then the grouping offsets can be right next to the pattern
itself which will be easier to read

[please use gerrit instead, 2016-04-25 23:19:24]

Sending to cq.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java
(right):

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java:39:
callback.onInstrumentsReady(this, instruments);
On 2016/04/25 22:49:02, Ted C wrote:
> On 2016/04/25 19:22:29, Rouslan wrote:
> > On 2016/04/22 03:59:36, Ted C wrote:
> > > Maybe we should be posting this (new Handler().post<...>) since this is
> > supposed
> > > to be async.  should make the calling sites more predictable.
> > 
> > Made AutofillPaymentApp implement the Runnable interface so that we can post
> via
> > "new Handler().post(this);". This should be more efficient, right?
> > 
> > PRO: Eliminate the need for the anonymous Runnable instance. You did not
want
> to
> > allocate an iterator on line 36, so I assume that allocating new objects is
> > expensive.
> > 
> > CON: Need to store "callback" and "instruments" as member variables.
> > 
> > getInstruments() is called only once, so there's no worry about overwriting
> the
> > member variables.
> 
> it is more efficient, but I would argue that readability is worse of here.
> 
> the iterator thing is again more of a convention.  It really only matters when
> you are doing something on the draw path that could push android over the java
> limit and force a GC.  I tell people to do it that way as a means of
consistency
> more than anything else.  If all places follow it then it's more likely that
new
> people will do that. etc, etc..  I would just define a runnable here.

Done.

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/1904553003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:196:
if (TextUtils.isEmpty(method)) return null;
On 2016/04/25 22:49:02, Ted C wrote:
> On 2016/04/25 19:22:31, Rouslan wrote:
> > On 2016/04/22 03:59:37, Ted C wrote:
> > > continue or return?
> > 
> > "return null" to indicate invalid method name.
> 
> I'm still somewhat surprised we don't just skip over the invalid ones instead
of
> aborting everything if we hit one, but I defer to you.

Acknowledged.

https://codereview.chromium.org/1904553003/diff/150001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java
(right):

https://codereview.chromium.org/1904553003/diff/150001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:37:
: "Country code should be in valid format";
On 2016/04/25 22:49:02, Ted C wrote:
> +4 indent

Done.

https://codereview.chromium.org/1904553003/diff/150001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java
(right):

https://codereview.chromium.org/1904553003/diff/150001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:53:
mAmountValuePattern = Pattern.compile("^(-?)([0-9]+)(\\.([0-9]+))?$");
On 2016/04/25 22:49:02, Ted C wrote:
> I would make these private static final constants (for the just the regex
> pattern strings), then the grouping offsets can be right next to the pattern
> itself which will be easier to read

Done.

[please use gerrit instead, 2016-04-25 23:20:02]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2016-04-25 23:20:02]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/1904553003/#ps170001
(title: "address comments")

[commit-bot: I haz the power, 2016-04-25 23:20:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1904553003/170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1904553003/170001

[commit-bot: I haz the power, 2016-04-26 00:37:01]

Description was changed from

==========
Java implementation of PaymentRequest mojo service

BUG=602657
==========

to

==========
Java implementation of PaymentRequest mojo service

BUG=602657
==========

[commit-bot: I haz the power, 2016-04-26 00:37:02]

Committed patchset #10 (id:170001)

[commit-bot: I haz the power, 2016-04-26 00:38:41]

Description was changed from

==========
Java implementation of PaymentRequest mojo service

BUG=602657
==========

to

==========
Java implementation of PaymentRequest mojo service

BUG=602657

Committed: https://crrev.com/82735e00e98486689f2d6bca470b0ad18d05a985
Cr-Commit-Position: refs/heads/master@{#389638}
==========

[commit-bot: I haz the power, 2016-04-26 00:38:42]

Patchset 10 (id:??) landed as
https://crrev.com/82735e00e98486689f2d6bca470b0ad18d05a985
Cr-Commit-Position: refs/heads/master@{#389638}