Improve error messaging of webRequest API in case of conflicts

chrome/browser/extensions/extension_warning_set.cc

Index: chrome/browser/extensions/extension_warning_set.cc
diff --git a/chrome/browser/extensions/extension_warning_set.cc b/chrome/browser/extensions/extension_warning_set.cc
index 2d73cd684c4b3ea58c9a276ba879757399b98583..16104e098d24cb1f5175837c9cc30b45c81712e7 100644
--- a/chrome/browser/extensions/extension_warning_set.cc
+++ b/chrome/browser/extensions/extension_warning_set.cc
@@ -4,112 +4,229 @@
 
 #include "chrome/browser/extensions/extension_warning_set.h"
 
+#include "base/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
-#include "chrome/browser/extensions/extension_global_error_badge.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
-#include "chrome/browser/ui/global_error/global_error_service.h"
-#include "chrome/browser/ui/global_error/global_error_service_factory.h"
 #include "chrome/common/chrome_notification_types.h"
+#include "chrome/common/extensions/extension.h"
+#include "chrome/common/extensions/extension_set.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
+#include "net/base/escape.h"
 #include "ui/base/l10n/l10n_util.h"
 
 using content::BrowserThread;
 
-// This class is used to represent warnings if extensions misbehave.
-class ExtensionWarning {
- public:
-  // Default constructor for storing ExtensionServiceWarning in STL containers
-  // do not use.
-  ExtensionWarning();
+namespace {
+// Prefix for message parameters indicating that the parameter needs to
+// be translated from an extension id to the extension name.
+const char kTranslate[] = "TO_TRANSLATE:";
+const size_t kMaxNumberOfParameters = 4;
+}
 
-  // Constructs a warning of type |type| for extension |extension_id|. This
-  // could be for example the fact that an extension conflicted with others.
-  ExtensionWarning(ExtensionWarningSet::WarningType type,
-                   const std::string& extension_id);
+namespace extensions {
 
-  ~ExtensionWarning();
+//
+// ExtensionWarning
+//
 
-  // Returns the specific warning type.
-  ExtensionWarningSet::WarningType warning_type() const { return type_; }
+ExtensionWarning::ExtensionWarning(
+    WarningType type,
+    const std::string& extension_id,
+    int message_id,
+    const std::vector<std::string>& message_parameters)
+    : type_(type),
+      extension_id_(extension_id),
+      message_id_(message_id),
+      message_parameters_(message_parameters) {
+  // These are invalid here because they do not have corresponding warning
+  // messages in the UI.
+  CHECK_NE(type, kInvalid);
+  CHECK_NE(type, kMaxWarningType);
+  CHECK_LE(message_parameters.size(), kMaxNumberOfParameters);
+}
 
-  // Returns the id of the extension for which this warning is valid.
-  const std::string& extension_id() const { return extension_id_; }
+ExtensionWarning::ExtensionWarning(const ExtensionWarning& other)
+  : type_(other.type_),
+    extension_id_(other.extension_id_),
+    message_id_(other.message_id_),
+    message_parameters_(other.message_parameters_) {}
 
- private:
-  ExtensionWarningSet::WarningType type_;
-  std::string extension_id_;
+ExtensionWarning::~ExtensionWarning() {
+}
 
-  // Allow implicit copy and assign operator.
-};
+ExtensionWarning& ExtensionWarning::operator=(const ExtensionWarning& other) {
+  type_ = other.type_;
+  extension_id_ = other.extension_id_;
+  message_id_ = other.message_id_;
+  message_parameters_ = other.message_parameters_;
+  return *this;
+}
 
-ExtensionWarning::ExtensionWarning() : type_(ExtensionWarningSet::kInvalid) {
+// static
+ExtensionWarning ExtensionWarning::CreateNetworkDelayWarning(
+    const std::string& extension_id) {
+  std::vector<std::string> message_parameters;
+  message_parameters.push_back(l10n_util::GetStringUTF8(IDS_PRODUCT_NAME));
+  return ExtensionWarning(
+      kNetworkDelay,
+      extension_id,
+      IDS_EXTENSION_WARNINGS_NETWORK_DELAY,
+      message_parameters);
 }
 
-ExtensionWarning::ExtensionWarning(
-    ExtensionWarningSet::WarningType type,
-    const std::string& extension_id)
-    : type_(type), extension_id_(extension_id) {
-  // These are invalid here because they do not have corresponding warning
-  // messages in the UI.
-  CHECK(type != ExtensionWarningSet::kInvalid);
-  CHECK(type != ExtensionWarningSet::kMaxWarningType);
+// static
+ExtensionWarning ExtensionWarning::CreateNetworkConflictWarning(
+    const std::string& extension_id) {
+  std::vector<std::string> message_parameters;
+  return ExtensionWarning(
+      kNetworkConflict,
+      extension_id,
+      IDS_EXTENSION_WARNINGS_NETWORK_CONFLICT,
+      message_parameters);
 }
 
-ExtensionWarning::~ExtensionWarning() {
+// static
+ExtensionWarning ExtensionWarning::CreateRedirectConflictWarning(
+    const std::string& extension_id,
+    const std::string& winning_extension_id,
+    const GURL& attempted_redirect_url,
+    const GURL& winning_redirect_url) {
+  std::vector<std::string> message_parameters;
+  message_parameters.push_back(attempted_redirect_url.spec());
+  message_parameters.push_back(kTranslate + winning_extension_id);
+  message_parameters.push_back(winning_redirect_url.spec());
+  return ExtensionWarning(
+      kRedirectConflict,
+      extension_id,
+      IDS_EXTENSION_WARNINGS_REDIRECT_CONFLICT,
+      message_parameters);
 }
 
-bool operator<(const ExtensionWarning& a, const ExtensionWarning& b) {
-  if (a.warning_type() == b.warning_type())
-    return a.extension_id() < b.extension_id();
-  return a.warning_type() < b.warning_type();
+// static
+ExtensionWarning ExtensionWarning::CreateRequestHeaderConflictWarning(
+    const std::string& extension_id,
+    const std::string& winning_extension_id,
+    const std::string& conflicting_header) {
+  std::vector<std::string> message_parameters;
+  message_parameters.push_back(conflicting_header);
+  message_parameters.push_back(kTranslate + winning_extension_id);
+  return ExtensionWarning(
+      kNetworkConflict,
+      extension_id,
+      IDS_EXTENSION_WARNINGS_REQUEST_HEADER_CONFLICT,
+      message_parameters);
+}
+
+// static
+ExtensionWarning ExtensionWarning::CreateResponseHeaderConflictWarning(
+    const std::string& extension_id,
+    const std::string& winning_extension_id,
+    const std::string& conflicting_header) {
+  std::vector<std::string> message_parameters;
+  message_parameters.push_back(conflicting_header);
+  message_parameters.push_back(kTranslate + winning_extension_id);
+  return ExtensionWarning(
+      kNetworkConflict,
+      extension_id,
+      IDS_EXTENSION_WARNINGS_RESPONSE_HEADER_CONFLICT,
+      message_parameters);
 }
 
-// Static
-string16 ExtensionWarningSet::GetLocalizedWarning(
-    ExtensionWarningSet::WarningType warning_type) {
-  switch (warning_type) {
-    case kInvalid:
-    case kMaxWarningType:
+// static
+ExtensionWarning ExtensionWarning::CreateCredentialsConflictWarning(
+    const std::string& extension_id,
+    const std::string& winning_extension_id) {
+  std::vector<std::string> message_parameters;
+  message_parameters.push_back(kTranslate + winning_extension_id);
+  return ExtensionWarning(
+      kNetworkConflict,
+      extension_id,
+      IDS_EXTENSION_WARNINGS_CREDENTIALS_CONFLICT,
+      message_parameters);
+}
+
+// static
+ExtensionWarning ExtensionWarning::CreateRepeatedCacheFlushesWarning(
+    const std::string& extension_id) {
+  std::vector<std::string> message_parameters;
+  message_parameters.push_back(l10n_util::GetStringUTF8(IDS_PRODUCT_NAME));
+  return ExtensionWarning(
+      kRepeatedCacheFlushes,
+      extension_id,
+      IDS_EXTENSION_WARNINGS_NETWORK_DELAY,
+      message_parameters);
+}
+
+const std::string ExtensionWarning::GetMessage(
+    const ExtensionSet* extensions) const {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  // These parameters may be unsafe (URLs and Extension names) and need
+  // to be HTML-escaped before being embedded in the UI. Also extension IDs
+  // are translated to full extension names.
+  std::vector<string16> final_parameters;
+  for (size_t i = 0; i < message_parameters_.size(); ++i) {
+    std::string message = message_parameters_[i];
+    if (StartsWithASCII(message, kTranslate, true)) {
+      std::string extension_id = message.substr(sizeof(kTranslate) - 1);
+      const extensions::Extension* extension =
+          extensions->GetByID(extension_id);
+      message = extension ? extension->name() : extension_id;
+    }
+    final_parameters.push_back(UTF8ToUTF16(net::EscapeForHTML(message)));
+  }
+
+  COMPILE_ASSERT(kMaxNumberOfParameters == 4u, YouNeedToAddMoreCaseStatements);
+  switch (final_parameters.size()) {
+    case 0:
+      return l10n_util::GetStringUTF8(message_id_);
+    case 1:
+      return l10n_util::GetStringFUTF8(message_id_, final_parameters[0]);
+    case 2:
+      return l10n_util::GetStringFUTF8(message_id_, final_parameters[0],
+          final_parameters[1]);
+    case 3:
+      return l10n_util::GetStringFUTF8(message_id_, final_parameters[0],
+          final_parameters[1], final_parameters[2]);
+    case 4:
+      return l10n_util::GetStringFUTF8(message_id_, final_parameters[0],
+          final_parameters[1], final_parameters[2], final_parameters[3]);
+    default:
       NOTREACHED();
-      return string16();
-    case kNetworkDelay:
-      return l10n_util::GetStringFUTF16(
-          IDS_EXTENSION_WARNINGS_NETWORK_DELAY,
-          l10n_util::GetStringUTF16(IDS_PRODUCT_NAME));
-    case kNetworkConflict:
-      return l10n_util::GetStringUTF16(IDS_EXTENSION_WARNINGS_NETWORK_CONFLICT);
-    case kRepeatedCacheFlushes:
-      return l10n_util::GetStringFUTF16(
-          IDS_EXTENSION_WARNINGS_NETWORK_DELAY,
-          l10n_util::GetStringUTF16(IDS_PRODUCT_NAME));
+      return std::string();
   }
-  NOTREACHED();  // Switch statement has no default branch.
-  return string16();
 }
 
-ExtensionWarningSet::ExtensionWarningSet(Profile* profile) : profile_(profile) {
+bool operator<(const ExtensionWarning& a, const ExtensionWarning& b) {
+  if (a.extension_id() != b.extension_id())
+    return a.extension_id() < b.extension_id();
+  return a.warning_type() < b.warning_type();
 }
 
-ExtensionWarningSet::~ExtensionWarningSet() {
-}
+//
+// ExtensionWarningSet
+//
 
-void ExtensionWarningSet::SetWarning(ExtensionWarningSet::WarningType type,
-                                     const std::string& extension_id) {
-  ExtensionWarning warning(type, extension_id);
-  bool inserted = warnings_.insert(warning).second;
-  if (inserted) {
-    NotifyWarningsChanged();
-    UpdateWarningBadge();
+ExtensionWarningSet::ExtensionWarningSet(Profile* profile)
+    : profile_(profile) {
+  DCHECK(CalledOnValidThread());
+  if (profile_) {
+    registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UNLOADED,

[Aaron Boodman, 2012/10/16 06:06:05]

Sigh, this isn't what I had in mind for a "dumb data class".

I guess this makes more sense with ExtensionWarningSet and
ExtensionWarningService combined together in the same file, and just the single
member of ExtensionSystem.

Pre-emptive LGTM so we don't have to have another review round.

+        content::Source<Profile>(profile_->GetOriginalProfile()));
   }
 }
 
+ExtensionWarningSet::~ExtensionWarningSet() {}
+
 void ExtensionWarningSet::ClearWarnings(
-    const std::set<ExtensionWarningSet::WarningType>& types) {
+    const std::set<ExtensionWarning::WarningType>& types) {
+  DCHECK(CalledOnValidThread());
   bool deleted_anything = false;
   for (iterator i = warnings_.begin(); i != warnings_.end();) {
     if (types.find(i->warning_type()) != types.end()) {
@@ -120,15 +237,14 @@ void ExtensionWarningSet::ClearWarnings(
     }
   }
 
-  if (deleted_anything) {
+  if (deleted_anything)
     NotifyWarningsChanged();
-    UpdateWarningBadge();
-  }
 }
 
-void ExtensionWarningSet::GetWarningsAffectingExtension(
+void ExtensionWarningSet::GetWarningTypesAffectingExtension(
     const std::string& extension_id,
-    std::set<ExtensionWarningSet::WarningType>* result) const {
+    std::set<ExtensionWarning::WarningType>* result) const {
+  DCHECK(CalledOnValidThread());
   result->clear();
   for (const_iterator i = warnings_.begin(); i != warnings_.end(); ++i) {
     if (i->extension_id() == extension_id)
@@ -136,12 +252,37 @@ void ExtensionWarningSet::GetWarningsAffectingExtension(
   }
 }
 
+void ExtensionWarningSet::GetWarningMessagesForExtension(
+    const std::string& extension_id,
+    std::vector<std::string>* result) const {
+  DCHECK(CalledOnValidThread());
+  result->clear();
+
+  const ExtensionService* extension_service =
+      ExtensionSystem::Get(profile_)->extension_service();
+
+  for (const_iterator i = warnings_.begin(); i != warnings_.end(); ++i) {
+    if (i->extension_id() == extension_id)
+      result->push_back(i->GetMessage(extension_service->extensions()));
+  }
+}
+
+void ExtensionWarningSet::AddWarnings(
+    const std::set<ExtensionWarning>& warnings) {
+  DCHECK(CalledOnValidThread());
+  size_t old_size = warnings_.size();
+
+  warnings_.insert(warnings.begin(), warnings.end());
+
+  if (old_size != warnings_.size())
+    NotifyWarningsChanged();
+}
+
 // static
 void ExtensionWarningSet::NotifyWarningsOnUI(
     void* profile_id,
-    std::set<std::string> extension_ids,
-    WarningType warning_type) {
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    std::set<ExtensionWarning> warnings) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   Profile* profile = reinterpret_cast<Profile*>(profile_id);
   if (!profile ||
       !g_browser_process->profile_manager() ||
@@ -149,47 +290,45 @@ void ExtensionWarningSet::NotifyWarningsOnUI(
     return;
   }
 
-  ExtensionWarningSet* warnings =
-      profile->GetExtensionService()->extension_warnings();
+  extensions::ExtensionWarningSet* warning_set =
+      extensions::ExtensionSystem::Get(profile)->warning_set();
 
-  for (std::set<std::string>::const_iterator i = extension_ids.begin();
-       i != extension_ids.end(); ++i) {
-    warnings->SetWarning(warning_type, *i);
-  }
+  warning_set->AddWarnings(warnings);
 }
 
-void ExtensionWarningSet::SuppressBadgeForCurrentWarnings() {
-  badge_suppressions_.insert(warnings_.begin(), warnings_.end());
-  UpdateWarningBadge();
+void ExtensionWarningSet::AddObserver(Observer* observer) {
+  observer_list_.AddObserver(observer);
+}
+
+void ExtensionWarningSet::RemoveObserver(Observer* observer) {
+  observer_list_.RemoveObserver(observer);
 }
 
 void ExtensionWarningSet::NotifyWarningsChanged() {
-  content::NotificationService::current()->Notify(
-      chrome::NOTIFICATION_EXTENSION_WARNING_CHANGED,
-      content::Source<Profile>(profile_),
-      content::NotificationService::NoDetails());
+  DCHECK(CalledOnValidThread());
+  FOR_EACH_OBSERVER(Observer, observer_list_, ExtensionWarningsChanged());
 }
 
-void ExtensionWarningSet::UpdateWarningBadge() {
-  // We need a badge if a warning exists that has not been suppressed.
-  bool need_warning_badge = false;
-  for (const_iterator i = warnings_.begin(); i != warnings_.end(); ++i) {
-    if (badge_suppressions_.find(*i) == badge_suppressions_.end()) {
-      need_warning_badge = true;
+void ExtensionWarningSet::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  switch (type) {
+    case chrome::NOTIFICATION_EXTENSION_UNLOADED: {
+      const Extension* extension =
+          content::Details<extensions::UnloadedExtensionInfo>(details)->
+          extension;
+      // Unloading one extension might have solved the problems of others.
+      // Therefore, we clear warnings of this type for all extensions.
+      std::set<ExtensionWarning::WarningType> warning_types;
+      GetWarningTypesAffectingExtension(extension->id(), &warning_types);
+      ClearWarnings(warning_types);
       break;
     }
-  }
-
-  GlobalErrorService* service =
-      GlobalErrorServiceFactory::GetForProfile(profile_);
-  GlobalError* error = service->GetGlobalErrorByMenuItemCommandID(
-      ExtensionGlobalErrorBadge::GetMenuItemCommandID());
-
-  // Activate or hide the warning badge in case the current state is incorrect.
-  if (error && !need_warning_badge) {
-    service->RemoveGlobalError(error);
-    delete error;
-  } else if (!error && need_warning_badge) {
-    service->AddGlobalError(new ExtensionGlobalErrorBadge);
+    default:
+      NOTREACHED();
+      break;
   }
 }
+
+}  // namespace extensions