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..43fd60c1e845dd897bfc50747a07b65377fbb90d 100644
--- a/content/browser/manifest/manifest_browsertest.cc
+++ b/content/browser/manifest/manifest_browsertest.cc
@@ -15,11 +15,42 @@
 
 namespace content {
 
+class ManifestBrowserTest;
+
+// Mock of a WebContentsDelegate that catches messages sent to the console.
+class MockWebContentsDelegate : public content::WebContentsDelegate {

[Peter Beverloo, 2014/11/27 14:37:37]

nit: no need for content::

[mlamouri (slow - plz ping), 2014/11/27 14:44:10]

Done.

+ public:
+  MockWebContentsDelegate(WebContents* web_contents, ManifestBrowserTest* test)
+      : web_contents_(web_contents),
+        test_(test) {
+  }
+
+  bool AddMessageToConsole(WebContents* source,
+                           int32 level,
+                           const base::string16& message,
+                           int32 line_no,
+                           const base::string16& source_id) override;
+
+ private:
+  WebContents* web_contents_;
+  ManifestBrowserTest* test_;
+};
+
 class ManifestBrowserTest : public ContentBrowserTest  {
  protected:
-  ManifestBrowserTest() {}
+  friend MockWebContentsDelegate;
+
+  ManifestBrowserTest() : console_error_count_(0) {}
   ~ManifestBrowserTest() override {}
 
+  void OverrideWebContentsDelegate() {

[Peter Beverloo, 2014/11/27 14:37:37]

I wonder if we could do this in SetUp(), avoiding having to call it everywhere?

[mlamouri (slow - plz ping), 2014/11/27 14:44:10]

I've been experimenting with that but the DCHECK would fail. It seems that
SetUpOnMainThread() is what I need.

+    DCHECK(shell()->web_contents());
+
+    mock_web_contents_delegate_.reset(
+        new MockWebContentsDelegate(shell()->web_contents(), this));
+    shell()->web_contents()->SetDelegate(mock_web_contents_delegate_.get());
+  }
+
   void GetManifestAndWait() {
     shell()->web_contents()->GetManifest(
         base::Bind(&ManifestBrowserTest::OnGetManifest,
@@ -38,24 +69,55 @@ class ManifestBrowserTest : public ContentBrowserTest  {
     return manifest_;
   }
 
+  unsigned int console_error_count() const {
+    return console_error_count_;
+  }
+
+  void OnReceivedConsoleError() {
+    console_error_count_++;
+  }
+
  private:
   scoped_refptr<MessageLoopRunner> message_loop_runner_;
+  scoped_ptr<MockWebContentsDelegate> mock_web_contents_delegate_;
   Manifest manifest_;
+  int console_error_count_;
 
   DISALLOW_COPY_AND_ASSIGN(ManifestBrowserTest);
 };
 
+// The implementation of AddMessageToConsole isn't inlined because it needs
+// to know about |test_|.
+bool MockWebContentsDelegate::AddMessageToConsole(
+    WebContents* source,
+    int32 level,
+    const base::string16& message,
+    int32 line_no,
+    const base::string16& source_id) {
+  if (source != web_contents_)

[Peter Beverloo, 2014/11/27 14:37:37]

When does this happen? Can it be a DCHECK?

[mlamouri (slow - plz ping), 2014/11/27 14:44:10]

Done.

+    return false;
+
+  if (level != logging::LOG_ERROR)
+    return false;
+
+  test_->OnReceivedConsoleError();
+  return false;
+}
+
 // If a page has no manifest, requesting a manifest should return the empty
 // manifest.
 IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, NoManifest) {
   GURL test_url = GetTestUrl("manifest", "no-manifest.html");
 
+  OverrideWebContentsDelegate();
+
   TestNavigationObserver navigation_observer(shell()->web_contents(), 1);
   shell()->LoadURL(test_url);
   navigation_observer.Wait();
 
   GetManifestAndWait();
   EXPECT_TRUE(manifest().IsEmpty());
+  EXPECT_EQ(0u, console_error_count());
 }
 
 // If a page manifest points to a 404 URL, requesting the manifest should return
@@ -63,12 +125,15 @@ IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, NoManifest) {
 IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, 404Manifest) {
   GURL test_url = GetTestUrl("manifest", "404-manifest.html");
 
+  OverrideWebContentsDelegate();
+
   TestNavigationObserver navigation_observer(shell()->web_contents(), 1);
   shell()->LoadURL(test_url);
   navigation_observer.Wait();
 
   GetManifestAndWait();
   EXPECT_TRUE(manifest().IsEmpty());
+  EXPECT_EQ(0u, console_error_count());
 }
 
 // If a page has an empty manifest, requesting the manifest should return the
@@ -76,12 +141,15 @@ IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, 404Manifest) {
 IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, EmptyManifest) {
   GURL test_url = GetTestUrl("manifest", "empty-manifest.html");
 
+  OverrideWebContentsDelegate();
+
   TestNavigationObserver navigation_observer(shell()->web_contents(), 1);
   shell()->LoadURL(test_url);
   navigation_observer.Wait();
 
   GetManifestAndWait();
   EXPECT_TRUE(manifest().IsEmpty());
+  EXPECT_EQ(0u, console_error_count());
 }
 
 // If a page's manifest can't be parsed correctly, requesting the manifest
@@ -89,12 +157,15 @@ IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, EmptyManifest) {
 IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, ParseErrorManifest) {
   GURL test_url = GetTestUrl("manifest", "parse-error-manifest.html");
 
+  OverrideWebContentsDelegate();
+
   TestNavigationObserver navigation_observer(shell()->web_contents(), 1);
   shell()->LoadURL(test_url);
   navigation_observer.Wait();
 
   GetManifestAndWait();
   EXPECT_TRUE(manifest().IsEmpty());
+  EXPECT_EQ(1u, console_error_count());
 }
 
 // If a page has a manifest that can be fetched and parsed, requesting the
@@ -102,12 +173,15 @@ IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, ParseErrorManifest) {
 IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, DummyManifest) {
   GURL test_url = GetTestUrl("manifest", "dummy-manifest.html");
 
+  OverrideWebContentsDelegate();
+
   TestNavigationObserver navigation_observer(shell()->web_contents(), 1);
   shell()->LoadURL(test_url);
   navigation_observer.Wait();
 
   GetManifestAndWait();
   EXPECT_FALSE(manifest().IsEmpty());
+  EXPECT_EQ(0u, console_error_count());
 }
 
 // If a page changes manifest during its life-time, requesting the manifest
@@ -115,6 +189,8 @@ IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, DummyManifest) {
 IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, DynamicManifest) {
   GURL test_url = GetTestUrl("manifest", "dynamic-manifest.html");
 
+  OverrideWebContentsDelegate();
+
   TestNavigationObserver navigation_observer(shell()->web_contents(), 1);
   shell()->LoadURL(test_url);
   navigation_observer.Wait();
@@ -143,6 +219,8 @@ IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, DynamicManifest) {
     GetManifestAndWait();
     EXPECT_TRUE(manifest().IsEmpty());
   }
+
+  EXPECT_EQ(0u, console_error_count());
 }
 
 // If a page's manifest lives in a different origin, it should follow the CORS
@@ -159,6 +237,8 @@ IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, CORSManifest) {
   GURL test_url =
       embedded_test_server()->GetURL("/manifest/dynamic-manifest.html");
 
+  OverrideWebContentsDelegate();
+
   TestNavigationObserver navigation_observer(shell()->web_contents(), 1);
   shell()->LoadURL(test_url);
   navigation_observer.Wait();
@@ -170,6 +250,8 @@ IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, CORSManifest) {
 
   GetManifestAndWait();
   EXPECT_TRUE(manifest().IsEmpty());
+
+  EXPECT_EQ(0u, console_error_count());
 }
 
 // If a page's manifest is in an unsecure origin while the page is in a secure
@@ -186,6 +268,8 @@ IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, MixedContentManifest) {
   GURL test_url =
       embedded_test_server()->GetURL("/manifest/dynamic-manifest.html");
 
+  OverrideWebContentsDelegate();
+
   TestNavigationObserver navigation_observer(shell()->web_contents(), 1);
   shell()->LoadURL(test_url);
   navigation_observer.Wait();
@@ -197,6 +281,24 @@ IN_PROC_BROWSER_TEST_F(ManifestBrowserTest, MixedContentManifest) {
 
   GetManifestAndWait();
   EXPECT_TRUE(manifest().IsEmpty());
+
+  EXPECT_EQ(0u, console_error_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");
+
+  OverrideWebContentsDelegate();
+
+  TestNavigationObserver navigation_observer(shell()->web_contents(), 1);
+  shell()->LoadURL(test_url);
+  navigation_observer.Wait();
+
+  GetManifestAndWait();
+  EXPECT_TRUE(manifest().IsEmpty());
+  EXPECT_EQ(6u, console_error_count());
 }
 
 } // namespace content