Report errors when parsing Manifest and expose them in developer console.

content/browser/manifest/manifest_browsertest.cc

Index: content/browser/manifest/manifest_browsertest.cc
diff --git a/content/browser/manifest/manifest_browsertest.cc b/content/browser/manifest/manifest_browsertest.cc
index 8eaca0a9445b8c0f71df17091ee0c244347f3a33..2cb0c428f807103975ccf54254f13d188ec1f7fa 100644
--- a/content/browser/manifest/manifest_browsertest.cc
+++ b/content/browser/manifest/manifest_browsertest.cc
@@ -17,8 +17,14 @@ namespace content {
 
 class ManifestBrowserTest : public ContentBrowserTest  {
  protected:
-  ManifestBrowserTest() {}
-  ~ManifestBrowserTest() override {}
+  ManifestBrowserTest() {
+    console_errors_count_ = 0;
+    logging::SetLogMessageHandler(&ManifestBrowserTest::CatchConsoleErrors);
+  }
+
+  ~ManifestBrowserTest() override {
+        logging::SetLogMessageHandler(nullptr);

[Peter Beverloo, 2014/11/27 13:24:31]

indentation.

[mlamouri (slow - plz ping), 2014/11/27 13:49:08]

Done.

+  }
 
   void GetManifestAndWait() {
     shell()->web_contents()->GetManifest(
@@ -38,6 +44,21 @@ class ManifestBrowserTest : public ContentBrowserTest  {
     return manifest_;
   }
 
+  static bool CatchConsoleErrors(int severity,
+                                 const char* file,
+                                 int line,
+                                 size_t message_start,
+                                 const std::string& str) {
+    if (severity == logging::LOG_INFO &&
+        file && file == std::string("CONSOLE")) {

[Peter Beverloo, 2014/11/27 13:24:31]

Mm. This feels rather fragile. Have you considered creating a 
WebContentsDelegate which overrides AddMessageToConsole? Several other tests
seem to be doing this.

[mlamouri (slow - plz ping), 2014/11/27 13:49:08]

Do you have examples?

[Peter Beverloo, 2014/11/27 13:55:15]

Not of this specific scenario. The tests using a similar technique to the one
you're using here haven't been disabled or marked as flaky, so I'm happy to try
this for now.

+      console_errors_count_++;
+    }
+
+    return false;
+  }
+
+  static int console_errors_count_;
+
  private:
   scoped_refptr<MessageLoopRunner> message_loop_runner_;
   Manifest manifest_;
@@ -45,6 +66,8 @@ class ManifestBrowserTest : public ContentBrowserTest  {
   DISALLOW_COPY_AND_ASSIGN(ManifestBrowserTest);
 };
 
+int ManifestBrowserTest::console_errors_count_ = 0;
+
 // If a page has no manifest, requesting a manifest should return the empty
 // manifest.
 IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, NoManifest) {
@@ -56,6 +79,7 @@ IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, NoManifest) {
 
   GetManifestAndWait();
   EXPECT_TRUE(manifest().IsEmpty());
+  EXPECT_EQ(0, ManifestBrowserTest::console_errors_count_);
 }
 
 // If a page manifest points to a 404 URL, requesting the manifest should return
@@ -69,6 +93,7 @@ IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, 404Manifest) {
 
   GetManifestAndWait();
   EXPECT_TRUE(manifest().IsEmpty());
+  EXPECT_EQ(0, ManifestBrowserTest::console_errors_count_);
 }
 
 // If a page has an empty manifest, requesting the manifest should return the
@@ -82,6 +107,7 @@ IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, EmptyManifest) {
 
   GetManifestAndWait();
   EXPECT_TRUE(manifest().IsEmpty());
+  EXPECT_EQ(0, ManifestBrowserTest::console_errors_count_);
 }
 
 // If a page's manifest can't be parsed correctly, requesting the manifest
@@ -95,6 +121,7 @@ IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, ParseErrorManifest) {
 
   GetManifestAndWait();
   EXPECT_TRUE(manifest().IsEmpty());
+  EXPECT_EQ(1, ManifestBrowserTest::console_errors_count_);
 }
 
 // If a page has a manifest that can be fetched and parsed, requesting the
@@ -108,6 +135,7 @@ IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, DummyManifest) {
 
   GetManifestAndWait();
   EXPECT_FALSE(manifest().IsEmpty());
+  EXPECT_EQ(0, ManifestBrowserTest::console_errors_count_);
 }
 
 // If a page changes manifest during its life-time, requesting the manifest
@@ -143,6 +171,8 @@ IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, DynamicManifest) {
     GetManifestAndWait();
     EXPECT_TRUE(manifest().IsEmpty());
   }
+
+  EXPECT_EQ(0, ManifestBrowserTest::console_errors_count_);
 }
 
 // If a page's manifest lives in a different origin, it should follow the CORS
@@ -170,6 +200,8 @@ IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, CORSManifest) {
 
   GetManifestAndWait();
   EXPECT_TRUE(manifest().IsEmpty());
+
+  EXPECT_EQ(0, ManifestBrowserTest::console_errors_count_);
 }
 
 // If a page's manifest is in an unsecure origin while the page is in a secure
@@ -197,6 +229,22 @@ IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, MixedContentManifest) {
 
   GetManifestAndWait();
   EXPECT_TRUE(manifest().IsEmpty());
+
+  EXPECT_EQ(0, ManifestBrowserTest::console_errors_count_);
+}
+
+// If a page's manifest has some parsing errors, they should show up in the
+// developer console.
+IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, ParsingErrorsManifest) {
+  GURL test_url = GetTestUrl("manifest", "parsing-errors.html");

[Peter Beverloo, 2014/11/27 13:24:31]

This file seems to be missing?

[mlamouri (slow - plz ping), 2014/11/27 13:49:08]

Done.

+
+  TestNavigationObserver navigation_observer(shell()->web_contents(), 1);
+  shell()->LoadURL(test_url);
+  navigation_observer.Wait();
+
+  GetManifestAndWait();
+  EXPECT_TRUE(manifest().IsEmpty());
+  EXPECT_EQ(6, ManifestBrowserTest::console_errors_count_);
 }
 
 } // namespace content