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..a6539feba2761cbe53e22efd5b8fd78382e0284d 100644
--- a/chrome/browser/extensions/extension_warning_set.h
+++ b/chrome/browser/extensions/extension_warning_set.h
@@ -7,17 +7,20 @@
 
 #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 ExtensionService;
 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 {
+// This class is used to represent warnings if extensions misbehave.
+class ExtensionWarning {

[Aaron Boodman, 2012/09/05 08:11:11]

Move all this into the extensions namespace and remove the 'Extension' prefix
from names.

[battre, 2012/09/06 14:25:48]

I have moved it into an extensions namespace. As removing the Extension prefix
requires renaming the files, I would like to do that in a separate CL. I have
added a TODO.

[Aaron Boodman, 2012/09/06 15:33:31]

ok

  public:
   enum WarningType {
     // Don't use this, it is only intended for the default constructor and
@@ -28,66 +31,163 @@ 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);
+  ~ExtensionWarning();
+
+  // Factory methods for various warning types.
+  static scoped_ptr<ExtensionWarning> CreateNetworkDelayWarning(
+      const std::string& extension_id);
+  static scoped_ptr<ExtensionWarning> CreateNetworkConflictWarning(
+      const std::string& extension_id);
+  static scoped_ptr<ExtensionWarning> CreateRedirectConflictWarning(
+      const std::string& extension_id,
+      const std::string& winning_extension_id,
+      const GURL& attempted_redirect_url,
+      const GURL& winning_redirect_url);
+  static scoped_ptr<ExtensionWarning> CreateRequestHeaderConflictWarning(
+      const std::string& extension_id,
+      const std::string& winning_extension_id,
+      const std::string& conflicting_header);
+  static scoped_ptr<ExtensionWarning> CreateResponseHeaderConflictWarning(
+      const std::string& extension_id,
+      const std::string& winning_extension_id,
+      const std::string& conflicting_header);
+  static scoped_ptr<ExtensionWarning> CreateCredentialsConflictWarning(
+      const std::string& extension_id,
+      const std::string& winning_extension_id);
+  static scoped_ptr<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(ExtensionService* extension_service) const;

[Aaron Boodman, 2012/09/05 08:11:11]

Hm, odd that this takes |extension_service|.

[battre, 2012/09/06 14:25:48]

I need to translate extension_ids to extension names and need to do this on the
UI thread, i.e. I cannot do it at construction time.

[Aaron Boodman, 2012/09/06 15:33:31]

Can you pass ExtensionSet instead?

[battre, 2012/09/10 17:21:38]

Done.

+
+ 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_;
+
+  DISALLOW_COPY_AND_ASSIGN(ExtensionWarning);
+};
+
+// 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 linked_ptr<ExtensionWarning>& a,
+               const linked_ptr<ExtensionWarning>& b);
+
+// Container for ExtensionWarnings intended to be passed between threads.
+// This exists to work around the lack of thread-safety of linke_ptrs.
+// Only one thread should have access to an ExtensionWarningSet at any time.
+class ExtensionWarningSet {

[Aaron Boodman, 2012/09/05 08:11:11]

Can ExtensionWarningSet and ExtensionWarningService be combined? I don't see the
distinction  between them, and I always prefer simple data objects to 'services'
that are designed to fit a certain place

[battre, 2012/09/06 14:25:48]

The reason for this class is that I need to pass a
"std::set<linked_ptr<ExtensionWarning> >" from the IO thread to the UI thread.
As linked_ptr is not thread safe, I cannot pass the
"std::set<linked_ptr<ExtensionWarning> >" directly but need to pass a wrapper.
This wrapper ensures that the IO thread cannot retain a reference to the
ExtensionWarning (because it passes a scoped_ptr<ExtensionWarning>). Also we
ensure that the ExtensionWarningSet does not retain any reference because of the
Release() implementation.

[Aaron Boodman, 2012/09/06 15:33:31]

Did you consider making this just std::set<ExtensionWarning> and passing by
value? It's less efficient, but significantly simpler, and this class should be
infrequently used (right?). We pass strings between threads all the time by
value, so this would follow convention.

[battre, 2012/09/10 17:21:38]

Done.

+ public:
+  ExtensionWarningSet();

[Aaron Boodman, 2012/09/05 08:11:11]

It seems like a map is a better fit for this class (from Extension* or
extension_id to std::vector<Warning>)

[battre, 2012/09/06 14:25:48]

We need set logic. For each pair of (extension_id, warning_type) we want to show
one error message. If we follow your suggestion, we would need to use a
std::map<std::string, std::map<ExtensionWarning::WarningType,
linked_ptr<WarningMessage> > >. This seems overkill to me.

[Aaron Boodman, 2012/09/06 15:33:31]

I see. OK.

+  ~ExtensionWarningSet();
+
+  // Inserts |warning| into the warning set if it is unique under operator<():
+  // If |warnings_| contains an entry of the same type and extension_id, the
+  // insertion is ignored.
+  void Insert(scoped_ptr<ExtensionWarning> warning);
+
+  // Returns and resets |warnings_|.
+  std::set<linked_ptr<ExtensionWarning> > Release();
+
+  bool IsEmpty() const;
+
+ private:
+  std::set<linked_ptr<ExtensionWarning> > warnings_;
 
+  DISALLOW_COPY_AND_ASSIGN(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
+// 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.

[Aaron Boodman, 2012/09/05 08:11:11]

We've been trying to reduce usage of notifications across the codebase. Can this
be phrased as an observer instead?

[battre, 2012/09/06 14:25:48]

I have created https://chromiumcodereview.appspot.com/10908119/ for this. I
would prefer to keep the current approach (IMO more readable, less coupled), but
that's your call.

   // 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 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 warnings for extension |extension_id| in |result|. The previous
   // content of |result| is erased.
   void GetWarningsAffectingExtension(
       const std::string& extension_id,
-      std::set<WarningType>* result) const;
+      std::set<linked_ptr<ExtensionWarning> >* 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(scoped_ptr<ExtensionWarningSet> warnings);
 
-  // Notifies the ExtensionWarningSet of profile |profile_id| that
-  // |extension_ids| caused warning |warning_type|. This function must only be
-  // called on the UI thread.
-  static void NotifyWarningsOnUI(void* profile_id,
-                                 std::set<std::string> extension_ids,
-                                 WarningType warning_type);
+  // Notifies the ExtensionWarningService of profile |profile_id| that new
+  // |warnings| occurred and triggers a warning badge.
+  static void NotifyWarningsOnUI(
+      void* profile_id,
+      scoped_ptr<ExtensionWarningSet> warnings);
 
  protected:
   // Virtual for testing.
   virtual void NotifyWarningsChanged();
 
  private:
-  typedef std::set<ExtensionWarning>::const_iterator const_iterator;
-  typedef std::set<ExtensionWarning>::iterator iterator;
+  typedef std::set<linked_ptr<ExtensionWarning> >::const_iterator
+      const_iterator;
+  typedef std::set<linked_ptr<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_;
+  std::set<linked_ptr<ExtensionWarning> > warnings_;

[Aaron Boodman, 2012/09/05 08:11:11]

We have been discouraged from using linked_ptr and encouraged to explicitly
delete items in the containing class' destructor. The argument is that
linked_ptr is essentially ref-counting and can make reasoning about lifetime
harder.

[battre, 2012/09/06 14:25:48]

I have created https://chromiumcodereview.appspot.com/10909096 but I would
highly appreciate not having to land it. I find this very ugly.

I don't see the point why people want immutable, passive objects not to be
refcounted.

I believe that in this CL the ownership of the linked_ptrs are now very clear
(either in ExtensionWarningSet or in ExtensionWarningService) - or temporarily
in a std::set<linked_ptr<ExtensionWarning> > while in flight between the two
classes.

 
   // Warnings that do not trigger a badge on the wrench menu.
-  std::set<ExtensionWarning> badge_suppressions_;
+  std::set<linked_ptr<ExtensionWarning> > badge_suppressions_;
 
   Profile* profile_;
 };