Improve add to homescreen data fetcher unit tests.

chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc

Index: chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc
diff --git a/chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc b/chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc
index ebe6b852474512e45d248f019add558b189a8fd1..aace21edfd5bf5be1afcb926f27bcf51238da8ad 100644
--- a/chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc
+++ b/chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc
@@ -6,30 +6,22 @@
 
 #include <memory>
 #include <string>
+#include <utility>
 
-#include "base/callback_forward.h"
-#include "base/files/file_path.h"
+#include "base/bind.h"
+#include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/strings/nullable_string16.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/time/time.h"
 #include "chrome/browser/installable/installable_manager.h"
 #include "chrome/common/web_application_info.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
-#include "content/browser/service_worker/embedded_worker_test_helper.h"
-#include "content/browser/service_worker/service_worker_context_core.h"
-#include "content/common/service_worker/service_worker_status_code.h"
-#include "content/public/browser/browser_thread.h"
-#include "content/public/browser/site_instance.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/manifest.h"
-#include "content/test/test_web_contents.h"
-#include "net/http/http_status_code.h"
 #include "third_party/WebKit/public/platform/WebDisplayMode.h"
 #include "ui/gfx/image/image_unittest_util.h"
 #include "url/gurl.h"
@@ -38,72 +30,15 @@ namespace {
 
 const char* kWebApplicationInfoTitle = "Meta Title";
 const char* kDefaultManifestUrl = "https://www.example.com/manifest.json";
+const char* kDefaultIconUrl = "https://www.example.com/icon.png";
 const char* kDefaultManifestName = "Default Name";
 const char* kDefaultManifestShortName = "Default Short Name";
 const char* kDefaultStartUrl = "https://www.example.com/index.html";
 const blink::WebDisplayMode kDefaultManifestDisplayMode =
     blink::kWebDisplayModeStandalone;
+const int kIconSizePx = 144;
 
-// WebContents subclass which mocks out image and manifest fetching.
-class MockWebContents : public content::TestWebContents {
- public:
-  explicit MockWebContents(content::BrowserContext* browser_context)
-      : content::TestWebContents(browser_context),
-        should_image_time_out_(false),
-        should_manifest_time_out_(false) {}
-
-  ~MockWebContents() override {}
-
-  void SetManifest(const GURL& manifest_url,
-                   const content::Manifest& manifest) {
-    manifest_url_ = manifest_url;
-    manifest_ = manifest;
-  }
-
-  int DownloadImage(const GURL& url,
-                    bool is_favicon,
-                    uint32_t max_bitmap_size,
-                    bool bypass_cache,
-                    const ImageDownloadCallback& callback) override {
-    if (should_image_time_out_)
-      return 0;
-
-    const int kIconSizePx = 144;
-    SkBitmap icon = gfx::test::CreateBitmap(kIconSizePx, kIconSizePx);
-    std::vector<SkBitmap> icons(1u, icon);
-    std::vector<gfx::Size> pixel_sizes(1u, gfx::Size(kIconSizePx, kIconSizePx));
-    content::BrowserThread::GetTaskRunnerForThread(content::BrowserThread::UI)
-        ->PostTask(FROM_HERE, base::Bind(callback, 0, net::HTTP_OK, url, icons,
-                                         pixel_sizes));
-    return 0;
-  }
-
-  void GetManifest(const GetManifestCallback& callback) override {
-    if (should_manifest_time_out_)
-      return;
-
-    content::BrowserThread::GetTaskRunnerForThread(content::BrowserThread::UI)
-        ->PostTask(FROM_HERE, base::Bind(callback, manifest_url_, manifest_));
-  }
-
-  void SetShouldImageTimeOut(bool should_time_out) {
-    should_image_time_out_ = should_time_out;
-  }
-
-  void SetShouldManifestTimeOut(bool should_time_out) {
-    should_manifest_time_out_ = should_time_out;
-  }
-
- private:
-  GURL manifest_url_;
-  content::Manifest manifest_;
-  bool should_image_time_out_;
-  bool should_manifest_time_out_;
-
-  DISALLOW_COPY_AND_ASSIGN(MockWebContents);
-};
-
-// Tracks which of the AddToHomescreenDataFetcher::Observer callbacks have been
+// Tracks which of the AddToHomescreenDataFetcher::Observer methods have been
 // called.
 class ObserverWaiter : public AddToHomescreenDataFetcher::Observer {
  public:
@@ -125,12 +60,16 @@ class ObserverWaiter : public AddToHomescreenDataFetcher::Observer {
   }
 
   void OnDidDetermineWebApkCompatibility(bool is_webapk_compatible) override {
+    // This should only be called once.
+    EXPECT_FALSE(determined_webapk_compatibility_);
     EXPECT_FALSE(title_available_);
     determined_webapk_compatibility_ = true;
     is_webapk_compatible_ = is_webapk_compatible;
   }
 
   void OnUserTitleAvailable(const base::string16& title) override {
+    // This should only be called once.
+    EXPECT_FALSE(title_available_);
     EXPECT_FALSE(data_available_);
     title_available_ = true;
     title_ = title;
@@ -146,6 +85,8 @@ class ObserverWaiter : public AddToHomescreenDataFetcher::Observer {
   void OnDataAvailable(const ShortcutInfo& info,
                        const SkBitmap& primary_icon,
                        const SkBitmap& badge_icon) override {
+    // This should only be called once.
+    EXPECT_FALSE(data_available_);
     EXPECT_TRUE(title_available_);
     data_available_ = true;
     if (!quit_closure_.is_null())
@@ -175,18 +116,20 @@ base::NullableString16 NullableStringFromUTF8(const std::string& value) {
   return base::NullableString16(base::UTF8ToUTF16(value), false);
 }
 
-content::Manifest BuildEmptyManifest() {
-  return content::Manifest();
-}
-
-// Builds WebAPK compatible content::Manifest.
-content::Manifest BuildDefaultManifest() {
+content::Manifest BuildNoIconManifest() {
   content::Manifest manifest;
   manifest.name = NullableStringFromUTF8(kDefaultManifestName);
   manifest.short_name = NullableStringFromUTF8(kDefaultManifestShortName);
   manifest.start_url = GURL(kDefaultStartUrl);
   manifest.display = kDefaultManifestDisplayMode;
 
+  return manifest;
+}
+
+// Builds WebAPK compatible content::Manifest.
+content::Manifest BuildDefaultManifest() {
+  content::Manifest manifest = BuildNoIconManifest();
+
   content::Manifest::Icon primary_icon;
   primary_icon.type = base::ASCIIToUTF16("image/png");
   primary_icon.sizes.push_back(gfx::Size(144, 144));
@@ -199,6 +142,88 @@ content::Manifest BuildDefaultManifest() {
 
 }  // anonymous namespace
 
+class TestInstallableManager : public InstallableManager {
+ public:
+  explicit TestInstallableManager(content::WebContents* web_contents)
+      : InstallableManager(web_contents) {}
+
+  void GetData(const InstallableParams& params,
+               const InstallableCallback& callback) override {
+    InstallableStatusCode code = code_;
+    if (params.check_installable) {
+      if (!IsManifestValidForWebApp(manifest_))

[pkotwicz, 2017/06/29 19:22:05]

Can you make IsManifestValidForWebApp() a static method which returns an error
code in InstallableManager? I think that it would make things less awkward.

[dominickn, 2017/06/30 02:42:31]

I'll do that in a follow up because it will mean changing a bunch of
InstallableManager's tests as well.

[pkotwicz, 2017/06/30 23:54:52]

Fair enough

+        code = valid_manifest_->error;
+      else if (!is_installable_)
+        code = NO_MATCHING_SERVICE_WORKER;
+    }
+
+    // Store the callback so it can be redispatched later.
+    InstallableData data{
+        code,           manifest_url_,
+        manifest_,      primary_icon_url_,
+        &primary_icon_, badge_icon_url_,
+        &badge_icon_,   params.check_installable ? is_installable_ : false};

[pkotwicz, 2017/06/29 19:22:06]

For the sake of clarity, you should only return a primary icon and primary icon
URL if InstallableParams::fetch_primary_icon is true

The benefit of this is that you can merge SetManifest() and
SetManifestAndIcons() and SetInstallableAndBadgeIcon()

I am envisioning:
void SetManifest(const GURL& url, const content::Manifest& manifest) {
  manifest_url_ = url;
  manifest_ = manifest;
  if (!manifest_.icons.empty()) {
    primary_icon_url_ = manifest_.icons[0].src;
    primary_icon_ = gfx::test::CreateBitmap(kIconSizePx, kIconSizePx));
    badge_icon_url_ = manifest_.icons[0].src;
    badge_icon_ = gfx::test::CreateBitmap(kIconSizePx, kIconSizePx));
  }
}

[dominickn, 2017/06/30 02:42:31]

Good suggestion, done

+    callback_ = base::BindOnce(callback, data);
+
+    if (should_manifest_time_out_ ||
+        (params.check_installable && should_installable_time_out_)) {

[pkotwicz, 2017/06/29 19:22:05]

Can this be simplified to:

if (should_manifest_time_out_ || params.check_installable) {}

or alternatively

if (should_manifest_time_out_ ||
    (params.check_installable && params.wait_for_worker)) {}

[dominickn, 2017/06/30 02:42:31]

Not really, because we want to test the first GetData() call (manifest + icon)
and the second (service worker) timing out / not timing out separately.

 - we can't use wait_for_worker because that's always true
 - similarly check_installable is always true if we're checking for
Webapk-compatibility. We need a separate mechanism so that we can test checking
compatibility but it times out.

[pkotwicz, 2017/06/30 23:54:52]

You are right, my suggestion has problems.

We should only time out if:
- we are checking for installability
- the page does not have a service worker
- there is no other error

I guess that would make it

if (should_manifest_time_out_ ||
    (params.check_installable && code == NO_MATCHING_SERVICE_WORKER)) {
  ...
}


I thinking that lines 153-158 could look like this:

if (params.check_installable) {
  if (!IsManifestValidForWebApp(manifest_)) {
    ...
  } else if (service_worker_error_code_ != NO_ERROR_DETECTED) {
    code = service_worker_error_code_;
  }
}

SetServiceWorkerErrorCode() would replace SetInstallable() and
SetShouldInstallableTimeOut()

[dominickn, 2017/07/05 07:03:26]

The initial manifest fetch is always made, and it could time out too (and that's
a separate code path so it should be tested).

Since we're no longer calling the callback on a delayed basis, I think using
booleans to indicate if the installable check should time out is sufficient
again (I don't really think it's necessary to check the distinction between
NO_MATCHING_SERVICE_WORKER or NOT_OFFLINE_CAPABLE in here).

[pkotwicz, 2017/07/05 21:37:42]

Because SetShouldInstallableTimeOut() takes precedence over SetInstallable(), I
think that we can safely remove all of the calls to SetInstallable(). All of the
current calls to SetInstallable(false) occur occur when
SetShouldInstallableTimeOut(true)

[pkotwicz, 2017/07/06 02:03:16]

Ping on this comment

[dominickn, 2017/07/06 02:53:39]

SetInstallable defaults to false and is explicitly set to true in a couple of
tests. Removing it doesn't make sense for the NOT_OFFLINE_CAPABLE case which
will deterministically return.

+      return;
+    }
+
+    std::move(callback_).Run();
+  }
+
+  bool IsDeferredCallbackNull() { return callback_.is_null(); }
+
+  void RunCallback() {
+    EXPECT_FALSE(IsDeferredCallbackNull());
+    std::move(callback_).Run();
+  }
+
+  void SetStatus(InstallableStatusCode code, bool is_installable) {
+    code_ = code;
+    is_installable_ = is_installable;
+  }
+
+  void SetManifest(const GURL& url, const content::Manifest& manifest) {
+    manifest_url_ = url;
+    manifest_ = manifest;
+  }
+
+  void SetPrimaryIcon(const GURL& url, const SkBitmap& icon) {
+    primary_icon_url_ = url;
+    primary_icon_ = icon;
+  }
+
+  void SetBadgeIcon(const GURL& url, const SkBitmap& icon) {
+    badge_icon_url_ = url;
+    badge_icon_ = icon;
+  }
+
+  void SetShouldManifestTimeOut(bool should_time_out) {
+    should_manifest_time_out_ = should_time_out;
+  }
+
+  void SetShouldInstallableTimeOut(bool should_time_out) {
+    should_installable_time_out_ = should_time_out;
+  }
+
+ private:
+  base::OnceClosure callback_;
+  InstallableStatusCode code_;
+  content::Manifest manifest_;
+  GURL manifest_url_;

[pkotwicz, 2017/06/29 19:22:05]

It seems like this is always kDefaultManifestUrl. Can we remove the 
|manifest_url_| variable?

[dominickn, 2017/06/30 02:42:32]

Done.

+  GURL primary_icon_url_;
+  GURL badge_icon_url_;
+  SkBitmap primary_icon_;
+  SkBitmap badge_icon_;
+
+  bool is_installable_ = false;

[pkotwicz, 2017/06/29 19:22:05]

Should this variable be renamed to |has_service_worker_|

[dominickn, 2017/06/30 02:42:31]

I want to keep it is_installable_ to match the name of the parameter passed back
from GetData(). It also checks if the manifest has display: standalone, which is
another separate thing we could test.

+
+  bool should_manifest_time_out_ = false;
+  bool should_installable_time_out_ = false;
+};
+
 // Tests AddToHomescreenDataFetcher. These tests should be browser tests but
 // Android does not support browser tests yet (crbug.com/611756).
 class AddToHomescreenDataFetcherTest : public ChromeRenderViewHostTestHarness {
@@ -212,23 +237,15 @@ class AddToHomescreenDataFetcherTest : public ChromeRenderViewHostTestHarness {
     ASSERT_TRUE(profile()->CreateHistoryService(false, true));
     profile()->CreateFaviconService();
 
-    embedded_worker_test_helper_.reset(
-        new content::EmbeddedWorkerTestHelper(base::FilePath()));
-
-    scoped_refptr<content::SiteInstance> site_instance =
-        content::SiteInstance::Create(browser_context());
-    site_instance->GetProcess()->Init();
-    MockWebContents* mock_web_contents = new MockWebContents(browser_context());
-    mock_web_contents->Init(content::WebContents::CreateParams(
-        browser_context(), std::move(site_instance)));
-    InstallableManager::CreateForWebContents(mock_web_contents);
-    SetContents(mock_web_contents);
-    NavigateAndCommit(GURL(kDefaultStartUrl));
-  }
-
-  void TearDown() override {
-    embedded_worker_test_helper_.reset();
-    ChromeRenderViewHostTestHarness::TearDown();
+    // Manually inject the TestInstallableManager as a "InstallableManager"
+    // WebContentsUserData. We can't directly call ::CreateForWebContents due to
+    // typing issues since TestInstallableManager doesn't directly inherit from
+    // WebContentsUserData.
+    web_contents()->SetUserData(
+        TestInstallableManager::UserDataKey(),
+        base::WrapUnique(new TestInstallableManager(web_contents())));
+    installable_manager_ = static_cast<TestInstallableManager*>(
+        web_contents()->GetUserData(TestInstallableManager::UserDataKey()));
   }
 
   scoped_refptr<AddToHomescreenDataFetcher> BuildFetcher(
@@ -238,50 +255,70 @@ class AddToHomescreenDataFetcherTest : public ChromeRenderViewHostTestHarness {
                                           check_webapk_compatible, observer);
   }
 
-  // Set the manifest to be returned as a result of WebContents::GetManifest().
-  void SetManifest(const GURL& manifest_url,
-                   const content::Manifest& manifest) {
-    MockWebContents* mock_web_contents =
-        static_cast<MockWebContents*>(web_contents());
-    mock_web_contents->SetManifest(manifest_url, manifest);
+  void RunFetcher(scoped_refptr<AddToHomescreenDataFetcher> fetcher,
+                  ObserverWaiter& waiter,
+                  const char* expected_title,
+                  blink::WebDisplayMode display_mode,
+                  bool is_webapk_compatible) {
+    WebApplicationInfo web_application_info;
+    web_application_info.title = base::UTF8ToUTF16(kWebApplicationInfoTitle);
+
+    fetcher->OnDidGetWebApplicationInfo(web_application_info);
+    waiter.WaitForDataAvailable();
+
+    EXPECT_EQ(check_webapk_compatibility(),
+              waiter.determined_webapk_compatibility());
+    EXPECT_EQ(is_webapk_compatible, waiter.is_webapk_compatible());
+    EXPECT_TRUE(waiter.title_available());
+    EXPECT_TRUE(base::EqualsASCII(waiter.title(), expected_title));
+    EXPECT_TRUE(
+        base::EqualsASCII(fetcher->shortcut_info().user_title, expected_title));
+    EXPECT_EQ(display_mode, fetcher->shortcut_info().display);
   }

[pkotwicz, 2017/06/29 19:22:05]

Maybe we can make RunFetcher() call a helper function

void CheckObserversNotified(ObserverWaiter& observer, int expected_count) {
  EXPECT_EQ(expected_count, observer.determined_webapk_compatibility_count());
  EXPECT_EQ(expected_count, observer.title_available_count());
  EXPECT_EQ(expected_count, observer.on_data_available_count());
}

- The benefit of having CheckObserversNotified() is that you can run it after:
waiter.ResetCounts()
RunDeferredCallback()

- I think that having accessors for the number of times that
OnDidDetermineWebApkCompatibility() and OnUserTitleAvailable() and
OnDataAvailable() are called is clearer than having EXPECT() calls in
ObserverWaiter

[dominickn, 2017/06/30 02:42:31]

I don't see a strong enough benefit here - just using scopes and refreshing the
state entirely with a new object seems just as clear. We don't have any scenario
where it's correct to call the observer methods more than once, but keeping
counts implies that it's okay to call them more than once. The EXPECT is very
explicit that for each observer's lifetime, it should get one call.

 
-  void SetShouldImageTimeOut(bool should_time_out) {
-    MockWebContents* mock_web_contents =
-        static_cast<MockWebContents*>(web_contents());
-    mock_web_contents->SetShouldImageTimeOut(should_time_out);
+  void SetManifest(const content::Manifest& manifest) {
+    installable_manager_->SetManifest(GURL(kDefaultManifestUrl), manifest);
+    installable_manager_->SetStatus(NO_ACCEPTABLE_ICON, false);
+  }
+
+  void SetManifestAndIcons(const content::Manifest& manifest) {
+    SetManifest(manifest);
+    installable_manager_->SetPrimaryIcon(
+        GURL(kDefaultIconUrl),
+        gfx::test::CreateBitmap(kIconSizePx, kIconSizePx));
+    installable_manager_->SetStatus(NO_ERROR_DETECTED, false);
+  }
+
+  void SetInstallable(const content::Manifest& manifest) {
+    SetManifestAndIcons(manifest);
+    installable_manager_->SetStatus(NO_ERROR_DETECTED, true);
+  }
+
+  void SetInstallableAndBadgeIcon(const content::Manifest& manifest) {
+    SetInstallable(manifest);
+    installable_manager_->SetBadgeIcon(
+        GURL(kDefaultIconUrl),
+        gfx::test::CreateBitmap(kIconSizePx, kIconSizePx));
   }
 
   void SetShouldManifestTimeOut(bool should_time_out) {
-    MockWebContents* mock_web_contents =
-        static_cast<MockWebContents*>(web_contents());
-    mock_web_contents->SetShouldManifestTimeOut(should_time_out);
+    installable_manager_->SetShouldManifestTimeOut(should_time_out);
   }
 
-  // Registers service worker at |url|. Blocks till the service worker is
-  // registered.
-  void RegisterServiceWorker(const GURL& url) {
-    base::RunLoop run_loop;
-    embedded_worker_test_helper_->context()->RegisterServiceWorker(
-        url, GURL(url.spec() + "/service_worker.js"), nullptr,
-        base::Bind(&AddToHomescreenDataFetcherTest::OnServiceWorkerRegistered,
-                   base::Unretained(this), run_loop.QuitClosure()));
+  void SetShouldInstallableTimeOut(bool should_time_out) {
+    installable_manager_->SetShouldInstallableTimeOut(should_time_out);
   }
 
- private:
-  // Callback for RegisterServiceWorker() for when service worker registration
-  // has completed.
-  void OnServiceWorkerRegistered(const base::Closure& callback,
-                                 content::ServiceWorkerStatusCode status,
-                                 const std::string& status_message,
-                                 int64_t registration_id) {
-    ASSERT_EQ(content::SERVICE_WORKER_OK, status)
-        << content::ServiceWorkerStatusToString(status);
-    callback.Run();
+  void RunDeferredCallback() { installable_manager_->RunCallback(); }

[pkotwicz, 2017/06/29 19:22:06]

We should call
base::RunLoop().RunUntilIdle()
after calling the callback in case the OnData() callback calls
CreateLauncherIcon()

[dominickn, 2017/06/30 02:42:31]

Done.

+
+  bool IsDeferredCallbackNull() {
+    return installable_manager_->IsDeferredCallbackNull();
   }
 
-  std::unique_ptr<content::EmbeddedWorkerTestHelper>
-      embedded_worker_test_helper_;
+  virtual bool check_webapk_compatibility() { return true; }
+
+ private:
+  TestInstallableManager* installable_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(AddToHomescreenDataFetcherTest);
 };
@@ -303,167 +340,271 @@ class AddToHomescreenDataFetcherTestCommon
 
   // The value of |check_webapk_compatible| used when building the
   // AddToHomescreenDataFetcher.
-  bool check_webapk_compatibility() { return GetParam(); }
+  bool check_webapk_compatibility() override { return GetParam(); }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(AddToHomescreenDataFetcherTestCommon);
 };
 
-// Checks that AddToHomescreenDataFetcher::Observer::OnUserTitleAvailable() is
-// called when the web manifest returned is empty. The add-to-homescreen dialog
-// makes the dialog's text field editable once OnUserTitleAvailable() is called.
 TEST_P(AddToHomescreenDataFetcherTestCommon, EmptyManifest) {
-  WebApplicationInfo web_application_info;
-  web_application_info.title = base::UTF8ToUTF16(kWebApplicationInfoTitle);
-
-  SetManifest(GURL(kDefaultManifestUrl), BuildEmptyManifest());
-
+  // Check that an empty manifest has the appropriate methods run.
   ObserverWaiter waiter;
   scoped_refptr<AddToHomescreenDataFetcher> fetcher(BuildFetcher(&waiter));
-  fetcher->OnDidGetWebApplicationInfo(web_application_info);
-  waiter.WaitForDataAvailable();
-
-  EXPECT_EQ(check_webapk_compatibility(),
-            waiter.determined_webapk_compatibility());
-  EXPECT_FALSE(waiter.is_webapk_compatible());
-  EXPECT_TRUE(waiter.title_available());
-  EXPECT_TRUE(base::EqualsASCII(waiter.title(), kWebApplicationInfoTitle));
-
+  RunFetcher(fetcher, waiter, kWebApplicationInfoTitle,
+             blink::kWebDisplayModeBrowser, false);
+  EXPECT_TRUE(IsDeferredCallbackNull());
   fetcher->set_weak_observer(nullptr);
 }
 
-// Test that when the manifest provides Manifest::short_name but not
-// Manifest::name that Manifest::short_name is used as the name instead of
-// WebApplicationInfo::title.
-TEST_P(AddToHomescreenDataFetcherTestCommon,
-       ManifestShortNameClobbersWebApplicationName) {
-  WebApplicationInfo web_application_info;
-  web_application_info.title = base::UTF8ToUTF16(kWebApplicationInfoTitle);
-
-  content::Manifest manifest(BuildDefaultManifest());
-  manifest.name = base::NullableString16();
-
-  RegisterServiceWorker(GURL(kDefaultStartUrl));
-  SetManifest(GURL(kDefaultManifestUrl), manifest);
-
+TEST_P(AddToHomescreenDataFetcherTestCommon, NoIconManifest) {
+  // Test a manifest with no icons. This should only use the metadata title and

[pkotwicz, 2017/06/29 19:22:06]

Nit: "only use" -> "use"

[dominickn, 2017/06/30 02:42:31]

Done.

+  // have an empty icon in this test runner (in production the empty icon would
+  // be replaced by a favicon or a generated icon).

[pkotwicz, 2017/06/29 19:22:06]

Thank you for the awesome explanatory comment! Things are much clearer now

[dominickn, 2017/06/30 02:42:31]

:)

+  SetManifest(BuildNoIconManifest());
   ObserverWaiter waiter;
   scoped_refptr<AddToHomescreenDataFetcher> fetcher(BuildFetcher(&waiter));
-  fetcher->OnDidGetWebApplicationInfo(web_application_info);
-  waiter.WaitForDataAvailable();
+  RunFetcher(fetcher, waiter, kDefaultManifestShortName,
+             blink::kWebDisplayModeStandalone, false);
+  EXPECT_TRUE(IsDeferredCallbackNull());
 
-  EXPECT_TRUE(base::EqualsASCII(waiter.title(), kDefaultManifestShortName));
-  EXPECT_TRUE(base::EqualsASCII(fetcher->shortcut_info().name,
-                                kDefaultManifestShortName));
+  EXPECT_TRUE(fetcher->primary_icon().drawsNothing());
+  EXPECT_TRUE(fetcher->shortcut_info().best_primary_icon_url.is_empty());
+  EXPECT_TRUE(fetcher->badge_icon().drawsNothing());
+  EXPECT_TRUE(fetcher->shortcut_info().best_badge_icon_url.is_empty());
 
   fetcher->set_weak_observer(nullptr);
 }
 
-// Test that when the manifest does not provide either Manifest::short_name nor
-// Manifest::name that:
-// - The page is not WebAPK compatible.
-// - WebApplicationInfo::title is used as the "name".
-TEST_P(AddToHomescreenDataFetcherTestCommon, ManifestNoNameNoShortName) {
-  WebApplicationInfo web_application_info;
-  web_application_info.title = base::UTF8ToUTF16(kWebApplicationInfoTitle);
+TEST_P(AddToHomescreenDataFetcherTestCommon, ManifestFetchTimesOut) {
+  // Check that the AddToHomescreenDataFetcher::Observer methods are called
+  // if the first call to InstallableManager::GetData times out. This should
+  // fall back to the metadata title and have an empty icon.
+  SetShouldManifestTimeOut(true);
 
-  content::Manifest manifest(BuildDefaultManifest());
-  manifest.name = base::NullableString16();
-  manifest.short_name = base::NullableString16();
+  {
+    // Check with a site that has a manifest and icons, but no service worker.

[pkotwicz, 2017/06/29 19:22:05]

If the manifest-fetch times out, I don't think that the nature of the manifest
matters

Maybe change the comment to:
"Check with a site with no service worker."

[dominickn, 2017/06/30 02:42:32]

Done.

+    SetManifestAndIcons(BuildDefaultManifest());
+    ObserverWaiter waiter;
+    scoped_refptr<AddToHomescreenDataFetcher> fetcher(BuildFetcher(&waiter));
+    RunFetcher(fetcher, waiter, kWebApplicationInfoTitle,
+               blink::kWebDisplayModeBrowser, false);
 
-  RegisterServiceWorker(GURL(kDefaultStartUrl));
-  SetManifest(GURL(kDefaultManifestUrl), manifest);
+    EXPECT_TRUE(fetcher->primary_icon().drawsNothing());
+    EXPECT_TRUE(fetcher->shortcut_info().best_primary_icon_url.is_empty());
 
-  ObserverWaiter waiter;
-  scoped_refptr<AddToHomescreenDataFetcher> fetcher(BuildFetcher(&waiter));
-  fetcher->OnDidGetWebApplicationInfo(web_application_info);
-  waiter.WaitForDataAvailable();
+    // Ensure GetData can call the data fetcher callback after the time out.

[pkotwicz, 2017/06/29 19:22:06]

Nit: "GetData" -> "GetData()"

Maybe add: "Test that the GetData() call does not call any of the observer's
methods a second time."

[dominickn, 2017/06/30 02:42:31]

Done.

+    RunDeferredCallback();
+    fetcher->set_weak_observer(nullptr);
+  }
 
-  EXPECT_EQ(check_webapk_compatibility(),
-            waiter.determined_webapk_compatibility());
-  EXPECT_FALSE(waiter.is_webapk_compatible());
-  EXPECT_TRUE(base::EqualsASCII(waiter.title(), kWebApplicationInfoTitle));
-  EXPECT_TRUE(base::EqualsASCII(fetcher->shortcut_info().name,
-                                kWebApplicationInfoTitle));
+  {
+    // Check with a service worker present.
+    SetInstallable(BuildDefaultManifest());
+    ObserverWaiter waiter;
+    scoped_refptr<AddToHomescreenDataFetcher> fetcher(BuildFetcher(&waiter));
+    RunFetcher(fetcher, waiter, kWebApplicationInfoTitle,
+               blink::kWebDisplayModeBrowser, false);
 
-  fetcher->set_weak_observer(nullptr);
-}
+    EXPECT_TRUE(fetcher->primary_icon().drawsNothing());
+    EXPECT_TRUE(fetcher->shortcut_info().best_primary_icon_url.is_empty());
 
-// Checks that the AddToHomescreenDataFetcher::Observer callbacks are called
-// when the manifest fetch times out.
-TEST_P(AddToHomescreenDataFetcherTestCommon, ManifestFetchTimesOut) {
-  WebApplicationInfo web_application_info;
-  web_application_info.title = base::UTF8ToUTF16(kWebApplicationInfoTitle);
+    // Ensure GetData can call the data fetcher callback after the time out.
+    RunDeferredCallback();
+    fetcher->set_weak_observer(nullptr);
+  }
+}
 
-  RegisterServiceWorker(GURL(kDefaultStartUrl));
-  SetManifest(GURL(kDefaultManifestUrl), BuildDefaultManifest());
-  SetShouldManifestTimeOut(true);
-  SetShouldImageTimeOut(false);
+TEST_F(AddToHomescreenDataFetcherTest, ServiceWorkerCheckTimesOut) {

[pkotwicz, 2017/06/29 19:22:05]

Because this is the only TEST_F() test, you can probably make this one TEST_P()
as well and early return if !check_webapk_compatibility()

[dominickn, 2017/06/30 02:42:31]

That leaves a do-nothing test, which is a bit misleading. I'd prefer to keep it
a TEST_F if that's okay.

[pkotwicz, 2017/06/30 23:54:52]

Fair enough

+  // Check that the AddToHomescreenDataFetcher::Observer methods are called
+  // if the service worker check times out (e.g. because we are waiting
+  // to detect a worker). This should use the short_name and icon from the
+  // manifest, but not be WebAPK-compatible. Only relevant when checking WebAPK
+  // compatibility.

[pkotwicz, 2017/06/29 19:22:06]

Can you make it clear that this test tests the case of a page which has a
service worker but where AddToHomescreenDataFetcher returns prior to the
asynchronous service worker check completing.

We can optionally have a separate test case which tests a page with an
installable manifest but no service worker

The cases are different because the error code sent as a result of
RunDeferredCallback() is different

[dominickn, 2017/06/30 02:42:31]

Very subtle difference. Added a new test for it.

+  SetInstallable(BuildDefaultManifest());
+  SetShouldInstallableTimeOut(true);
 
   ObserverWaiter waiter;
-  scoped_refptr<AddToHomescreenDataFetcher> fetcher(BuildFetcher(&waiter));
-  fetcher->OnDidGetWebApplicationInfo(web_application_info);
-  waiter.WaitForDataAvailable();
+  scoped_refptr<AddToHomescreenDataFetcher> fetcher(
+      BuildFetcher(true, &waiter));
+  RunFetcher(fetcher, waiter, kDefaultManifestShortName,
+             blink::kWebDisplayModeStandalone, false);
 
-  EXPECT_EQ(check_webapk_compatibility(),
-            waiter.determined_webapk_compatibility());
-  EXPECT_FALSE(waiter.is_webapk_compatible());
-  EXPECT_TRUE(base::EqualsASCII(waiter.title(), kWebApplicationInfoTitle));
-  EXPECT_TRUE(waiter.title_available());
+  EXPECT_FALSE(fetcher->primary_icon().drawsNothing());
+  EXPECT_EQ(fetcher->shortcut_info().best_primary_icon_url,
+            GURL(kDefaultIconUrl));
 
+  // Ensure GetData can call the data fetcher callback after the time out.
+  RunDeferredCallback();
   fetcher->set_weak_observer(nullptr);
 }
 
-// Checks that the AddToHomescreenDataFetcher::Observer callbacks are called
-// when the image fetch times out.
-TEST_P(AddToHomescreenDataFetcherTestCommon, ImageFetchTimesOut) {
-  WebApplicationInfo web_application_info;
-  web_application_info.title = base::UTF8ToUTF16(kWebApplicationInfoTitle);
-
-  RegisterServiceWorker(GURL(kDefaultStartUrl));
-  SetManifest(GURL(kDefaultManifestUrl), BuildDefaultManifest());
-  SetShouldManifestTimeOut(false);
-  SetShouldImageTimeOut(true);
-
-  ObserverWaiter waiter;
-  scoped_refptr<AddToHomescreenDataFetcher> fetcher(BuildFetcher(&waiter));
-  fetcher->OnDidGetWebApplicationInfo(web_application_info);
-  waiter.WaitForDataAvailable();
+TEST_P(AddToHomescreenDataFetcherTestCommon, InstallableManifest) {
+  content::Manifest manifest(BuildDefaultManifest());
 
-  EXPECT_EQ(check_webapk_compatibility(),
-            waiter.determined_webapk_compatibility());
-  EXPECT_FALSE(waiter.is_webapk_compatible());
-  EXPECT_TRUE(waiter.title_available());
-  EXPECT_TRUE(base::EqualsASCII(waiter.title(), kWebApplicationInfoTitle));
+  {
+    // Test an installable manifest for a site that has a service worker.
+    SetInstallable(manifest);
+    ObserverWaiter waiter;
+    scoped_refptr<AddToHomescreenDataFetcher> fetcher(BuildFetcher(&waiter));
+    RunFetcher(fetcher, waiter, kDefaultManifestShortName,
+               blink::kWebDisplayModeStandalone, check_webapk_compatibility());
+    EXPECT_TRUE(IsDeferredCallbackNull());
+
+    EXPECT_FALSE(fetcher->primary_icon().drawsNothing());
+    EXPECT_EQ(fetcher->shortcut_info().best_primary_icon_url,
+              GURL(kDefaultIconUrl));
+    EXPECT_TRUE(fetcher->badge_icon().drawsNothing());
+    EXPECT_TRUE(fetcher->shortcut_info().best_badge_icon_url.is_empty());
+    fetcher->set_weak_observer(nullptr);
+  }
 
-  fetcher->set_weak_observer(nullptr);
+  {
+    // Test that the provided badge icon is used when checking for WebAPK
+    // compatibility.

[pkotwicz, 2017/06/29 19:22:06]

Maybe change the comment to:
"Check that the badge icon is requested only when AddToHomescreenDataFetcher
checks for WebAPK compatibility."

This test case should be in its own test. Perhaps:
TEST_P(AddToHomescreenDataFetcherTestCommon,
BadgeIconOnlyRequestedWhenCheckingForWebApkCompatibility) {}

[dominickn, 2017/06/30 02:42:31]

Done.

+    SetInstallableAndBadgeIcon(BuildDefaultManifest());
+    ObserverWaiter waiter;
+    scoped_refptr<AddToHomescreenDataFetcher> fetcher(BuildFetcher(&waiter));
+    RunFetcher(fetcher, waiter, kDefaultManifestShortName,
+               blink::kWebDisplayModeStandalone, check_webapk_compatibility());
+    EXPECT_TRUE(IsDeferredCallbackNull());
+
+    EXPECT_FALSE(fetcher->primary_icon().drawsNothing());
+    EXPECT_EQ(fetcher->shortcut_info().best_primary_icon_url,
+              GURL(kDefaultIconUrl));
+    if (check_webapk_compatibility()) {
+      EXPECT_FALSE(fetcher->badge_icon().drawsNothing());
+      EXPECT_EQ(fetcher->shortcut_info().best_badge_icon_url,
+                GURL(kDefaultIconUrl));
+    }
+    fetcher->set_weak_observer(nullptr);
+  }
 }
 
-// Checks that the AddToHomescreenDataFetcher::Observer callbacks are called
-// when the service worker check times out.
-TEST_P(AddToHomescreenDataFetcherTestCommon, ServiceWorkerCheckTimesOut) {
-  WebApplicationInfo web_application_info;
-  web_application_info.title = base::UTF8ToUTF16(kWebApplicationInfoTitle);
+TEST_P(AddToHomescreenDataFetcherTestCommon,
+       ManifestShortNameClobbersWebApplicationName) {
+  // Test that when the manifest provides Manifest::short_name but not
+  // Manifest::name that Manifest::short_name is used as the name.
+  {
+    // Check the case where we have no icons.
+    content::Manifest manifest(BuildNoIconManifest());
+    manifest.name = base::NullableString16();
+    SetManifest(manifest);
+
+    ObserverWaiter waiter;
+    scoped_refptr<AddToHomescreenDataFetcher> fetcher(BuildFetcher(&waiter));
+    RunFetcher(fetcher, waiter, kDefaultManifestShortName,
+               blink::kWebDisplayModeStandalone, false);
+    EXPECT_TRUE(IsDeferredCallbackNull());
+
+    EXPECT_TRUE(fetcher->primary_icon().drawsNothing());
+    EXPECT_TRUE(fetcher->shortcut_info().best_primary_icon_url.is_empty());
+    EXPECT_TRUE(base::EqualsASCII(fetcher->shortcut_info().name,
+                                  kDefaultManifestShortName));
+
+    fetcher->set_weak_observer(nullptr);
+  }
 
-  // Not registering a service worker means we'll wait and time out for the
-  // worker.
-  SetManifest(GURL(kDefaultManifestUrl), BuildDefaultManifest());
-  SetShouldManifestTimeOut(false);
-  SetShouldImageTimeOut(false);
+  {
+    // Check the case where the service worker check times out if we are
+    // checking WebAPK compatibility.

[pkotwicz, 2017/06/29 19:22:05]

Maybe change the comment to: "Check page with no service worker." This way the
comment applies regardless of whether AddToHomescreenDataFetcher checks for
WebAPK-compatibility

[dominickn, 2017/06/30 02:42:30]

Done.

+    content::Manifest manifest(BuildDefaultManifest());
+    manifest.name = base::NullableString16();
+    SetInstallable(manifest);
+    SetShouldInstallableTimeOut(true);
+
+    ObserverWaiter waiter;
+    scoped_refptr<AddToHomescreenDataFetcher> fetcher(BuildFetcher(&waiter));
+    RunFetcher(fetcher, waiter, kDefaultManifestShortName,
+               blink::kWebDisplayModeStandalone, false);
+
+    EXPECT_FALSE(fetcher->primary_icon().drawsNothing());
+    EXPECT_EQ(fetcher->shortcut_info().best_primary_icon_url,
+              GURL(kDefaultIconUrl));
+    EXPECT_TRUE(base::EqualsASCII(fetcher->shortcut_info().name,
+                                  kDefaultManifestShortName));
+
+    // Ensure GetData can call the data fetcher callback after the time out.
+    if (check_webapk_compatibility())
+      RunDeferredCallback();

[pkotwicz, 2017/06/29 19:22:06]

Maybe change the comment to:
if (check_webapk_compatibility() {
  // InstallableManager::GetData() should have timed out waiting for a service
  // worker to be registered. Test that the GetData() call does not call any
  // of the observer's methods a second time.
  RunDeferredCallback();
}

[dominickn, 2017/06/30 02:42:31]

Done.

+
+    EXPECT_TRUE(IsDeferredCallbackNull());
+    fetcher->set_weak_observer(nullptr);
+  }
 
-  ObserverWaiter waiter;
-  scoped_refptr<AddToHomescreenDataFetcher> fetcher(BuildFetcher(&waiter));
-  fetcher->OnDidGetWebApplicationInfo(web_application_info);
-  waiter.WaitForDataAvailable();
+  {
+    // Check the case where the service worker check doesn't time out.

[pkotwicz, 2017/06/29 19:22:06]

Maybe change the comment to: "Check page with service worker."

[dominickn, 2017/06/30 02:42:31]

Done.

+    SetShouldInstallableTimeOut(false);
+    ObserverWaiter waiter;
+    scoped_refptr<AddToHomescreenDataFetcher> fetcher(BuildFetcher(&waiter));
+    RunFetcher(fetcher, waiter, kDefaultManifestShortName,
+               blink::kWebDisplayModeStandalone, check_webapk_compatibility());
+    EXPECT_TRUE(IsDeferredCallbackNull());
+
+    EXPECT_FALSE(fetcher->primary_icon().drawsNothing());
+    EXPECT_EQ(fetcher->shortcut_info().best_primary_icon_url,
+              GURL(kDefaultIconUrl));
+    EXPECT_TRUE(base::EqualsASCII(fetcher->shortcut_info().name,
+                                  kDefaultManifestShortName));
+
+    fetcher->set_weak_observer(nullptr);
+  }
+}
 
-  EXPECT_EQ(check_webapk_compatibility(),
-            waiter.determined_webapk_compatibility());
-  EXPECT_FALSE(waiter.is_webapk_compatible());
-  EXPECT_TRUE(waiter.title_available());
-  EXPECT_TRUE(base::EqualsASCII(waiter.title(), kDefaultManifestShortName));
-  EXPECT_TRUE(base::EqualsASCII(fetcher->shortcut_info().user_title,
-                                kDefaultManifestShortName));
+TEST_P(AddToHomescreenDataFetcherTestCommon, ManifestNoNameNoShortName) {
+  // Test that when the manifest does not provide either Manifest::short_name
+  // nor Manifest::name that: - The page is not WebAPK compatible. -
+  // WebApplicationInfo::title is used as the "name". - We still use the icons
+  // from the manifest.

[pkotwicz, 2017/06/29 19:22:05]

Nit: Please put each '-' on a separate line

[dominickn, 2017/06/30 02:42:31]

Ugh that was a clang-format fail.

+  content::Manifest manifest(BuildDefaultManifest());
+  manifest.name = base::NullableString16();
+  manifest.short_name = base::NullableString16();
 
-  fetcher->set_weak_observer(nullptr);
+  {
+    // Check the case where waiting for the service worker times out if we check
+    // WebAPK compatibility.
+    SetManifestAndIcons(manifest);
+    SetShouldInstallableTimeOut(true);

[pkotwicz, 2017/06/29 19:22:06]

This case cannot occur in practice because IsManifestValidForWebApp() sets the
MANIFEST_MISSING_NAME_OR_SHORT_NAME error code.

The InstallableManager should not time out

[dominickn, 2017/06/30 02:42:30]

Done.

+    ObserverWaiter waiter;
+    scoped_refptr<AddToHomescreenDataFetcher> fetcher(BuildFetcher(&waiter));
+    RunFetcher(fetcher, waiter, kWebApplicationInfoTitle,
+               blink::kWebDisplayModeStandalone, false);
+
+    EXPECT_TRUE(base::EqualsASCII(fetcher->shortcut_info().name,
+                                  kWebApplicationInfoTitle));
+    EXPECT_TRUE(base::EqualsASCII(fetcher->shortcut_info().short_name,
+                                  kWebApplicationInfoTitle));
+    EXPECT_FALSE(fetcher->primary_icon().drawsNothing());
+    EXPECT_EQ(fetcher->shortcut_info().best_primary_icon_url,
+              GURL(kDefaultIconUrl));
+
+    // Ensure GetData can call the data fetcher callback after the time out.
+    if (check_webapk_compatibility())
+      RunDeferredCallback();
+
+    EXPECT_TRUE(IsDeferredCallbackNull());
+    fetcher->set_weak_observer(nullptr);
+  }
+
+  {
+    // Check the case where we don't time out waiting for the service worker.
+    SetInstallable(manifest);
+    SetShouldInstallableTimeOut(false);
+    ObserverWaiter waiter;
+    scoped_refptr<AddToHomescreenDataFetcher> fetcher(BuildFetcher(&waiter));
+    RunFetcher(fetcher, waiter, kWebApplicationInfoTitle,
+               blink::kWebDisplayModeStandalone, false);
+    EXPECT_TRUE(IsDeferredCallbackNull());
+
+    EXPECT_TRUE(base::EqualsASCII(fetcher->shortcut_info().name,
+                                  kWebApplicationInfoTitle));
+    EXPECT_TRUE(base::EqualsASCII(fetcher->shortcut_info().short_name,
+                                  kWebApplicationInfoTitle));
+    EXPECT_FALSE(fetcher->primary_icon().drawsNothing());
+    EXPECT_EQ(fetcher->shortcut_info().best_primary_icon_url,
+              GURL(kDefaultIconUrl));
+
+    fetcher->set_weak_observer(nullptr);
+  }
 }
 
 INSTANTIATE_TEST_CASE_P(CheckWebApkCompatibility,