Return cached query result for canMakeActivePayment.

Return cached query result for canMakeActivePayment.

The PaymentRequest.canMakeActivePayment() feature, which is currently
hidden behind a flag, throttles queries of supported methods to reduce
the risk of user fingerprinting. Since the upcoming (not yet
implemented) 'basic-card' method can be used with different subsets of
card networks ('visa', 'mastercard', etc.) and types ('credit', 'debit',
'prepaid'), this patch changes canMakeActivePayment() to return last
cached result instead of re-querying every time. This also happens to be
more inline with the proposed specification language:

"Multiple calls to canMakeActivePayment will result in cached response
from previous call."

Proposed PaymentRequest.canMakeActivePayment() specification:
https://github.com/w3c/browser-payment-api/pull/316/files

BUG=662931
Committed: https://crrev.com/639ffbbca80f848266bb78f5ce28f0eddc7649fd
Cr-Commit-Position: refs/heads/master@{#433689}

[please use gerrit instead, 2016-11-18 20:52:12]

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

[commit-bot: I haz the power, 2016-11-18 20:53:31]

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

[please use gerrit instead, 2016-11-18 20:57:50]

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

[please use gerrit instead, 2016-11-18 20:57:50]

Ian, ptal.

This is a part of http://crrev.com/2501593003, which Dan has asked me to split
up. I did not want to overload Dan with all shards of that patch, so I was
hoping that you could take a look at this shard. Thank you!

[commit-bot: I haz the power, 2016-11-18 21:41:06]

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

[commit-bot: I haz the power, 2016-11-18 21:41:06]

Dry run: This issue passed the CQ dry run.

[Ian Wen, 2016-11-18 23:32:41]

lgtm.

Things I mentioned are optional.

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

https://codereview.chromium.org/2507063009/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:108:
private static class ActivePaymentQuery {
Does it make sense to rename the class to "PaymentQueryCache" or
"LastPaymentQuery"? IIUC this is essentially a size-1 cache that indicates
"previous", not "current".

https://codereview.chromium.org/2507063009/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:128:
public boolean matchesPaymentMethods(Set<String> methods) {
Optional: One way to simplify the signatures of this class is to have a method
like:

public Boolean getPreviousRespond(Set<> methods),

where it returns null if no such result exists, or true/false if results are
cached.

This way you could replace hasBeenResponded(), getResult() and
matchsPaymentMethods() with one method.

Yet it's up to you. Returning a object boolean looks weird to many people.

[please use gerrit instead, 2016-11-21 21:46:15]

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

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

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

[please use gerrit instead, 2016-11-21 22:08:39]

Addressed comments, added more tests, simplified code. Sending to CQ now.

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

https://codereview.chromium.org/2507063009/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:108:
private static class ActivePaymentQuery {
On 2016/11/18 23:32:41, Ian Wen wrote:
> Does it make sense to rename the class to "PaymentQueryCache" or
> "LastPaymentQuery"? IIUC this is essentially a size-1 cache that indicates
> "previous", not "current".

This cache may be larger than size-1, because it's shared among all WebContents.
User might have several tabs open for shopping, each one calling into
canMakeActivePayment(), which would results in one of these ActivePaymentQuery
objects per tab. Therefore, let's avoid calling this "last" or "previous".

https://codereview.chromium.org/2507063009/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:128:
public boolean matchesPaymentMethods(Set<String> methods) {
On 2016/11/18 23:32:41, Ian Wen wrote:
> Optional: One way to simplify the signatures of this class is to have a method
> like:
> 
> public Boolean getPreviousRespond(Set<> methods),
> 
> where it returns null if no such result exists, or true/false if results are
> cached.
> 
> This way you could replace hasBeenResponded(), getResult() and
> matchsPaymentMethods() with one method.
> 
> Yet it's up to you. Returning a object boolean looks weird to many people.

Merged getResult() and hasBeenResponded() into "Boolean getPreviousRespond()".
Cannot merge "matchesPaymentMethods()" into there, because I need to distinguish
between "set does not match" and "set matches, but result has not been
calculated yet."

[please use gerrit instead, 2016-11-21 22:08:44]

The CQ bit was unchecked by rouslan@chromium.org

[please use gerrit instead, 2016-11-21 22:08:48]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2016-11-21 22:08:48]

The patchset sent to the CQ was uploaded after l-g-t-m from ianwen@chromium.org
Link to the patchset: https://codereview.chromium.org/2507063009/#ps20001
(title: "Merge methods, add more tests, simplify impl"")

[commit-bot: I haz the power, 2016-11-21 22:09:17]

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-11-21 23:24:50]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479766128505830,
"parent_rev": "64315856497e333688b1a089fb67b2f7c7f3ad0d", "commit_rev":
"62cf6ba1735a6b114a2960d61e3d251101b429be"}

[commit-bot: I haz the power, 2016-11-21 23:25:19]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-21 23:30:52]

Description was changed from

==========
Return cached query result for canMakeActivePayment.

The PaymentRequest.canMakeActivePayment() feature, which is currently
hidden behind a flag, throttles queries of supported methods to reduce
the risk of user fingerprinting. Since the upcoming (not yet
implemented) 'basic-card' method can be used with different subsets of
card networks ('visa', 'mastercard', etc.) and types ('credit', 'debit',
'prepaid'), this patch changes canMakeActivePayment() to return last
cached result instead of re-querying every time. This also happens to be
more inline with the proposed specification language:

"Multiple calls to canMakeActivePayment will result in cached response
from previous call."

Proposed PaymentRequest.canMakeActivePayment() specification:
https://github.com/w3c/browser-payment-api/pull/316/files

BUG=662931
==========

to

==========
Return cached query result for canMakeActivePayment.

The PaymentRequest.canMakeActivePayment() feature, which is currently
hidden behind a flag, throttles queries of supported methods to reduce
the risk of user fingerprinting. Since the upcoming (not yet
implemented) 'basic-card' method can be used with different subsets of
card networks ('visa', 'mastercard', etc.) and types ('credit', 'debit',
'prepaid'), this patch changes canMakeActivePayment() to return last
cached result instead of re-querying every time. This also happens to be
more inline with the proposed specification language:

"Multiple calls to canMakeActivePayment will result in cached response
from previous call."

Proposed PaymentRequest.canMakeActivePayment() specification:
https://github.com/w3c/browser-payment-api/pull/316/files

BUG=662931

Committed: https://crrev.com/639ffbbca80f848266bb78f5ce28f0eddc7649fd
Cr-Commit-Position: refs/heads/master@{#433689}
==========

[commit-bot: I haz the power, 2016-11-21 23:30:53]

Patchset 2 (id:??) landed as
https://crrev.com/639ffbbca80f848266bb78f5ce28f0eddc7649fd
Cr-Commit-Position: refs/heads/master@{#433689}