Improve error messaging of webRequest API in case of conflicts

chrome/browser/extensions/extension_warning_set.h

Index: chrome/browser/extensions/extension_warning_set.h
diff --git a/chrome/browser/extensions/extension_warning_set.h b/chrome/browser/extensions/extension_warning_set.h
index d366c816c5d2174a167dc7c4e15225d73a7c2289..778bb6376b065f5b3c798904349eafb3cb995ca0 100644
--- a/chrome/browser/extensions/extension_warning_set.h
+++ b/chrome/browser/extensions/extension_warning_set.h
@@ -7,17 +7,24 @@
 
 #include <set>
 #include <string>
+#include <vector>
 
+#include "base/memory/linked_ptr.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/string16.h"
+#include "base/threading/non_thread_safe.h"
+#include "googleurl/src/gurl.h"
 
-class ExtensionWarning;
 class ExtensionGlobalErrorBadge;
+class ExtensionSet;
 class Profile;
 
-// A set of warnings caused by extensions. These warnings (e.g. conflicting
-// modifications of network requests by extensions, slow extensions, etc.)
-// trigger a warning badge in the UI and and provide means to resolve them.
-class ExtensionWarningSet {
+// TODO(battre) Remove the Extension prefix.
+
+namespace extensions {
+
+// This class is used to represent warnings if extensions misbehave.
+class ExtensionWarning {
  public:
   enum WarningType {
     // Don't use this, it is only intended for the default constructor and
@@ -28,48 +35,122 @@ class ExtensionWarningSet {
     // This extension failed to modify a network request because the
     // modification conflicted with a modification of another extension.
     kNetworkConflict,
+    // This extension failed to redirect a network request because another
+    // extension with higher precedence redirected to a different target.
+    kRedirectConflict,
     // The extension repeatedly flushed WebKit's in-memory cache, which slows
     // down the overall performance.
     kRepeatedCacheFlushes,
     kMaxWarningType
   };
 
-  // Returns a localized string describing |warning_type|.
-  static string16 GetLocalizedWarning(WarningType warning_type);
+  // We allow copy&assign for passing containers of ExtensionWarnings between

[Aaron Boodman, 2012/09/18 18:48:56]

More generally, it's a value type. We have a lot of these in Chrome. I'm not
sure it's really worth calling out.

[battre, 2012/09/20 14:23:07]

Done.

+  // threads.
+  ExtensionWarning(const ExtensionWarning& other);
+  ~ExtensionWarning();
+  void operator=(const ExtensionWarning& other);

[Aaron Boodman, 2012/09/18 18:48:56]

What is this used by?

[battre, 2012/09/20 14:23:07]

I have fixed the operator= definition (to return ExtensionWarning&). I think if
you define a copy constructor you should also define an assignment operator.

+
+  // Factory methods for various warning types.
+  static ExtensionWarning CreateNetworkDelayWarning(
+      const std::string& extension_id);
+  static ExtensionWarning CreateNetworkConflictWarning(
+      const std::string& extension_id);
+  static ExtensionWarning CreateRedirectConflictWarning(
+      const std::string& extension_id,
+      const std::string& winning_extension_id,
+      const GURL& attempted_redirect_url,
+      const GURL& winning_redirect_url);
+  static ExtensionWarning CreateRequestHeaderConflictWarning(
+      const std::string& extension_id,
+      const std::string& winning_extension_id,
+      const std::string& conflicting_header);
+  static ExtensionWarning CreateResponseHeaderConflictWarning(
+      const std::string& extension_id,
+      const std::string& winning_extension_id,
+      const std::string& conflicting_header);
+  static ExtensionWarning CreateCredentialsConflictWarning(
+      const std::string& extension_id,
+      const std::string& winning_extension_id);
+  static ExtensionWarning CreateRepeatedCacheFlushesWarning(
+      const std::string& extension_id);
+
+  // Returns the specific warning type.
+  WarningType warning_type() const { return type_; }
+
+  // Returns the id of the extension for which this warning is valid.
+  const std::string& extension_id() const { return extension_id_; }
+
+  // Returns a localized warning message.
+  const std::string GetMessage(const ExtensionSet* extensions) const;
+
+ private:
+  // Constructs a warning of type |type| for extension |extension_id|. This
+  // could indicate for example the fact that an extension conflicted with
+  // others. The |message_id| refers to an IDS_ string ID. The
+  // |message_parameters| are filled into the message template.
+  ExtensionWarning(WarningType type,
+                   const std::string& extension_id,
+                   int message_id,
+                   const std::vector<std::string>& message_parameters);
+
+  WarningType type_;
+  std::string extension_id_;
+  // IDS_* resource ID.
+  int message_id_;
+  // Parameters to be filled into the string identified by |message_id_|.
+  std::vector<std::string> message_parameters_;
+};
+
+// Compare ExtensionWarnings based on the tuple of (extension_id, type).
+// The message associated with ExtensionWarnings is purely informational
+// and does not contribute to distinguishing extensions.
+bool operator<(const ExtensionWarning& a, const ExtensionWarning& b);
 
+typedef std::set<ExtensionWarning> ExtensionWarningSet;
+
+// Manages a set of warnings caused by extensions. These warnings (e.g.
+// conflicting modifications of network requests by extensions, slow extensions,
+// etc.) trigger a warning badge in the UI and and provide means to resolve

[Aaron Boodman, 2012/09/18 18:48:56]

It seems like a layering violation to have this class directly poke at Chrome's
UI (the global error service).

Can ExtensionWarningService just become ExtensionWarningSet - a nice
dependency-free data class, with an observer interface, that something in the UI
implements to wire up to the error service?

ExtensionSystem would own one instance of ExtensionWarningSet, and would create
the observer that connects it to the error service.

It seems like that would also make testing this class easier. No mocking
necessary.

[battre, 2012/09/20 14:23:07]

Done. Except that I left some mocking.

+// them. This class must be used on the UI thread only.
+class ExtensionWarningService : public base::NonThreadSafe {
+ public:
   // |profile| may be NULL for testing. In this case, be sure to not insert
   // any warnings.
-  explicit ExtensionWarningSet(Profile* profile);
-  virtual ~ExtensionWarningSet();
-
-  // Adds a warning and triggers a chrome::NOTIFICATION_EXTENSION_WARNING
-  // message if this warning is is new. If the warning is new and has not
-  // been suppressed, this may activate a badge on the wrench menu.
-  void SetWarning(ExtensionWarningSet::WarningType type,
-                  const std::string& extension_id);
+  explicit ExtensionWarningService(Profile* profile);
+  virtual ~ExtensionWarningService();
 
   // Clears all warnings of types contained in |types| and triggers a
   // chrome::NOTIFICATION_EXTENSION_WARNING message if such warnings existed.
   // If no warning remains that is not suppressed, this may deactivate a
   // warning badge on the wrench mennu.
-  void ClearWarnings(const std::set<WarningType>& types);
+  void ClearWarnings(const std::set<ExtensionWarning::WarningType>& types);
 
   // Suppresses showing a badge for all currently existing warnings in the
   // future.
   void SuppressBadgeForCurrentWarnings();
 
-  // Stores all warnings for extension |extension_id| in |result|. The previous
-  // content of |result| is erased.
-  void GetWarningsAffectingExtension(
+  // Stores all types of warnings effecting extension |extension_id| in
+  // |result|. The previous content of |result| is erased.
+  void GetWarningTypesAffectingExtension(
+      const std::string& extension_id,
+      std::set<ExtensionWarning::WarningType>* result) const;
+
+  // Stores all localized warnings for extension |extension_id| in |result|.
+  // The previous content of |result| is erased.
+  void GetWarningMessagesForExtension(
       const std::string& extension_id,
-      std::set<WarningType>* result) const;
+      std::vector<std::string>* result) const;
 
-  // Notifies the ExtensionWarningSet of profile |profile_id| that
-  // |extension_ids| caused warning |warning_type|. This function must only be
-  // called on the UI thread.
+  // Adds a set of warnings and triggers a
+  // chrome::NOTIFICATION_EXTENSION_WARNING message if any warning is new.
+  // If the warning is new and has not been suppressed, this may activate a
+  // badge on the wrench menu.
+  void AddWarnings(const ExtensionWarningSet& warnings);
+
+  // Notifies the ExtensionWarningService of profile |profile_id| that new
+  // |warnings| occurred and triggers a warning badge.
   static void NotifyWarningsOnUI(void* profile_id,
-                                 std::set<std::string> extension_ids,
-                                 WarningType warning_type);
+                                 ExtensionWarningSet warnings);
 
  protected:
   // Virtual for testing.
@@ -84,12 +165,14 @@ class ExtensionWarningSet {
   void UpdateWarningBadge();
 
   // Currently existing warnings.
-  std::set<ExtensionWarning> warnings_;
+  ExtensionWarningSet warnings_;
 
   // Warnings that do not trigger a badge on the wrench menu.
-  std::set<ExtensionWarning> badge_suppressions_;
+  ExtensionWarningSet badge_suppressions_;
 
   Profile* profile_;
 };
 
+}  // namespace extensions
+
 #endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_WARNING_SET_H_