PaymentApp: Implement PaymentAppManager.getManifest().

PaymentApp: Implement PaymentAppManager.getManifest().

The getManifest() method is used to retrieve the properties associated with
a registered payment app. (from service worker storage)

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

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

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

[zino, 2016-11-20 14:01:25]

Description was changed from

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

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

to

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

The getManifest() method is used to retrieve the properties associated with
a registered payment app. (from service worker storage)

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

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

[zino, 2016-11-20 14:01:25]

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

[zino, 2016-11-20 14:01:31]

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

[commit-bot: I haz the power, 2016-11-20 14:01:38]

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

[zino, 2016-11-20 14:50:59]

PTAL

[commit-bot: I haz the power, 2016-11-20 15:50:22]

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

[commit-bot: I haz the power, 2016-11-20 15:50:22]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-11-21 17:57:12]

lgtm

[please use gerrit instead, 2016-11-21 22:02:13]

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

https://codereview.chromium.org/2506093002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager.cc:145:
payments::mojom::PaymentAppManifestError::STORE_MANIFEST_FAILED);
Change this error message to either GET_MANIFST_FAILED or
MANIFEST_STORAGE_OPERATION_FAILED, the latter of which can be used instead of
STORE_MANIFEST_FAILED as well.

https://codereview.chromium.org/2506093002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager.cc:149: DCHECK(data.size() == 1);
Data on disk can be corrupted. I'm not aware of any error correction. So I
recommend you handle data.size()!=1 together with status!=SERVICE_WORK_OK
instead of assuming that it's always going to be 1 element long.

https://codereview.chromium.org/2506093002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager.cc:151: bool success =
manifest_proto.ParseFromString(data[0]);
Let's also gracefully handle !success, because data on disk can have outdated
format.

https://codereview.chromium.org/2506093002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager.cc:166: for (int i = 0; i <
option_proto.enabled_methods_size(); i++)
Can we use "const auto&" loop?

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

https://codereview.chromium.org/2506093002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager.h:47: const GetManifestCallback&
callback) override;
Please put only SetManifest() and GetManifest() in "//
payments::mojom::PaymentAppManager" section and nothing else.

https://codereview.chromium.org/2506093002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager.h:59: // payment_app_context_ owns
this.
"this" is confusing here. I guess you wanted to say something lke
"PaymentAppContext owns PaymentAppManager".

[zino, 2016-11-21 23:48:40]

I addressed all your comments.

Thank you for detailed review. PTAL

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

https://codereview.chromium.org/2506093002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager.cc:145:
payments::mojom::PaymentAppManifestError::STORE_MANIFEST_FAILED);
On 2016/11/21 22:02:13, rouslan wrote:
> Change this error message to either GET_MANIFST_FAILED or
> MANIFEST_STORAGE_OPERATION_FAILED, the latter of which can be used instead of
> STORE_MANIFEST_FAILED as well.

Done.

https://codereview.chromium.org/2506093002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager.cc:149: DCHECK(data.size() == 1);
On 2016/11/21 22:02:13, rouslan wrote:
> Data on disk can be corrupted. I'm not aware of any error correction. So I
> recommend you handle data.size()!=1 together with status!=SERVICE_WORK_OK
> instead of assuming that it's always going to be 1 element long.

Done.

https://codereview.chromium.org/2506093002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager.cc:151: bool success =
manifest_proto.ParseFromString(data[0]);
On 2016/11/21 22:02:13, rouslan wrote:
> Let's also gracefully handle !success, because data on disk can have outdated
> format.

Done.

https://codereview.chromium.org/2506093002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager.cc:166: for (int i = 0; i <
option_proto.enabled_methods_size(); i++)
On 2016/11/21 22:02:13, rouslan wrote:
> Can we use "const auto&" loop?

Done.

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

https://codereview.chromium.org/2506093002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager.h:47: const GetManifestCallback&
callback) override;
On 2016/11/21 22:02:13, rouslan wrote:
> Please put only SetManifest() and GetManifest() in "//
> payments::mojom::PaymentAppManager" section and nothing else.

Done.

https://codereview.chromium.org/2506093002/diff/20001/content/browser/payment...
content/browser/payments/payment_app_manager.h:59: // payment_app_context_ owns
this.
On 2016/11/21 22:02:13, rouslan wrote:
> "this" is confusing here. I guess you wanted to say something lke
> "PaymentAppContext owns PaymentAppManager".

Done.

[zino, 2016-11-22 00:03:47]

Patchset #5 (id:80001) has been deleted

[please use gerrit instead, 2016-11-22 00:42:08]

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

https://codereview.chromium.org/2506093002/diff/60001/content/browser/payment...
content/browser/payments/payment_app_manager.cc:142: if (status !=
SERVICE_WORKER_OK && data.size() != 1) {
&& should be ||.

[zino, 2016-11-22 02:16:22]

On 2016/11/22 00:42:08, rouslan wrote:
>
https://codereview.chromium.org/2506093002/diff/60001/content/browser/payment...
> File content/browser/payments/payment_app_manager.cc (right):
> 
>
https://codereview.chromium.org/2506093002/diff/60001/content/browser/payment...
> content/browser/payments/payment_app_manager.cc:142: if (status !=
> SERVICE_WORKER_OK && data.size() != 1) {
> && should be ||.

OMG.. Done.

Thanks.

[please use gerrit instead, 2016-11-22 14:19:31]

lgtm

[zino, 2016-11-22 14:44:27]

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

[zino, 2016-11-22 14:44:28]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2506093002/#ps100001
(title: "PaymentApp: Implement PaymentAppManager.getManifest().")

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

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

[commit-bot: I haz the power, 2016-11-22 14:58:34]

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

[commit-bot: I haz the power, 2016-11-22 14:58:35]

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

[zino, 2016-11-22 15:02:17]

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

[zino, 2016-11-22 15:02:17]

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

[commit-bot: I haz the power, 2016-11-22 15:02:40]

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

[commit-bot: I haz the power, 2016-11-22 15:05:26]

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

[commit-bot: I haz the power, 2016-11-22 15:05:27]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[zino, 2016-11-22 19:15:25]

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

[commit-bot: I haz the power, 2016-11-22 19:15:55]

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

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

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1479842125965920,
"parent_rev": "98979265b7ee89affa00f16dd15ecb5cb018e8a9", "commit_rev":
"8c7f965e2a2ca46b058d10976ecb7fd96b7028bb"}

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

Description was changed from

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

The getManifest() method is used to retrieve the properties associated with
a registered payment app. (from service worker storage)

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

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

to

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

The getManifest() method is used to retrieve the properties associated with
a registered payment app. (from service worker storage)

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

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

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

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-11-22 20:14:26]

Description was changed from

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

The getManifest() method is used to retrieve the properties associated with
a registered payment app. (from service worker storage)

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

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

to

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

The getManifest() method is used to retrieve the properties associated with
a registered payment app. (from service worker storage)

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

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

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

[commit-bot: I haz the power, 2016-11-22 20:14:27]

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