PaymentApp: Add PaymentAppProvider class.

PaymentApp: Add PaymentAppProvider class.

The purpose of this class is to expose the following APIs to the chrome layer:
 - GetAllManifests(): query all stored payment app manifests.
 - InvokePaymentApp(): activate service worker and dispatch a payment event.

This has several advantages. First, the role of each class will be clearer.
After this patch, each role is as follows:
 - PaymentAppProvider: Expose the APIs for use in chrome layer.
 - PaymentAppContext: Manage requests coming from Blink and Chrome
   (PaymentAppManager and PaymentAppProvider) to store/query manifest data.
 - PaymentAppDatabase: It's owned by PaymentAppContext. Write or read the
   manifest data of payment app to associated service worker DB.
Second, no longer consider to query each storage partition iteratively in the
chrome layer. So, we can share the logic in all implementation.
(e.g. android and desktop in chrome layer)

This patch modified many files, but it can be summarized as follows:
 - Write a PaymentAppProvider class as singleton.
 - Move GetAllManifests() method from PaymentAppContext to PaymentAppProvider.
 - Move the logic that queries the manifest data from the payment app context
   per storage partition, from the chrome layer to content layer.
 - Rename payment_app_context_impl_unittest.cc with
   payment_app_provider_impl_unittest.cc

BUG=661608
TEST=payment_app_provider_impl_unittest.cc

Review-Url: https://codereview.chromium.org/2609103002
Cr-Commit-Position: refs/heads/master@{#441639}
Committed: https://chromium.googlesource.com/chromium/src/+/ac34b3a3ff3cbe7b56d7e5b840eccf1efcb7ff2e

[zino, 2017-01-03 11:34:40]

Description was changed from

==========
PaymentAppProvider class

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

to

==========
PaymentAppProvider class

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

[zino, 2017-01-03 11:34:41]

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

[zino, 2017-01-03 19:35:42]

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

[commit-bot: I haz the power, 2017-01-03 19:36:07]

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

[commit-bot: I haz the power, 2017-01-03 19:59:41]

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

[commit-bot: I haz the power, 2017-01-03 19:59:41]

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

[zino, 2017-01-04 14:23:27]

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

[commit-bot: I haz the power, 2017-01-04 14:23:39]

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

[commit-bot: I haz the power, 2017-01-04 14:45:58]

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

[commit-bot: I haz the power, 2017-01-04 14:45:58]

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

[zino, 2017-01-04 15:25:39]

Description was changed from

==========
PaymentAppProvider class

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

to

==========
PaymentApp: Add PaymentAppProvider class.

The purpose of this class is to expose the following APIs to the chrome layer:
 - GetAllManifests(): query all stored payment app manifests.
 - InvokePaymentApp(): activate service worker and dispatch a payment event.

This has several advantages. First, the role of each class will be clearer.
After this patch, each role is as follows:
 - PaymentAppProvider: Expose the APIs for use in chrome layer.
 - PaymentAppContext: Manage requests coming from Blink and Chrome
   (PaymentAppManager and PaymentAppProvider) to store/query manifest data.
 - PaymentAppDatabase: It's owned by PaymentAppContext. Write or read the
   manifest data of payment app to associated service worker DB.
Second, no longer consider to query each storage partition iteratively in the
chrome layer. So, we can share the logic in all implementation.
(e.g. android and desktop in chrome layer)

This patch modified many files, but it can be summarized as follows:
 - Write a PaymentAppProvider class as singleton.
 - Move GetAllManifests() method from PaymentAppContext to PaymentAppProvider.
 - Move the logic that queries the manifest data from the payment app context
   per storage partition, from the chrome layer to content layer.
 - Rename payment_app_context_impl_unittest.cc with
   payment_app_provider_impl_unittest.cc

BUG=661608
TEST=payment_app_provider_impl_unittest.cc
==========

[zino, 2017-01-04 15:29:28]

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

[commit-bot: I haz the power, 2017-01-04 15:29:53]

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

[zino, 2017-01-04 15:54:46]

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

[commit-bot: I haz the power, 2017-01-04 15:55:11]

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

[zino, 2017-01-04 16:05:43]

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

[zino, 2017-01-04 16:05:43]

PTAL

rouslan@, tommyt@ for payments
avi@ for content directory files.
dfalcantara@ for chrome files.

[commit-bot: I haz the power, 2017-01-04 16:51:44]

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

[commit-bot: I haz the power, 2017-01-04 16:51:45]

Dry run: This issue passed the CQ dry run.

[please use gerrit instead, 2017-01-04 16:55:44]

https://codereview.chromium.org/2609103002/diff/100001/content/browser/paymen...
File content/browser/payments/payment_app_content_unittest_base.h (right):

https://codereview.chromium.org/2609103002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_content_unittest_base.h:47:
StoragePartitionImpl* storage_partition_impl_;
Here and below:

Raw pointers lead to memory errors and are discouraged. If you must use raw
pointers, add comments explaining ownership and lifetime of these member
variables.

https://codereview.chromium.org/2609103002/diff/100001/content/browser/paymen...
File content/browser/payments/payment_app_context_impl.h (left):

https://codereview.chromium.org/2609103002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_context_impl.h:76: ~PaymentAppContextImpl()
override;
base::RefCountedThreadSafe has a destructor, so we should keep the override
keyword. Does the default destructor in the parent class prevent use from
overriding the parent destructor?

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

https://codereview.chromium.org/2609103002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_provider_impl.cc:38: 
Also needs "// static" comment.

https://codereview.chromium.org/2609103002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_provider_impl.cc:48: void
PaymentAppProviderImpl::GetAllManifests(
Please keep all methods in .cc in the same order as in the corresponding .h
file.

https://codereview.chromium.org/2609103002/diff/100001/content/browser/paymen...
File content/browser/payments/payment_app_provider_impl.h (right):

https://codereview.chromium.org/2609103002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_provider_impl.h:25: // Should be accessed
only on the UI thread.
"Should be accessed only on the UI thread." -- This comment should be in
PaymentAppProvider header file.

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

https://codereview.chromium.org/2609103002/diff/100001/content/public/browser...
content/public/browser/payment_app_provider.h:24: static PaymentAppProvider*
GetInstance();
Add a comment about where this is implemented.

[zino, 2017-01-04 17:30:00]

Thank you for review!

https://codereview.chromium.org/2609103002/diff/100001/content/browser/paymen...
File content/browser/payments/payment_app_content_unittest_base.h (right):

https://codereview.chromium.org/2609103002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_content_unittest_base.h:47:
StoragePartitionImpl* storage_partition_impl_;
On 2017/01/04 16:55:43, rouslan wrote:
> Here and below:
> 
> Raw pointers lead to memory errors and are discouraged. If you must use raw
> pointers, add comments explaining ownership and lifetime of these member
> variables.

The pointers is owned by BrowserContext and it manages their lifetime.
I left these variables to access each object easily but it wasn't a good way.
Instead I added two getter methods and then used them.

Done.

https://codereview.chromium.org/2609103002/diff/100001/content/browser/paymen...
File content/browser/payments/payment_app_context_impl.h (left):

https://codereview.chromium.org/2609103002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_context_impl.h:76: ~PaymentAppContextImpl()
override;
On 2017/01/04 16:55:43, rouslan wrote:
> base::RefCountedThreadSafe has a destructor, so we should keep the override
> keyword. Does the default destructor in the parent class prevent use from
> overriding the parent destructor?

Done.

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

https://codereview.chromium.org/2609103002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_provider_impl.cc:38: 
On 2017/01/04 16:55:43, rouslan wrote:
> Also needs "// static" comment.

Done.

https://codereview.chromium.org/2609103002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_provider_impl.cc:48: void
PaymentAppProviderImpl::GetAllManifests(
On 2017/01/04 16:55:43, rouslan wrote:
> Please keep all methods in .cc in the same order as in the corresponding .h
> file.

Done.

https://codereview.chromium.org/2609103002/diff/100001/content/browser/paymen...
File content/browser/payments/payment_app_provider_impl.h (right):

https://codereview.chromium.org/2609103002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_provider_impl.h:25: // Should be accessed
only on the UI thread.
On 2017/01/04 16:55:43, rouslan wrote:
> "Should be accessed only on the UI thread." -- This comment should be in
> PaymentAppProvider header file.

I already left a comment at the top of the class definition.
But I will leave this comment on the method again.

Done.

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

https://codereview.chromium.org/2609103002/diff/100001/content/public/browser...
content/public/browser/payment_app_provider.h:24: static PaymentAppProvider*
GetInstance();
On 2017/01/04 16:55:43, rouslan wrote:
> Add a comment about where this is implemented.

Done.

[zino, 2017-01-04 17:31:52]

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

[zino, 2017-01-04 17:34:40]

+ Bernhard Bauer for
	chrome/browser/browsing_data/browsing_data_remover_unittest.cc

(dfalcantara@ is not owner in the directory. :-) )

[please use gerrit instead, 2017-01-04 18:03:28]

lgtm

[Avi (use Gerrit), 2017-01-04 19:20:11]

lgtm

[gone, 2017-01-04 19:58:43]

chrome/ lgtm

[zino, 2017-01-04 23:19:05]

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

[commit-bot: I haz the power, 2017-01-04 23:19:52]

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

[commit-bot: I haz the power, 2017-01-04 23:34:53]

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

[commit-bot: I haz the power, 2017-01-04 23:34: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, 2017-01-05 00:06:04]

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

[commit-bot: I haz the power, 2017-01-05 00:06:44]

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

[zino, 2017-01-05 00:08:04]

rouslan@

I removed 'override' keyword in PaymentAppContextImpl class.
There was no virtual destructor in parent class.

https://codereview.chromium.org/2609103002/diff/100001/content/browser/paymen...
File content/browser/payments/payment_app_context_impl.h (left):

https://codereview.chromium.org/2609103002/diff/100001/content/browser/paymen...
content/browser/payments/payment_app_context_impl.h:76: ~PaymentAppContextImpl()
override;
On 2017/01/04 17:29:59, zino wrote:
> On 2017/01/04 16:55:43, rouslan wrote:
> > base::RefCountedThreadSafe has a destructor, so we should keep the override
> > keyword. Does the default destructor in the parent class prevent use from
> > overriding the parent destructor?
> 
> Done.

There was no virtual destructor in RefCountedThreadSafe class.
 -
https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?q=RefCountedTh...

The class seems to use type traits with static_cast pattern instead of virtual
destructor.
 -
https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?q=RefCountedTh...

So, I'll remove override keyword.

[commit-bot: I haz the power, 2017-01-05 01:20:58]

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

[commit-bot: I haz the power, 2017-01-05 01:20:59]

Dry run: This issue passed the CQ dry run.

[zino, 2017-01-05 02:49:36]

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

[zino, 2017-01-05 02:49:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, avi@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2609103002/#ps140001
(title: "Remove override keyword")

[commit-bot: I haz the power, 2017-01-05 02:50: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, 2017-01-05 02:56:54]

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

[commit-bot: I haz the power, 2017-01-05 02:56:57]

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

[Bernhard Bauer, 2017-01-05 09:26:24]

browsing_data LGTM.

[zino, 2017-01-05 12:25:56]

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

[commit-bot: I haz the power, 2017-01-05 12:26:11]

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-01-05 12:51:25]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1483619156430170,
"parent_rev": "e7c05695fbc1407a034269b83871159ca662cceb", "commit_rev":
"ac34b3a3ff3cbe7b56d7e5b840eccf1efcb7ff2e"}

[commit-bot: I haz the power, 2017-01-05 12:51:57]

Description was changed from

==========
PaymentApp: Add PaymentAppProvider class.

The purpose of this class is to expose the following APIs to the chrome layer:
 - GetAllManifests(): query all stored payment app manifests.
 - InvokePaymentApp(): activate service worker and dispatch a payment event.

This has several advantages. First, the role of each class will be clearer.
After this patch, each role is as follows:
 - PaymentAppProvider: Expose the APIs for use in chrome layer.
 - PaymentAppContext: Manage requests coming from Blink and Chrome
   (PaymentAppManager and PaymentAppProvider) to store/query manifest data.
 - PaymentAppDatabase: It's owned by PaymentAppContext. Write or read the
   manifest data of payment app to associated service worker DB.
Second, no longer consider to query each storage partition iteratively in the
chrome layer. So, we can share the logic in all implementation.
(e.g. android and desktop in chrome layer)

This patch modified many files, but it can be summarized as follows:
 - Write a PaymentAppProvider class as singleton.
 - Move GetAllManifests() method from PaymentAppContext to PaymentAppProvider.
 - Move the logic that queries the manifest data from the payment app context
   per storage partition, from the chrome layer to content layer.
 - Rename payment_app_context_impl_unittest.cc with
   payment_app_provider_impl_unittest.cc

BUG=661608
TEST=payment_app_provider_impl_unittest.cc
==========

to

==========
PaymentApp: Add PaymentAppProvider class.

The purpose of this class is to expose the following APIs to the chrome layer:
 - GetAllManifests(): query all stored payment app manifests.
 - InvokePaymentApp(): activate service worker and dispatch a payment event.

This has several advantages. First, the role of each class will be clearer.
After this patch, each role is as follows:
 - PaymentAppProvider: Expose the APIs for use in chrome layer.
 - PaymentAppContext: Manage requests coming from Blink and Chrome
   (PaymentAppManager and PaymentAppProvider) to store/query manifest data.
 - PaymentAppDatabase: It's owned by PaymentAppContext. Write or read the
   manifest data of payment app to associated service worker DB.
Second, no longer consider to query each storage partition iteratively in the
chrome layer. So, we can share the logic in all implementation.
(e.g. android and desktop in chrome layer)

This patch modified many files, but it can be summarized as follows:
 - Write a PaymentAppProvider class as singleton.
 - Move GetAllManifests() method from PaymentAppContext to PaymentAppProvider.
 - Move the logic that queries the manifest data from the payment app context
   per storage partition, from the chrome layer to content layer.
 - Rename payment_app_context_impl_unittest.cc with
   payment_app_provider_impl_unittest.cc

BUG=661608
TEST=payment_app_provider_impl_unittest.cc

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

[commit-bot: I haz the power, 2017-01-05 12:51:59]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/ac34b3a3ff3cbe7b56d7e5b840ec...