PaymentHandler: Initial implementation for PaymentInstruments.

PaymentHandler: Initial implementation for PaymentInstruments.

This CL only adds IDLs and empty implementations. This feature is behind runtime
flag.

Related spec:
  https://w3c.github.io/webpayments-payment-apps-api/#payment-instruments

BUG=661608
Review-Url: https://codereview.chromium.org/2775343002
Cr-Commit-Position: refs/heads/master@{#460844}
Committed: https://chromium.googlesource.com/chromium/src/+/c8e4d733fe61ac65f4c834d7fbe73a63dda1b2ed

[zino, 2017-03-27 16:04:21]

Description was changed from

==========
PaymentHandler: Initial implementation for PaymentInstruments.

BUG=
==========

to

==========
PaymentHandler: Initial implementation for PaymentInstruments.

BUG=
==========

[zino, 2017-03-28 16:08:31]

Description was changed from

==========
PaymentHandler: Initial implementation for PaymentInstruments.

BUG=
==========

to

==========
PaymentHandler: Initial implementation for PaymentInstruments.

This CL only adds IDLs and empty implementations. This feature is behind runtime
flag.

Related spec:
  https://w3c.github.io/webpayments-payment-apps-api/#payment-instruments

BUG=669876
==========

[zino, 2017-03-28 16:08:31]

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

[zino, 2017-03-28 16:19:37]

PTAL.

[please use gerrit instead, 2017-03-28 16:33:30]

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentAppManager.idl (right):

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentAppManager.idl:14: readonly
attribute PaymentInstruments instruments;
Can you put this into PaymentManager interface instead? That would match
https://w3c.github.io/webpayments-payment-apps-api/#idl-def-paymentmanager
better.

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentInstrument.idl (right):

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstrument.idl:7: dictionary
PaymentInstrument {
Let's put the RuntimeEnabled=PaymentApp in here just for clarity.

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstrument.idl:9: //
TODO(zino): sequence<ImageObject> icons;
Why comment this out?

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp (right):

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:9: #include
"platform/wtf/text/WTFString.h"
Lets include this in the header instead.

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:19:
NOTIMPLEMENTED();
#include "base/logging.h"

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentInstruments.h (right):

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstruments.h:17: class
MODULES_EXPORT PaymentInstruments final
#include "modules/ModulesExport.h"

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstruments.h:18: : public
GarbageCollectedFinalized<PaymentInstruments>,
#include "platform/heap/GarbageCollected.h"

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstruments.h:21:
WTF_MAKE_NONCOPYABLE(PaymentInstruments);
#include "platform/wtf/Noncopyable.h"

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstruments.h:23:
DECLARE_TRACE();
I believe DECLARE_TRACE() should be public.

Also:

#include "platform/heap/Visitor.h"

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstruments.h:31:
ScriptPromise has(const String& instrumentKey);
#include "platform/wtf/text/WTFString.h"

[zino, 2017-03-29 15:51:23]

Thank you for review.

I addressed some your comments and I replied for some comments.

Thanks!

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentAppManager.idl (right):

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentAppManager.idl:14: readonly
attribute PaymentInstruments instruments;
On 2017/03/28 16:33:29, ಠ_ಠ wrote:
> Can you put this into PaymentManager interface instead? That would match
> https://w3c.github.io/webpayments-payment-apps-api/#idl-def-paymentmanager
> better.

I uploaded another CL:
https://codereview.chromium.org/2785523003/

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentInstrument.idl (right):

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstrument.idl:7: dictionary
PaymentInstrument {
On 2017/03/28 16:33:29, ಠ_ಠ wrote:
> Let's put the RuntimeEnabled=PaymentApp in here just for clarity.

Is it possible? I didn't find any case of use.
As far as I know, the dictionary only needs the flag for member attributes.

Related CL link:
  https://codereview.chromium.org/1412433004

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstrument.idl:9: //
TODO(zino): sequence<ImageObject> icons;
On 2017/03/28 16:33:29, ಠ_ಠ wrote:
> Why comment this out?

The icons is not be clear yet. There is no ImageObject in IDL level.
Instead we should use string or define another interface.
Related link:
 - https://github.com/w3c/webpayments-payment-apps-api/issues/69

I'll leave a comment in more detail.

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp (right):

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:9: #include
"platform/wtf/text/WTFString.h"
On 2017/03/28 16:33:29, ಠ_ಠ wrote:
> Lets include this in the header instead.

Done.

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:19:
NOTIMPLEMENTED();
On 2017/03/28 16:33:29, ಠ_ಠ wrote:
> #include "base/logging.h"

Can we include this? Not sure but it might be allowed in platform layer?
anyway presubmit error happens if including this file.

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentInstruments.h (right):

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstruments.h:17: class
MODULES_EXPORT PaymentInstruments final
On 2017/03/28 16:33:29, ಠ_ಠ wrote:
> #include "modules/ModulesExport.h"

The header file was already included :)

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstruments.h:18: : public
GarbageCollectedFinalized<PaymentInstruments>,
On 2017/03/28 16:33:29, ಠ_ಠ wrote:
> #include "platform/heap/GarbageCollected.h"

I think #include "platform/heap/Handle.h" is enough according to oilpan guide.
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstruments.h:21:
WTF_MAKE_NONCOPYABLE(PaymentInstruments);
On 2017/03/28 16:33:29, ಠ_ಠ wrote:
> #include "platform/wtf/Noncopyable.h"

Done.

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstruments.h:23:
DECLARE_TRACE();
On 2017/03/28 16:33:29, ಠ_ಠ wrote:
> I believe DECLARE_TRACE() should be public.

It's my mistake. Done!

> 
> Also:
> 
> #include "platform/heap/Visitor.h"

#include "platform/heap/Handle.h" is enough.

https://codereview.chromium.org/2775343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstruments.h:31:
ScriptPromise has(const String& instrumentKey);
On 2017/03/28 16:33:29, ಠ_ಠ wrote:
> #include "platform/wtf/text/WTFString.h"

Done.

[please use gerrit instead, 2017-03-29 21:30:16]

lgtm % comments

https://codereview.chromium.org/2775343002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentInstruments.h (right):

https://codereview.chromium.org/2775343002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstruments.h:25:
DECLARE_TRACE();
Appears to still be private.

https://codereview.chromium.org/2775343002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentManager.cpp (right):

https://codereview.chromium.org/2775343002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentManager.cpp:133: if
(!m_instruments) {
No need for {} in a single-line if statement body.

[haraken, 2017-03-30 00:00:46]

Implementation-wise LGTM

https://codereview.chromium.org/2775343002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentInstruments.h (right):

https://codereview.chromium.org/2775343002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstruments.h:20: : public
GarbageCollectedFinalized<PaymentInstruments>,

"Finalized" wouldn't be needed.

[zino, 2017-03-30 00:12:53]

I addressed your comments.

Thank you for review.

https://codereview.chromium.org/2775343002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentInstruments.h (right):

https://codereview.chromium.org/2775343002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstruments.h:20: : public
GarbageCollectedFinalized<PaymentInstruments>,
On 2017/03/30 00:00:45, haraken wrote:
> 
> "Finalized" wouldn't be needed.

Done.

https://codereview.chromium.org/2775343002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentInstruments.h:25:
DECLARE_TRACE();
On 2017/03/29 21:30:16, ಠ_ಠ wrote:
> Appears to still be private.

Done.

https://codereview.chromium.org/2775343002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentManager.cpp (right):

https://codereview.chromium.org/2775343002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentManager.cpp:133: if
(!m_instruments) {
On 2017/03/29 21:30:16, ಠ_ಠ wrote:
> No need for {} in a single-line if statement body.

Done.

[zino, 2017-03-30 15:14:31]

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

[zino, 2017-03-30 15:14:31]

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

[commit-bot: I haz the power, 2017-03-30 15:14:56]

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

[zino, 2017-03-30 16:09:44]

Description was changed from

==========
PaymentHandler: Initial implementation for PaymentInstruments.

This CL only adds IDLs and empty implementations. This feature is behind runtime
flag.

Related spec:
  https://w3c.github.io/webpayments-payment-apps-api/#payment-instruments

BUG=669876
==========

to

==========
PaymentHandler: Initial implementation for PaymentInstruments.

This CL only adds IDLs and empty implementations. This feature is behind runtime
flag.

Related spec:
  https://w3c.github.io/webpayments-payment-apps-api/#payment-instruments

BUG=661608
==========

[commit-bot: I haz the power, 2017-03-30 16:27:09]

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

[commit-bot: I haz the power, 2017-03-30 16:27:10]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[zino, 2017-03-30 16:57:19]

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

[zino, 2017-03-30 16:57:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2775343002/#ps120001
(title: "PaymentHandler: Initial implementation for PaymentInstruments.")

[commit-bot: I haz the power, 2017-03-30 16:57:56]

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-03-30 19:09:23]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490893039094970,
"parent_rev": "410577f4d719ecdd656e563b58ba327b98e7fe26", "commit_rev":
"c8e4d733fe61ac65f4c834d7fbe73a63dda1b2ed"}

[commit-bot: I haz the power, 2017-03-30 19:10:13]

Description was changed from

==========
PaymentHandler: Initial implementation for PaymentInstruments.

This CL only adds IDLs and empty implementations. This feature is behind runtime
flag.

Related spec:
  https://w3c.github.io/webpayments-payment-apps-api/#payment-instruments

BUG=661608
==========

to

==========
PaymentHandler: Initial implementation for PaymentInstruments.

This CL only adds IDLs and empty implementations. This feature is behind runtime
flag.

Related spec:
  https://w3c.github.io/webpayments-payment-apps-api/#payment-instruments

BUG=661608

Review-Url: https://codereview.chromium.org/2775343002
Cr-Commit-Position: refs/heads/master@{#460844}
Committed:
https://chromium.googlesource.com/chromium/src/+/c8e4d733fe61ac65f4c834d7fbe7...
==========

[commit-bot: I haz the power, 2017-03-30 19:10:14]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/c8e4d733fe61ac65f4c834d7fbe7...