PaymentRequest: complete() method should take PaymentComplete enum value.

PaymentRequest: complete() method should take PaymentComplete enum value.

Related spec links
  [1] https://w3c.github.io/browser-payment-api/#idl-def-paymentcomplete
  [2] https://github.com/w3c/browser-payment-api/pull/161

BUG=617191
Committed: https://crrev.com/5368cdb012e5b9e64f18d9b5bdd914694a6aef94
Cr-Commit-Position: refs/heads/master@{#400410}

[zino, 2016-06-09 14:44:15]

Description was changed from

==========
PaymentRequest: complete() method should take PaymentComplete enum value.

Related spec links
  [1] https://w3c.github.io/browser-payment-api/#idl-def-paymentcomplete
  [2] https://github.com/w3c/browser-payment-api/pull/161

BUG=617191
==========

to

==========
PaymentRequest: complete() method should take PaymentComplete enum value.

TODO: android
TODO: add tests
TODO: review the code my self.
TODO: review request.

Related spec links
  [1] https://w3c.github.io/browser-payment-api/#idl-def-paymentcomplete
  [2] https://github.com/w3c/browser-payment-api/pull/161

BUG=617191
==========

[zino, 2016-06-10 18:47:50]

Description was changed from

==========
PaymentRequest: complete() method should take PaymentComplete enum value.

TODO: android
TODO: add tests
TODO: review the code my self.
TODO: review request.

Related spec links
  [1] https://w3c.github.io/browser-payment-api/#idl-def-paymentcomplete
  [2] https://github.com/w3c/browser-payment-api/pull/161

BUG=617191
==========

to

==========
PaymentRequest: complete() method should take PaymentComplete enum value.

Related spec links
  [1] https://w3c.github.io/browser-payment-api/#idl-def-paymentcomplete
  [2] https://github.com/w3c/browser-payment-api/pull/161

BUG=617191
==========

[zino, 2016-06-10 18:47:51]

jinho.bang@samsung.com changed reviewers:
+ rouslan@chromium.org

[zino, 2016-06-10 18:52:06]

Patchset #2 (id:20001) has been deleted

[zino, 2016-06-10 18:52:16]

Patchset #4 (id:80001) has been deleted

[zino, 2016-06-10 18:52:22]

Patchset #3 (id:60001) has been deleted

[zino, 2016-06-10 18:52:26]

Patchset #2 (id:40001) has been deleted

[zino, 2016-06-10 18:53:19]

Please take a look.

Thank you!

[please use gerrit instead, 2016-06-10 19:15:34]

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

[please use gerrit instead, 2016-06-10 19:15:35]

+haraken@: Does IDL generator provide no help in generating an enum? It seems
suboptimal to define enum in IDL, then get a string in C++, then convert string
into my own enum in C++.

https://codereview.chromium.org/2054823002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2054823002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:672:
PaymentComplete.validate(result);
What does this method do? May want to add a comment.

https://codereview.chromium.org/2054823002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:674:
mUI.close(result, new Runnable() {
Replace "result" with "result == PaymentComplete.SUCCESS".

UI should not know about mojo. This is my preference for layering the
components. There's no other technical reason for this.

https://codereview.chromium.org/2054823002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java
(right):

https://codereview.chromium.org/2054823002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:402:
public void close(int result, final Runnable callback) {
Continue using a boolean.

https://codereview.chromium.org/2054823002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentResultUIManager.java
(right):

https://codereview.chromium.org/2054823002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentResultUIManager.java:19:
import org.chromium.mojom.payments.PaymentComplete;
UI should not interact with mojo. Continue using a boolean.

https://codereview.chromium.org/2054823002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentCompleter.h (right):

https://codereview.chromium.org/2054823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentCompleter.h:14: enum
PaymentComplete {
Doesn't PaymentResponse.idl generate the enum for us to use?

https://codereview.chromium.org/2054823002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentResponse.h (right):

https://codereview.chromium.org/2054823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentResponse.h:36: ScriptPromise
complete(ScriptState*, const String& result = "");
I have not worked with enums in Blink before. Is this really how it works? Right
now you have IDL enum represented as strings in C++ that you convert into your
own enum values in C++: that seems suboptimal. I was hoping that IDL generator
could help us here.

[haraken, 2016-06-13 01:37:20]

On 2016/06/10 19:15:35, Rouslan (ツ) wrote:
> +haraken@: Does IDL generator provide no help in generating an enum? It seems
> suboptimal to define enum in IDL, then get a string in C++, then convert
string
> into my own enum in C++.

We can do that but I'm a bit afraid that it will increase implicit conversion
rules. Enum types in Web IDL must be strings. So it would be straightforward to
pass them as strings to C++ implementation (instead of passing
V8PaymentRequest::Success or something). Some customers just need the strings
and don't need C++'s enum types. What do you think?

[please use gerrit instead, 2016-06-13 15:26:34]

On 2016/06/13 01:37:20, haraken wrote:
> On 2016/06/10 19:15:35, Rouslan (ツ) wrote:
> > +haraken@: Does IDL generator provide no help in generating an enum? It
seems
> > suboptimal to define enum in IDL, then get a string in C++, then convert
> string
> > into my own enum in C++.
> 
> We can do that but I'm a bit afraid that it will increase implicit conversion
> rules. Enum types in Web IDL must be strings. So it would be straightforward
to
> pass them as strings to C++ implementation (instead of passing
> V8PaymentRequest::Success or something). Some customers just need the strings
> and don't need C++'s enum types. What do you think?

Sounds good. Let's continue using the current implementation. I think palmer@
(Chrome Security) prefers enums when passing values over IPC and mojo. So this
patch is doing the right thing, it seems.

[please use gerrit instead, 2016-06-13 15:28:11]

https://codereview.chromium.org/2054823002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentCompleter.h (right):

https://codereview.chromium.org/2054823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentCompleter.h:14: enum
PaymentComplete {
On 2016/06/10 19:15:35, Rouslan (ツ) wrote:
> Doesn't PaymentResponse.idl generate the enum for us to use?

Nevermind, ignore this comment.

https://codereview.chromium.org/2054823002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentResponse.h (right):

https://codereview.chromium.org/2054823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentResponse.h:36: ScriptPromise
complete(ScriptState*, const String& result = "");
On 2016/06/10 19:15:35, Rouslan (ツ) wrote:
> I have not worked with enums in Blink before. Is this really how it works?
Right
> now you have IDL enum represented as strings in C++ that you convert into your
> own enum values in C++: that seems suboptimal. I was hoping that IDL generator
> could help us here.

Nevermind, ignore this comment.

[zino, 2016-06-15 23:06:30]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-15 23:07:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2054823002/180001

[zino, 2016-06-16 00:12:34]

Patchset #3 (id:120001) has been deleted

[zino, 2016-06-16 00:12:55]

Patchset #4 (id:160001) has been deleted

[zino, 2016-06-16 00:13:50]

I've just uploaded a new patch set.

PTAL

[commit-bot: I haz the power, 2016-06-16 00:19:02]

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

[commit-bot: I haz the power, 2016-06-16 00:19:03]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[please use gerrit instead, 2016-06-16 00:31:46]

This is close to the desired outcome. Good job!

You've changed "success" to "immediateClose", which is a better name. I think
you've accidently flipped several "false" flags to "true".  The logic should
remain the same, however.

success=true  => don't show error
success=false => show error

immediateClose=true  => don't show error
immediateClose=false => show error

Perhaps "immediateClose" name is still confusing? I can think of a better name
later. For now, let's just flip the "true" back to "false" in the few places
that I've indicated below. Then you should be ready to submit the code.

https://codereview.chromium.org/2054823002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2054823002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:583:
closeUI(true);
false

https://codereview.chromium.org/2054823002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:601:
closeUI(true);
false

https://codereview.chromium.org/2054823002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:619:
closeUI(true);
false

https://codereview.chromium.org/2054823002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:628:
closeUI(true);
false

https://codereview.chromium.org/2054823002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:691:
closeUI(true);
false

[zino, 2016-06-16 19:26:04]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-16 19:26:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2054823002/200001

[commit-bot: I haz the power, 2016-06-16 20:53:03]

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

[commit-bot: I haz the power, 2016-06-16 20:53:04]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[please use gerrit instead, 2016-06-16 21:02:46]

patch 5 lgtm

[please use gerrit instead, 2016-06-16 21:03:28]

rouslan@chromium.org changed reviewers:
+ dfalcantara@chromium.org, palmer@chromium.org

[please use gerrit instead, 2016-06-16 21:03:29]

dfalcantara@, ptal android.

palmer@, ptal payment_request.mojom.

[palmer, 2016-06-16 22:00:56]

https://codereview.chromium.org/2054823002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java
(right):

https://codereview.chromium.org/2054823002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:401:
* @param immediateClose Whether dismissing the dialog immediately without
describing error.
This comment is not a complete English sentence. How about:

  @param shouldCloseImmediately If true, this function will immediately dismiss
the dialog without describing the error.

https://codereview.chromium.org/2054823002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentResultUIManager.java
(right):

https://codereview.chromium.org/2054823002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentResultUIManager.java:66:
* @param immediateClose Whether dismissing the dialog immediately without
describing error.
Same issue here.

https://codereview.chromium.org/2054823002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentResultUIManager.java:71:
// if the complete() method takes success or empty string, immediateClose will
be true.
I don't understand this comment.

[please use gerrit instead, 2016-06-16 22:06:11]

https://codereview.chromium.org/2054823002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentResultUIManager.java
(right):

https://codereview.chromium.org/2054823002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentResultUIManager.java:71:
// if the complete() method takes success or empty string, immediateClose will
be true.
On 2016/06/16 22:00:55, palmer wrote:
> I don't understand this comment.

English is my second language as well, but perhaps this could be better:

// The immediateClose boolean is true when the merchant calls
// instrumentResponse.complete("success") or instrumentResponse.complete("")
// in JavaScript.

Thoughts?

[palmer, 2016-06-16 22:10:41]

> English is my second language as well, but perhaps this could be better:
> 
> // The immediateClose boolean is true when the merchant calls
> // instrumentResponse.complete("success") or instrumentResponse.complete("")
> // in JavaScript.
> 
> Thoughts?

LGTM with that comment or one like it. Thanks!

[gone, 2016-06-16 22:30:27]

android bits lgtm % palmer's comments

[zino, 2016-06-17 06:24:56]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-17 06:25:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2054823002/240001

[zino, 2016-06-17 08:15:43]

The CQ bit was checked by jinho.bang@samsung.com

[zino, 2016-06-17 08:15:43]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
rouslan@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2054823002/#ps260001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-17 08:16:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2054823002/260001

[commit-bot: I haz the power, 2016-06-17 10:08:21]

Description was changed from

==========
PaymentRequest: complete() method should take PaymentComplete enum value.

Related spec links
  [1] https://w3c.github.io/browser-payment-api/#idl-def-paymentcomplete
  [2] https://github.com/w3c/browser-payment-api/pull/161

BUG=617191
==========

to

==========
PaymentRequest: complete() method should take PaymentComplete enum value.

Related spec links
  [1] https://w3c.github.io/browser-payment-api/#idl-def-paymentcomplete
  [2] https://github.com/w3c/browser-payment-api/pull/161

BUG=617191
==========

[commit-bot: I haz the power, 2016-06-17 10:08:23]

Committed patchset #8 (id:260001)

[commit-bot: I haz the power, 2016-06-17 10:09:44]

Description was changed from

==========
PaymentRequest: complete() method should take PaymentComplete enum value.

Related spec links
  [1] https://w3c.github.io/browser-payment-api/#idl-def-paymentcomplete
  [2] https://github.com/w3c/browser-payment-api/pull/161

BUG=617191
==========

to

==========
PaymentRequest: complete() method should take PaymentComplete enum value.

Related spec links
  [1] https://w3c.github.io/browser-payment-api/#idl-def-paymentcomplete
  [2] https://github.com/w3c/browser-payment-api/pull/161

BUG=617191

Committed: https://crrev.com/5368cdb012e5b9e64f18d9b5bdd914694a6aef94
Cr-Commit-Position: refs/heads/master@{#400410}
==========

[commit-bot: I haz the power, 2016-06-17 10:09:46]

Patchset 8 (id:??) landed as
https://crrev.com/5368cdb012e5b9e64f18d9b5bdd914694a6aef94
Cr-Commit-Position: refs/heads/master@{#400410}