PaymentApp: Implement PaymentAppManager.setManifest().

PaymentApp: Implement PaymentAppManager.setManifest().

The method is to register payment app with the user agent for future use
associating manifest’s attributes(e.g. label and icon) set with the payment
app for user reference. The manifest data is stored into service worker
storage by associated service worker.

This is a series of patches to implement setManifest() in PaymentAppManager:
  https://codereview.chromium.org/2476343002/

This CL includes browser side implementation and unit/layout tests.

Related spec link:
  https://w3c.github.io/webpayments-payment-apps-api/#set-manifest

BUG=661608
TEST=payment_app_manager_unittest.cc
TEST=payment-app-manager.html

Committed: https://crrev.com/5c1f7a29337a6636a4329a9d16c4061bb51980c8
Cr-Commit-Position: refs/heads/master@{#433119}

[zino, 2016-11-14 16:45:56]

Description was changed from

==========
PaymentApp: Implement PaymentAppManager.setManifest().

This is a series of patches to implement setManifest() in PaymentAppManager:
  https://codereview.chromium.org/2476343002/

This CL includes browser side implementation and unit/layout tests.

Related spec link:
  https://w3c.github.io/webpayments-payment-apps-api/#set-manifest

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

to

==========
PaymentApp: Implement PaymentAppManager.setManifest().

This is a series of patches to implement setManifest() in PaymentAppManager:
  https://codereview.chromium.org/2476343002/

This CL includes browser side implementation and unit/layout tests.

Related spec link:
  https://w3c.github.io/webpayments-payment-apps-api/#set-manifest

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

[zino, 2016-11-14 16:46:25]

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

[commit-bot: I haz the power, 2016-11-14 16:46:45]

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-14 16:48:52]

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

[zino, 2016-11-14 16:50:34]

rouslan@ for payments
jkarlin@ for service worker and storage
Tom Sepez@ for mojom

PTAL

[zino, 2016-11-14 16:52:20]

Description was changed from

==========
PaymentApp: Implement PaymentAppManager.setManifest().

This is a series of patches to implement setManifest() in PaymentAppManager:
  https://codereview.chromium.org/2476343002/

This CL includes browser side implementation and unit/layout tests.

Related spec link:
  https://w3c.github.io/webpayments-payment-apps-api/#set-manifest

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

to

==========
PaymentApp: Implement PaymentAppManager.setManifest().

This is a series of patches to implement setManifest() in PaymentAppManager:
  https://codereview.chromium.org/2476343002/

This CL includes browser side implementation and unit/layout tests.

Related spec link:
  https://w3c.github.io/webpayments-payment-apps-api/#set-manifest

BUG=661608
TEST=payment_app_manager_unittest.cc
TEST=payment-app-manager.html
==========

[jkarlin, 2016-11-14 17:29:44]

Happy to look but note that I'm not an OWNER for either service_workers or
storage.

[zino, 2016-11-14 18:01:28]

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

[zino, 2016-11-14 18:03:07]

On 2016/11/14 17:29:44, jkarlin wrote:
> Happy to look but note that I'm not an OWNER for either service_workers or
> storage.

+nhiroki@ for service worker review. He is OWNER in service worker part.

[Tom Sepez, 2016-11-14 18:07:27]

mojom LGTM (adding error codes).

[commit-bot: I haz the power, 2016-11-14 18:16:43]

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

[commit-bot: I haz the power, 2016-11-14 18:16:44]

Dry run: This issue passed the CQ dry run.

[please use gerrit instead, 2016-11-14 20:44:00]

I am missing the big picture of where you're going with this patch. Can you
describe your approach either in CL description or the bug? It's hard to review
without knowing the large picture of where all the pieces fit.

[nhiroki, 2016-11-15 02:32:19]

serviceworker part looks good. Added some minor comments.

https://codereview.chromium.org/2497983002/diff/1/content/browser/payments/pa...
File content/browser/payments/payment_app_context.cc (right):

https://codereview.chromium.org/2497983002/diff/1/content/browser/payments/pa...
content/browser/payments/payment_app_context.cc:20: :
service_worker_context_(service_worker_context) {
std::move(service_worker_context) ?

https://codereview.chromium.org/2497983002/diff/1/content/browser/payments/pa...
File content/browser/payments/payment_app_context.h (right):

https://codereview.chromium.org/2497983002/diff/1/content/browser/payments/pa...
content/browser/payments/payment_app_context.h:24: PaymentAppContext(
explicit

https://codereview.chromium.org/2497983002/diff/1/content/browser/payments/pa...
File content/browser/payments/payment_app_manager.cc (right):

https://codereview.chromium.org/2497983002/diff/1/content/browser/payments/pa...
content/browser/payments/payment_app_manager.cc:20: 
nit: remove this blank line or add one after line 21.

https://codereview.chromium.org/2497983002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2497983002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_wrapper.cc:689:
callback.Run(SERVICE_WORKER_ERROR_ABORT, nullptr);
FindReadyRegistrationForPattern() should asynchronously run |callback| to align
with FindRegistrationForPattern() that always asynchronously runs a callback.

RunSoon(FROM_HERE, base::Bind(callback, ABORT, nullptr));

(See
https://cs.chromium.org/chromium/src/content/browser/service_worker/service_w...)

https://codereview.chromium.org/2497983002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_context_wrapper.h (right):

https://codereview.chromium.org/2497983002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_wrapper.h:147: void
FindReadyRegistrationForPattern(
It'd be nice if we have a detailed comment like other FindReady* functions (or
merge them) because ServiceWorkerContextWrapper is an interface between
ServiceWorker component and other components.

[zino, 2016-11-15 17:51:04]

Description was changed from

==========
PaymentApp: Implement PaymentAppManager.setManifest().

This is a series of patches to implement setManifest() in PaymentAppManager:
  https://codereview.chromium.org/2476343002/

This CL includes browser side implementation and unit/layout tests.

Related spec link:
  https://w3c.github.io/webpayments-payment-apps-api/#set-manifest

BUG=661608
TEST=payment_app_manager_unittest.cc
TEST=payment-app-manager.html
==========

to

==========
PaymentApp: Implement PaymentAppManager.setManifest().

The method is to register payment app with the user agent for future use,
associating manifest’s attributes(e.g. label and icon) set with the payment
app for user reference.

This is a series of patches to implement setManifest() in PaymentAppManager:
  https://codereview.chromium.org/2476343002/

This CL includes browser side implementation and unit/layout tests.

Related spec link:
  https://w3c.github.io/webpayments-payment-apps-api/#set-manifest

BUG=661608
TEST=payment_app_manager_unittest.cc
TEST=payment-app-manager.html
==========

[zino, 2016-11-15 17:57:38]

nhiroki@,

Thank you for review.

I addressed your comments.

https://codereview.chromium.org/2497983002/diff/1/content/browser/payments/pa...
File content/browser/payments/payment_app_context.cc (right):

https://codereview.chromium.org/2497983002/diff/1/content/browser/payments/pa...
content/browser/payments/payment_app_context.cc:20: :
service_worker_context_(service_worker_context) {
On 2016/11/15 02:32:18, nhiroki wrote:
> std::move(service_worker_context) ?

Done.

https://codereview.chromium.org/2497983002/diff/1/content/browser/payments/pa...
File content/browser/payments/payment_app_context.h (right):

https://codereview.chromium.org/2497983002/diff/1/content/browser/payments/pa...
content/browser/payments/payment_app_context.h:24: PaymentAppContext(
On 2016/11/15 02:32:18, nhiroki wrote:
> explicit

Done.

https://codereview.chromium.org/2497983002/diff/1/content/browser/payments/pa...
File content/browser/payments/payment_app_manager.cc (right):

https://codereview.chromium.org/2497983002/diff/1/content/browser/payments/pa...
content/browser/payments/payment_app_manager.cc:20: 
On 2016/11/15 02:32:19, nhiroki wrote:
> nit: remove this blank line or add one after line 21.

Done.

https://codereview.chromium.org/2497983002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2497983002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_wrapper.cc:689:
callback.Run(SERVICE_WORKER_ERROR_ABORT, nullptr);
On 2016/11/15 02:32:19, nhiroki wrote:
> FindReadyRegistrationForPattern() should asynchronously run |callback| to
align
> with FindRegistrationForPattern() that always asynchronously runs a callback.
> 
> RunSoon(FROM_HERE, base::Bind(callback, ABORT, nullptr));
> 
> (See
>
https://cs.chromium.org/chromium/src/content/browser/service_worker/service_w...)

Done.

https://codereview.chromium.org/2497983002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_context_wrapper.h (right):

https://codereview.chromium.org/2497983002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_wrapper.h:147: void
FindReadyRegistrationForPattern(
On 2016/11/15 02:32:19, nhiroki wrote:
> It'd be nice if we have a detailed comment like other FindReady* functions (or
> merge them) because ServiceWorkerContextWrapper is an interface between
> ServiceWorker component and other components.

Done.

[zino, 2016-11-15 18:00:16]

rouslan@,

I updated CL description and I wrote a simple design docs for it.
https://docs.google.com/document/d/1rWsvKQAwIboN2ZDuYYAkfce8GF27twi4UHTt0hcby...

Please take a look.

Thanks.

[zino, 2016-11-15 18:13:28]

Description was changed from

==========
PaymentApp: Implement PaymentAppManager.setManifest().

The method is to register payment app with the user agent for future use,
associating manifest’s attributes(e.g. label and icon) set with the payment
app for user reference.

This is a series of patches to implement setManifest() in PaymentAppManager:
  https://codereview.chromium.org/2476343002/

This CL includes browser side implementation and unit/layout tests.

Related spec link:
  https://w3c.github.io/webpayments-payment-apps-api/#set-manifest

BUG=661608
TEST=payment_app_manager_unittest.cc
TEST=payment-app-manager.html
==========

to

==========
PaymentApp: Implement PaymentAppManager.setManifest().

The method is to register payment app with the user agent for future use
associating manifest’s attributes(e.g. label and icon) set with the payment
app for user reference. The manifest data is stored into service worker
storage by associated service worker.

This is a series of patches to implement setManifest() in PaymentAppManager:
  https://codereview.chromium.org/2476343002/

This CL includes browser side implementation and unit/layout tests.

Related spec link:
  https://w3c.github.io/webpayments-payment-apps-api/#set-manifest

BUG=661608
TEST=payment_app_manager_unittest.cc
TEST=payment-app-manager.html
==========

[zino, 2016-11-15 18:13:59]

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

[zino, 2016-11-15 18:15:55]

+ Avi@ for BUILD.gn and content/browser/storage_partition_impl.*

PTAL

[Avi (use Gerrit), 2016-11-15 18:18:04]

lgtm

stampity stamp

[please use gerrit instead, 2016-11-15 20:04:27]

lgtm % comments

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
File content/browser/payments/payment_app_manager.cc (right):

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager.cc:18: 
Usually we don't put newlines between two nested namespace beginnings. 

namespace content {
namespace {

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager.cc:23: }
Anonymous namespace should end with "}  // namespace". A lonely "}" on its own
line is confusing to readers.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager.cc:54: // the payment app to be
registered.
All TODOs should have a bug link. The linked bug should keep track of work to
implement this TODO.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager.cc:112:
callback.Run(payments::mojom::PaymentAppManifestError::NONE);
Cleaner to use a single statement with a ternary conditional.

callback.Run(status ==  SERVICE_WORKER_OK ? 
payments::mojom::PaymentAppManifestError::NONE :
payments::mojom::PaymentAppManifestError::STORE_MANIFEST_FAILED);

You'd need to clang-format the line.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
File content/browser/payments/payment_app_manager_unittest.cc (right):

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:25: 
No newline.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:60: }
If you have a constructor, you should have a destructor. Note that this
destructor should have "override" keyword because you're deriving from
"testing::Test" class.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:62: void SetUp()
override {
SetUp is usually not used. You can put all these function calls in the
constructor.

https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:70: void TearDown()
override {
TearDown is usually not used. You can put all these function calls in the
destructor.

https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:73:
payment_app_context_ = nullptr;
Clearing the scoped_refptr should not be necessary immediately before its
destruction. (The googletest framework does not reuse instances of this class.)

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:93: new
EmbeddedWorkerTestHelper(base::FilePath()));
When a function is used only once, it's generally preferred to place its
contents directly at the call site (SetUp) instead. (I usually call this copying
"inlining", but it's not the same as the "inline" keyword, which is confusing,
sorry.)

Furthermore, if you follow my advice of initializing in the constructor instead
of the constructor, then you can initialize embedded_worker_helper_ even simpler
like so:

PaymentAppManagerTest()
    : thread_bundle_(
          new TestBrowserThreadBundle(TestBrowserThreadBundle::IO_MAINLOOP)),
      embedded_worker_helper_(new EmbeddedWorkerTestHelper(base::FilePath())) {}

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:102:
storage_partition_impl_.get());
Also can be "inlined".

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:107: new
PaymentAppContext(embedded_worker_helper_->context_wrapper());
Also can be "inlined".

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:116:
base::RunLoop().RunUntilIdle();
Is there no way to avoid running the loop?

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:117:
EXPECT_TRUE(called);
Also can be "inlined".

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:124:
base::RunLoop().RunUntilIdle();
Is there no way to avoid running the loop?

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:126:
EXPECT_TRUE(manager_);
Also can be "inlined".

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:135: PaymentAppManager*
manager_;
Add a note about ownership of this pointer. If you own it, then it should be
std::unique_ptr.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:136: };
DISALLOW_COPY_AND_ASSIGN is always a good idea.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:160:
EXPECT_EQ(data.size(), 1UL);
ASSERT_EQ

If the data size is not 1UL, you want to terminate the test, so use ASSERT_EQ
instead of EXPECT_EQ for this check. If you don't terminate the test, data[0] on
line 164 will crash it. Crashing tests are harder to debug.

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

https://codereview.chromium.org/2497983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentAppManager.cpp:108: default:
If PaymentAppManifestError is an enum, remove the default clause. This will make
compiler force you to handle all possible values of the enum, thus eliminating
hard-to-catch bugs.

[nhiroki, 2016-11-15 22:50:20]

SW lgtm

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

Thank you for review.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
File content/browser/payments/payment_app_manager.cc (right):

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager.cc:18: 
On 2016/11/15 20:04:26, rouslan wrote:
> Usually we don't put newlines between two nested namespace beginnings. 
> 
> namespace content {
> namespace {

Done.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager.cc:23: }
On 2016/11/15 20:04:26, rouslan wrote:
> Anonymous namespace should end with "}  // namespace". A lonely "}" on its own
> line is confusing to readers.

Done.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager.cc:54: // the payment app to be
registered.
On 2016/11/15 20:04:26, rouslan wrote:
> All TODOs should have a bug link. The linked bug should keep track of work to
> implement this TODO.

Done.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager.cc:112:
callback.Run(payments::mojom::PaymentAppManifestError::NONE);
On 2016/11/15 20:04:26, rouslan wrote:
> Cleaner to use a single statement with a ternary conditional.
> 
> callback.Run(status ==  SERVICE_WORKER_OK ? 
> payments::mojom::PaymentAppManifestError::NONE :
> payments::mojom::PaymentAppManifestError::STORE_MANIFEST_FAILED);
> 
> You'd need to clang-format the line.

Done.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
File content/browser/payments/payment_app_manager_unittest.cc (right):

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:25: 
On 2016/11/15 20:04:27, rouslan wrote:
> No newline.

Done.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:60: }
On 2016/11/15 20:04:27, rouslan wrote:
> If you have a constructor, you should have a destructor. Note that this
> destructor should have "override" keyword because you're deriving from
> "testing::Test" class.

Done.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:62: void SetUp()
override {
On 2016/11/15 20:04:27, rouslan wrote:
> SetUp is usually not used. You can put all these function calls in the
> constructor.
> 
>
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...

Done.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:70: void TearDown()
override {
On 2016/11/15 20:04:27, rouslan wrote:
> TearDown is usually not used. You can put all these function calls in the
> destructor.
> 
>
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...

Done.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:73:
payment_app_context_ = nullptr;
On 2016/11/15 20:04:27, rouslan wrote:
> Clearing the scoped_refptr should not be necessary immediately before its
> destruction. (The googletest framework does not reuse instances of this
class.)

Done.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:93: new
EmbeddedWorkerTestHelper(base::FilePath()));
On 2016/11/15 20:04:27, rouslan wrote:
> When a function is used only once, it's generally preferred to place its
> contents directly at the call site (SetUp) instead. (I usually call this
copying
> "inlining", but it's not the same as the "inline" keyword, which is confusing,
> sorry.)
> 
> Furthermore, if you follow my advice of initializing in the constructor
instead
> of the constructor, then you can initialize embedded_worker_helper_ even
simpler
> like so:
> 
> PaymentAppManagerTest()
>     : thread_bundle_(
>           new TestBrowserThreadBundle(TestBrowserThreadBundle::IO_MAINLOOP)),
>       embedded_worker_helper_(new EmbeddedWorkerTestHelper(base::FilePath()))
{}

Done.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:102:
storage_partition_impl_.get());
On 2016/11/15 20:04:27, rouslan wrote:
> Also can be "inlined".

Done.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:107: new
PaymentAppContext(embedded_worker_helper_->context_wrapper());
On 2016/11/15 20:04:27, rouslan wrote:
> Also can be "inlined".

Done.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:116:
base::RunLoop().RunUntilIdle();
On 2016/11/15 20:04:27, rouslan wrote:
> Is there no way to avoid running the loop?

The RegisterServiceWorker() will run asynchronously. So, this is needed to make
this test easier.
Background Sync and Many Service Worker tests are already using it.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:117:
EXPECT_TRUE(called);
On 2016/11/15 20:04:27, rouslan wrote:
> Also can be "inlined".

Done.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:124:
base::RunLoop().RunUntilIdle();
On 2016/11/15 20:04:27, rouslan wrote:
> Is there no way to avoid running the loop?

ditto

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:126:
EXPECT_TRUE(manager_);
On 2016/11/15 20:04:27, rouslan wrote:
> Also can be "inlined".

Done.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:135: PaymentAppManager*
manager_;
On 2016/11/15 20:04:26, rouslan wrote:
> Add a note about ownership of this pointer. If you own it, then it should be
> std::unique_ptr.

This class doesn't have the ownership of this pointer. PaymentAppContext owns
this.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:136: };
On 2016/11/15 20:04:26, rouslan wrote:
> DISALLOW_COPY_AND_ASSIGN is always a good idea.

Done.

https://codereview.chromium.org/2497983002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:160:
EXPECT_EQ(data.size(), 1UL);
On 2016/11/15 20:04:27, rouslan wrote:
> ASSERT_EQ
> 
> If the data size is not 1UL, you want to terminate the test, so use ASSERT_EQ
> instead of EXPECT_EQ for this check. If you don't terminate the test, data[0]
on
> line 164 will crash it. Crashing tests are harder to debug.

Done.

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

https://codereview.chromium.org/2497983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentAppManager.cpp:108: default:
On 2016/11/15 20:04:27, rouslan wrote:
> If PaymentAppManifestError is an enum, remove the default clause. This will
make
> compiler force you to handle all possible values of the enum, thus eliminating
> hard-to-catch bugs.

Done.

[zino, 2016-11-16 18:19:29]

rouslan@,

I left two comments. If you agree, I'll land this patch :)

Thanks.

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

Please add that comment and submit the patch.

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

https://codereview.chromium.org/2497983002/diff/40001/content/browser/payment...
content/browser/payments/payment_app_manager_unittest.cc:114: PaymentAppManager*
manager_;
Please add a comment here for ownership of |manager_|. For example:

// Owned by payment_app_context_.

[zino, 2016-11-16 18:35:12]

On 2016/11/16 18:22:05, rouslan wrote:
> Please add that comment and submit the patch.
> 
>
https://codereview.chromium.org/2497983002/diff/40001/content/browser/payment...
> File content/browser/payments/payment_app_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/2497983002/diff/40001/content/browser/payment...
> content/browser/payments/payment_app_manager_unittest.cc:114:
PaymentAppManager*
> manager_;
> Please add a comment here for ownership of |manager_|. For example:
> 
> // Owned by payment_app_context_.

Done

Thanks.

[zino, 2016-11-16 18:35:18]

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

[zino, 2016-11-16 18:35:18]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
rouslan@chromium.org, avi@chromium.org, nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/2497983002/#ps60001
(title: "PaymentApp: Implement PaymentAppManager.setManifest().")

[commit-bot: I haz the power, 2016-11-16 18:36:01]

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-16 19:33:56]

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

[commit-bot: I haz the power, 2016-11-16 19:33:58]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zino, 2016-11-17 00:01:40]

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

[commit-bot: I haz the power, 2016-11-17 00:03:16]

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-17 02:59:26]

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

[commit-bot: I haz the power, 2016-11-17 02:59:32]

Failed to apply patch for content/browser/BUILD.gn:
While running git apply --index -p1;
  error: patch failed: content/browser/BUILD.gn:56
  error: content/browser/BUILD.gn: patch does not apply

Patch:       content/browser/BUILD.gn
Index: content/browser/BUILD.gn
diff --git a/content/browser/BUILD.gn b/content/browser/BUILD.gn
index
0248825a887d7457aff6fdd0a6bfc1b3ec6cfb70..2c6ffe00ba49294fef24d169ecfcdefb58bd6e5d
100644
--- a/content/browser/BUILD.gn
+++ b/content/browser/BUILD.gn
@@ -56,6 +56,7 @@ source_set("browser") {
     "//content/browser/devtools:gen_devtools_protocol_handler",
     "//content/browser/devtools:resources",
     "//content/browser/notifications:notification_proto",
+    "//content/browser/payments:payment_app_proto",
     "//content/browser/service_worker:service_worker_proto",
     "//content/browser/speech/proto",
     "//content/common",

[zino, 2016-11-17 19:42:23]

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

[zino, 2016-11-17 19:42:24]

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

[commit-bot: I haz the power, 2016-11-17 19:43:24]

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-18 00:29:53]

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

[commit-bot: I haz the power, 2016-11-18 00:29:55]

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

[zino, 2016-11-18 01:17:30]

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

[commit-bot: I haz the power, 2016-11-18 01:18: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, 2016-11-18 05:30:06]

Description was changed from

==========
PaymentApp: Implement PaymentAppManager.setManifest().

The method is to register payment app with the user agent for future use
associating manifest’s attributes(e.g. label and icon) set with the payment
app for user reference. The manifest data is stored into service worker
storage by associated service worker.

This is a series of patches to implement setManifest() in PaymentAppManager:
  https://codereview.chromium.org/2476343002/

This CL includes browser side implementation and unit/layout tests.

Related spec link:
  https://w3c.github.io/webpayments-payment-apps-api/#set-manifest

BUG=661608
TEST=payment_app_manager_unittest.cc
TEST=payment-app-manager.html
==========

to

==========
PaymentApp: Implement PaymentAppManager.setManifest().

The method is to register payment app with the user agent for future use
associating manifest’s attributes(e.g. label and icon) set with the payment
app for user reference. The manifest data is stored into service worker
storage by associated service worker.

This is a series of patches to implement setManifest() in PaymentAppManager:
  https://codereview.chromium.org/2476343002/

This CL includes browser side implementation and unit/layout tests.

Related spec link:
  https://w3c.github.io/webpayments-payment-apps-api/#set-manifest

BUG=661608
TEST=payment_app_manager_unittest.cc
TEST=payment-app-manager.html
==========

[commit-bot: I haz the power, 2016-11-18 05:30:07]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-18 05:41:33]

Description was changed from

==========
PaymentApp: Implement PaymentAppManager.setManifest().

The method is to register payment app with the user agent for future use
associating manifest’s attributes(e.g. label and icon) set with the payment
app for user reference. The manifest data is stored into service worker
storage by associated service worker.

This is a series of patches to implement setManifest() in PaymentAppManager:
  https://codereview.chromium.org/2476343002/

This CL includes browser side implementation and unit/layout tests.

Related spec link:
  https://w3c.github.io/webpayments-payment-apps-api/#set-manifest

BUG=661608
TEST=payment_app_manager_unittest.cc
TEST=payment-app-manager.html
==========

to

==========
PaymentApp: Implement PaymentAppManager.setManifest().

The method is to register payment app with the user agent for future use
associating manifest’s attributes(e.g. label and icon) set with the payment
app for user reference. The manifest data is stored into service worker
storage by associated service worker.

This is a series of patches to implement setManifest() in PaymentAppManager:
  https://codereview.chromium.org/2476343002/

This CL includes browser side implementation and unit/layout tests.

Related spec link:
  https://w3c.github.io/webpayments-payment-apps-api/#set-manifest

BUG=661608
TEST=payment_app_manager_unittest.cc
TEST=payment-app-manager.html

Committed: https://crrev.com/5c1f7a29337a6636a4329a9d16c4061bb51980c8
Cr-Commit-Position: refs/heads/master@{#433119}
==========

[commit-bot: I haz the power, 2016-11-18 05:41:34]

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