PaymentHandler: Implement GetAllPaymentApps().

PaymentHandler: Implement GetAllPaymentApps().

The method is used to query all stored payment app data in Chrome layer. In the
PaymentRequest UI, we can display all possible payment instruments associated
service worker.

This change will replace the legacy GetAllManifests() with new thing.

Also, this change exposes a new contnt public struct(StorePaymentInstrument)
to pass the stored payment instrument data to Chrome layer. The second CL of
the following links contains this usage of the public API.

See the other CLs in this series:
  [1/2] http://crrev.com/2873683002 (This patch)
  [2/2] http://crrev.com/2867273003 (Use GetAllPaymentApps() in android)

Design Doc:
  https://docs.google.com/document/d/1rWsvKQAwIboN2ZDuYYAkfce8GF27twi4UHTt0hcbyxQ/edit?usp=sharing

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

[zino, 2017-05-09 16:29:52]

Description was changed from

==========
PaymentHandler: Implement GetAllPaymentApps().

The method is used to query all stored payment app data in Chrome layer. In the
PaymentRequest UI, we can display all possible payment instruments associated
service worker.

This change will replace the legacy GetAllManifests() with new thing.

See the other CLs in this series:
  [1/3] http://crrev.com/ (This patch)
  [2/3] http://crrev.com/ (Use GetAllPaymentApps() in android)
  [3/3] http://crrev.com/ (Remove GetAllManifests())

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

to

==========
PaymentHandler: Implement GetAllPaymentApps().

The method is used to query all stored payment app data in Chrome layer. In the
PaymentRequest UI, we can display all possible payment instruments associated
service worker.

This change will replace the legacy GetAllManifests() with new thing.

See the other CLs in this series:
  [1/3] http://crrev.com/2867273003 (This patch)
  [2/3] http://crrev.com/ (Use GetAllPaymentApps() in android)
  [3/3] http://crrev.com/ (Remove GetAllManifests())

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

[zino, 2017-05-09 16:31:04]

Description was changed from

==========
PaymentHandler: Implement GetAllPaymentApps().

The method is used to query all stored payment app data in Chrome layer. In the
PaymentRequest UI, we can display all possible payment instruments associated
service worker.

This change will replace the legacy GetAllManifests() with new thing.

See the other CLs in this series:
  [1/3] http://crrev.com/2867273003 (This patch)
  [2/3] http://crrev.com/ (Use GetAllPaymentApps() in android)
  [3/3] http://crrev.com/ (Remove GetAllManifests())

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

to

==========
PaymentHandler: Implement GetAllPaymentApps().

The method is used to query all stored payment app data in Chrome layer. In the
PaymentRequest UI, we can display all possible payment instruments associated
service worker.

This change will replace the legacy GetAllManifests() with new thing.

See the other CLs in this series:
  [1/3] http://crrev.com/2873683002 (This patch)
  [2/3] http://crrev.com/2867273003 (Use GetAllPaymentApps() in android)
  [3/3] http://crrev.com/ (Remove GetAllManifests())

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

[zino, 2017-05-09 16:52:01]

Description was changed from

==========
PaymentHandler: Implement GetAllPaymentApps().

The method is used to query all stored payment app data in Chrome layer. In the
PaymentRequest UI, we can display all possible payment instruments associated
service worker.

This change will replace the legacy GetAllManifests() with new thing.

See the other CLs in this series:
  [1/3] http://crrev.com/2873683002 (This patch)
  [2/3] http://crrev.com/2867273003 (Use GetAllPaymentApps() in android)
  [3/3] http://crrev.com/ (Remove GetAllManifests())

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

to

==========
PaymentHandler: Implement GetAllPaymentApps().

The method is used to query all stored payment app data in Chrome layer. In the
PaymentRequest UI, we can display all possible payment instruments associated
service worker.

This change will replace the legacy GetAllManifests() with new thing.

See the other CLs in this series:
  [1/2] http://crrev.com/2873683002 (This patch)
  [2/2] http://crrev.com/2867273003 (Use GetAllPaymentApps() in android)

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

[zino, 2017-05-09 16:53:48]

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

[zino, 2017-05-09 16:54:51]

PTAL

rouslan@ for payments
Tom Sepez@ for mojom
nhiroki@ for service worker
nasko@ for content/public

Thanks.

[Tom Sepez, 2017-05-09 17:07:21]

https://codereview.chromium.org/2873683002/diff/20001/components/payments/moj...
File components/payments/mojom/payment_app.mojom (right):

https://codereview.chromium.org/2873683002/diff/20001/components/payments/moj...
components/payments/mojom/payment_app.mojom:40: int64 registration_id;
Where does this ID come from?  Is unique to a renderer?  Is it guessable?  If
some other renderer guesses this ID, what's the worst that can happen?

https://codereview.chromium.org/2873683002/diff/20001/content/browser/payment...
File content/browser/payments/payment_app.proto (right):

https://codereview.chromium.org/2873683002/diff/20001/content/browser/payment...
content/browser/payments/payment_app.proto:30: optional int64 registration_id =
1;
Breaking compatibility ?

[zino, 2017-05-09 17:16:57]

Thank you for review.

https://codereview.chromium.org/2873683002/diff/20001/components/payments/moj...
File components/payments/mojom/payment_app.mojom (right):

https://codereview.chromium.org/2873683002/diff/20001/components/payments/moj...
components/payments/mojom/payment_app.mojom:40: int64 registration_id;
On 2017/05/09 17:07:21, Tom Sepez wrote:
> Where does this ID come from?  Is unique to a renderer?  Is it guessable?  If
> some other renderer guesses this ID, what's the worst that can happen?

These three members are only used in browser side.
Is this usage restricted? (if do not allow this way, we will write a new
additional class?)
If not so, I leave a comment here like that:
"These three members are only used in browser side.. do not use these members in
renderer side blah blah.."

https://codereview.chromium.org/2873683002/diff/20001/content/browser/payment...
File content/browser/payments/payment_app.proto (right):

https://codereview.chromium.org/2873683002/diff/20001/content/browser/payment...
content/browser/payments/payment_app.proto:30: optional int64 registration_id =
1;
On 2017/05/09 17:07:21, Tom Sepez wrote:
> Breaking compatibility ?

This feature is still behind runtime flag. So, there is no problem I think.

[Tom Sepez, 2017-05-09 17:37:25]

https://codereview.chromium.org/2873683002/diff/20001/components/payments/moj...
File components/payments/mojom/payment_app.mojom (right):

https://codereview.chromium.org/2873683002/diff/20001/components/payments/moj...
components/payments/mojom/payment_app.mojom:40: int64 registration_id;
I'm not sure the comment is helpful.  I'm also not sure what you mean by "only
used browser side".  If so, why are they being passed across mojo?

[please use gerrit instead, 2017-05-09 18:01:47]

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

https://codereview.chromium.org/2873683002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_provider_impl_unittest.cc:18: using
payments::mojom::PaymentAppManifestPtr;
Please move these using statements below, so they are together with your new
using statements.

https://codereview.chromium.org/2873683002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/2873683002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:1065:
std::vector<std::string> splited_data = base::SplitString(
s/splited_data/parts/

https://codereview.chromium.org/2873683002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:1068:
DCHECK(splited_data.size() == 2);
Database can be corrupted on disk, so don't make assumptions via DCHECK(). Use
if statement to check whether the size is 2. If not, then skip over this piece
of data. Better yet, erase the corrupt data.

[nasko, 2017-05-09 22:46:44]

I'll stamp content/public once all other reviewers are happy.

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

https://codereview.chromium.org/2873683002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_provider_impl.cc:189:
BrowserContext::GetDefaultStoragePartition(browser_context));
Are payments supported in Chrome Apps? If yes, then this is going to break in
those cases.

[nhiroki, 2017-05-09 23:49:15]

Looks good. Added some comments.

https://codereview.chromium.org/2873683002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/2873683002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:1033:
ServiceWorkerDatabase::ReadUserDataForAllRegistrationsByKeyPrefix(
Can you add unittests for this API?

https://codereview.chromium.org/2873683002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:1034: const
std::string& user_data_name_prefix,
Can you explain key/value format that we're about to read? Are there design
document or impl comments about it? I'd like to understand database access
pattern.

https://codereview.chromium.org/2873683002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:1068:
DCHECK(splited_data.size() == 2);
On 2017/05/09 18:01:47, ಠ_ಠ wrote:
> Database can be corrupted on disk, so don't make assumptions via DCHECK(). Use
> if statement to check whether the size is 2. If not, then skip over this piece
> of data. Better yet, erase the corrupt data.

+1 to erase the corrupt data. Maybe it's like this?

  if (splited_data.size() != 2) {
    status = STATUS_ERROR_CORRUPTED;
    user_data->clear();
    break;
  }

HandleReadResult() disables the database and
ServiceWorkerStorage::DidGetUserDataForAllRegistrations() will start database
recreation.

[zino, 2017-05-10 15:57:43]

Thank you for review,

rouslan@, nhiroki@, nasko@,

I addressed your comments or left reply.

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

https://codereview.chromium.org/2873683002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_provider_impl.cc:189:
BrowserContext::GetDefaultStoragePartition(browser_context));
On 2017/05/09 22:46:44, nasko wrote:
> Are payments supported in Chrome Apps? If yes, then this is going to break in
> those cases.

This feature shouldn't be supported in Chrome Apps.
This Payment Handler is providing third-party payment processing to merchant
site.
When the Payment Request API is initiated in merchant site, the browser will
display the proper payment instruments and then invoke the payment instrument by
user interaction.
At that time, if the third-party payment app is Chrome App, then the payment app
can get a excessive permission.
So, it can't be Chrome App for security reason.
(In the past CL's comment: https://codereview.chromium.org/2586213002/#msg33)

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

https://codereview.chromium.org/2873683002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_provider_impl_unittest.cc:18: using
payments::mojom::PaymentAppManifestPtr;
On 2017/05/09 18:01:47, ಠ_ಠ wrote:
> Please move these using statements below, so they are together with your new
> using statements.

These will be removed here:
  - https://codereview.chromium.org/2875493003/

So, I don't think we need to modify these in this CL.

https://codereview.chromium.org/2873683002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/2873683002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:1033:
ServiceWorkerDatabase::ReadUserDataForAllRegistrationsByKeyPrefix(
On 2017/05/09 23:49:15, nhiroki wrote:
> Can you add unittests for this API?

Done.

https://codereview.chromium.org/2873683002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:1034: const
std::string& user_data_name_prefix,
On 2017/05/09 23:49:15, nhiroki wrote:
> Can you explain key/value format that we're about to read? Are there design
> document or impl comments about it? I'd like to understand database access
> pattern.

Unfortunately, there is no latest version of design document.
[1]
https://docs.google.com/document/d/1rWsvKQAwIboN2ZDuYYAkfce8GF27twi4UHTt0hcby...
[2]
https://drive.google.com/file/d/0B6zWycIQV6zHa2R2U3JfVkllc0U/view?usp=sharing

Instead I'll explain them here.

You are already very familiar with cache storage API in service worker.
The following code will store a response cache into disk cache.
   cache.put(request, response);

The |request| is key and the |response| is value.
The following code will store two request/response pairs into disk cache.
  assert request1.url == 'https://example.com/?abcd';
  assert request2.url == 'https://example.com/?defg';
  cache.put(request1, response1);
  cache.put(request2, response2);

We can use cache.match/matchAll() to query stored cache data.
  // The following statement means that key is 'https://example.com/?abcd'.
  assert request1.url == 'https://example.com/?abcd';

  var responses = await cache.matchAll(request1);
  assert responses.length == 1;
  assert responses[0] == response1;

Also, we can use |ignoreSearch| option with them.
  // The following statement means that key is 'https://example.com/?abcd'.
  assert request1.url == 'https://example.com/?abcd';

  // But matchAll() with ignoreSearch: true will ignore the query string in URL.
  // So, the key(prefix) is 'https://example.com/';
  // Then, matchAll() will resolve all responses that match the key prefix.
  var responses = await cache.matchAll(request1, { ignoreSearch: true });
  assert responses.length == 2;
  assert responses[0] == response1;
  assert responses[1] == response2;

We need a way to query all stored data that match with key prefix. (similar to
case of ignoreSearch)
For example, the following code stores instruments into service worker database.
  registration.paymentManager.instruments.set('abcd', instrument1);
  registration.paymentManager.instruments.set('efgh', instrument2);

When merchant site requests payment to browser via PaymentRequest API, then
the UA will display the all proper stored payment instruments by
GetAllPaymentApps() method. (this implementation)

At this point, the UA doesn't know which key is used by JS author.
(The key can be 'abcd' or 'efgh' or other things..)
So, we need this function for UA to query all data by key prefix.

The new additional unit test can be helpful for your understanding.

https://codereview.chromium.org/2873683002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:1065:
std::vector<std::string> splited_data = base::SplitString(
On 2017/05/09 18:01:47, ಠ_ಠ wrote:
> s/splited_data/parts/

Done.

https://codereview.chromium.org/2873683002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:1068:
DCHECK(splited_data.size() == 2);
On 2017/05/09 23:49:15, nhiroki wrote:
> On 2017/05/09 18:01:47, ಠ_ಠ wrote:
> > Database can be corrupted on disk, so don't make assumptions via DCHECK().
Use
> > if statement to check whether the size is 2. If not, then skip over this
piece
> > of data. Better yet, erase the corrupt data.
> 
> +1 to erase the corrupt data. Maybe it's like this?
> 
>   if (splited_data.size() != 2) {
>     status = STATUS_ERROR_CORRUPTED;
>     user_data->clear();
>     break;
>   }
> 
> HandleReadResult() disables the database and
> ServiceWorkerStorage::DidGetUserDataForAllRegistrations() will start database
> recreation.
> 

Done.

[zino, 2017-05-10 17:54:42]

Tom Sepez@,

Thank you for review. PTAL.

https://codereview.chromium.org/2873683002/diff/20001/components/payments/moj...
File components/payments/mojom/payment_app.mojom (right):

https://codereview.chromium.org/2873683002/diff/20001/components/payments/moj...
components/payments/mojom/payment_app.mojom:40: int64 registration_id;
On 2017/05/09 17:37:25, Tom Sepez wrote:
> I'm not sure the comment is helpful.  I'm also not sure what you mean by "only
> used browser side".  If so, why are they being passed across mojo?

I removed these members in mojom and then add a new struct in public directory.
(payment_instrument.h and .cc)

So, there is no mojom change in this patch.

[Tom Sepez, 2017-05-10 18:13:20]

On 2017/05/10 17:54:42, zino wrote:
> Tom Sepez@,
> 
> Thank you for review. PTAL.
> 
>
https://codereview.chromium.org/2873683002/diff/20001/components/payments/moj...
> File components/payments/mojom/payment_app.mojom (right):
> 
>
https://codereview.chromium.org/2873683002/diff/20001/components/payments/moj...
> components/payments/mojom/payment_app.mojom:40: int64 registration_id;
> On 2017/05/09 17:37:25, Tom Sepez wrote:
> > I'm not sure the comment is helpful.  I'm also not sure what you mean by
"only
> > used browser side".  If so, why are they being passed across mojo?
> 
> I removed these members in mojom and then add a new struct in public
directory.
> (payment_instrument.h and .cc)
> 
> So, there is no mojom change in this patch.
Ok, thanks, removing myself as reviewer.

[Tom Sepez, 2017-05-10 18:13:37]

tsepez@chromium.org changed reviewers:
- tsepez@chromium.org

[please use gerrit instead, 2017-05-10 18:14:36]

https://codereview.chromium.org/2873683002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/2873683002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:1034: const
std::string& user_data_name_prefix,
On 2017/05/10 15:57:43, zino wrote:
> On 2017/05/09 23:49:15, nhiroki wrote:
> > Can you explain key/value format that we're about to read? Are there design
> > document or impl comments about it? I'd like to understand database access
> > pattern.
> 
> Unfortunately, there is no latest version of design document.
> [1]
>
https://docs.google.com/document/d/1rWsvKQAwIboN2ZDuYYAkfce8GF27twi4UHTt0hcby...
> [2]
> https://drive.google.com/file/d/0B6zWycIQV6zHa2R2U3JfVkllc0U/view?usp=sharing
> 
> Instead I'll explain them here.
> 
> You are already very familiar with cache storage API in service worker.
> The following code will store a response cache into disk cache.
>    cache.put(request, response);
> 
> The |request| is key and the |response| is value.
> The following code will store two request/response pairs into disk cache.
>   assert request1.url == 'https://example.com/?abcd';
>   assert request2.url == 'https://example.com/?defg';
>   cache.put(request1, response1);
>   cache.put(request2, response2);
> 
> We can use cache.match/matchAll() to query stored cache data.
>   // The following statement means that key is 'https://example.com/?abcd'.
>   assert request1.url == 'https://example.com/?abcd';
> 
>   var responses = await cache.matchAll(request1);
>   assert responses.length == 1;
>   assert responses[0] == response1;
> 
> Also, we can use |ignoreSearch| option with them.
>   // The following statement means that key is 'https://example.com/?abcd'.
>   assert request1.url == 'https://example.com/?abcd';
> 
>   // But matchAll() with ignoreSearch: true will ignore the query string in
URL.
>   // So, the key(prefix) is 'https://example.com/';
>   // Then, matchAll() will resolve all responses that match the key prefix.
>   var responses = await cache.matchAll(request1, { ignoreSearch: true });
>   assert responses.length == 2;
>   assert responses[0] == response1;
>   assert responses[1] == response2;
> 
> We need a way to query all stored data that match with key prefix. (similar to
> case of ignoreSearch)
> For example, the following code stores instruments into service worker
database.
>   registration.paymentManager.instruments.set('abcd', instrument1);
>   registration.paymentManager.instruments.set('efgh', instrument2);
> 
> When merchant site requests payment to browser via PaymentRequest API, then
> the UA will display the all proper stored payment instruments by
> GetAllPaymentApps() method. (this implementation)
> 
> At this point, the UA doesn't know which key is used by JS author.
> (The key can be 'abcd' or 'efgh' or other things..)
> So, we need this function for UA to query all data by key prefix.
> 
> The new additional unit test can be helpful for your understanding.

Please put this information in the design doc. Link to the design doc from the
header file.

[zino, 2017-05-10 18:35:53]

Description was changed from

==========
PaymentHandler: Implement GetAllPaymentApps().

The method is used to query all stored payment app data in Chrome layer. In the
PaymentRequest UI, we can display all possible payment instruments associated
service worker.

This change will replace the legacy GetAllManifests() with new thing.

See the other CLs in this series:
  [1/2] http://crrev.com/2873683002 (This patch)
  [2/2] http://crrev.com/2867273003 (Use GetAllPaymentApps() in android)

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

to

==========
PaymentHandler: Implement GetAllPaymentApps().

The method is used to query all stored payment app data in Chrome layer. In the
PaymentRequest UI, we can display all possible payment instruments associated
service worker.

This change will replace the legacy GetAllManifests() with new thing.

See the other CLs in this series:
  [1/2] http://crrev.com/2873683002 (This patch)
  [2/2] http://crrev.com/2867273003 (Use GetAllPaymentApps() in android)

Design Doc:
 
https://docs.google.com/document/d/1rWsvKQAwIboN2ZDuYYAkfce8GF27twi4UHTt0hcby...

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

[zino, 2017-05-10 18:36:32]

https://codereview.chromium.org/2873683002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/2873683002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:1034: const
std::string& user_data_name_prefix,
On 2017/05/10 18:14:35, ಠ_ಠ wrote:
> On 2017/05/10 15:57:43, zino wrote:
> > On 2017/05/09 23:49:15, nhiroki wrote:
> > > Can you explain key/value format that we're about to read? Are there
design
> > > document or impl comments about it? I'd like to understand database access
> > > pattern.
> > 
> > Unfortunately, there is no latest version of design document.
> > [1]
> >
>
https://docs.google.com/document/d/1rWsvKQAwIboN2ZDuYYAkfce8GF27twi4UHTt0hcby...
> > [2]
> >
https://drive.google.com/file/d/0B6zWycIQV6zHa2R2U3JfVkllc0U/view?usp=sharing
> > 
> > Instead I'll explain them here.
> > 
> > You are already very familiar with cache storage API in service worker.
> > The following code will store a response cache into disk cache.
> >    cache.put(request, response);
> > 
> > The |request| is key and the |response| is value.
> > The following code will store two request/response pairs into disk cache.
> >   assert request1.url == 'https://example.com/?abcd';
> >   assert request2.url == 'https://example.com/?defg';
> >   cache.put(request1, response1);
> >   cache.put(request2, response2);
> > 
> > We can use cache.match/matchAll() to query stored cache data.
> >   // The following statement means that key is 'https://example.com/?abcd'.
> >   assert request1.url == 'https://example.com/?abcd';
> > 
> >   var responses = await cache.matchAll(request1);
> >   assert responses.length == 1;
> >   assert responses[0] == response1;
> > 
> > Also, we can use |ignoreSearch| option with them.
> >   // The following statement means that key is 'https://example.com/?abcd'.
> >   assert request1.url == 'https://example.com/?abcd';
> > 
> >   // But matchAll() with ignoreSearch: true will ignore the query string in
> URL.
> >   // So, the key(prefix) is 'https://example.com/';
> >   // Then, matchAll() will resolve all responses that match the key prefix.
> >   var responses = await cache.matchAll(request1, { ignoreSearch: true });
> >   assert responses.length == 2;
> >   assert responses[0] == response1;
> >   assert responses[1] == response2;
> > 
> > We need a way to query all stored data that match with key prefix. (similar
to
> > case of ignoreSearch)
> > For example, the following code stores instruments into service worker
> database.
> >   registration.paymentManager.instruments.set('abcd', instrument1);
> >   registration.paymentManager.instruments.set('efgh', instrument2);
> > 
> > When merchant site requests payment to browser via PaymentRequest API, then
> > the UA will display the all proper stored payment instruments by
> > GetAllPaymentApps() method. (this implementation)
> > 
> > At this point, the UA doesn't know which key is used by JS author.
> > (The key can be 'abcd' or 'efgh' or other things..)
> > So, we need this function for UA to query all data by key prefix.
> > 
> > The new additional unit test can be helpful for your understanding.
> 
> Please put this information in the design doc. Link to the design doc from the
> header file.

I don't disagree but I didn't find any past case in cs.chromium.org.
Is it usually recommended to put the design doc link to the header file?

If you don't mind, I'll add the link to this patch description. WDYT?

[rouslan, 2017-05-10 18:38:45]

On 2017/05/10 18:36:32, zino wrote:
> I don't disagree but I didn't find any past case in http://cs.chromium.org.
> Is it usually recommended to put the design doc link to the header file?

Only for sufficiently complex features.

> If you don't mind, I'll add the link to this patch description. WDYT?

That also works.

[rouslan, 2017-05-10 18:38:55]

Example:

https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

[zino, 2017-05-10 19:34:12]

rouslan@ PTAL

https://codereview.chromium.org/2873683002/diff/100001/content/public/browser...
File content/public/browser/payment_app_provider.h (right):

https://codereview.chromium.org/2873683002/diff/100001/content/public/browser...
content/public/browser/payment_app_provider.h:28: //  
https://docs.google.com/document/d/1rWsvKQAwIboN2ZDuYYAkfce8GF27twi4UHTt0hcby...
Done

[zino, 2017-05-10 19:34:28]

On 2017/05/10 18:38:55, rouslan wrote:
> Example:
> 
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

Thank you for this example.

[please use gerrit instead, 2017-05-10 19:49:17]

LGTM % comments

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

https://codereview.chromium.org/2873683002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_provider_impl_unittest.cc:18: using
payments::mojom::PaymentAppManifestPtr;
Move down?

https://codereview.chromium.org/2873683002/diff/100001/content/public/browser...
File content/public/browser/payment_instrument.cc (right):

https://codereview.chromium.org/2873683002/diff/100001/content/public/browser...
content/public/browser/payment_instrument.cc:9:
PaymentInstrument::PaymentInstrument() = default;
Please initialize the registration_id to 0. Otherwise it's random.

[zino, 2017-05-11 01:29:57]

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

https://codereview.chromium.org/2873683002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_provider_impl_unittest.cc:18: using
payments::mojom::PaymentAppManifestPtr;
On 2017/05/10 19:49:17, ಠ_ಠ wrote:
> Move down?
I already commented for this. :P
These will be removed here:
  - https://codereview.chromium.org/2875493003/

So, I don't think we need to modify these in this CL.

https://codereview.chromium.org/2873683002/diff/100001/content/public/browser...
File content/public/browser/payment_instrument.cc (right):

https://codereview.chromium.org/2873683002/diff/100001/content/public/browser...
content/public/browser/payment_instrument.cc:9:
PaymentInstrument::PaymentInstrument() = default;
On 2017/05/10 19:49:17, ಠ_ಠ wrote:
> Please initialize the registration_id to 0. Otherwise it's random.

Done.

[nhiroki, 2017-05-11 07:12:12]

On 2017/05/10 15:57:43, zino wrote:
>
https://codereview.chromium.org/2873683002/diff/20001/content/browser/service...
> content/browser/service_worker/service_worker_database.cc:1034: const
> std::string& user_data_name_prefix,
> On 2017/05/09 23:49:15, nhiroki wrote:
> > Can you explain key/value format that we're about to read? Are there design
> > document or impl comments about it? I'd like to understand database access
> > pattern.
> 
> Unfortunately, there is no latest version of design document.
> [1]
>
https://docs.google.com/document/d/1rWsvKQAwIboN2ZDuYYAkfce8GF27twi4UHTt0hcby...
> [2]
> https://drive.google.com/file/d/0B6zWycIQV6zHa2R2U3JfVkllc0U/view?usp=sharing
> 
> Instead I'll explain them here.
> 
> You are already very familiar with cache storage API in service worker.
> The following code will store a response cache into disk cache.
>    cache.put(request, response);
> 
> The |request| is key and the |response| is value.
> The following code will store two request/response pairs into disk cache.
>   assert request1.url == 'https://example.com/?abcd';
>   assert request2.url == 'https://example.com/?defg';
>   cache.put(request1, response1);
>   cache.put(request2, response2);
> 
> We can use cache.match/matchAll() to query stored cache data.
>   // The following statement means that key is 'https://example.com/?abcd'.
>   assert request1.url == 'https://example.com/?abcd';
> 
>   var responses = await cache.matchAll(request1);
>   assert responses.length == 1;
>   assert responses[0] == response1;
> 
> Also, we can use |ignoreSearch| option with them.
>   // The following statement means that key is 'https://example.com/?abcd'.
>   assert request1.url == 'https://example.com/?abcd';
> 
>   // But matchAll() with ignoreSearch: true will ignore the query string in
URL.
>   // So, the key(prefix) is 'https://example.com/';
>   // Then, matchAll() will resolve all responses that match the key prefix.
>   var responses = await cache.matchAll(request1, { ignoreSearch: true });
>   assert responses.length == 2;
>   assert responses[0] == response1;
>   assert responses[1] == response2;
> 
> We need a way to query all stored data that match with key prefix. (similar to
> case of ignoreSearch)
> For example, the following code stores instruments into service worker
database.
>   registration.paymentManager.instruments.set('abcd', instrument1);
>   registration.paymentManager.instruments.set('efgh', instrument2);
> 
> When merchant site requests payment to browser via PaymentRequest API, then
> the UA will display the all proper stored payment instruments by
> GetAllPaymentApps() method. (this implementation)
> 
> At this point, the UA doesn't know which key is used by JS author.
> (The key can be 'abcd' or 'efgh' or other things..)
> So, we need this function for UA to query all data by key prefix.
> 
> The new additional unit test can be helpful for your understanding.

Thank you for the clarification. I probably understand how this will work.

[nhiroki, 2017-05-11 07:12:40]

LGTM

https://codereview.chromium.org/2873683002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/2873683002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_database.cc:1073:
DCHECK(parts.size() == 2);
Can you remove this check because we already checked it at line 1068?

https://codereview.chromium.org/2873683002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_database_unittest.cc (right):

https://codereview.chromium.org/2873683002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_database_unittest.cc:1148:
{{"key_prefix:key2", "value2"}}));
Can you write one more user data that has a different key prefix and make sure
that's not read?

[zino, 2017-05-11 14:21:05]

Thank you for review.

nasko@ PTAL

https://codereview.chromium.org/2873683002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/2873683002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_database.cc:1073:
DCHECK(parts.size() == 2);
On 2017/05/11 07:12:40, nhiroki (ooo may 15 - 19) wrote:
> Can you remove this check because we already checked it at line 1068?

Done.

https://codereview.chromium.org/2873683002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_database_unittest.cc (right):

https://codereview.chromium.org/2873683002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_database_unittest.cc:1148:
{{"key_prefix:key2", "value2"}}));
On 2017/05/11 07:12:40, nhiroki (ooo may 15 - 19) wrote:
> Can you write one more user data that has a different key prefix and make sure
> that's not read?

Done.

[nasko, 2017-05-11 21:45:40]

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

https://codereview.chromium.org/2873683002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_provider_impl.cc:189:
BrowserContext::GetDefaultStoragePartition(browser_context));
On 2017/05/10 15:57:42, zino wrote:
> On 2017/05/09 22:46:44, nasko wrote:
> > Are payments supported in Chrome Apps? If yes, then this is going to break
in
> > those cases.
> 
> This feature shouldn't be supported in Chrome Apps.
> This Payment Handler is providing third-party payment processing to merchant
> site.
> When the Payment Request API is initiated in merchant site, the browser will
> display the proper payment instruments and then invoke the payment instrument
by
> user interaction.
> At that time, if the third-party payment app is Chrome App, then the payment
app
> can get a excessive permission.

If this is the case, do we block the API in the case of Chrome Apps? Do we have
a test to ensure we won't regress that?

> So, it can't be Chrome App for security reason.
> (In the past CL's comment: https://codereview.chromium.org/2586213002/#msg33)

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

https://codereview.chromium.org/2873683002/diff/150001/content/browser/paymen...
content/browser/payments/payment_app_database.cc:80: PaymentInstrumentPtr
DeserializePaymentInstrumentForMojo(
It is a bit strange to have the same type name defined differently for Mojo and
protobuf. I understand why the changes were removed from the Mojo struct and
kept in the content:: version, but maybe the name should also be changed to
reflect that.

nit: One usually deserializes "from" a source.

https://codereview.chromium.org/2873683002/diff/150001/content/public/browser...
File content/public/browser/payment_instrument.h (right):

https://codereview.chromium.org/2873683002/diff/150001/content/public/browser...
content/public/browser/payment_instrument.h:18: struct CONTENT_EXPORT
PaymentInstrument {
Since this struct is contained entirely inside content/, there is no reason to
expose it in the public API. Please move it to content/browser/payments/
subdirectory, since it is specific to that code.

[zino, 2017-05-12 20:59:21]

Thank you for review.

Could you review a new patch set again?

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

https://codereview.chromium.org/2873683002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_provider_impl.cc:189:
BrowserContext::GetDefaultStoragePartition(browser_context));
On 2017/05/11 21:45:39, nasko wrote:
> On 2017/05/10 15:57:42, zino wrote:
> > On 2017/05/09 22:46:44, nasko wrote:
> > > Are payments supported in Chrome Apps? If yes, then this is going to break
> in
> > > those cases.
> > 
> > This feature shouldn't be supported in Chrome Apps.
> > This Payment Handler is providing third-party payment processing to merchant
> > site.
> > When the Payment Request API is initiated in merchant site, the browser will
> > display the proper payment instruments and then invoke the payment
instrument
> by
> > user interaction.
> > At that time, if the third-party payment app is Chrome App, then the payment
> app
> > can get a excessive permission.
> 
> If this is the case, do we block the API in the case of Chrome Apps? Do we
have
> a test to ensure we won't regress that?
> 
> > So, it can't be Chrome App for security reason.
> > (In the past CL's comment:
https://codereview.chromium.org/2586213002/#msg33)

Oh, I thought that the service worker is only allowed in SecureContext but it's
not true.
Okay I'll follow-up this issue. I think we should block accessing the
PaymentManager in Extension side.
(FYI, the PaymentManager is an extension of SW registration like PushManager.
and it is used to store payment instruments.)

If you don't mind, can I fix this in separated CL?
The change should be in Blink and extension side(api test) as well as has a bit
different content with this patch's title.
I already filed a bug here:
https://bugs.chromium.org/p/chromium/issues/detail?id=721897

Anyway, this feature is still behind runtime flag. So, the PaymentManager is not
exposed.
Also, even if someone enables the feature and then store some instruments, this
patch ignores reading the data (using GetDefaultStoragePartition only).

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

https://codereview.chromium.org/2873683002/diff/150001/content/browser/paymen...
content/browser/payments/payment_app_database.cc:80: PaymentInstrumentPtr
DeserializePaymentInstrumentForMojo(
On 2017/05/11 21:45:39, nasko wrote:
> It is a bit strange to have the same type name defined differently for Mojo
and
> protobuf. I understand why the changes were removed from the Mojo struct and
> kept in the content:: version, but maybe the name should also be changed to
> reflect that.
> 
> nit: One usually deserializes "from" a source.

Done.

https://codereview.chromium.org/2873683002/diff/150001/content/public/browser...
File content/public/browser/payment_instrument.h (right):

https://codereview.chromium.org/2873683002/diff/150001/content/public/browser...
content/public/browser/payment_instrument.h:18: struct CONTENT_EXPORT
PaymentInstrument {
On 2017/05/11 21:45:39, nasko wrote:
> Since this struct is contained entirely inside content/, there is no reason to
> expose it in the public API. Please move it to content/browser/payments/
> subdirectory, since it is specific to that code.

The struct is used in chrome layer.
Once the payment request API is called from mechant site, browser should display
all payment instruments.
At that time, we can use this implementation(GetAllPaymentApps()). The
instrument data is passed to chrome side to display instruments on UI.

Looking into the following link (line number 46), you can see the use case.
https://codereview.chromium.org/2867273003/diff/60001/chrome/browser/android/...

[nasko, 2017-05-12 21:12:10]

LGTM with some nits.

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

https://codereview.chromium.org/2873683002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_provider_impl.cc:189:
BrowserContext::GetDefaultStoragePartition(browser_context));
On 2017/05/12 20:59:21, zino wrote:
> On 2017/05/11 21:45:39, nasko wrote:
> > On 2017/05/10 15:57:42, zino wrote:
> > > On 2017/05/09 22:46:44, nasko wrote:
> > > > Are payments supported in Chrome Apps? If yes, then this is going to
break
> > in
> > > > those cases.
> > > 
> > > This feature shouldn't be supported in Chrome Apps.
> > > This Payment Handler is providing third-party payment processing to
merchant
> > > site.
> > > When the Payment Request API is initiated in merchant site, the browser
will
> > > display the proper payment instruments and then invoke the payment
> instrument
> > by
> > > user interaction.
> > > At that time, if the third-party payment app is Chrome App, then the
payment
> > app
> > > can get a excessive permission.
> > 
> > If this is the case, do we block the API in the case of Chrome Apps? Do we
> have
> > a test to ensure we won't regress that?
> > 
> > > So, it can't be Chrome App for security reason.
> > > (In the past CL's comment:
> https://codereview.chromium.org/2586213002/#msg33)
> 
> Oh, I thought that the service worker is only allowed in SecureContext but
it's
> not true.

chrome-extension: is likely treated as a secure context.

> Okay I'll follow-up this issue. I think we should block accessing the
> PaymentManager in Extension side.
> (FYI, the PaymentManager is an extension of SW registration like PushManager.
> and it is used to store payment instruments.)
> 
> If you don't mind, can I fix this in separated CL?

Yes, separate CL is totally fine, and actually encouraged : ).

> The change should be in Blink and extension side(api test) as well as has a
bit
> different content with this patch's title.
> I already filed a bug here:
> https://bugs.chromium.org/p/chromium/issues/detail?id=721897

Thanks!

> Anyway, this feature is still behind runtime flag. So, the PaymentManager is
not
> exposed.
> Also, even if someone enables the feature and then store some instruments,
this
> patch ignores reading the data (using GetDefaultStoragePartition only).

https://codereview.chromium.org/2873683002/diff/150001/content/public/browser...
File content/public/browser/payment_instrument.h (right):

https://codereview.chromium.org/2873683002/diff/150001/content/public/browser...
content/public/browser/payment_instrument.h:18: struct CONTENT_EXPORT
PaymentInstrument {
On 2017/05/12 20:59:21, zino wrote:
> On 2017/05/11 21:45:39, nasko wrote:
> > Since this struct is contained entirely inside content/, there is no reason
to
> > expose it in the public API. Please move it to content/browser/payments/
> > subdirectory, since it is specific to that code.
> 
> The struct is used in chrome layer.
> Once the payment request API is called from mechant site, browser should
display
> all payment instruments.
> At that time, we can use this implementation(GetAllPaymentApps()). The
> instrument data is passed to chrome side to display instruments on UI.
> 
> Looking into the following link (line number 46), you can see the use case.
>
https://codereview.chromium.org/2867273003/diff/60001/chrome/browser/android/...

I don't see an include statement for that file there. It should be explicitly
included, not pulled in through another dependency.

The content/public/ API policy is that we don't expose it without its usage.
Please update the CL description to clarify that the follow up CL contains the
usage of the public API.

https://codereview.chromium.org/2873683002/diff/170001/content/public/browser...
File content/public/browser/installed_payment_instrument.h (right):

https://codereview.chromium.org/2873683002/diff/170001/content/public/browser...
content/public/browser/installed_payment_instrument.h:17: // This class
represents the stored payment instrument in UA.
nit: installed instead of stored?

[zino, 2017-05-12 21:32:19]

Description was changed from

==========
PaymentHandler: Implement GetAllPaymentApps().

The method is used to query all stored payment app data in Chrome layer. In the
PaymentRequest UI, we can display all possible payment instruments associated
service worker.

This change will replace the legacy GetAllManifests() with new thing.

See the other CLs in this series:
  [1/2] http://crrev.com/2873683002 (This patch)
  [2/2] http://crrev.com/2867273003 (Use GetAllPaymentApps() in android)

Design Doc:
 
https://docs.google.com/document/d/1rWsvKQAwIboN2ZDuYYAkfce8GF27twi4UHTt0hcby...

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

to

==========
PaymentHandler: Implement GetAllPaymentApps().

The method is used to query all stored payment app data in Chrome layer. In the
PaymentRequest UI, we can display all possible payment instruments associated
service worker.

This change will replace the legacy GetAllManifests() with new thing.

Also, this change exposes a new contnt public struct(StorePaymentInstrument)
to pass the stored payment instrument data to Chrome layer. The second CL of
the following links contains this usage of the public API.

See the other CLs in this series:
  [1/2] http://crrev.com/2873683002 (This patch)
  [2/2] http://crrev.com/2867273003 (Use GetAllPaymentApps() in android)

Design Doc:
 
https://docs.google.com/document/d/1rWsvKQAwIboN2ZDuYYAkfce8GF27twi4UHTt0hcby...

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

[zino, 2017-05-12 21:33:32]

Thank you for review!

I updated CL description and addressed nits.

https://codereview.chromium.org/2873683002/diff/170001/content/public/browser...
File content/public/browser/installed_payment_instrument.h (right):

https://codereview.chromium.org/2873683002/diff/170001/content/public/browser...
content/public/browser/installed_payment_instrument.h:17: // This class
represents the stored payment instrument in UA.
On 2017/05/12 21:12:10, nasko wrote:
> nit: installed instead of stored?

Done.

[zino, 2017-05-12 21:34:12]

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

[zino, 2017-05-12 21:34:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, nhiroki@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2873683002/#ps230001
(title: "installed -> stored in public")

[commit-bot: I haz the power, 2017-05-12 21:35:33]

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-05-13 02:52:52]

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

[commit-bot: I haz the power, 2017-05-13 02:52:53]

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, 2017-05-13 03:47:45]

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

[commit-bot: I haz the power, 2017-05-13 03:48:03]

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-05-13 07:55:16]

CQ is committing da patch.

Bot data: {"patchset_id": 230001, "attempt_start_ts": 1494647265743800,
"parent_rev": "14981e159fc950c0a628869e85c166c6c09809ca", "commit_rev":
"5f5a9b49ed920d6c4ca3760531a9f8fa842db2dc"}

[commit-bot: I haz the power, 2017-05-13 07:55:27]

Description was changed from

==========
PaymentHandler: Implement GetAllPaymentApps().

The method is used to query all stored payment app data in Chrome layer. In the
PaymentRequest UI, we can display all possible payment instruments associated
service worker.

This change will replace the legacy GetAllManifests() with new thing.

Also, this change exposes a new contnt public struct(StorePaymentInstrument)
to pass the stored payment instrument data to Chrome layer. The second CL of
the following links contains this usage of the public API.

See the other CLs in this series:
  [1/2] http://crrev.com/2873683002 (This patch)
  [2/2] http://crrev.com/2867273003 (Use GetAllPaymentApps() in android)

Design Doc:
 
https://docs.google.com/document/d/1rWsvKQAwIboN2ZDuYYAkfce8GF27twi4UHTt0hcby...

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

to

==========
PaymentHandler: Implement GetAllPaymentApps().

The method is used to query all stored payment app data in Chrome layer. In the
PaymentRequest UI, we can display all possible payment instruments associated
service worker.

This change will replace the legacy GetAllManifests() with new thing.

Also, this change exposes a new contnt public struct(StorePaymentInstrument)
to pass the stored payment instrument data to Chrome layer. The second CL of
the following links contains this usage of the public API.

See the other CLs in this series:
  [1/2] http://crrev.com/2873683002 (This patch)
  [2/2] http://crrev.com/2867273003 (Use GetAllPaymentApps() in android)

Design Doc:
 
https://docs.google.com/document/d/1rWsvKQAwIboN2ZDuYYAkfce8GF27twi4UHTt0hcby...

BUG=661608

Review-Url: https://codereview.chromium.org/2873683002
Cr-Commit-Position: refs/heads/master@{#471579}
Committed:
https://chromium.googlesource.com/chromium/src/+/5f5a9b49ed920d6c4ca3760531a9...
==========

[commit-bot: I haz the power, 2017-05-13 07:55:28]

Committed patchset #13 (id:230001) as
https://chromium.googlesource.com/chromium/src/+/5f5a9b49ed920d6c4ca3760531a9...