Add ErrorConsole UI for Extension Install Warnings

chrome/browser/extensions/error_console/error_console_browsertest.cc

+// Copyright 2013 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/extensions/error_console/error_console.h"
+
+#include "base/files/file_path.h"
+#include "base/prefs/pref_service.h"
+#include "base/strings/string16.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_system.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/extensions/extension.h"
+#include "chrome/common/extensions/extension_manifest_constants.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "extensions/browser/extension_error.h"
+#include "extensions/common/constants.h"
+#include "extensions/common/error_utils.h"
+#include "extensions/common/manifest_constants.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+using base::string16;
+using base::UTF8ToUTF16;
+
+namespace errors = extension_manifest_errors;
+
+namespace extensions {
+
+namespace {
+
+base::string16 GetManifestFilename() {
+  return base::FilePath(kManifestFilename).AsUTF16Unsafe();
+}
+
+// Verify that all properites of a given |error| are correct.

[Yoyo Zhou, 2013/08/16 00:17:26]

typo: properties

[Devlin, 2013/08/16 18:07:49]

Done.

+void CheckError(const ExtensionError* error,
+                ExtensionError::Type type,
+                const std::string& id,
+                const string16& source,
+                bool from_incognito,
+                const string16& message) {
+  ASSERT_TRUE(error);
+  EXPECT_EQ(type, error->type());
+  EXPECT_EQ(id, error->extension_id());
+  EXPECT_EQ(source, error->source());
+  EXPECT_EQ(from_incognito, error->from_incognito());
+  EXPECT_EQ(message, error->message());
+}
+
+void CheckManifestError(const ExtensionError* error,
+                        const std::string& id,
+                        const string16& message,
+                        const string16& manifest_key,
+                        const string16& manifest_specific) {
+  CheckError(error,
+             ExtensionError::MANIFEST_PARSING_ERROR,
+             id,
+             GetManifestFilename(),  // source is always the manifest.
+             false,  // manifest errors are never from incognito.
+             message);
+
+  const ManifestParsingError* manifest_error =
+      static_cast<const ManifestParsingError*>(error);
+  EXPECT_EQ(manifest_key, manifest_error->manifest_key());
+  EXPECT_EQ(manifest_specific, manifest_error->manifest_specific());
+}
+
+}  // namespace
+
+class ErrorConsoleBrowserTest : public ExtensionBrowserTest {
+ public:
+  ErrorConsoleBrowserTest() : error_console_(NULL) { }
+  virtual ~ErrorConsoleBrowserTest() { }
+
+ protected:
+  // A helper class in order to wait for the proper number of errors to be
+  // caught by the ErrorConsole. This will run the MessageLoop until a given
+  // number of errors are observed.
+  // Usage:
+  //   ...
+  //   ErrorObserver observer(3, error_console);
+  //   <Cause three errors...>
+  //   observer.WaitForErrors();
+  //   <Perform any additional checks...>
+  class ErrorObserver : public ErrorConsole::Observer {
+   public:
+    ErrorObserver(size_t errors_expected, ErrorConsole* error_console)
+        : errors_observed_(0),
+          errors_expected_(errors_expected),
+          waiting_(false),
+          error_console_(error_console) {
+      error_console_->AddObserver(this);
+    }
+    virtual ~ErrorObserver() {
+      if (error_console_)
+        error_console_->RemoveObserver(this);
+    }
+
+    // ErrorConsole::Observer implementation.
+    virtual void OnErrorAdded(const ExtensionError* error) OVERRIDE {
+      ++errors_observed_;
+      if (errors_observed_ >= errors_expected_) {
+        if (waiting_)
+          base::MessageLoopForUI::current()->Quit();
+      }
+    }
+
+    virtual void OnErrorConsoleDestroyed() OVERRIDE {
+      error_console_ = NULL;
+    }
+
+    // Spin until the appropriate number of errors have been observed.
+    void WaitForErrors() {
+      if (errors_observed_ < errors_expected_) {
+        waiting_ = true;
+        content::RunMessageLoop();
+        waiting_ = false;
+      }
+    }
+
+   private:
+    size_t errors_observed_;
+    size_t errors_expected_;
+    bool waiting_;
+
+    ErrorConsole* error_console_;
+
+    DISALLOW_COPY_AND_ASSIGN(ErrorObserver);
+  };
+
+  virtual void SetUpOnMainThread() OVERRIDE {
+    ExtensionBrowserTest::SetUpOnMainThread();
+
+    // Errors are only kept if we have Developer Mode enabled.
+    profile()->GetPrefs()->SetBoolean(prefs::kExtensionsUIDeveloperMode, true);
+
+    error_console_ = ErrorConsole::Get(profile());
+    CHECK(error_console_);
+
+    test_data_dir_ = test_data_dir_.AppendASCII("error_console");
+  }
+
+  // Load the extension at |path| wait for |expected_errors| errors.

[Yoyo Zhou, 2013/08/16 00:17:26]

nit: missing an and

[Devlin, 2013/08/16 18:07:49]

Done.

+  // Populates |extension| with a pointer to the loaded extension.

[Yoyo Zhou, 2013/08/16 00:17:26]

This could return Extension*.

[Devlin, 2013/08/16 18:07:49]

Unfortunately, it can't.  Use of ASSERT* in a function means that it has to have
a void return type.  If we substitute the ASSERTs for CHECKs, we can return any
type - but this kinda goes against testing practice.

+  void LoadExtensionAndCheckErrors(
+      const std::string& path,
+      int flags,
+      size_t errors_expected,
+      const Extension** extension) {
+    ErrorObserver observer(errors_expected, error_console_);
+    *extension =
+        LoadExtensionWithFlags(test_data_dir_.AppendASCII(path), flags);
+    ASSERT_TRUE(extension);
+    observer.WaitForErrors();
+
+    // We should only have errors for a single extension, or should have no
+    // entries, if no errors were expected.
+    ASSERT_EQ(errors_expected > 0 ? 1u : 0u, error_console()->errors().size());
+    ASSERT_EQ(
+        errors_expected,
+        error_console()->GetErrorsForExtension((*extension)->id()).size());
+  }
+
+  ErrorConsole* error_console() { return error_console_; }
+ private:
+  // Weak reference to the ErrorConsole object.
+  ErrorConsole* error_console_;
+};
+
+// Test to ensure that we are successfully reporting manifest errors as an
+// extension is installed.
+IN_PROC_BROWSER_TEST_F(ErrorConsoleBrowserTest, ReportManifestErrors) {
+  const Extension* extension = NULL;
+  // We expect two errors - one for an invalid permission, and a second for
+  // an unknown key.
+  LoadExtensionAndCheckErrors("manifest_warnings",
+                              ExtensionBrowserTest::kFlagIgnoreManifestWarnings,
+                              2,
+                              &extension);
+
+  const ErrorConsole::ErrorList& errors =
+      error_console()->GetErrorsForExtension(extension->id());
+
+  // Unfortunately, there's not always a hard guarantee of order in parsing the
+  // manifest, so there's not a definitive order in which these errors may
+  // occur. As such, we need to determine which error corresponds to which
+  // expected error.
+  ASSERT_EQ(ExtensionError::MANIFEST_PARSING_ERROR, errors[0]->type());
+  const ManifestParsingError* temp_error =
+      static_cast<const ManifestParsingError*>(errors[0]);
+
+  const ExtensionError* permissions_error = NULL;
+  const ExtensionError* unknown_key_error = NULL;
+  if (temp_error->manifest_key() == base::UTF8ToUTF16("permissions")) {

[Yoyo Zhou, 2013/08/16 00:17:26]

This is more extensible if you write it like:
for (size_t i = 0; ...) {
  if (errors[i]->key() == ..."permissions")
    permissions_error = errors[i];
  else if (... == ..."fakepermission")
    unknown_key_error = errors[i];
}
ASSERT_TRUE(permissions_error);
ASSERT_TRUE(unknown_key_error);

[Devlin, 2013/08/16 18:07:49]

Done.

+    permissions_error = errors[0];
+    unknown_key_error = errors[1];
+  } else {
+    permissions_error = errors[1];
+    unknown_key_error = errors[0];
+  }
+
+  const char kFakePermission[] = "not_a_real_permission";
+  CheckManifestError(permissions_error,
+                     extension->id(),
+                     base::UTF8ToUTF16(
+                         ErrorUtils::FormatErrorMessage(
+                             errors::kPermissionUnknownOrMalformed,
+                             kFakePermission)),
+                     base::UTF8ToUTF16(manifest_keys::kPermissions),
+                     base::UTF8ToUTF16(kFakePermission));
+
+  const char kFakeKey[] = "not_a_real_key";
+  CheckManifestError(unknown_key_error,
+                     extension->id(),
+                     base::UTF8ToUTF16(
+                         ErrorUtils::FormatErrorMessage(
+                             errors::kUnrecognizedManifestKey,
+                             kFakeKey)),
+                     base::UTF8ToUTF16(kFakeKey),
+                     EmptyString16());
+}
+
+// Test that we do not store any errors unless the Developer Mode switch is
+// toggled on the profile.
+IN_PROC_BROWSER_TEST_F(ErrorConsoleBrowserTest,
+                       DontStoreErrorsWithoutDeveloperMode) {
+  profile()->GetPrefs()->SetBoolean(prefs::kExtensionsUIDeveloperMode, false);
+
+  const Extension* extension = NULL;
+  // Same test as ReportManifestErrors, except we don't expect any errors since
+  // we disable Developer Mode.
+  LoadExtensionAndCheckErrors("manifest_warnings",
+                              ExtensionBrowserTest::kFlagIgnoreManifestWarnings,
+                              0,
+                              &extension);
+
+  // Now if we enable developer mode, the errors should be reported...
+  profile()->GetPrefs()->SetBoolean(prefs::kExtensionsUIDeveloperMode, true);
+  EXPECT_EQ(2u, error_console()->GetErrorsForExtension(extension->id()).size());
+
+  // ... and if we disable it again, all errors which we were holding should be
+  // removed.
+  profile()->GetPrefs()->SetBoolean(prefs::kExtensionsUIDeveloperMode, false);
+  EXPECT_EQ(0u, error_console()->GetErrorsForExtension(extension->id()).size());
+}
+
+}  // namespace extensions