Declarative Web Request: firstPartyForCookiesUrl

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

 // Copyright (c) 2013 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_DECLARATIVE_RULE_H__
 #define CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_DECLARATIVE_RULE_H__
 
 #include <limits>
+#include <vector>
 
 #include "base/memory/linked_ptr.h"
 #include "base/memory/scoped_vector.h"
 #include "base/time.h"
 #include "chrome/common/extensions/api/events.h"
 #include "chrome/common/extensions/matcher/url_matcher.h"
 
 namespace base {
 class Time;
 class Value;
 }
 
 namespace extensions {
 
 // 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.
 //
 // ConditionT should be immutable after creation.  It must define the following
 // members:
 //
 //   // Arguments passed through from ConditionSet::Create.

[battre, 2013/01/21 17:32:08]

can you please update ConditionSet->DeclarativeConditionSet

[vabr (Chromium), 2013/01/21 18:39:29]

Done. To be correct, we would need to also add <ConditionT>, though.

(Personally I liked the original short version, which was easy to read, and with
the typedef from DeclarativeRule it was also quite clear to understand.)

 //   static scoped_ptr<ConditionT> Create(
-//       URLMatcherConditionFactory*,
+//       const std::vector<URLMatcherConditionFactory*>&
+//           url_matcher_condition_factories,
 //       // Except this argument gets elements of the AnyVector.
 //       const base::Value& definition,
 //       std::string* error);
-//   // If the Condition needs to be filtered by some
-//   // URLMatcherConditionSets, append them to this argument.
+//   // If the Condition needs to be filtered by some URLMatcherConditionSets,
+//   // append them to |condition_sets|. Use |index| to select the type of a
+//   // URL attribute, multiple can be present. Valid values of |index| start
+//   // at 0 and are nonnegative. The return value always gives the next higher
+//   // valid index, or -1 if there is none. It is thus possible to iterate over
+//   // all valid indices by starting with 0 and using the return values in
+//   // subsequent calls until -1 is returned.
 //   // DeclarativeConditionSet::GetURLMatcherConditionSets forwards here.
-//   void GetURLMatcherConditionSets(
-//       URLMatcherConditionSet::Vector* condition_sets)
-//   // True if GetURLMatcherConditionSets would append anything to its
-//   // argument.
-//   bool has_url_matcher_condition_set();
+//   int GetURLMatcherConditionSets(
+//       URLMatcherConditionSet::Vector* condition_sets,
+//       int index);
 //   // |url_matches| and |match_data| passed through from
 //   // ConditionSet::IsFulfilled.
 //   bool IsFulfilled(
 //       const std::set<URLMatcherConditionSet::ID>& url_matches,
 //       const ConditionT::MatchData&);
 template<typename ConditionT>
 class DeclarativeConditionSet {
  public:
   typedef std::vector<linked_ptr<base::Value> > AnyVector;
   typedef std::vector<linked_ptr<const ConditionT> > Conditions;
   typedef typename Conditions::const_iterator const_iterator;
 
   // Factory method that creates an WebRequestConditionSet according to the JSON
   // array |conditions| passed by the extension API.

[battre, 2013/01/21 17:32:08]

Please add:
// As IsFulfilled gets passed the union of IDs of all matching
URLMatcherConditionSets coming from all URLMatcherConditionFactories, you should
make sure that the factories use strictly disjoined URLMatcherConditionSets::IDs
if you need to know which one had a match in IsFulfilled.

[vabr (Chromium), 2013/01/21 18:39:29]

Done, with minor edits. Please check that you are fine with them.

   // Sets |error| and returns NULL in case of an error.
   static scoped_ptr<DeclarativeConditionSet> Create(
-      URLMatcherConditionFactory* url_matcher_condition_factory,
+      const std::vector<URLMatcherConditionFactory*>&
+          url_matcher_condition_factories,
       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(); }
 
-  // If |url_match_trigger| is a member of |url_matches|, then this
-  // returns whether the corresponding condition is fulfilled
-  // wrt. |request_data|. If |url_match_trigger| is -1, this function
-  // returns whether any of the conditions without URL attributes is
-  // satisfied.
-  //
-  // Conditions for which has_url_matcher_condition_set() is false are always
-  // checked (aside from short-circuiting when an earlier condition already
-  // matched.)
-  //
-  // Conditions for which has_url_matcher_condition_set() is true are only
-  // checked if one of the URLMatcherConditionSets returned by
-  // GetURLMatcherConditionSets() has an id listed in url_matches.  That means
-  // that if |match_data| contains URL matches for two pieces of a request,
-  // their union should appear in |url_matches|.  For kinds of MatchData that
-  // only have one type of URL, |url_matches| is forwarded on to
+  // If |url_match_trigger| is a member of |url_matches|, then this returns
+  // whether the corresponding condition is fulfilled wrt. |request_data|.
+  // Therefore, if |match_data| contains URL matches from more matchers, the
+  // union of the matches should appear in |url_matches|. For kinds of MatchData
+  // that only have one type of URL, |url_matches| is forwarded on to
   // ConditionT::IsFulfilled(), so it doesn't need to appear in |match_data|.
+  // If |url_match_trigger| is -1, this function returns whether any of the
+  // conditions without URL attributes is satisfied.
   bool IsFulfilled(URLMatcherConditionSet::ID url_match_trigger,
                    const std::set<URLMatcherConditionSet::ID>& url_matches,
                    const typename ConditionT::MatchData& match_data) const;
 
   // Appends the URLMatcherConditionSet from all conditions to |condition_sets|.

[battre, 2013/01/21 17:32:08]

Please comment on the return value.

[vabr (Chromium), 2013/01/21 18:39:29]

Done.

-  void GetURLMatcherConditionSets(
-      URLMatcherConditionSet::Vector* condition_sets) const;
+  int GetURLMatcherConditionSets(
+      URLMatcherConditionSet::Vector* condition_sets,
+      int index) const;
 
   // Returns whether there are some conditions without UrlFilter attributes.
   bool HasConditionsWithoutUrls() const {
     return !conditions_without_urls_.empty();
   }
 
  private:
   typedef std::map<URLMatcherConditionSet::ID, const ConditionT*>
       URLMatcherIdToCondition;
 
@@ skipping 106 matching lines @@
 
   // Creates a DeclarativeRule for an extension given a json definition.  The
   // format of each condition and action's json is up to the specific ConditionT
   // and ActionT.
   //
   // Before constructing the final rule, 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::vector<URLMatcherConditionFactory*>&
+          url_matcher_condition_factories,
       const std::string& extension_id,
       base::Time extension_installation_time,
       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 ConditionSet& conditions() const { return *conditions_; }
   const ActionSet& actions() const { return *actions_; }
@@ skipping 43 matching lines @@
     return false;
   }
 
   typename URLMatcherIdToCondition::const_iterator triggered =
       match_id_to_condition_.find(url_match_trigger);
   return (triggered != match_id_to_condition_.end() &&
           triggered->second->IsFulfilled(url_matches, match_data));
 }
 
 template<typename ConditionT>
-void DeclarativeConditionSet<ConditionT>::GetURLMatcherConditionSets(
-    URLMatcherConditionSet::Vector* condition_sets) const {
+int DeclarativeConditionSet<ConditionT>::GetURLMatcherConditionSets(
+    URLMatcherConditionSet::Vector* condition_sets,
+    int index) const {
+  bool positive_returned = false;

[battre, 2013/01/21 17:32:08]

nit: rename this to "more_condition_sets_exist"?

[vabr (Chromium), 2013/01/21 18:39:29]

Done.

   for (typename Conditions::const_iterator i = conditions_.begin();
        i != conditions_.end(); ++i) {
-    (*i)->GetURLMatcherConditionSets(condition_sets);
+    if ((*i)->GetURLMatcherConditionSets(condition_sets, index) >= 0)
+      positive_returned = true;
   }
+  return (positive_returned ? (index + 1) : -1);
 }
 
 // static
 template<typename ConditionT>
 scoped_ptr<DeclarativeConditionSet<ConditionT> >
 DeclarativeConditionSet<ConditionT>::Create(
-    URLMatcherConditionFactory* url_matcher_condition_factory,
+    const std::vector<URLMatcherConditionFactory*>&
+        url_matcher_condition_factories,
     const AnyVector& conditions,
     std::string* error) {
   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, error);
+        ConditionT::Create(url_matcher_condition_factories, **i, error);
     if (!error->empty())
       return scoped_ptr<DeclarativeConditionSet>(NULL);
     result.push_back(make_linked_ptr(condition.release()));
   }
 
   URLMatcherIdToCondition match_id_to_condition;
   std::vector<const ConditionT*> conditions_without_urls;
   URLMatcherConditionSet::Vector condition_sets;
 
   for (typename Conditions::const_iterator i = result.begin();
        i != result.end(); ++i) {
     condition_sets.clear();
-    (*i)->GetURLMatcherConditionSets(&condition_sets);
+    int index = 0;
+    while (index >= 0)
+      index = (*i)->GetURLMatcherConditionSets(&condition_sets, index);

[battre, 2013/01/21 17:32:08]

should we check the invariants here?
int old_index = 0;
int index = 0;
while (index >= 0) {
  old_index = index;
  index = ...;
  CHECK(index < 0 || index = old_index + 1);
}

How about also checking that each GetURLMatcherConditionSets here returns unique
IDs?

[vabr (Chromium), 2013/01/22 08:34:53]

Done.

     if (condition_sets.empty()) {
       conditions_without_urls.push_back(i->get());
     } else {
       for (URLMatcherConditionSet::Vector::const_iterator
                match_set = condition_sets.begin();
            match_set != condition_sets.end(); ++match_set)
         match_id_to_condition[(*match_set)->id()] = i->get();
     }
   }
 
@@ skipping 88 matching lines @@
       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::vector<URLMatcherConditionFactory*>&
+        url_matcher_condition_factories,
     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);
+      url_matcher_condition_factories, 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.
@@ skipping 29 matching lines @@
 }
 
 template<typename ConditionT, typename ActionT>
 int DeclarativeRule<ConditionT, ActionT>::GetMinimumPriority() const {
   return actions_->GetMinimumPriority();
 }
 
 }  // namespace extensions
 
 #endif  // CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_DECLARATIVE_RULE_H__