App banner requires image/png icon. Really.

This is fixing the stupid mistake and also adding some unit tests.

BUG=462551

Review URL: https://codereview.chromium.org/961313002

Cr-Commit-Position: refs/heads/master@{#318505}

4 files changed, 126 insertions, 5 deletions

diff --git a/chrome/browser/android/banners/app_banner_manager.cc b/chrome/browser/android/banners/app_banner_manager.cc
index f428abb..0283d2e 100644
--- a/chrome/browser/android/banners/app_banner_manager.cc
+++ b/chrome/browser/android/banners/app_banner_manager.cc
@@ -54,7 +54,7 @@ const int kIconMinimumSize = 144;
 // The requirement for now is an image/png that is at least 144x144.
 bool DoesManifestContainRequiredIcon(const content::Manifest& manifest) {
   for (const auto& icon : manifest.icons) {
-    if (EqualsASCII(icon.type.string(), "image/png"))
+    if (!EqualsASCII(icon.type.string(), "image/png"))
       continue;
 
     for (const auto& size : icon.sizes) {
@@ -166,14 +166,25 @@ void AppBannerManager::DidFinishLoad(
                                          weak_factory_.GetWeakPtr()));
 }
 
+// static
+bool AppBannerManager::IsManifestValid(const content::Manifest& manifest) {
+  if (manifest.IsEmpty())
+    return false;
+  if (!manifest.start_url.is_valid())
+    return false;
+  if (manifest.name.is_null() && manifest.short_name.is_null())
+    return false;
+  if (!DoesManifestContainRequiredIcon(manifest))
+    return false;
+
+  return true;
+}
+
 void AppBannerManager::OnDidGetManifest(const content::Manifest& manifest) {
   if (web_contents()->IsBeingDestroyed())
     return;
 
-  if (manifest.IsEmpty()
-      || !manifest.start_url.is_valid()
-      || (manifest.name.is_null() && manifest.short_name.is_null())
-      || !DoesManifestContainRequiredIcon(manifest)) {
+  if (!IsManifestValid(manifest)) {
     // No usable manifest, see if there is a play store meta tag.
     if (!IsEnabledForNativeApps())
       return;
diff --git a/chrome/browser/android/banners/app_banner_manager.h b/chrome/browser/android/banners/app_banner_manager.h
index d4f7c26..78aa3d3 100644
--- a/chrome/browser/android/banners/app_banner_manager.h
+++ b/chrome/browser/android/banners/app_banner_manager.h
@@ -87,6 +87,12 @@ class AppBannerManager : public content::WebContentsObserver {
   bool OnMessageReceived(const IPC::Message& message) override;
 
  private:
+  friend class AppBannerManagerTest;
+
+  // Returns whether the given Manifest is following the requirements to show
+  // a web app banner.
+  static bool IsManifestValid(const content::Manifest& manifest);
+
   // Called when the manifest has been retrieved, or if there is no manifest to
   // retrieve.
   void OnDidGetManifest(const content::Manifest& manifest);
diff --git a/chrome/browser/android/banners/app_banner_manager_unittest.cc b/chrome/browser/android/banners/app_banner_manager_unittest.cc
new file mode 100644
index 0000000..93b94ea
--- /dev/null
+++ b/chrome/browser/android/banners/app_banner_manager_unittest.cc
@@ -0,0 +1,103 @@
+// 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/banners/app_banner_manager.h"
+
+#include "base/strings/utf_string_conversions.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace banners {
+
+class AppBannerManagerTest : public testing::Test {
+ protected:
+  static base::NullableString16 ToNullableUTF16(const std::string& str) {
+    return base::NullableString16(base::UTF8ToUTF16(str), false);
+  }
+
+  static content::Manifest GetValidManifest() {
+    content::Manifest manifest;
+    manifest.name = ToNullableUTF16("foo");
+    manifest.short_name = ToNullableUTF16("bar");
+    manifest.start_url = GURL("http://example.com");
+
+    content::Manifest::Icon icon;
+    icon.type = ToNullableUTF16("image/png");
+    icon.sizes.push_back(gfx::Size(144, 144));
+    manifest.icons.push_back(icon);
+
+    return manifest;
+  }
+  static bool IsManifestValid(const content::Manifest& manifest) {
+    return AppBannerManager::IsManifestValid(manifest);
+  }
+};
+
+TEST_F(AppBannerManagerTest, EmptyManifestIsInvalid) {
+  content::Manifest manifest;
+  EXPECT_FALSE(IsManifestValid(manifest));
+}
+
+TEST_F(AppBannerManagerTest, CheckMinimalValidManifest) {
+  content::Manifest manifest = GetValidManifest();
+  EXPECT_TRUE(IsManifestValid(manifest));
+}
+
+TEST_F(AppBannerManagerTest, ManifestRequiresNameORShortName) {
+  content::Manifest manifest = GetValidManifest();
+
+  manifest.name = base::NullableString16();
+  EXPECT_TRUE(IsManifestValid(manifest));
+
+  manifest.name = ToNullableUTF16("foo");
+  manifest.short_name = base::NullableString16();
+  EXPECT_TRUE(IsManifestValid(manifest));
+
+  manifest.name = base::NullableString16();
+  EXPECT_FALSE(IsManifestValid(manifest));
+}
+
+TEST_F(AppBannerManagerTest, ManifestRequiresValidStartURL) {
+  content::Manifest manifest = GetValidManifest();
+
+  manifest.start_url = GURL();
+  EXPECT_FALSE(IsManifestValid(manifest));
+
+  manifest.start_url = GURL("/");
+  EXPECT_FALSE(IsManifestValid(manifest));
+}
+
+TEST_F(AppBannerManagerTest, ManifestRequiresImagePNG) {
+  content::Manifest manifest = GetValidManifest();
+
+  manifest.icons[0].type = ToNullableUTF16("image/gif");
+  EXPECT_FALSE(IsManifestValid(manifest));
+  manifest.icons[0].type = base::NullableString16();
+  EXPECT_FALSE(IsManifestValid(manifest));
+}
+
+TEST_F(AppBannerManagerTest, ManifestRequiresMinimalSize) {
+  content::Manifest manifest = GetValidManifest();
+
+  // The icon MUST be 144x144 size at least.
+  manifest.icons[0].sizes[0] = gfx::Size(1, 1);
+  EXPECT_FALSE(IsManifestValid(manifest));
+
+  // If one of the sizes match the requirement, it should be accepted.
+  manifest.icons[0].sizes.push_back(gfx::Size(144, 144));
+  EXPECT_TRUE(IsManifestValid(manifest));
+
+  // Higher than the required size is okay.
+  manifest.icons[0].sizes[1] = gfx::Size(200, 200);
+  EXPECT_TRUE(IsManifestValid(manifest));
+
+  // Non-square is okay.
+  manifest.icons[0].sizes[1] = gfx::Size(144, 200);
+  EXPECT_TRUE(IsManifestValid(manifest));
+
+  // The representation of the keyword 'any' should be recognized.
+  manifest.icons[0].sizes[1] = gfx::Size(0, 0);
+  EXPECT_TRUE(IsManifestValid(manifest));
+}
+
+}  // namespace banners
diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi
index c368c5c..e49743e 100644
--- a/chrome/chrome_tests_unit.gypi
+++ b/chrome/chrome_tests_unit.gypi
@@ -14,6 +14,7 @@
       '../tools/metrics/histograms/histograms.xml',
       # All unittests in browser, common, renderer and service.
       'browser/about_flags_unittest.cc',
+      'browser/android/banners/app_banner_manager_unittest.cc',
       'browser/android/bookmarks/partner_bookmarks_shim_unittest.cc',
       'browser/android/manifest_icon_selector_unittest.cc',
       # TODO(newt): move this to test_support_unit?