PaymentHandler: Implement set/get methods in PaymentInstruments (content)

PaymentHandler: Implement set/get methods in PaymentInstruments (content)

The methods are simply writing and reading PaymentInstrument data to storage.

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

See the other CLs in this series:
  [1/3] http://crrev.com/2790013002 (blink side)
  [2/3] http://crrev.com/2806133002 (this patch)
  [3/3] http://crrev.com/2811163002 (js test)

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

[zino, 2017-04-09 16:14:06]

Description was changed from

==========
PaymentHandler: Implement read/write PaymentInstrument.

BUG=none
==========

to

==========
PaymentHandler: Implement read/write PaymentInstrument.

BUG=none
==========

[zino, 2017-04-11 16:51:48]

Description was changed from

==========
PaymentHandler: Implement read/write PaymentInstrument.

BUG=none
==========

to

==========
PaymentHandler: Implement set/get methods in PaymentInstruments (blink)

The methods are simply writing and reading PaymentInstrument data to storage.

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

See the other CLs in this series:
  [1/2] http://crrev.com/2790013002 (This patch)
  [2/2] http://crrev.com/2806133002 (content side and test)

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

[zino, 2017-04-11 16:52:05]

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

[zino, 2017-04-11 16:52:24]

Description was changed from

==========
PaymentHandler: Implement set/get methods in PaymentInstruments (blink)

The methods are simply writing and reading PaymentInstrument data to storage.

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

See the other CLs in this series:
  [1/2] http://crrev.com/2790013002 (This patch)
  [2/2] http://crrev.com/2806133002 (content side and test)

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

to

==========
PaymentHandler: Implement set/get methods in PaymentInstruments (blink)

The methods are simply writing and reading PaymentInstrument data to storage.

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

See the other CLs in this series:
  [1/2] http://crrev.com/2790013002 (blink side)
  [2/2] http://crrev.com/2806133002 (this patch)

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

[zino, 2017-04-11 19:16:47]

Description was changed from

==========
PaymentHandler: Implement set/get methods in PaymentInstruments (blink)

The methods are simply writing and reading PaymentInstrument data to storage.

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

See the other CLs in this series:
  [1/2] http://crrev.com/2790013002 (blink side)
  [2/2] http://crrev.com/2806133002 (this patch)

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

to

==========
PaymentHandler: Implement set/get methods in PaymentInstruments (blink)

The methods are simply writing and reading PaymentInstrument data to storage.

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

See the other CLs in this series:
  [1/3] http://crrev.com/2790013002 (blink side)
  [2/3] http://crrev.com/2806133002 (this patch)
  [3/3] http://crrev.com/2811163002 (js test)

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

[zino, 2017-04-11 19:23:01]

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

[zino, 2017-04-11 19:23:24]

PTAL

[Tom Sepez, 2017-04-11 20:26:04]

On 2017/04/11 19:23:24, zino wrote:
> PTAL

Presumably its up to the app database to validate the origin from which the
request is coming is one that the user previously approved?  Otherwise, can a
compromised rendered add/modify/delete payment instruments for any origin?

[zino, 2017-04-11 23:34:45]

On 2017/04/11 20:26:04, Tom Sepez wrote:
> On 2017/04/11 19:23:24, zino wrote:
> > PTAL
> 
> Presumably its up to the app database to validate the origin from which the
> request is coming is one that the user previously approved?  Otherwise, can a
> compromised rendered add/modify/delete payment instruments for any origin?

Thank you for your review.

The former is correct but it's not implemented yet. (https://crbug.com/665949)
But I have a plan to implement it like push notification case.
The browser will request a permission when calling subscribe() function in
PushManager)
Similarly, I'll make instruments ask a permission from user before calling
operations(get/set and so on) of instruments.
FYI, this feature is not |payment request| and then not enabled yet in any
platform.
It's still behind runtime flag. So, it's not problem although the asking
permission is not implemented yet.

[Tom Sepez, 2017-04-12 16:58:05]

On 2017/04/11 23:34:45, zino wrote:
> On 2017/04/11 20:26:04, Tom Sepez wrote:
> > On 2017/04/11 19:23:24, zino wrote:
> > > PTAL
> > 
> > Presumably its up to the app database to validate the origin from which the
> > request is coming is one that the user previously approved?  Otherwise, can
a
> > compromised rendered add/modify/delete payment instruments for any origin?
> 
> Thank you for your review.
> 
> The former is correct but it's not implemented yet. (https://crbug.com/665949)
> But I have a plan to implement it like push notification case.
> The browser will request a permission when calling subscribe() function in
> PushManager)
> Similarly, I'll make instruments ask a permission from user before calling
> operations(get/set and so on) of instruments.
> FYI, this feature is not |payment request| and then not enabled yet in any
> platform.
> It's still behind runtime flag. So, it's not problem although the asking
> permission is not implemented yet.

OK, LGTM but please keep me in the loop for the future permission checks.

[please use gerrit instead, 2017-04-12 19:11:31]

LGTM % comments

https://codereview.chromium.org/2806133002/diff/100001/content/browser/paymen...
File content/browser/payments/payment_app_database.cc (right):

https://codereview.chromium.org/2806133002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_database.cc:56:
instrument->enabled_methods.push_back(method);
Does this work?

instrument->enabled_methods = instrument_proto.enabled_methods();

https://codereview.chromium.org/2806133002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_database.cc:117:
base::Passed(std::move(callback))));
Is base::Passed(std::move()) wrapper necessary? You don't use it on line 103,
for example. If you find that std::move() is necessary, try removing
base::Passed(), if you can.

https://codereview.chromium.org/2806133002/diff/100001/content/browser/paymen...
File content/browser/payments/payment_app_database.h (right):

https://codereview.chromium.org/2806133002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_database.h:20: using
payments::mojom::PaymentHandlerStatus;
Please don't put such "using" statements in headers, because they pollute the
namespaces of everyone who includes this file.

https://codereview.chromium.org/2806133002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_database.h:42:
base::OnceCallback<void(PaymentHandlerStatus)>;
These "using" statements are OK, by the way.

https://codereview.chromium.org/2806133002/diff/100001/content/browser/paymen...
File content/browser/payments/payment_manager_unittest.cc (right):

https://codereview.chromium.org/2806133002/diff/100001/content/browser/paymen...
content/browser/payments/payment_manager_unittest.cc:18: using
payments::mojom::PaymentInstrumentPtr;
Please prepend the top namespace with :: and move the "using" statements inside
of the anonymous namespace.

namespace content {
namespace {

using ::payments::mojom::PaymentAppManifestError;
...

[zino, 2017-04-14 10:42:19]

Patchset #7 (id:120001) has been deleted

[zino, 2017-04-15 04:54:43]

Patchset #7 (id:140001) has been deleted

[zino, 2017-04-15 04:54:56]

Description was changed from

==========
PaymentHandler: Implement set/get methods in PaymentInstruments (blink)

The methods are simply writing and reading PaymentInstrument data to storage.

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

See the other CLs in this series:
  [1/3] http://crrev.com/2790013002 (blink side)
  [2/3] http://crrev.com/2806133002 (this patch)
  [3/3] http://crrev.com/2811163002 (js test)

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

to

==========
PaymentHandler: Implement set/get methods in PaymentInstruments (content)

The methods are simply writing and reading PaymentInstrument data to storage.

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

See the other CLs in this series:
  [1/3] http://crrev.com/2790013002 (blink side)
  [2/3] http://crrev.com/2806133002 (this patch)
  [3/3] http://crrev.com/2811163002 (js test)

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

[zino, 2017-04-15 05:18:09]

Thank you for review.

https://codereview.chromium.org/2806133002/diff/100001/content/browser/paymen...
File content/browser/payments/payment_app_database.cc (right):

https://codereview.chromium.org/2806133002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_database.cc:56:
instrument->enabled_methods.push_back(method);
On 2017/04/12 19:11:31, ಠ_ಠ wrote:
> Does this work?
> 
> instrument->enabled_methods = instrument_proto.enabled_methods();

We can not, the instrument_proto.enabled_methods() returns |const
::google::protobuf::RepeatedPtrField< ::std::string>&| type.

https://codereview.chromium.org/2806133002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_database.cc:117:
base::Passed(std::move(callback))));
On 2017/04/12 19:11:31, ಠ_ಠ wrote:
> Is base::Passed(std::move()) wrapper necessary? You don't use it on line 103,
> for example. If you find that std::move() is necessary, try removing
> base::Passed(), if you can.

We can not..
The FindReadyRegistrationForPattern still requires a legacy callback as
parameter. So, we should use base::Bind instead of base::BindOnce. On the other
hand, the |ReadPaymentInstrumentCallback| is OnceCallback and it's move only
type. Therefore, we should use base::Passed right now. If we change the function
can take base::BindOnce instead of base::Bind, then we can just use std::move
without base::passed().

https://codereview.chromium.org/2806133002/diff/100001/content/browser/paymen...
File content/browser/payments/payment_app_database.h (right):

https://codereview.chromium.org/2806133002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_database.h:20: using
payments::mojom::PaymentHandlerStatus;
On 2017/04/12 19:11:31, ಠ_ಠ wrote:
> Please don't put such "using" statements in headers, because they pollute the
> namespaces of everyone who includes this file.

Done.

https://codereview.chromium.org/2806133002/diff/100001/content/browser/paymen...
File content/browser/payments/payment_manager_unittest.cc (right):

https://codereview.chromium.org/2806133002/diff/100001/content/browser/paymen...
content/browser/payments/payment_manager_unittest.cc:18: using
payments::mojom::PaymentInstrumentPtr;
On 2017/04/12 19:11:31, ಠ_ಠ wrote:
> Please prepend the top namespace with :: and move the "using" statements
inside
> of the anonymous namespace.
> 
> namespace content {
> namespace {
> 
> using ::payments::mojom::PaymentAppManifestError;
> ...

Done.

[zino, 2017-04-17 16:10:13]

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

[zino, 2017-04-17 16:10:13]

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

[commit-bot: I haz the power, 2017-04-17 16:10:30]

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-04-17 17:03:06]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1492445413395560,
"parent_rev": "cffb01887fc1b0719aea82138aef58a1054405a8", "commit_rev":
"0e7bbf149b075810960c71f8eb873d08f3984b89"}

[commit-bot: I haz the power, 2017-04-17 17:03:16]

Description was changed from

==========
PaymentHandler: Implement set/get methods in PaymentInstruments (content)

The methods are simply writing and reading PaymentInstrument data to storage.

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

See the other CLs in this series:
  [1/3] http://crrev.com/2790013002 (blink side)
  [2/3] http://crrev.com/2806133002 (this patch)
  [3/3] http://crrev.com/2811163002 (js test)

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

to

==========
PaymentHandler: Implement set/get methods in PaymentInstruments (content)

The methods are simply writing and reading PaymentInstrument data to storage.

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

See the other CLs in this series:
  [1/3] http://crrev.com/2790013002 (blink side)
  [2/3] http://crrev.com/2806133002 (this patch)
  [3/3] http://crrev.com/2811163002 (js test)

BUG=661608

Review-Url: https://codereview.chromium.org/2806133002
Cr-Commit-Position: refs/heads/master@{#464934}
Committed:
https://chromium.googlesource.com/chromium/src/+/0e7bbf149b075810960c71f8eb87...
==========

[commit-bot: I haz the power, 2017-04-17 17:03:17]

Committed patchset #8 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/0e7bbf149b075810960c71f8eb87...