Improve error messaging of webRequest API in case of conflicts

chrome/browser/extensions/extension_warning_set.h

 // Copyright (c) 2012 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.
 
 #ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_WARNING_SET_H_

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

Seems like this file should be called extension_warning_service. There's only
one line of code here that implements ExtensionWarningSet (a typedef).

[Aaron Boodman, 2012/09/18 18:50:06]

Sorry, this comment only applies if you don't do the thing I suggested elsewhere
about making a dependency-free ExtensionWarningSet and getting rid of
ExtensionWarningService.

 #define CHROME_BROWSER_EXTENSIONS_EXTENSION_WARNING_SET_H_
 
 #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
     // does not have localized warning messages for the UI.
     kInvalid = 0,
     // An extension caused excessive network delays.
     kNetworkDelay,
     // 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<WarningType>* result) const;
+      std::set<ExtensionWarning::WarningType>* 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.
+  // 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::vector<std::string>* result) const;
+
+  // 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.
   virtual void NotifyWarningsChanged();
 
  private:
   typedef std::set<ExtensionWarning>::const_iterator const_iterator;
   typedef std::set<ExtensionWarning>::iterator iterator;
 
   // Shows or hides the warning badge on the wrench menu depending on whether
   // any non-suppressed warnings exist.
   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_