Remove obsoleted function and add test

chrome/browser/android/offline_pages/offline_page_utils_unittest.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/android/offline_pages/offline_page_utils.h"
 
 #include <stdint.h>
 #include <utility>
 
 #include "base/callback.h"
@@ skipping 35 matching lines @@
 class OfflinePageUtilsTest
     : public testing::Test,
       public OfflinePageTestArchiver::Observer,
       public base::SupportsWeakPtr<OfflinePageUtilsTest> {
  public:
   OfflinePageUtilsTest();
   ~OfflinePageUtilsTest() override;
 
   void SetUp() override;
   void RunUntilIdle();
+  GURL GetOfflineURLForOnlineURL(GURL online_url,
+                                                       base::Callback<void(const GURL&)> callback);

[fgorski, 2016/06/10 04:46:24]

nit: you need to set up your editor to convert tabs to spaces properly

Another trick to get it right is to run "git cl format" before submitting

[Vivian, 2016/06/10 17:38:14]

Done.
I used git cl format for now. Will set up editor later.

 
   // Necessary callbacks for the offline page model.
   void OnSavePageDone(SavePageResult result, int64_t offlineId);
   void OnClearAllDone();
   void OnExpirePageDone(bool success);
+  void OnGetURLDone(const GURL& url);
 
   // OfflinePageTestArchiver::Observer implementation:
   void SetLastPathCreatedByArchiver(const base::FilePath& file_path) override;
 
   // Offline page URL for the first page.
   const GURL& offline_url_page_1() const { return offline_url_page_1_; }
   // Offline page URL for the second page.
   const GURL& offline_url_page_2() const { return offline_url_page_2_; }
   // Offline page URL not related to any page.
   const GURL& offline_url_missing() const { return offline_url_missing_; }
   // Offline page URL for expired page.
   const GURL& offline_url_expired() const { return offline_url_expired_; }
 
   TestingProfile* profile() { return &profile_; }
 
   int64_t offline_id() const { return offline_id_; }
 
+  const GURL& url() { return url_; }

[fgorski, 2016/06/10 04:46:24]

nit: Signature of this method can be updated.
const GURL& url() const { return url_; }

Adding the second const means that the method does not change the state of the
class and hence can be called when you only have const reference to it.

[Vivian, 2016/06/10 17:38:15]

Done.

+
  private:
   void CreateOfflinePages();
   std::unique_ptr<OfflinePageTestArchiver> BuildArchiver(
       const GURL& url,
       const base::FilePath& file_name);
 
   GURL offline_url_page_1_;
   GURL offline_url_page_2_;
   GURL offline_url_missing_;
   GURL offline_url_expired_;
 
   int64_t offline_id_;
+  GURL url_;
 
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
   base::ThreadTaskRunnerHandle task_runner_handle_;
   TestingProfile profile_;
 };
 
 OfflinePageUtilsTest::OfflinePageUtilsTest()
     : task_runner_(new base::TestSimpleTaskRunner),
       task_runner_handle_(task_runner_) {}
 
@@ skipping 28 matching lines @@
 }
 
 void OfflinePageUtilsTest::OnExpirePageDone(bool success) {
   // Result ignored here.
 }
 
 void OfflinePageUtilsTest::OnClearAllDone() {
   // Result ignored here.
 }
 
+void OfflinePageUtilsTest::OnGetURLDone(const GURL& url) {
+  url_ = url;
+}
+
+GURL OfflinePageUtilsTest::GetOfflineURLForOnlineURL(GURL online_url,
+                                                     base::Callback<void(const GURL&)> callback) {

[fgorski, 2016/06/10 04:46:25]

This line is too long. Again "git cl format" handles that for you.

Chromium allows 80 chars per line in C++ code and 100 chars per line in Java
code.

Please take a look here: http://www.chromium.org/developers/coding-style after
you are done with this patch and start familiarizing yourself with the coding
style (C++ and Java).
Consistency will help you read the code later on (once you get used to it).

[Vivian, 2016/06/10 17:38:14]

Done.

+  OfflinePageUtils::GetOfflineURLForOnlineURL(profile(), online_url, callback);
+  RunUntilIdle();
+  return url();

[fgorski, 2016/06/10 04:46:24]

return url_;

This should work for you.

[Vivian, 2016/06/10 17:38:15]

Done.

+}
+
 void OfflinePageUtilsTest::SetLastPathCreatedByArchiver(
     const base::FilePath& file_path) {}
 
 void OfflinePageUtilsTest::CreateOfflinePages() {
   OfflinePageModel* model =
       OfflinePageModelFactory::GetForBrowserContext(profile());
 
   // Create page 1.
   std::unique_ptr<OfflinePageTestArchiver> archiver(BuildArchiver(
       kTestPage1Url, base::FilePath(FILE_PATH_LITERAL("page1.mhtml"))));
@@ skipping 56 matching lines @@
   // URL is invalid.
   EXPECT_FALSE(OfflinePageUtils::MightBeOfflineURL(GURL("/test.mhtml")));
   // Scheme is not file.
   EXPECT_FALSE(OfflinePageUtils::MightBeOfflineURL(GURL("http://test.com/")));
   // Does not end with .mhtml.
   EXPECT_FALSE(OfflinePageUtils::MightBeOfflineURL(GURL("file:///test.txt")));
   // Might still be an offline page.
   EXPECT_TRUE(OfflinePageUtils::MightBeOfflineURL(GURL("file:///test.mhtml")));
 }
 
-TEST_F(OfflinePageUtilsTest, MaybeGetOfflineURLForOnlineURL) {
-  EXPECT_EQ(offline_url_page_1(),
-            OfflinePageUtils::MaybeGetOfflineURLForOnlineURL(profile(),
-                                                             kTestPage1Url));
-  EXPECT_EQ(offline_url_page_2(),
-            OfflinePageUtils::MaybeGetOfflineURLForOnlineURL(profile(),
-                                                             kTestPage2Url));
-  EXPECT_EQ(GURL::EmptyGURL(), OfflinePageUtils::MaybeGetOfflineURLForOnlineURL(
-                                   profile(), kTestPage3Url));
-  EXPECT_EQ(GURL::EmptyGURL(), OfflinePageUtils::MaybeGetOfflineURLForOnlineURL(
-                                   profile(), kTestPage4Url));
+TEST_F(OfflinePageUtilsTest, GetOfflineURLForOnlineURL) {
+  EXPECT_EQ(offline_url_page_1(), OfflinePageUtilsTest::GetOfflineURLForOnlineURL(
+      kTestPage1Url, base::Bind(&OfflinePageUtilsTest::OnGetURLDone, AsWeakPtr())));

[fgorski, 2016/06/10 04:46:25]

Lines in this test are too long as well.

[Vivian, 2016/06/10 17:38:15]

Done.

+
+  EXPECT_EQ(offline_url_page_2(), OfflinePageUtilsTest::GetOfflineURLForOnlineURL(
+      kTestPage2Url, base::Bind(&OfflinePageUtilsTest::OnGetURLDone, AsWeakPtr())));
+
+  EXPECT_EQ(GURL(), OfflinePageUtilsTest::GetOfflineURLForOnlineURL(
+      kTestPage3Url, base::Bind(&OfflinePageUtilsTest::OnGetURLDone, AsWeakPtr())));

[fgorski, 2016/06/10 04:46:24]

Please use GURL::EmptyGURL() in place of GURL()

Also, it seems that you need to add one more EXPECT_EQ based on the rebased
version on the left.

[Vivian, 2016/06/10 17:38:15]

Done.

 }
 
+

[fgorski, 2016/06/10 04:46:25]

nit: use one empty line only to separate methods.

[Vivian, 2016/06/10 17:38:14]

Done.

 TEST_F(OfflinePageUtilsTest, MaybeGetOnlineURLForOfflineURL) {
   EXPECT_EQ(kTestPage1Url, OfflinePageUtils::MaybeGetOnlineURLForOfflineURL(
                                profile(), offline_url_page_1()));
   EXPECT_EQ(kTestPage2Url, OfflinePageUtils::MaybeGetOnlineURLForOfflineURL(
                                profile(), offline_url_page_2()));
   EXPECT_EQ(GURL::EmptyGURL(), OfflinePageUtils::MaybeGetOnlineURLForOfflineURL(
                                    profile(), offline_url_missing()));
   EXPECT_EQ(kTestPage4Url, OfflinePageUtils::MaybeGetOnlineURLForOfflineURL(
                                profile(), offline_url_expired()));
 }
@@ skipping 15 matching lines @@
       OfflinePageUtils::HasOfflinePageForOnlineURL(profile(), kTestPage1Url));
   EXPECT_TRUE(
       OfflinePageUtils::HasOfflinePageForOnlineURL(profile(), kTestPage2Url));
   EXPECT_FALSE(
       OfflinePageUtils::HasOfflinePageForOnlineURL(profile(), kTestPage3Url));
   EXPECT_FALSE(
       OfflinePageUtils::HasOfflinePageForOnlineURL(profile(), kTestPage4Url));
 }
 
 }  // namespace offline_pages