PaymentApp: Introduce PaymentAppDatabase class.

PaymentApp: Introduce PaymentAppDatabase class.

The class is providing APIs to read/write payment app related data(e.g. manifest
and associated service worker). It can be shared between payment_app_manager.cc
and payment_app_context.cc.

The payment_app_manager.cc contains a actual implementation of the
PaymentAppManager interface[1] used on the page or worker.
Also, the payment_app_context.cc can be used to query all manifests data[2]
in order to update payment request UI in Chrome layer.

We can summarize this CL as follows.
  1. Rename PaymentAppContext in content/browser/payments to
     PaymentAppContextImpl and then add PaymentAppContext interface in
     content/public to expose some APIs to Chrome layer.
  2. Write PaymentAppDatabase class and then move most of logics to access
     database from PaymentAppManager to the new class.
  3. Add a interface to query all manifests data in Chrome layer but it's not
     implemented yet in this CL. It will be implemented in follow-up CL[2].

[1] https://w3c.github.io/webpayments-payment-apps-api/#payment-app-manager
[2] https://codereview.chromium.org/2556433002

BUG=661608
TEST=existing unittests

Committed: https://crrev.com/3b0d3c1f7b4ab17b5abb62b6767615e5ca5a4eb1
Cr-Commit-Position: refs/heads/master@{#437855}

[zino, 2016-12-09 20:17:57]

Description was changed from

==========
PaymentApp: Introduce PaymentAppDatabase class.

The class is providing APIs to read/write payment app related data(e.g. manifest
and associated service worker). It can be shared between payment_app_manager.cc
and payment_app_context.cc.

The payment_app_manager.cc contains a actual implementation of the
PaymentAppManager interface[1] used on the page or worker.
Also, the payment_app_context.cc can be used to query all manifests data[2]
in order to update payment request UI in Chrome layer.

We can summarize this CL as follows.
  1. Rename PaymentAppContext in content/browser/payments to
     PaymentAppContextImpl and then add PaymentAppContext interface in
     content/public to expose some APIs to Chrome layer.
  2. Write PaymentAppDatabase class and then move most of logics to access
     database from PaymentAppManager to the new class.
  3. Add a interface to query all manifests data in Chrome layer but it's not
     implemented yet in this CL. It will be implemented in follow-up CL[2].

[1] https://w3c.github.io/webpayments-payment-apps-api/#payment-app-manager
[2] https://codereview.chromium.org/12345

BUG=661608
TEST=existing unittests
==========

to

==========
PaymentApp: Introduce PaymentAppDatabase class.

The class is providing APIs to read/write payment app related data(e.g. manifest
and associated service worker). It can be shared between payment_app_manager.cc
and payment_app_context.cc.

The payment_app_manager.cc contains a actual implementation of the
PaymentAppManager interface[1] used on the page or worker.
Also, the payment_app_context.cc can be used to query all manifests data[2]
in order to update payment request UI in Chrome layer.

We can summarize this CL as follows.
  1. Rename PaymentAppContext in content/browser/payments to
     PaymentAppContextImpl and then add PaymentAppContext interface in
     content/public to expose some APIs to Chrome layer.
  2. Write PaymentAppDatabase class and then move most of logics to access
     database from PaymentAppManager to the new class.
  3. Add a interface to query all manifests data in Chrome layer but it's not
     implemented yet in this CL. It will be implemented in follow-up CL[2].

[1] https://w3c.github.io/webpayments-payment-apps-api/#payment-app-manager
[2] https://codereview.chromium.org/12345

BUG=661608
TEST=existing unittests
==========

[zino, 2016-12-09 20:17:57]

jinho.bang@samsung.com changed reviewers:
+ avi@chromium.org, rouslan@chromium.org, tommyt@opera.com

[zino, 2016-12-09 20:18:41]

Description was changed from

==========
PaymentApp: Introduce PaymentAppDatabase class.

The class is providing APIs to read/write payment app related data(e.g. manifest
and associated service worker). It can be shared between payment_app_manager.cc
and payment_app_context.cc.

The payment_app_manager.cc contains a actual implementation of the
PaymentAppManager interface[1] used on the page or worker.
Also, the payment_app_context.cc can be used to query all manifests data[2]
in order to update payment request UI in Chrome layer.

We can summarize this CL as follows.
  1. Rename PaymentAppContext in content/browser/payments to
     PaymentAppContextImpl and then add PaymentAppContext interface in
     content/public to expose some APIs to Chrome layer.
  2. Write PaymentAppDatabase class and then move most of logics to access
     database from PaymentAppManager to the new class.
  3. Add a interface to query all manifests data in Chrome layer but it's not
     implemented yet in this CL. It will be implemented in follow-up CL[2].

[1] https://w3c.github.io/webpayments-payment-apps-api/#payment-app-manager
[2] https://codereview.chromium.org/12345

BUG=661608
TEST=existing unittests
==========

to

==========
PaymentApp: Introduce PaymentAppDatabase class.

The class is providing APIs to read/write payment app related data(e.g. manifest
and associated service worker). It can be shared between payment_app_manager.cc
and payment_app_context.cc.

The payment_app_manager.cc contains a actual implementation of the
PaymentAppManager interface[1] used on the page or worker.
Also, the payment_app_context.cc can be used to query all manifests data[2]
in order to update payment request UI in Chrome layer.

We can summarize this CL as follows.
  1. Rename PaymentAppContext in content/browser/payments to
     PaymentAppContextImpl and then add PaymentAppContext interface in
     content/public to expose some APIs to Chrome layer.
  2. Write PaymentAppDatabase class and then move most of logics to access
     database from PaymentAppManager to the new class.
  3. Add a interface to query all manifests data in Chrome layer but it's not
     implemented yet in this CL. It will be implemented in follow-up CL[2].

[1] https://w3c.github.io/webpayments-payment-apps-api/#payment-app-manager
[2] https://codereview.chromium.org/2556433002

BUG=661608
TEST=existing unittests
==========

[zino, 2016-12-09 20:20:26]

PTAL

+ rouslan@ (and tommyt@) for content/browser/payments
+ Avi@ for other parts of content directory.

[Avi (use Gerrit), 2016-12-09 21:30:49]

non-payment content stuff lgtm

[please use gerrit instead, 2016-12-09 21:58:47]

lgtm

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

https://codereview.chromium.org/2560293002/diff/1/content/browser/payments/pa...
content/browser/payments/payment_app_database.h:36: void WriteManifest(const
std::string& scope,
Can we use GURL for scope?

https://codereview.chromium.org/2560293002/diff/1/content/public/browser/paym...
File content/public/browser/payment_app_context.h (right):

https://codereview.chromium.org/2560293002/diff/1/content/public/browser/paym...
content/public/browser/payment_app_context.h:17: using GetAllManifestsCallback =
base::Callback<void(
Please add a comment about the meaning of this data type. A reader would have
hard time understanding what the int64_t is doing here.

[zino, 2016-12-09 22:30:17]

Thank you for review.

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

https://codereview.chromium.org/2560293002/diff/1/content/browser/payments/pa...
content/browser/payments/payment_app_database.h:36: void WriteManifest(const
std::string& scope,
On 2016/12/09 21:58:47, rouslan wrote:
> Can we use GURL for scope?

Done.

https://codereview.chromium.org/2560293002/diff/1/content/public/browser/paym...
File content/public/browser/payment_app_context.h (right):

https://codereview.chromium.org/2560293002/diff/1/content/public/browser/paym...
content/public/browser/payment_app_context.h:17: using GetAllManifestsCallback =
base::Callback<void(
On 2016/12/09 21:58:47, rouslan wrote:
> Please add a comment about the meaning of this data type. A reader would have
> hard time understanding what the int64_t is doing here.

I agree with you.
So, I split it into subtypes and left a comment.

Done

[zino, 2016-12-09 22:34:35]

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

[commit-bot: I haz the power, 2016-12-09 22:35:37]

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

[please use gerrit instead, 2016-12-09 23:56:30]

https://codereview.chromium.org/2560293002/diff/20001/content/public/browser/...
File content/public/browser/payment_app_context.h (right):

https://codereview.chromium.org/2560293002/diff/20001/content/public/browser/...
content/public/browser/payment_app_context.h:20: std::pair<int64_t,
payments::mojom::PaymentAppManifestPtr>;
tommyt@'s implementation of PaymentAppManifest uses scopeUrl to identify payment
apps. You two should reconcile this difference ;-)

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

[zino, 2016-12-10 00:19:58]

tommyt@,

Could you please check my comment again?

https://codereview.chromium.org/2560293002/diff/20001/content/public/browser/...
File content/public/browser/payment_app_context.h (right):

https://codereview.chromium.org/2560293002/diff/20001/content/public/browser/...
content/public/browser/payment_app_context.h:20: std::pair<int64_t,
payments::mojom::PaymentAppManifestPtr>;
On 2016/12/09 23:56:30, rouslan wrote:
> tommyt@'s implementation of PaymentAppManifest uses scopeUrl to identify
payment
> apps. You two should reconcile this difference ;-)
> 
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

Sure,

As I remember, we already discussed about it. :)

To clarify, I left this comment.

I think it's favorable to use registration_id.
Because most of service worker APIs in content layer takes registration id
instead of scope_url. We can use scope_url but we will have to find
a SW registration with scope_url once more to find the registration id.

If some API is initiated in blink side, scope_url can be necessary because
the registration id is generated in content/child.
(e.g. paymentManager.setManifest())

On the other hands, some API is initiated in chromium side,
registration_id is enough. (e.g. invokePaymentApp())

[commit-bot: I haz the power, 2016-12-10 00:23:29]

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

[commit-bot: I haz the power, 2016-12-10 00:23:30]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[zino, 2016-12-10 07:18:03]

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

[commit-bot: I haz the power, 2016-12-10 07:18:17]

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-12-10 08:01:54]

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

[commit-bot: I haz the power, 2016-12-10 08:01:54]

Dry run: 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-12-10 08:27:33]

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

[commit-bot: I haz the power, 2016-12-10 08:27: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-12-10 09:12:15]

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

[commit-bot: I haz the power, 2016-12-10 09:12:16]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tommyt, 2016-12-10 10:04:47]

lgtm

https://codereview.chromium.org/2560293002/diff/20001/content/public/browser/...
File content/public/browser/payment_app_context.h (right):

https://codereview.chromium.org/2560293002/diff/20001/content/public/browser/...
content/public/browser/payment_app_context.h:20: std::pair<int64_t,
payments::mojom::PaymentAppManifestPtr>;
On 2016/12/10 00:19:58, zino wrote:
> On 2016/12/09 23:56:30, rouslan wrote:
> > tommyt@'s implementation of PaymentAppManifest uses scopeUrl to identify
> payment
> > apps. You two should reconcile this difference ;-)
> > 
> >
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> 
> Sure,
> 
> As I remember, we already discussed about it. :)
> 
> To clarify, I left this comment.
> 
> I think it's favorable to use registration_id.
> Because most of service worker APIs in content layer takes registration id
> instead of scope_url. We can use scope_url but we will have to find
> a SW registration with scope_url once more to find the registration id.
> 
> If some API is initiated in blink side, scope_url can be necessary because
> the registration id is generated in content/child.
> (e.g. paymentManager.setManifest())
> 
> On the other hands, some API is initiated in chromium side,
> registration_id is enough. (e.g. invokePaymentApp())

I'm fine with switching to int64_t, and I even have a WIP change that does that.
There is no known requirement for knowing this scopeUrl in the UI, so there is
no particular reason to keep it there.

[zino, 2016-12-10 12:57:24]

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

[commit-bot: I haz the power, 2016-12-10 12:57:31]

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-12-10 13:46:03]

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

[commit-bot: I haz the power, 2016-12-10 13:46:04]

Dry run: 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-12-10 15:43:46]

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

[commit-bot: I haz the power, 2016-12-10 15:43:59]

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-12-10 15:54:47]

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

[commit-bot: I haz the power, 2016-12-10 15:54:48]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[zino, 2016-12-10 17:57:20]

Patchset #3 (id:40001) has been deleted

[zino, 2016-12-10 18:03:50]

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

[commit-bot: I haz the power, 2016-12-10 18:04:00]

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

[zino, 2016-12-10 18:09:59]

Patchset #3 (id:60001) has been deleted

[zino, 2016-12-10 18:13:29]

Patchset #3 (id:80001) has been deleted

[zino, 2016-12-10 18:13:42]

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

[commit-bot: I haz the power, 2016-12-10 18:13:50]

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-12-10 19:56:41]

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

[commit-bot: I haz the power, 2016-12-10 19:56:42]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[zino, 2016-12-11 18:28:22]

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

[zino, 2016-12-11 18:28:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, avi@chromium.org, tommyt@opera.com
Link to the patchset: https://codereview.chromium.org/2560293002/#ps100001
(title: "PaymentApp: Introduce PaymentAppDatabase class.")

[commit-bot: I haz the power, 2016-12-11 18:28:38]

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-12-11 20:22:12]

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

[commit-bot: I haz the power, 2016-12-11 20:22:16]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[zino, 2016-12-12 12:41:39]

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

[commit-bot: I haz the power, 2016-12-12 12:41:53]

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-12-12 12:45:43]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481546499921050,
"parent_rev": "bb98e5fa32592f4aa82692370ff06f548750dde8", "commit_rev":
"dd5116124a08ed4a13a53ff2f75a8c61788e592d"}

[commit-bot: I haz the power, 2016-12-12 12:46:11]

Description was changed from

==========
PaymentApp: Introduce PaymentAppDatabase class.

The class is providing APIs to read/write payment app related data(e.g. manifest
and associated service worker). It can be shared between payment_app_manager.cc
and payment_app_context.cc.

The payment_app_manager.cc contains a actual implementation of the
PaymentAppManager interface[1] used on the page or worker.
Also, the payment_app_context.cc can be used to query all manifests data[2]
in order to update payment request UI in Chrome layer.

We can summarize this CL as follows.
  1. Rename PaymentAppContext in content/browser/payments to
     PaymentAppContextImpl and then add PaymentAppContext interface in
     content/public to expose some APIs to Chrome layer.
  2. Write PaymentAppDatabase class and then move most of logics to access
     database from PaymentAppManager to the new class.
  3. Add a interface to query all manifests data in Chrome layer but it's not
     implemented yet in this CL. It will be implemented in follow-up CL[2].

[1] https://w3c.github.io/webpayments-payment-apps-api/#payment-app-manager
[2] https://codereview.chromium.org/2556433002

BUG=661608
TEST=existing unittests
==========

to

==========
PaymentApp: Introduce PaymentAppDatabase class.

The class is providing APIs to read/write payment app related data(e.g. manifest
and associated service worker). It can be shared between payment_app_manager.cc
and payment_app_context.cc.

The payment_app_manager.cc contains a actual implementation of the
PaymentAppManager interface[1] used on the page or worker.
Also, the payment_app_context.cc can be used to query all manifests data[2]
in order to update payment request UI in Chrome layer.

We can summarize this CL as follows.
  1. Rename PaymentAppContext in content/browser/payments to
     PaymentAppContextImpl and then add PaymentAppContext interface in
     content/public to expose some APIs to Chrome layer.
  2. Write PaymentAppDatabase class and then move most of logics to access
     database from PaymentAppManager to the new class.
  3. Add a interface to query all manifests data in Chrome layer but it's not
     implemented yet in this CL. It will be implemented in follow-up CL[2].

[1] https://w3c.github.io/webpayments-payment-apps-api/#payment-app-manager
[2] https://codereview.chromium.org/2556433002

BUG=661608
TEST=existing unittests

Review-Url: https://codereview.chromium.org/2560293002
==========

[commit-bot: I haz the power, 2016-12-12 12:46:12]

Committed patchset #3 (id:100001)

[vasilii, 2016-12-12 13:44:51]

A revert of this CL (patchset #3 id:100001) has been created in
https://codereview.chromium.org/2565423002/ by vasilii@chromium.org.

The reason for reverting is: Broke content_unittests:
[ RUN      ] PaymentAppManagerTest.SetAndGetManifest

[libprotobuf ERROR
c:\b\c\b\win\src\third_party\protobuf\src\google\protobuf\message_lite.cc:120]
Can't parse message of type "content.PaymentAppManifestProto" because it is
missing required fields: (cannot determine missing fields for lite message)

c:\b\c\b\win\src\content\browser\payments\payment_app_manager_unittest.cc(145):
error: Value of: payments::mojom::PaymentAppManifestError::NONE

  Actual: PaymentAppManifestError::NONE

Expected: read_error

Which is: PaymentAppManifestError::MANIFEST_STORAGE_OPERATION_FAILED

[  FAILED  ] PaymentAppManagerTest.SetAndGetManifest (3 ms).

[findit-for-me, 2016-12-12 14:02:24]

FYI: Findit identified this CL at revision 437855 as the culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[zino, 2016-12-12 14:28:14]

On 2016/12/12 14:02:24, findit-for-me wrote:
> FYI: Findit identified this CL at revision 437855 as the culprit for
> failures in the build cycles as shown on:
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

Thank you for information.

I'll investigate this.

[commit-bot: I haz the power, 2016-12-12 15:11:44]

Description was changed from

==========
PaymentApp: Introduce PaymentAppDatabase class.

The class is providing APIs to read/write payment app related data(e.g. manifest
and associated service worker). It can be shared between payment_app_manager.cc
and payment_app_context.cc.

The payment_app_manager.cc contains a actual implementation of the
PaymentAppManager interface[1] used on the page or worker.
Also, the payment_app_context.cc can be used to query all manifests data[2]
in order to update payment request UI in Chrome layer.

We can summarize this CL as follows.
  1. Rename PaymentAppContext in content/browser/payments to
     PaymentAppContextImpl and then add PaymentAppContext interface in
     content/public to expose some APIs to Chrome layer.
  2. Write PaymentAppDatabase class and then move most of logics to access
     database from PaymentAppManager to the new class.
  3. Add a interface to query all manifests data in Chrome layer but it's not
     implemented yet in this CL. It will be implemented in follow-up CL[2].

[1] https://w3c.github.io/webpayments-payment-apps-api/#payment-app-manager
[2] https://codereview.chromium.org/2556433002

BUG=661608
TEST=existing unittests

Review-Url: https://codereview.chromium.org/2560293002
==========

to

==========
PaymentApp: Introduce PaymentAppDatabase class.

The class is providing APIs to read/write payment app related data(e.g. manifest
and associated service worker). It can be shared between payment_app_manager.cc
and payment_app_context.cc.

The payment_app_manager.cc contains a actual implementation of the
PaymentAppManager interface[1] used on the page or worker.
Also, the payment_app_context.cc can be used to query all manifests data[2]
in order to update payment request UI in Chrome layer.

We can summarize this CL as follows.
  1. Rename PaymentAppContext in content/browser/payments to
     PaymentAppContextImpl and then add PaymentAppContext interface in
     content/public to expose some APIs to Chrome layer.
  2. Write PaymentAppDatabase class and then move most of logics to access
     database from PaymentAppManager to the new class.
  3. Add a interface to query all manifests data in Chrome layer but it's not
     implemented yet in this CL. It will be implemented in follow-up CL[2].

[1] https://w3c.github.io/webpayments-payment-apps-api/#payment-app-manager
[2] https://codereview.chromium.org/2556433002

BUG=661608
TEST=existing unittests

Committed: https://crrev.com/3b0d3c1f7b4ab17b5abb62b6767615e5ca5a4eb1
Cr-Commit-Position: refs/heads/master@{#437855}
==========

[commit-bot: I haz the power, 2016-12-12 15:11:45]

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