PaymentApp: Remove scope_url parameter from Get/SetManifest methods.

PaymentApp: Remove scope_url parameter from Get/SetManifest methods.

When calling setManifest()/getManifest() from Blink side, we currently pass
scope_url to find associated service worker in Content layer. However, it is
not a good way to pass scope_url each time. Of course, since the implementations
are all private methods, there is no risk of calling it wrong from the outside.
But some test classes are declared as friend class and it can be misused in the
classes.

Therefore, this CL is adding Init() in payment_app.mojom and then using it in
Blink side PaymentAppManager when creating the object. Then, we don't need to
pass scope_url each time when calling setManifest()/getManifest().

BUG=661608
TEST=existing tests

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

[zino, 2016-12-19 17:53:13]

Description was changed from

==========
PaymentApp: Remove scope_url parameter from Get/SetManifest method.

When calling setManifest()/getManifest() from Blink side, we currently pass
scope_url to find associated service worker in Content layer. However, it is
not a good way to pass scope_url each time. Of course, since the implementations
are all private methods, there is no risk of calling it wrong from the outside.
But some test classes are declared as friend class and it can be misused in the
classes.

Therefore, this CL is adding Init() in payment_app.mojom and then using it in
Blink side PaymentAppManager when creating the object. Then, we don't need to
pass scope_url each time when calling setManifest()/getManifest().

BUG=661608
TEST=existing tests
==========

to

==========
PaymentApp: Remove scope_url parameter from Get/SetManifest methods.

When calling setManifest()/getManifest() from Blink side, we currently pass
scope_url to find associated service worker in Content layer. However, it is
not a good way to pass scope_url each time. Of course, since the implementations
are all private methods, there is no risk of calling it wrong from the outside.
But some test classes are declared as friend class and it can be misused in the
classes.

Therefore, this CL is adding Init() in payment_app.mojom and then using it in
Blink side PaymentAppManager when creating the object. Then, we don't need to
pass scope_url each time when calling setManifest()/getManifest().

BUG=661608
TEST=existing tests
==========

[zino, 2016-12-19 17:55:00]

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

[zino, 2016-12-19 17:55:32]

PTAL

rouslan@ for payments
Tom Sepez@ for mojom

[Tom Sepez, 2016-12-19 18:09:39]

mojom lgtm

[please use gerrit instead, 2016-12-19 20:45:59]

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

https://codereview.chromium.org/2586203003/diff/1/content/browser/payments/pa...
content/browser/payments/payment_app_content_unittest_base.h:41: void
UnregisterServiceWorker(const GURL& scope_url);
This change seems orthogonal to your goal. Can you split into a separate patch?
Given the past flakiness of SetAndGetManifest unit test, I'd prefer patches here
to be smaller.

[zino, 2016-12-20 11:08:55]

Thank you for review.
I left comments.

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

https://codereview.chromium.org/2586203003/diff/1/content/browser/payments/pa...
content/browser/payments/payment_app_content_unittest_base.h:41: void
UnregisterServiceWorker(const GURL& scope_url);
On 2016/12/19 20:45:59, rouslan-intermittent-holidays wrote:
> This change seems orthogonal to your goal. Can you split into a separate
patch?
> Given the past flakiness of SetAndGetManifest unit test, I'd prefer patches
here
> to be smaller.

I agree that the smaller patches are better but this patch is atomic enough.
The UnregisterServiceWorker() is used in the following unit tests.
 - PaymentAppManagerTest.SetManifestWithoutAssociatedServiceWorker
 - PaymentAppManagerTest.GetManifestWithoutAssociatedServiceWorker

Previous code could allow to pass scope_url to SetManifest() or GetManifest().
So, these unit tests are to test that the Set/GetManifest() methods can prevent
invalid scope_url instead of normal scope_url of ServiceWorker which creates
PaymentAppManager.

After current patch, the Set/GetManifest() methods no longer can take scope_url.
So, we may think the tests are unnecessary anymore. But, it can still happen.
For example, ServiceWorker is valid when creating PaymentAppManager
but the ServiceWorker is unregistered before calling Set/GetManifest().

https://codereview.chromium.org/2586203003/diff/1/content/browser/payments/pa...
content/browser/payments/payment_app_content_unittest_base.h:42: 
About flakiness unit test..

I still doubt the status of the bot at that time. After reverting my patch,
according to your guide I splited the patch into smaller patches and
pushed them. They were already finished at https://crrev.com/2556433002/.

The important thing is that there was no logic change. Thanks to your meticulous
review, the quality of my patches has improved actually.
However, there was still no logic change to write/read manifest data as well as
I copied and pasted the contents of the previous patch and used them.
Nevertheless, the bot seems to be working well until now (since my patch).

There are several rasons why I suspect the bot. (I'm not sure but..)

 1. Couldn't reproduce the unit test error with my patch in my local environment
    and trybot. It happend in post landing bot at that time only.

 2. At that time, the most of the bots saw unstable during the weekend.
    I checked commit button on Saturday but it was landed actually on Monday.

 3. After you l-g-t-m stamp, in my final fixed patch set,
    I used hard-coded value internally when calling GetManifest() as follows.
      - DeserializePaymentAppManifest("Hard coded value!");
    It made bot errors of course and I found it soon.
    The bot error would be the same to post-landing bot error.
    Because the hardcoded value can not be parsed in Proto module.
    So, I removed the final patch set and then re-uploaded correct patch set.
    I doubt this part most. I'm not sure but post-landing bot might have picked
    the old patch(used hard-coded value).

[please use gerrit instead, 2016-12-20 16:57:54]

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

https://codereview.chromium.org/2586203003/diff/1/content/browser/payments/pa...
content/browser/payments/payment_app_content_unittest_base.cc:98:
candidate_manager.first->Init(scope_url.spec());
Run the loop here as well, because Init() posts a task to IO thread.

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

https://codereview.chromium.org/2586203003/diff/1/content/browser/payments/pa...
content/browser/payments/payment_app_manager.h:45: GURL m_scope;
scope_ instead of m_scope.

[zino, 2016-12-20 17:18:37]

Thank you for review.

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

https://codereview.chromium.org/2586203003/diff/1/content/browser/payments/pa...
content/browser/payments/payment_app_content_unittest_base.cc:98:
candidate_manager.first->Init(scope_url.spec());
On 2016/12/20 16:57:54, rouslan-intermittent-holidays wrote:
> Run the loop here as well, because Init() posts a task to IO thread.

Done.

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

https://codereview.chromium.org/2586203003/diff/1/content/browser/payments/pa...
content/browser/payments/payment_app_manager.h:45: GURL m_scope;
On 2016/12/20 16:57:54, rouslan-intermittent-holidays wrote:
> scope_ instead of m_scope.

Ah! yeap..

I was confused Blink and Chromium.. :P

[please use gerrit instead, 2016-12-20 17:25:17]

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

https://codereview.chromium.org/2586203003/diff/20001/content/browser/payment...
content/browser/payments/payment_app_context_impl_unittest.cc:103:
EXPECT_EQ(manifest.second->icon.value(), "payment-app-icon");
You seem to have reversed the "expected" and "actual" here. The expected should
be on the left. That way error messages look like this:

Expected "payment-app-icon", but found manifest.second->icon.value(), which is
"not-what-was-expected".

[zino, 2016-12-20 17:30:53]

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

https://codereview.chromium.org/2586203003/diff/20001/content/browser/payment...
content/browser/payments/payment_app_context_impl_unittest.cc:103:
EXPECT_EQ(manifest.second->icon.value(), "payment-app-icon");
On 2016/12/20 17:25:17, rouslan-intermittent-holidays wrote:
> You seem to have reversed the "expected" and "actual" here. The expected
should
> be on the left. That way error messages look like this:
> 
> Expected "payment-app-icon", but found manifest.second->icon.value(), which is
> "not-what-was-expected".

Sure,

So, I made a CL for this. You are already a reviewer in the CL.
I'll rebase this CL after landing the test CL.

[zino, 2016-12-20 18:36:34]

I've just rebased this CL.

Thank you.

[zino, 2016-12-20 19:16:53]

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

[commit-bot: I haz the power, 2016-12-20 19:17:23]

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-20 19:24:06]

lgtm

[commit-bot: I haz the power, 2016-12-20 22:29:03]

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

[commit-bot: I haz the power, 2016-12-20 22:29:04]

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

[zino, 2016-12-20 22:40:46]

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

[zino, 2016-12-20 22:40:46]

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/2586203003/#ps40001
(title: "rebased")

[commit-bot: I haz the power, 2016-12-20 22:41:35]

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-21 00:06:27]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482273646235900,
"parent_rev": "e0a186993bbcbd1ce663f0a7ef0ed6bc696ea32e", "commit_rev":
"4ded730e7536f53b93f710c5a8828a01820b63a6"}

[commit-bot: I haz the power, 2016-12-21 00:07:04]

Description was changed from

==========
PaymentApp: Remove scope_url parameter from Get/SetManifest methods.

When calling setManifest()/getManifest() from Blink side, we currently pass
scope_url to find associated service worker in Content layer. However, it is
not a good way to pass scope_url each time. Of course, since the implementations
are all private methods, there is no risk of calling it wrong from the outside.
But some test classes are declared as friend class and it can be misused in the
classes.

Therefore, this CL is adding Init() in payment_app.mojom and then using it in
Blink side PaymentAppManager when creating the object. Then, we don't need to
pass scope_url each time when calling setManifest()/getManifest().

BUG=661608
TEST=existing tests
==========

to

==========
PaymentApp: Remove scope_url parameter from Get/SetManifest methods.

When calling setManifest()/getManifest() from Blink side, we currently pass
scope_url to find associated service worker in Content layer. However, it is
not a good way to pass scope_url each time. Of course, since the implementations
are all private methods, there is no risk of calling it wrong from the outside.
But some test classes are declared as friend class and it can be misused in the
classes.

Therefore, this CL is adding Init() in payment_app.mojom and then using it in
Blink side PaymentAppManager when creating the object. Then, we don't need to
pass scope_url each time when calling setManifest()/getManifest().

BUG=661608
TEST=existing tests

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

[commit-bot: I haz the power, 2016-12-21 00:07:05]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-21 00:08:57]

Description was changed from

==========
PaymentApp: Remove scope_url parameter from Get/SetManifest methods.

When calling setManifest()/getManifest() from Blink side, we currently pass
scope_url to find associated service worker in Content layer. However, it is
not a good way to pass scope_url each time. Of course, since the implementations
are all private methods, there is no risk of calling it wrong from the outside.
But some test classes are declared as friend class and it can be misused in the
classes.

Therefore, this CL is adding Init() in payment_app.mojom and then using it in
Blink side PaymentAppManager when creating the object. Then, we don't need to
pass scope_url each time when calling setManifest()/getManifest().

BUG=661608
TEST=existing tests

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

to

==========
PaymentApp: Remove scope_url parameter from Get/SetManifest methods.

When calling setManifest()/getManifest() from Blink side, we currently pass
scope_url to find associated service worker in Content layer. However, it is
not a good way to pass scope_url each time. Of course, since the implementations
are all private methods, there is no risk of calling it wrong from the outside.
But some test classes are declared as friend class and it can be misused in the
classes.

Therefore, this CL is adding Init() in payment_app.mojom and then using it in
Blink side PaymentAppManager when creating the object. Then, we don't need to
pass scope_url each time when calling setManifest()/getManifest().

BUG=661608
TEST=existing tests

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

[commit-bot: I haz the power, 2016-12-21 00:08:58]

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