Declarative Web Request: firstPartyForCookiesUrl

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/declarative_rule.h b/chrome/browser/extensions/api/declarative/declarative_rule.h
index b2dbcd2eac1cc4af6e828195fc842a78b683c63a..06e6c134357d0de08d2be626e029f7759872a6fe 100644
--- a/chrome/browser/extensions/api/declarative/declarative_rule.h
+++ b/chrome/browser/extensions/api/declarative/declarative_rule.h
@@ -11,6 +11,7 @@
 #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"
@@ -34,18 +35,22 @@ namespace extensions {
 //
 //   // 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(
@@ -62,7 +67,8 @@ class DeclarativeConditionSet {
   // 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);
 
@@ -73,30 +79,22 @@ class DeclarativeConditionSet {
   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 {
@@ -223,7 +221,8 @@ class DeclarativeRule {
   // 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,
@@ -287,19 +286,24 @@ bool DeclarativeConditionSet<ConditionT>::IsFulfilled(
 }
 
 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;
@@ -308,7 +312,7 @@ DeclarativeConditionSet<ConditionT>::Create(
        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()));
@@ -321,7 +325,9 @@ DeclarativeConditionSet<ConditionT>::Create(
   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 {
@@ -430,7 +436,8 @@ DeclarativeRule<ConditionT, ActionT>::DeclarativeRule(
 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,
@@ -439,7 +446,7 @@ DeclarativeRule<ConditionT, ActionT>::Create(
   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());