PaymentApp: Initial implementation for PaymentAppManager.setManifest().

PaymentApp: Initial implementation for PaymentAppManager.setManifest().

This CL includes an initial implementation in blink side and browser side.
by using mojo IPC. Currently, when setManifest() is called, just sends
NOT_IMPLEMENTED in browser side and then rejects NotSupportedError in blink.

BUG=661608
TEST=payment-app-interfaces.html
TEST=payment-app-manager.html

Committed: https://crrev.com/ef456add5796bfdae498c3de4f4e8e2607d5173a
Cr-Commit-Position: refs/heads/master@{#430760}

[zino, 2016-11-07 16:59:18]

Description was changed from

==========
PaymentApp: Initial implementation for PaymentAppManager.setManifest().

This CL includes an initial implementation in blink side and browser side.
by using mojo IPC. Currently, when setManifest() is called, just sends
NOT_IMPLEMENTED in browser side and then rejects NotSupportedError in blink.

BUG=661608
TEST=payment-app-interfaces.html
TEST=payment-app-manager.html
==========

to

==========
PaymentApp: Initial implementation for PaymentAppManager.setManifest().

This CL includes an initial implementation in blink side and browser side.
by using mojo IPC. Currently, when setManifest() is called, just sends
NOT_IMPLEMENTED in browser side and then rejects NotSupportedError in blink.

BUG=661608
TEST=payment-app-interfaces.html
TEST=payment-app-manager.html
==========

[zino, 2016-11-07 16:59:18]

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

[zino, 2016-11-07 17:00:24]

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

[zino, 2016-11-07 17:01:02]

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

[zino, 2016-11-07 17:01:02]

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

[zino, 2016-11-07 17:02:26]

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

[zino, 2016-11-07 17:03:29]

jinho.bang@samsung.com changed reviewers:
+ avi@chromium.org
- jkarlin@chromium.org

[zino, 2016-11-07 17:03:29]

jinho.bang@samsung.com changed reviewers:
+ avi@chromium.org
- jkarlin@chromium.org

[zino, 2016-11-07 17:04:29]

jinho.bang@samsung.com changed reviewers:
- kinuko@chromium.org

[zino, 2016-11-07 17:05:42]

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

[zino, 2016-11-07 17:13:30]

PTAL

rouslan@ for payments
haraken@ for blink parts.
Tom Sepez@ for Mojo IPC
Avi@ for content/*

jkarlin@,
Could you review this patch?
As I know, you're expert for service worker families (background sync and cache
storage).
This payment app feature also uses service worker and its storage.
If you're busy, I'll find other reviewer.

[zino, 2016-11-07 17:14:06]

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

[commit-bot: I haz the power, 2016-11-07 17:14:43]

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, 2016-11-07 17:23:28]

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

[commit-bot: I haz the power, 2016-11-07 17:23:29]

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

[Tom Sepez, 2016-11-07 17:52:04]

lgtm

https://codereview.chromium.org/2476343002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/modules/payments/payment_app.mojom
(right):

https://codereview.chromium.org/2476343002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/modules/payments/payment_app.mojom:16:
array<string> enabled_methods;
None of these strings are JSON any more, right?

[Avi (use Gerrit), 2016-11-07 18:13:09]

lgtm

[please use gerrit instead, 2016-11-07 20:05:30]

https://codereview.chromium.org/2476343002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/modules/payments/payment_app.mojom
(right):

https://codereview.chromium.org/2476343002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/modules/payments/payment_app.mojom:16:
array<string> enabled_methods;
On 2016/11/07 17:52:04, Tom Sepez wrote:
> None of these strings are JSON any more, right?

These a short strings like "visa", "mastercard", etc. Could also be URLs like
"https://android.com/pay".

[please use gerrit instead, 2016-11-07 20:06:08]

https://codereview.chromium.org/2476343002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/modules/payments/payment_app.mojom
(right):

https://codereview.chromium.org/2476343002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/modules/payments/payment_app.mojom:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
This file should be in components/payments/

[zino, 2016-11-07 22:30:51]

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

[commit-bot: I haz the power, 2016-11-07 22:31:32]

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

[zino, 2016-11-07 22:32:08]

Thank you for review.

I moved the mojom file to component/payments. (and changed namespace from blink
to payments.)

PTAL.

https://codereview.chromium.org/2476343002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/modules/payments/payment_app.mojom
(right):

https://codereview.chromium.org/2476343002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/modules/payments/payment_app.mojom:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
On 2016/11/07 20:06:08, rouslan wrote:
> This file should be in components/payments/

Done.

[zino, 2016-11-07 22:44:07]

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

[commit-bot: I haz the power, 2016-11-07 22:44:47]

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, 2016-11-07 22:50:41]

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

[commit-bot: I haz the power, 2016-11-07 22:50:42]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[zino, 2016-11-07 22:55:05]

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

[commit-bot: I haz the power, 2016-11-07 22:55:39]

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, 2016-11-07 23:56:40]

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

[commit-bot: I haz the power, 2016-11-07 23:56:41]

Dry run: 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_...)

[haraken, 2016-11-08 02:06:57]

modules/ LGTM on my side.

[zino, 2016-11-08 12:55:06]

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

[commit-bot: I haz the power, 2016-11-08 12:55:17]

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

[jkarlin, 2016-11-08 13:48:53]

Looks good with one question.

https://codereview.chromium.org/2476343002/diff/40001/content/browser/payment...
File content/browser/payments/payment_app_context.cc (right):

https://codereview.chromium.org/2476343002/diff/40001/content/browser/payment...
content/browser/payments/payment_app_context.cc:57: services_[service] =
base::WrapUnique(service);
Prefer to use base::MakeUnique. Suggest:

services_[service] = base::MakeUnique<PaymentAppManager>(this,
std::move(request));

https://codereview.chromium.org/2476343002/diff/40001/content/browser/payment...
File content/browser/payments/payment_app_context.h (right):

https://codereview.chromium.org/2476343002/diff/40001/content/browser/payment...
content/browser/payments/payment_app_context.h:28: void Init(const
scoped_refptr<ServiceWorkerContextWrapper>& context);
Passing a const scoped_refptr<X>& is now against best practice. Instead, pass
scoped_refptr<ServiceWorkerContextWrapper> to make it clear that Init is taking
ownership.

https://codereview.chromium.org/2476343002/diff/40001/content/browser/payment...
content/browser/payments/payment_app_context.h:62: #endif  //
CONTENT_BROWSER_PAYMENT_APP_PAYMENT_APP_CONTEXT_H_
Should be CONTENT_BROWSER_PAYMENTS_PAYMENT_APP_CONTEXT_H_

https://codereview.chromium.org/2476343002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentAppManager.cpp (right):

https://codereview.chromium.org/2476343002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentAppManager.cpp:118:
m_manager.reset();
If m_manager can be reset, should it be checked for nullptr before use (e.g., in
setManifest)?

[commit-bot: I haz the power, 2016-11-08 14:27:36]

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

[commit-bot: I haz the power, 2016-11-08 14:27:37]

Dry run: This issue passed the CQ dry run.

[zino, 2016-11-08 14:31:45]

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

[commit-bot: I haz the power, 2016-11-08 14:32:12]

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

[zino, 2016-11-08 14:34:22]

Thank you for review.

I left a comment. If I'm missing something, please correct me.

Thanks! :)

https://codereview.chromium.org/2476343002/diff/40001/content/browser/payment...
File content/browser/payments/payment_app_context.cc (right):

https://codereview.chromium.org/2476343002/diff/40001/content/browser/payment...
content/browser/payments/payment_app_context.cc:57: services_[service] =
base::WrapUnique(service);
On 2016/11/08 13:48:53, jkarlin wrote:
> Prefer to use base::MakeUnique. Suggest:
> 
> services_[service] = base::MakeUnique<PaymentAppManager>(this,
> std::move(request));

Sorry, I don't understand your comment exactly.

Do we have to do as follows?

std::unique_ptr<PaymentAppManager> service =
base::MakeUnique<PaymentAppManager>(this, std::move(request));
services_[service.get()] = std::move(service);

The service is still necessary because it's a key for hash map.

https://codereview.chromium.org/2476343002/diff/40001/content/browser/payment...
File content/browser/payments/payment_app_context.h (right):

https://codereview.chromium.org/2476343002/diff/40001/content/browser/payment...
content/browser/payments/payment_app_context.h:28: void Init(const
scoped_refptr<ServiceWorkerContextWrapper>& context);
On 2016/11/08 13:48:53, jkarlin wrote:
> Passing a const scoped_refptr<X>& is now against best practice. Instead, pass
> scoped_refptr<ServiceWorkerContextWrapper> to make it clear that Init is
taking
> ownership.

Done.

https://codereview.chromium.org/2476343002/diff/40001/content/browser/payment...
content/browser/payments/payment_app_context.h:62: #endif  //
CONTENT_BROWSER_PAYMENT_APP_PAYMENT_APP_CONTEXT_H_
On 2016/11/08 13:48:53, jkarlin wrote:
> Should be CONTENT_BROWSER_PAYMENTS_PAYMENT_APP_CONTEXT_H_

Done.

https://codereview.chromium.org/2476343002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentAppManager.cpp (right):

https://codereview.chromium.org/2476343002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentAppManager.cpp:118:
m_manager.reset();
On 2016/11/08 13:48:53, jkarlin wrote:
> If m_manager can be reset, should it be checked for nullptr before use (e.g.,
in
> setManifest)?

Done.

[jkarlin, 2016-11-08 14:45:03]

lgtm with nits, see 'git cl lint' warnings below:

content/browser/payments/payment_app_context.cc:56:  Add #include <utility> for
move  [build/include_what_you_use] [4]
Done processing content/browser/payments/payment_app_context.cc
Done processing content/browser/payments/payment_app_context.h
content/browser/payments/payment_app_manager.cc:21:  Add #include <utility> for
move  [build/include_what_you_use] [4]
content/browser/payments/payment_app_manager.cc:36:  Add #include <string> for
string  [build/include_what_you_use] [4]
Done processing content/browser/payments/payment_app_manager.cc
content/browser/payments/payment_app_manager.h:29:  Add #include <string> for
string  [build/include_what_you_use] [4]
Done processing content/browser/payments/payment_app_manager.h

https://codereview.chromium.org/2476343002/diff/40001/content/browser/payment...
File content/browser/payments/payment_app_context.cc (right):

https://codereview.chromium.org/2476343002/diff/40001/content/browser/payment...
content/browser/payments/payment_app_context.cc:57: services_[service] =
base::WrapUnique(service);
On 2016/11/08 14:34:21, zino wrote:
> On 2016/11/08 13:48:53, jkarlin wrote:
> > Prefer to use base::MakeUnique. Suggest:
> > 
> > services_[service] = base::MakeUnique<PaymentAppManager>(this,
> > std::move(request));
> 
> Sorry, I don't understand your comment exactly.
> 
> Do we have to do as follows?
> 
> std::unique_ptr<PaymentAppManager> service =
> base::MakeUnique<PaymentAppManager>(this, std::move(request));
> services_[service.get()] = std::move(service);
> 
> The service is still necessary because it's a key for hash map.

Ah, sorry, didn't notice that service was the key to the hash. This is fine.

[zino, 2016-11-08 14:56:22]

I've just fixed the lint errors.

Thanks!

[zino, 2016-11-08 15:45:37]

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

[commit-bot: I haz the power, 2016-11-08 15:46:01]

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, 2016-11-08 17:06:10]

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

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

Dry run: This issue passed the CQ dry run.

[please use gerrit instead, 2016-11-08 20:44:31]

lgtm

[zino, 2016-11-08 23:27:32]

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

[zino, 2016-11-08 23:27:32]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
avi@chromium.org, haraken@chromium.org, jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/2476343002/#ps100001
(title: "fix lint error")

[commit-bot: I haz the power, 2016-11-08 23:28:20]

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, 2016-11-08 23:36:49]

Description was changed from

==========
PaymentApp: Initial implementation for PaymentAppManager.setManifest().

This CL includes an initial implementation in blink side and browser side.
by using mojo IPC. Currently, when setManifest() is called, just sends
NOT_IMPLEMENTED in browser side and then rejects NotSupportedError in blink.

BUG=661608
TEST=payment-app-interfaces.html
TEST=payment-app-manager.html
==========

to

==========
PaymentApp: Initial implementation for PaymentAppManager.setManifest().

This CL includes an initial implementation in blink side and browser side.
by using mojo IPC. Currently, when setManifest() is called, just sends
NOT_IMPLEMENTED in browser side and then rejects NotSupportedError in blink.

BUG=661608
TEST=payment-app-interfaces.html
TEST=payment-app-manager.html
==========

[commit-bot: I haz the power, 2016-11-08 23:36:51]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-08 23:41:52]

Description was changed from

==========
PaymentApp: Initial implementation for PaymentAppManager.setManifest().

This CL includes an initial implementation in blink side and browser side.
by using mojo IPC. Currently, when setManifest() is called, just sends
NOT_IMPLEMENTED in browser side and then rejects NotSupportedError in blink.

BUG=661608
TEST=payment-app-interfaces.html
TEST=payment-app-manager.html
==========

to

==========
PaymentApp: Initial implementation for PaymentAppManager.setManifest().

This CL includes an initial implementation in blink side and browser side.
by using mojo IPC. Currently, when setManifest() is called, just sends
NOT_IMPLEMENTED in browser side and then rejects NotSupportedError in blink.

BUG=661608
TEST=payment-app-interfaces.html
TEST=payment-app-manager.html

Committed: https://crrev.com/ef456add5796bfdae498c3de4f4e8e2607d5173a
Cr-Commit-Position: refs/heads/master@{#430760}
==========

[commit-bot: I haz the power, 2016-11-08 23:41:53]

Patchset 6 (id:??) landed as
https://crrev.com/ef456add5796bfdae498c3de4f4e8e2607d5173a
Cr-Commit-Position: refs/heads/master@{#430760}