Remove obsoleted function and add test

chrome/browser/android/offline_pages/offline_page_utils_unittest.cc

Index: chrome/browser/android/offline_pages/offline_page_utils_unittest.cc
diff --git a/chrome/browser/android/offline_pages/offline_page_utils_unittest.cc b/chrome/browser/android/offline_pages/offline_page_utils_unittest.cc
index 06173eacea2d88d0a9f0b27416e7e9476826e093..8e4d81b0e5c61fdd102d670aee679354f8550cfb 100644
--- a/chrome/browser/android/offline_pages/offline_page_utils_unittest.cc
+++ b/chrome/browser/android/offline_pages/offline_page_utils_unittest.cc
@@ -55,6 +55,7 @@ class OfflinePageUtilsTest
   // Necessary callbacks for the offline page model.
   void OnSavePageDone(SavePageResult result, int64_t offlineId);
   void OnClearAllDone();
+  void OnGetURLDone(const GURL& url);
 
   // OfflinePageTestArchiver::Observer implementation:
   void SetLastPathCreatedByArchiver(const base::FilePath& file_path) override;
@@ -70,6 +71,8 @@ class OfflinePageUtilsTest
 
   int64_t offline_id() const { return offline_id_; }
 
+  GURL url() const { return url_; }

[dewittj, 2016/06/09 22:13:58]

nit: const GURL&

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

Done.

+
  private:
   void CreateOfflinePages();
   std::unique_ptr<OfflinePageTestArchiver> BuildArchiver(
@@ -81,6 +84,7 @@ class OfflinePageUtilsTest
   GURL offline_url_missing_;
 
   int64_t offline_id_;
+  GURL url_;
 
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
   base::ThreadTaskRunnerHandle task_runner_handle_;
@@ -125,6 +129,11 @@ void OfflinePageUtilsTest::OnClearAllDone() {
   // Result ignored here.
 }
 
+void OfflinePageUtilsTest::OnGetURLDone(const GURL& url) {
+  // Save url here to verify
+  url_ = url;
+}
+
 void OfflinePageUtilsTest::SetLastPathCreatedByArchiver(
     const base::FilePath& file_path) {}
 
@@ -188,17 +197,22 @@ TEST_F(OfflinePageUtilsTest, MightBeOfflineURL) {
   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(), OfflinePageUtils::MaybeGetOfflineURLForOnlineURL(
-                        profile(), GURL(kTestPage3Url)));
+
+TEST_F(OfflinePageUtilsTest, GetOfflineURLForOnlineURL) {
+  OfflinePageUtils::GetOfflineURLForOnlineURL(profile(), kTestPage1Url, base::Bind(&OfflinePageUtilsTest::OnGetURLDone, AsWeakPtr()));

[dewittj, 2016/06/09 22:13:58]

It doesn't look like this is waiting for the call to complete so url() will
never be set by the time you call it.

Looks like you need a RunUntilIdle(); here, but I'd recommend making a helper
function :

GURL OfflinePageUtilsTest::GetOfflineURLForOnlineURL(GURL online_url,
base::Callback<void(const GURL&)> callback)

that does the actual OfflinePageUtils call, runs until idle, and then returns
url().  That'd make the actual test code more straightforward.

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

Done.

+  GURL url_1 = url();
+  EXPECT_EQ(offline_url_page_1(), url_1);
+
+  OfflinePageUtils::GetOfflineURLForOnlineURL(profile(), kTestPage2Url, base::Bind(&OfflinePageUtilsTest::OnGetURLDone, AsWeakPtr()));
+  GURL url_2 = url();
+  EXPECT_EQ(offline_url_page_2(), url_2);
+
+  OfflinePageUtils::GetOfflineURLForOnlineURL(profile(), GURL(kTestPage3Url), base::Bind(&OfflinePageUtilsTest::OnGetURLDone, AsWeakPtr()));
+  GURL url_3 = url();
+  EXPECT_EQ(GURL(), url_3);
 }
 
+
 TEST_F(OfflinePageUtilsTest, MaybeGetOnlineURLForOfflineURL) {
   EXPECT_EQ(kTestPage1Url, OfflinePageUtils::MaybeGetOnlineURLForOfflineURL(
                                profile(), offline_url_page_1()));