Don't cache canMakePayment results.

Don't cache canMakePayment results.

If a merchant calls PaymentRequest.canMakePayment() for the same method
names and method data multiple times, then the method should return the
most up-to-date result every time. This enables the merchant to prompt
the use to setup their payment instrument. Note that different payment
method data will trigger query quota exceeded error instead of returning
cached result from a prior query with different payment method data.

BUG=695931
Review-Url: https://codereview.chromium.org/2711263003
Cr-Commit-Position: refs/heads/master@{#453376}
Committed: https://chromium.googlesource.com/chromium/src/+/7085ee86276c0f53f36e89ef5372493b621c9374

[please use gerrit instead, 2017-02-24 18:13:18]

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

[commit-bot: I haz the power, 2017-02-24 18:13:47]

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, 2017-02-24 18:30:36]

Patchset #1 (id:1) has been deleted

[please use gerrit instead, 2017-02-24 18:32:18]

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

[please use gerrit instead, 2017-02-24 18:32:18]

Ganggui, ptal.

[commit-bot: I haz the power, 2017-02-24 21:19:53]

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

[commit-bot: I haz the power, 2017-02-24 21:19:54]

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

[please use gerrit instead, 2017-02-24 21:39:45]

Actually, this is not ready for review yet. I need to prevent the issue of
querying for 'basic-card' multiple times, but with different
data.supportedNetworks.

[please use gerrit instead, 2017-02-24 21:39:56]

Description was changed from

==========
Don't cache canMakePayment results.

If a merchant calls PaymentRequest.canMakePayment() for the same method
names, multiple times, then the method should return the most up-to-date
result every time. This enables the merchant to prompt the use to setup
their payment instrument.

BUG=695931
==========

to

==========
[DO NOT REVIEW] Don't cache canMakePayment results.

If a merchant calls PaymentRequest.canMakePayment() for the same method
names, multiple times, then the method should return the most up-to-date
result every time. This enables the merchant to prompt the use to setup
their payment instrument.

BUG=695931
==========

[please use gerrit instead, 2017-02-27 19:30:47]

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

[commit-bot: I haz the power, 2017-02-27 19:31:14]

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, 2017-02-27 19:33:57]

Description was changed from

==========
[DO NOT REVIEW] Don't cache canMakePayment results.

If a merchant calls PaymentRequest.canMakePayment() for the same method
names, multiple times, then the method should return the most up-to-date
result every time. This enables the merchant to prompt the use to setup
their payment instrument.

BUG=695931
==========

to

==========
Don't cache canMakePayment results.

If a merchant calls PaymentRequest.canMakePayment() for the same method
names and method data multiple times, then the method should return the
most up-to-date result every time. This enables the merchant to prompt
the use to setup their payment instrument. Note that different payment
method data will trigger query quota exceeded error instead of returning cached
result from a prior query with different payment method data.

BUG=695931
==========

[please use gerrit instead, 2017-02-27 19:40:28]

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

[commit-bot: I haz the power, 2017-02-27 19:40:57]

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, 2017-02-27 19:41:21]

Description was changed from

==========
Don't cache canMakePayment results.

If a merchant calls PaymentRequest.canMakePayment() for the same method
names and method data multiple times, then the method should return the
most up-to-date result every time. This enables the merchant to prompt
the use to setup their payment instrument. Note that different payment
method data will trigger query quota exceeded error instead of returning cached
result from a prior query with different payment method data.

BUG=695931
==========

to

==========
Don't cache canMakePayment results.

If a merchant calls PaymentRequest.canMakePayment() for the same method
names and method data multiple times, then the method should return the
most up-to-date result every time. This enables the merchant to prompt
the use to setup their payment instrument. Note that different payment
method data will trigger query quota exceeded error instead of returning
cached result from a prior query with different payment method data.

BUG=695931
==========

[please use gerrit instead, 2017-02-27 19:41:39]

Ganggui, ptal patch 3. It's ready for review now.

[commit-bot: I haz the power, 2017-02-27 20:32:00]

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

[commit-bot: I haz the power, 2017-02-27 20:32:01]

Dry run: This issue passed the CQ dry run.

[gogerald1, 2017-02-27 22:02:19]

lgtm with comments

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

https://codereview.chromium.org/2711263003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:130:
* @param methods The payment methods that are being queried.
Map of the payment methods that are being queried and their data?

https://codereview.chromium.org/2711263003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:144:
* @param methods The payment methods that are being queried.
ditto

https://codereview.chromium.org/2711263003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1247:
if (!query.matchesPaymentMethods(Collections.unmodifiableMap(mMethodData))) {
Might only do this match in above else statement. Or return in "if(query ==
null)"

No need to change it in this CL though,

https://codereview.chromium.org/2711263003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1256:
if (isFinishedQueryingPaymentApps())
query.notifyObserversOfResponse(mCanMakePayment);
Move above two sentences to "if(query == null)" bracket?

[please use gerrit instead, 2017-02-27 22:12:41]

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

https://codereview.chromium.org/2711263003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:130:
* @param methods The payment methods that are being queried.
On 2017/02/27 22:02:19, gogerald1 wrote:
> Map of the payment methods that are being queried and their data?

Done.

https://codereview.chromium.org/2711263003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:144:
* @param methods The payment methods that are being queried.
On 2017/02/27 22:02:19, gogerald1 wrote:
> ditto

Done.

https://codereview.chromium.org/2711263003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1247:
if (!query.matchesPaymentMethods(Collections.unmodifiableMap(mMethodData))) {
On 2017/02/27 22:02:19, gogerald1 wrote:
> Might only do this match in above else statement. Or return in "if(query ==
> null)"
> 
> No need to change it in this CL though, 

Done.

https://codereview.chromium.org/2711263003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1256:
if (isFinishedQueryingPaymentApps())
query.notifyObserversOfResponse(mCanMakePayment);
On 2017/02/27 22:02:19, gogerald1 wrote:
> Move above two sentences to "if(query == null)" bracket?

That would not handle the case of multiple calls to
PaymentRequest.canMakePayment() on the  same object, so not going to change
this.

[please use gerrit instead, 2017-02-27 22:12:52]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2017-02-27 22:12:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
gogerald@chromium.org
Link to the patchset: https://codereview.chromium.org/2711263003/#ps80001
(title: "Addressed comments")

[commit-bot: I haz the power, 2017-02-27 22:13:44]

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

[gogerald1, 2017-02-27 22:40:53]

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

https://codereview.chromium.org/2711263003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1256:
if (isFinishedQueryingPaymentApps())
query.notifyObserversOfResponse(mCanMakePayment);
On 2017/02/27 22:12:41, rouslan wrote:
> On 2017/02/27 22:02:19, gogerald1 wrote:
> > Move above two sentences to "if(query == null)" bracket?
> 
> That would not handle the case of multiple calls to
> PaymentRequest.canMakePayment() on the  same object, so not going to change
> this.

Just want to make sure I understand it correct.

If the call is on the same object, then query.addObserver(this); has been set
before, so we no need to do it again, right?

And we also no need to check isFinishedQueryingPaymentApps() since
notifyObserversOfResponse will be called in onInstrumentsReady.

Assume all of these operations in PaymentRequestImpl are in UI thread.

[gogerald1, 2017-02-27 22:47:17]

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

https://codereview.chromium.org/2711263003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1256:
if (isFinishedQueryingPaymentApps())
query.notifyObserversOfResponse(mCanMakePayment);
On 2017/02/27 22:40:53, gogerald1 wrote:
> On 2017/02/27 22:12:41, rouslan wrote:
> > On 2017/02/27 22:02:19, gogerald1 wrote:
> > > Move above two sentences to "if(query == null)" bracket?
> > 
> > That would not handle the case of multiple calls to
> > PaymentRequest.canMakePayment() on the  same object, so not going to change
> > this.
> 
> Just want to make sure I understand it correct.
> 
> If the call is on the same object, then query.addObserver(this); has been set
> before, so we no need to do it again, right?
> 
> And we also no need to check isFinishedQueryingPaymentApps() since
> notifyObserversOfResponse will be called in onInstrumentsReady.
> 
> Assume all of these operations in PaymentRequestImpl are in UI thread.  
> 

Ah, got your idea, the problem might be the merchant website might call
canMakePayment again after get response,

[commit-bot: I haz the power, 2017-02-27 23:08:04]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488233572434690,
"parent_rev": "33e60b30fadf6d9b3b129a884e8b1b2eeec79988", "commit_rev":
"7085ee86276c0f53f36e89ef5372493b621c9374"}

[commit-bot: I haz the power, 2017-02-27 23:08:46]

Description was changed from

==========
Don't cache canMakePayment results.

If a merchant calls PaymentRequest.canMakePayment() for the same method
names and method data multiple times, then the method should return the
most up-to-date result every time. This enables the merchant to prompt
the use to setup their payment instrument. Note that different payment
method data will trigger query quota exceeded error instead of returning
cached result from a prior query with different payment method data.

BUG=695931
==========

to

==========
Don't cache canMakePayment results.

If a merchant calls PaymentRequest.canMakePayment() for the same method
names and method data multiple times, then the method should return the
most up-to-date result every time. This enables the merchant to prompt
the use to setup their payment instrument. Note that different payment
method data will trigger query quota exceeded error instead of returning
cached result from a prior query with different payment method data.

BUG=695931

Review-Url: https://codereview.chromium.org/2711263003
Cr-Commit-Position: refs/heads/master@{#453376}
Committed:
https://chromium.googlesource.com/chromium/src/+/7085ee86276c0f53f36e89ef5372...
==========

[commit-bot: I haz the power, 2017-02-27 23:08:47]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/7085ee86276c0f53f36e89ef5372...