PaymentApp: Remove scope_url parameter from Get/SetManifest methods.

content/browser/payments/payment_app_content_unittest_base.h

Index: content/browser/payments/payment_app_content_unittest_base.h
diff --git a/content/browser/payments/payment_app_content_unittest_base.h b/content/browser/payments/payment_app_content_unittest_base.h
index d9cc41e4b7ca3675ddb7c07bac3de03579d06fe5..e05c8c82d3061eb69c3a353757fd370c9f7ec3f8 100644
--- a/content/browser/payments/payment_app_content_unittest_base.h
+++ b/content/browser/payments/payment_app_content_unittest_base.h
@@ -34,12 +34,11 @@ class PaymentAppContentUnitTestBase : public testing::Test {
   PaymentAppManager* CreatePaymentAppManager(const GURL& scope_url,
                                              const GURL& sw_script_url);
   void SetManifest(PaymentAppManager* manager,
-                   const std::string& scope,
                    payments::mojom::PaymentAppManifestPtr manifest,
                    const PaymentAppManager::SetManifestCallback& callback);
   void GetManifest(PaymentAppManager* manager,
-                   const std::string& scope,
                    const PaymentAppManager::GetManifestCallback& callback);
+  void UnregisterServiceWorker(const GURL& scope_url);

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

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]

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().

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

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).

  private:
   std::unique_ptr<TestBrowserThreadBundle> thread_bundle_;