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

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 (this patch)
  [2/3] http://crrev.com/2806133002 (content side and unittests)
  [3/3] http://crrev.com/2811163002 (js test)

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

[zino, 2017-04-01 13:38:14]

Description was changed from

==========
PaymentHandler: Implement set/get methods in PaymentInstruments.

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

to

==========
PaymentHandler: Implement set/get methods in PaymentInstruments.

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

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

Description was changed from

==========
PaymentHandler: Implement set/get methods in PaymentInstruments.

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 (This patch)
  [2/2] http://crrev.com/2806133002 (content side and test)

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

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

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

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

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 (this patch)
  [2/2] http://crrev.com/2806133002 (content side and test)

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

[Tom Sepez, 2017-04-11 17:20:59]

I didn't see any code that actually does the writing.  Where will that happen?
Does that happen deep inside the android-specific java?

How do we know that the process making this call has the rights to see the
response?

[zino, 2017-04-11 17:48:02]

Patchset #6 (id:100001) has been deleted

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

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/3] http://crrev.com/2790013002 (this patch)
  [2/3] http://crrev.com/2806133002 (content side and unittests)
  [3/3] http://crrev.com/2811163002 (js test)

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

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

On 2017/04/11 17:20:59, Tom Sepez wrote:
> I didn't see any code that actually does the writing.  Where will that happen?
> Does that happen deep inside the android-specific java?
> 
> How do we know that the process making this call has the rights to see the
> response?

Please see the other CLs in this series:
  [1/3] http://crrev.com/2790013002 (this patch)
  [2/3] http://crrev.com/2806133002 (content side and unittests)
  [3/3] http://crrev.com/2811163002 (js test)

[Tom Sepez, 2017-04-11 21:53:25]

This one is LGTM, too.  We'll discuss on the second CL.

[please use gerrit instead, 2017-04-12 19:02:37]

lgtm % comments

https://codereview.chromium.org/2790013002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentInstruments.h (right):

https://codereview.chromium.org/2790013002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentInstruments.h:30:
PaymentInstruments(const payments::mojom::blink::PaymentManagerPtr&);
explicit.

https://codereview.chromium.org/2790013002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentManager.cpp (right):

https://codereview.chromium.org/2790013002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentManager.cpp:133: if
(!instruments_) {
{} is not necessary.

https://codereview.chromium.org/2790013002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentManager.cpp:134: instruments_
= new PaymentInstruments(manager_);
The order of deletion in this object is manager_, instruments_, registration_.
Therefore, you can have an instruments_ object with an invalid reference to
manager_. Please move the instruments_ member variable to be below the manager_
variable in the header to remedy this problem.

[haraken, 2017-04-13 01:32:31]

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

[haraken, 2017-04-13 01:32:32]

https://codereview.chromium.org/2790013002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp (right):

https://codereview.chromium.org/2790013002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:107:
WTF::Vector<WTF::String>(details.enabledMethods());

Why do we need the static cast?

https://codereview.chromium.org/2790013002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:120:
V8StringToWebCoreString<String>(value, kDoNotExternalize);

ToCoreString

https://codereview.chromium.org/2790013002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:144:
Vector<WTF::String> enabled_methods;

Drop WTF::.

https://codereview.chromium.org/2790013002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:161:
resolver->Reject(exception_state.GetException());

I guess you need to call exception_state.Clear(). Otherwise, the exception will
be thrown...?

https://codereview.chromium.org/2790013002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentManager.cpp (right):

https://codereview.chromium.org/2790013002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentManager.cpp:134: instruments_
= new PaymentInstruments(manager_);
On 2017/04/12 19:02:37, ಠ_ಠ wrote:
> The order of deletion in this object is manager_, instruments_, registration_.
> Therefore, you can have an instruments_ object with an invalid reference to
> manager_. Please move the instruments_ member variable to be below the
manager_
> variable in the header to remedy this problem.

I don't fully understand what you mean, but a destructor of an Oilpan object is
not allowed to touch any other Oilpan objects (because there's no guarantee
about the order in which they get destructed).

[zino, 2017-04-14 11:06:46]

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

[commit-bot: I haz the power, 2017-04-14 11:07:00]

Dry run: 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-14 11:16:32]

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

[commit-bot: I haz the power, 2017-04-14 11:16:33]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[zino, 2017-04-15 03:10:35]

Patchset #8 (id:160001) has been deleted

[zino, 2017-04-15 03:30:10]

haraken@

PTAL, I addressed your comments.

Thanks :)

https://codereview.chromium.org/2790013002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp (right):

https://codereview.chromium.org/2790013002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:107:
WTF::Vector<WTF::String>(details.enabledMethods());
On 2017/04/13 01:32:31, haraken wrote:
> 
> Why do we need the static cast?

Done.

https://codereview.chromium.org/2790013002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:120:
V8StringToWebCoreString<String>(value, kDoNotExternalize);
On 2017/04/13 01:32:31, haraken wrote:
> 
> ToCoreString

Done.

https://codereview.chromium.org/2790013002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:144:
Vector<WTF::String> enabled_methods;
On 2017/04/13 01:32:31, haraken wrote:
> 
> Drop WTF::.

Done.

https://codereview.chromium.org/2790013002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:161:
resolver->Reject(exception_state.GetException());
On 2017/04/13 01:32:31, haraken wrote:
> 
> I guess you need to call exception_state.Clear(). Otherwise, the exception
will
> be thrown...?

Done.

I used exception_state.Reject instead.
The function calls ClearException internally.

https://codereview.chromium.org/2790013002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentInstruments.h (right):

https://codereview.chromium.org/2790013002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentInstruments.h:30:
PaymentInstruments(const payments::mojom::blink::PaymentManagerPtr&);
On 2017/04/12 19:02:37, ಠ_ಠ wrote:
> explicit.

Done.

https://codereview.chromium.org/2790013002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentManager.cpp (right):

https://codereview.chromium.org/2790013002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentManager.cpp:134: instruments_
= new PaymentInstruments(manager_);
On 2017/04/13 01:32:31, haraken wrote:
> On 2017/04/12 19:02:37, ಠ_ಠ wrote:
> > The order of deletion in this object is manager_, instruments_,
registration_.
> > Therefore, you can have an instruments_ object with an invalid reference to
> > manager_. Please move the instruments_ member variable to be below the
> manager_
> > variable in the header to remedy this problem.
> 
> I don't fully understand what you mean, but a destructor of an Oilpan object
is
> not allowed to touch any other Oilpan objects (because there's no guarantee
> about the order in which they get destructed).

Done.

Anyway I modified order of declaration in header.

[zino, 2017-04-17 01:28:49]

haraken@, gentle ping.

[haraken, 2017-04-17 07:15:56]

LGTM

https://codereview.chromium.org/2790013002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp (right):

https://codereview.chromium.org/2790013002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:38: "There is
no stored instrument"));

Is "There is no stored instrument" a proper error message?

https://codereview.chromium.org/2790013002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:42: "There is
no stored instrument"));

Ditto.

[zino, 2017-04-17 14:17:56]

Thank you for review.

https://codereview.chromium.org/2790013002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp (right):

https://codereview.chromium.org/2790013002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:38: "There is
no stored instrument"));
On 2017/04/17 07:15:56, haraken wrote:
> 
> Is "There is no stored instrument" a proper error message?

Done.

https://codereview.chromium.org/2790013002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:42: "There is
no stored instrument"));
On 2017/04/17 07:15:56, haraken wrote:
> 
> Ditto.

Done.

[zino, 2017-04-17 14:18:11]

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

[zino, 2017-04-17 14:18:11]

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

[commit-bot: I haz the power, 2017-04-17 14:18:23]

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 15:58:19]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1492438691132840,
"parent_rev": "b23e35c8ae0fcc9597eaf908bbcc9c9d5fb382de", "commit_rev":
"fe1c747c077c99272ebbc2eafe7983b9b9e2d927"}

[commit-bot: I haz the power, 2017-04-17 15:58:31]

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 (this patch)
  [2/3] http://crrev.com/2806133002 (content side and unittests)
  [3/3] http://crrev.com/2811163002 (js 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/3] http://crrev.com/2790013002 (this patch)
  [2/3] http://crrev.com/2806133002 (content side and unittests)
  [3/3] http://crrev.com/2811163002 (js test)

BUG=661608

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

[commit-bot: I haz the power, 2017-04-17 15:58:33]

Committed patchset #9 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/fe1c747c077c99272ebbc2eafe79...