Create DeclarativeConditionSet, DeclarativeActionSet, and DeclarativeRule templates

chrome/browser/extensions/api/declarative/declarative_rule.h

Index: chrome/browser/extensions/api/declarative/declarative_rule.h
diff --git a/chrome/browser/extensions/api/declarative_webrequest/webrequest_rule.h b/chrome/browser/extensions/api/declarative/declarative_rule.h
similarity index 10%
rename from chrome/browser/extensions/api/declarative_webrequest/webrequest_rule.h
rename to chrome/browser/extensions/api/declarative/declarative_rule.h
index e7c2dcf0330308c63fa3dd2a1c10814a6d2720b8..4b073fc512e0ad052e23b47a76f3e061e8a7f093 100644
--- a/chrome/browser/extensions/api/declarative_webrequest/webrequest_rule.h
+++ b/chrome/browser/extensions/api/declarative/declarative_rule.h
@@ -1,98 +1,173 @@
 // 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.
+//
+// DeclarativeRule<>, DeclarativeConditionSet<>, and DeclarativeActionSet<>
+// templates usable with multiple different declarativeFoo systems.  These are
+// templated on the Condition and Action types that define the behavior of a
+// particular declarative event.
 
-#ifndef CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_WEBREQUEST_WEBREQUEST_RULE_H_
-#define CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_WEBREQUEST_WEBREQUEST_RULE_H_
+#ifndef CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_DECLARATIVE_RULE_H__
+#define CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_DECLARATIVE_RULE_H__
 
-#include <list>
-#include <vector>
+#include <limits>
 
-#include "base/compiler_specific.h"
+#include "base/memory/linked_ptr.h"
+#include "base/memory/scoped_vector.h"
 #include "base/time.h"
-#include "chrome/browser/extensions/api/declarative/rules_registry.h"
-#include "chrome/browser/extensions/api/declarative_webrequest/request_stage.h"
+#include "chrome/common/extensions/api/events.h"
+#include "chrome/common/extensions/matcher/url_matcher.h"
 
-class ExtensionInfoMap;
-class WebRequestPermissions;
+namespace base {
+class Time;
+}
 
 namespace extensions {
-class Extension;
-class URLMatcherConditionFactory;
-class WebRequestActionSet;
-class WebRequestConditionSet;
-}
 
-namespace extension_web_request_api_helpers {
-struct EventResponseDelta;
-}
+// This class stores a set of conditions that may be part of a DeclarativeRule.
+// If any condition is fulfilled, the Actions of the DeclarativeRule can be
+// triggered.
+template<typename ConditionT>

[battre, 2012/12/17 11:46:06]

The problem I see with template parameters is that it is always pretty difficult
to see which requirements they need to fulfill (here: have a typedef for
MatchData, have a static Create() method that takes a certain set of parameters,
IsFulfilled() method, etc...). Should we document these requirements here?

I think that we should also document that conditions and actions are supposed to
be strictly immutable objects.

[Jeffrey Yasskin, 2012/12/19 08:23:06]

Done.

+class DeclarativeConditionSet {
+   public:
+  typedef std::vector<linked_ptr<json_schema_compiler::any::Any> > AnyVector;
+  typedef std::vector<linked_ptr<ConditionT> > Conditions;
+  typedef typename Conditions::const_iterator const_iterator;
 
-namespace net {
-class HttpResponseHeaders;
-class URLRequest;
-}
+  explicit DeclarativeConditionSet(const Conditions& conditions);
 
-namespace extensions {
+  // Factory method that creates an WebRequestConditionSet according to the JSON
+  // array |conditions| passed by the extension API.
+  // Sets |error| and returns NULL in case of an error.
+  static scoped_ptr<DeclarativeConditionSet> Create(
+      URLMatcherConditionFactory* url_matcher_condition_factory,
+      const AnyVector& conditions,
+      std::string* error);
+
+  const Conditions& conditions() const {
+    return conditions_;
+  }
+
+  const_iterator begin() const { return conditions_.begin(); }
+  const_iterator end() const { return conditions_.end(); }
+
+  // Returns whether any condition in the condition set is fulfilled
+  // based on a match |url_match| and the value of |request_data.request|.
+  // This function should be called for each URLMatcherConditionSet::ID
+  // that was found by the URLMatcher to ensure that the each trigger in
+  // |match_triggers_| is found.
+  bool IsFulfilled(
+      URLMatcherConditionSet::ID url_match,

[battre, 2012/12/17 11:46:06]

This one will break with vabr's change:
He is implementing a CL to allow the decl. WebRequest API to filter on
request-urls abut also main frame-urls. PTAL here
https://codereview.chromium.org/11414230/. Can we unify this?

That CL is not finished yet. Here is how I envision it:

We'll have two URL matchers, one for the request URL and the other one for the
main frame url (actually that's the firstPartyForCookies URL).

We would introduce two MatchTrigger instances for rules with the respective URL
matchers and a list of Rules that have no URLFilters.

The biggest problem I see is that the decl. Content API has different
interpretations of URLMatchers:
- for the decl. WebRequest API it is request URL and firstPartyForCookies URL
- for the decl. Content API that you are currently writing it will be main frame
URL
- for the decl. Content API extensions (for blocking resource loads directly in
the renderers) that I will write it will be request URL and main frame URL

[Jeffrey Yasskin, 2012/12/18 02:44:26]

I've commented on https://codereview.chromium.org/11569007/. I suspect what we
want to do pass the matching ID sets as part of the MatchData struct. Then each
kind of rule can apply their own interpretation.

+      const typename ConditionT::MatchData& match_data) const;
+
+  // Appends the URLMatcherConditionSet from all conditions to |condition_sets|.
+  void GetURLMatcherConditionSets(
+      URLMatcherConditionSet::Vector* condition_sets) const;
+
+ private:
+  Conditions conditions_;
+
+  typedef std::map<URLMatcherConditionSet::ID, ConditionT*>
+      MatchTriggers;
+  MatchTriggers match_triggers_;
+
+  DISALLOW_COPY_AND_ASSIGN(DeclarativeConditionSet);
+};
+
+// Immutable container for multiple actions.
+//
+// TODO(battre): As DeclarativeActionSet can become the single owner of all
+// actions, we can optimize here by making some of them singletons (e.g. Cancel
+// actions).
+template<typename ActionT>
+class DeclarativeActionSet {
+ public:
+  typedef std::vector<linked_ptr<json_schema_compiler::any::Any> > AnyVector;
+  typedef std::vector<linked_ptr<ActionT> > Actions;
 
-typedef linked_ptr<extension_web_request_api_helpers::EventResponseDelta>
-    LinkedPtrEventResponseDelta;
+  explicit DeclarativeActionSet(const Actions& actions);
 
-// Representation of a rule of the declarative Web Request API
-class WebRequestRule {
+  // Factory method that instantiates a DeclarativeActionSet according to
+  // |actions| which represents the array of actions received from the
+  // extension API.
+  static scoped_ptr<DeclarativeActionSet> Create(const AnyVector& actions,
+                                                 std::string* error,
+                                                 bool* bad_message);
+
+  // Rules call this method when their conditions are fulfilled.
+  void Apply(const std::string& extension_id,
+             const base::Time& extension_install_time,
+             const typename ActionT::ApplyInfo& apply_info) const;

[battre, 2012/12/17 11:46:06]

const apply_info?

[Jeffrey Yasskin, 2012/12/18 02:44:26]

It's already a const reference. What do you mean?

[battre, 2012/12/18 08:38:49]

That's my point. Why is it const. This is the variable that we want to modify,
it is an output parameter.

I'd suggest to pass a "typename ActionT::ApplyInfo* apply_info"

[Jeffrey Yasskin, 2012/12/19 08:23:06]

It was const because it holds a non-const pointer to the out parameter. It's
easy enough to make the input parameters const instead, or just to rely on
Action implementations to do the right thing so they cooperate with their
RulesRegistry.

+
+  // Rules call this method when they have stateful conditions, and those
+  // conditions stop being fulfilled.  Rules with event-based conditions (e.g. a
+  // network request happened) will never Revert() an action.
+  void Revert(const std::string& extension_id,
+              const base::Time& extension_install_time,
+              const typename ActionT::ApplyInfo& apply_info) const;

[battre, 2012/12/17 11:46:06]

const apply_info?

[Jeffrey Yasskin, 2012/12/19 08:23:06]

Done.

+
+  // Returns the minimum priority of rules that may be evaluated after
+  // this rule. Defaults to MIN_INT.
+  int GetMinimumPriority() const;
+
+  const Actions& actions() const { return actions_; }
+
+ private:
+  Actions actions_;
+
+  DISALLOW_COPY_AND_ASSIGN(DeclarativeActionSet);
+};
+
+template<typename ConditionT, typename ActionT>
+class DeclarativeRule {

[Jeffrey Yasskin, 2012/12/19 08:23:06]

What? No complaint that I completely forgot a comment on this class? ;) (Added
one)

  public:
   typedef std::string ExtensionId;
   typedef std::string RuleId;
   typedef std::pair<ExtensionId, RuleId> GlobalRuleId;
   typedef int Priority;
+  typedef DeclarativeConditionSet<ConditionT> ConditionSet;
+  typedef DeclarativeActionSet<ActionT> ActionSet;
+  typedef extensions::api::events::Rule JsonRule;
+
+  // Checks whether the set of |conditions| and |actions| are consistent.
+  // Returns true in case of consistency and MUST set |error| otherwise.
+  typedef bool (*ConsistencyChecker)(ConditionSet* conditions,
+                                     ActionSet* actions,
+                                     std::string* error);
 
-  struct RequestData {
-    RequestData(net::URLRequest* request, RequestStage stage)
-        : request(request), stage(stage),
-          original_response_headers(NULL) {}
-    RequestData(net::URLRequest* request, RequestStage stage,
-                const net::HttpResponseHeaders* original_response_headers)
-        : request(request), stage(stage),
-          original_response_headers(original_response_headers) {}
-    net::URLRequest* request;
-    RequestStage stage;
-    // Additional information about requests that is not
-    // available in all request stages.
-    const net::HttpResponseHeaders* original_response_headers;
-  };
-
-  WebRequestRule(const GlobalRuleId& id,
-                 base::Time extension_installation_time,
-                 scoped_ptr<WebRequestConditionSet> conditions,
-                 scoped_ptr<WebRequestActionSet> actions,
-                 Priority priority);
-  virtual ~WebRequestRule();
-
-  // If |error| is empty, the translation was successful and the returned
-  // rule is internally consistent.
-  static scoped_ptr<WebRequestRule> Create(
+  DeclarativeRule(const GlobalRuleId& id,
+                  base::Time extension_installation_time,
+                  scoped_ptr<ConditionSet> conditions,
+                  scoped_ptr<ActionSet> actions,
+                  Priority priority);
+
+  // Calls check_consistency(conditions, actions, error) and returns
+  // NULL if it fails.  Pass NULL if no consistency check is needed.
+  // If |error| is empty, the translation was successful and the
+  // returned rule is internally consistent.
+  static scoped_ptr<DeclarativeRule> Create(
       URLMatcherConditionFactory* url_matcher_condition_factory,
       const std::string& extension_id,
       base::Time extension_installation_time,
-      linked_ptr<RulesRegistry::Rule> rule,
+      linked_ptr<JsonRule> rule,
+      ConsistencyChecker check_consistency,
       std::string* error);
 
   const GlobalRuleId& id() const { return id_; }
   const std::string& extension_id() const { return id_.first; }
-  const WebRequestConditionSet& conditions() const { return *conditions_; }
-  const WebRequestActionSet& actions() const { return *actions_; }
+  const ConditionSet& conditions() const { return *conditions_; }
+  const ActionSet& actions() const { return *actions_; }
   Priority priority() const { return priority_; }
 
-  // Creates all deltas resulting from the ActionSet. This function should
-  // only be called when the conditions_ are fulfilled (from a semantic point
-  // of view; no harm is done if this function is called at other times for
-  // testing purposes).
+  // Creates all deltas resulting from the ActionSet and fills them into
+  // apply_info.deltas. This function should only be called when the conditions_
+  // are fulfilled (from a semantic point of view; no harm is done if this
+  // function is called at other times for testing purposes).
+  //
   // If |extension| is set, deltas are suppressed if the |extension| does not
   // have have sufficient permissions to modify the request. The returned list
   // may be empty in this case.

[battre, 2012/12/17 11:46:06]

nit: there is no returned list.

[Jeffrey Yasskin, 2012/12/19 08:23:06]

Indeed. I've rewritten this comment to avoid discussing WebRequest details.

-  std::list<LinkedPtrEventResponseDelta> CreateDeltas(
-      const ExtensionInfoMap* extension_info_map,
-      const RequestData& request_data,
-      bool crosses_incognito) const;
+  void Apply(const typename ActionT::ApplyInfo& apply_info) const;

[battre, 2012/12/17 11:46:06]

const?

[Jeffrey Yasskin, 2012/12/19 08:23:06]

Done.

 
   // Returns the minimum priority of rules that may be evaluated after
   // this rule. Defaults to MAX_INT. Only valid if the conditions of this rule
@@ -100,23 +175,218 @@ class WebRequestRule {
   Priority GetMinimumPriority() const;
 
  private:
-  // Checks whether the set of |conditions| and |actions| are consistent,
-  // meaning for example that we do not allow combining an |action| that needs
-  // to be executed before the |condition| can be fulfilled.
-  // Returns true in case of consistency and MUST set |error| otherwise.
-  static bool CheckConsistency(WebRequestConditionSet* conditions,
-                               WebRequestActionSet* actions,
-                               std::string* error);
-
   GlobalRuleId id_;
   base::Time extension_installation_time_;  // For precedences of rules.
-  scoped_ptr<WebRequestConditionSet> conditions_;
-  scoped_ptr<WebRequestActionSet> actions_;
+  scoped_ptr<ConditionSet> conditions_;
+  scoped_ptr<ActionSet> actions_;
   Priority priority_;
 
-  DISALLOW_COPY_AND_ASSIGN(WebRequestRule);
+  DISALLOW_COPY_AND_ASSIGN(DeclarativeRule);
 };
 
+// Implementation details below here.
+
+//
+// DeclarativeConditionSet
+//
+
+template<typename ConditionT>
+DeclarativeConditionSet<ConditionT>::DeclarativeConditionSet(
+    const DeclarativeConditionSet::Conditions& conditions)
+    : conditions_(conditions) {
+  for (typename Conditions::iterator i = conditions_.begin();
+       i != conditions_.end(); ++i) {
+    URLMatcherConditionSet::ID trigger_id =
+        (*i)->url_matcher_condition_set_id();
+    match_triggers_[trigger_id] = i->get();
+  }
+}
+
+template<typename ConditionT>
+bool DeclarativeConditionSet<ConditionT>::IsFulfilled(
+    URLMatcherConditionSet::ID url_match,
+    const typename ConditionT::MatchData& match_data) const {
+  typename MatchTriggers::const_iterator trigger =
+      match_triggers_.find(url_match);
+  DCHECK(trigger != match_triggers_.end());
+  DCHECK_EQ(url_match, trigger->second->url_matcher_condition_set_id());
+  return trigger->second->IsFulfilled(match_data);

[battre, 2012/12/17 11:46:06]

should we do
return const_cast<const ConditionT*>(trigger->second)->IsFulfilled(match_data)
to enforce that conditions are immutable?

[Jeffrey Yasskin, 2012/12/18 02:44:26]

We could make match_triggers_ into a std::map<URLMatcherConditionSet::ID, const
ConditionT*>. I'm not sure why it's non-const in WebRequestConditionSet.

[battre, 2012/12/18 08:38:49]

SGTM

[Jeffrey Yasskin, 2012/12/19 08:23:06]

match_triggers_ is gone, but I made all the lists hold const conditions and
actions.

+}
+
+template<typename ConditionT>
+void DeclarativeConditionSet<ConditionT>::GetURLMatcherConditionSets(
+    URLMatcherConditionSet::Vector* condition_sets) const {
+  for (typename Conditions::const_iterator i = conditions_.begin();
+       i != conditions_.end(); ++i) {
+    condition_sets->push_back((*i)->url_matcher_condition_set());
+  }
+}
+
+// static
+template<typename ConditionT>
+scoped_ptr<DeclarativeConditionSet<ConditionT> >
+DeclarativeConditionSet<ConditionT>::Create(
+    URLMatcherConditionFactory* url_matcher_condition_factory,
+    const AnyVector& conditions,
+    std::string* error) {
+  DeclarativeConditionSet::Conditions result;
+
+  for (AnyVector::const_iterator i = conditions.begin();
+       i != conditions.end(); ++i) {
+    CHECK(i->get());
+    scoped_ptr<ConditionT> condition =
+        ConditionT::Create(url_matcher_condition_factory,
+                           (*i)->value(), error);
+    if (!error->empty())
+      return scoped_ptr<DeclarativeConditionSet>(NULL);
+    result.push_back(make_linked_ptr(condition.release()));
+  }
+
+  return make_scoped_ptr(new DeclarativeConditionSet(result));
+}
+
+//
+// DeclarativeActionSet
+//
+
+template<typename ActionT>
+DeclarativeActionSet<ActionT>::DeclarativeActionSet(const Actions& actions)
+    : actions_(actions) {}
+
+// static
+template<typename ActionT>
+scoped_ptr<DeclarativeActionSet<ActionT> >
+DeclarativeActionSet<ActionT>::Create(
+    const AnyVector& actions,
+    std::string* error,
+    bool* bad_message) {
+  *error = "";
+  *bad_message = false;
+  Actions result;
+
+  for (AnyVector::const_iterator i = actions.begin();
+       i != actions.end(); ++i) {
+    CHECK(i->get());
+    scoped_ptr<ActionT> action =
+        ActionT::Create((*i)->value(), error, bad_message);
+    if (!error->empty() || *bad_message)
+      return scoped_ptr<DeclarativeActionSet>(NULL);
+    result.push_back(make_linked_ptr(action.release()));
+  }
+
+  return scoped_ptr<DeclarativeActionSet>(new DeclarativeActionSet(result));
+}
+
+template<typename ActionT>
+void DeclarativeActionSet<ActionT>::Apply(
+    const std::string& extension_id,
+    const base::Time& extension_install_time,
+    const typename ActionT::ApplyInfo& apply_info) const {
+  for (typename Actions::const_iterator i = actions_.begin();
+       i != actions_.end(); ++i)
+    (*i)->Apply(extension_id, extension_install_time, apply_info);
+}
+
+template<typename ActionT>
+void DeclarativeActionSet<ActionT>::Revert(
+    const std::string& extension_id,
+    const base::Time& extension_install_time,
+    const typename ActionT::ApplyInfo& apply_info) const {
+  for (typename Actions::const_iterator i = actions_.begin();
+       i != actions_.end(); ++i)
+    (*i)->Revert(extension_id, extension_install_time, apply_info);
+}
+
+template<typename ActionT>
+int DeclarativeActionSet<ActionT>::GetMinimumPriority() const {
+  int minimum_priority = std::numeric_limits<int>::min();
+  for (typename Actions::const_iterator i = actions_.begin();
+       i != actions_.end(); ++i) {
+    minimum_priority = std::max(minimum_priority, (*i)->GetMinimumPriority());
+  }
+  return minimum_priority;
+}
+
+//
+// DeclarativeRule
+//
+
+template<typename ConditionT, typename ActionT>
+DeclarativeRule<ConditionT, ActionT>::DeclarativeRule(
+    const GlobalRuleId& id,
+    base::Time extension_installation_time,
+    scoped_ptr<ConditionSet> conditions,
+    scoped_ptr<ActionSet> actions,
+    Priority priority)
+    : id_(id),
+      extension_installation_time_(extension_installation_time),
+      conditions_(conditions.release()),
+      actions_(actions.release()),
+      priority_(priority) {
+  CHECK(conditions_.get());
+  CHECK(actions_.get());
+}
+
+// static
+template<typename ConditionT, typename ActionT>
+scoped_ptr<DeclarativeRule<ConditionT, ActionT> >
+DeclarativeRule<ConditionT, ActionT>::Create(
+    URLMatcherConditionFactory* url_matcher_condition_factory,
+    const std::string& extension_id,
+    base::Time extension_installation_time,
+    linked_ptr<JsonRule> rule,
+    ConsistencyChecker check_consistency,
+    std::string* error) {
+  scoped_ptr<DeclarativeRule> error_result;
+
+  scoped_ptr<ConditionSet> conditions = ConditionSet::Create(
+      url_matcher_condition_factory, rule->conditions, error);
+  if (!error->empty())
+    return error_result.Pass();
+  CHECK(conditions.get());
+
+  bool bad_message = false;
+  scoped_ptr<ActionSet> actions =
+      ActionSet::Create(rule->actions, error, &bad_message);
+  if (bad_message) {
+    // TODO(battre) Export concept of bad_message to caller, the extension
+    // should be killed in case it is true.
+    *error = "An action of a rule set had an invalid "
+        "structure that should have been caught by the JSON validator.";
+    return error_result.Pass();
+  }
+  if (!error->empty() || bad_message)
+    return error_result.Pass();
+  CHECK(actions.get());
+
+  if (check_consistency &&
+      !check_consistency(conditions.get(), actions.get(), error)) {
+    DCHECK(!error->empty());
+    return error_result.Pass();
+  }
+
+  CHECK(rule->priority.get());
+  int priority = *(rule->priority);
+
+  GlobalRuleId rule_id(extension_id, *(rule->id));
+  return scoped_ptr<DeclarativeRule>(
+      new DeclarativeRule(rule_id, extension_installation_time,
+                         conditions.Pass(), actions.Pass(), priority));
+}
+
+template<typename ConditionT, typename ActionT>
+void DeclarativeRule<ConditionT, ActionT>::Apply(
+    const typename ActionT::ApplyInfo& apply_info) const {
+  return actions_->Apply(extension_id(),
+                         extension_installation_time_,
+                         apply_info);
+}
+
+template<typename ConditionT, typename ActionT>
+int DeclarativeRule<ConditionT, ActionT>::GetMinimumPriority() const {
+  return actions_->GetMinimumPriority();
+}
+
 }  // namespace extensions
 
-#endif  // CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_WEBREQUEST_WEBREQUEST_RULE_H_
+#endif  // CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_DECLARATIVE_RULE_H__