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..a2958f83832b4a06f0457233f391fd09b7e89cce 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"
@@ -32,22 +33,26 @@ namespace extensions {
 // ConditionT should be immutable after creation.  It must define the following
 // members:
 //
-//   // Arguments passed through from ConditionSet::Create.
+//   // Arguments passed through from DeclarativeConditionSet::Create.
 //   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 increase by 1. The return value always gives the next higher

[Jeffrey Yasskin, 2013/01/22 08:21:06]

This seems too complicated, and you didn't manage to obey the contract (which
allows returning a value > index+1) in ConditionSet::GetURLMatcherConditionSets.
Maybe have it return true if index+1 is valid, or false if it's the highest
already?

+//   // 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.
+//   // DeclarativeConditionSet::IsFulfilled.
 //   bool IsFulfilled(
 //       const std::set<URLMatcherConditionSet::ID>& url_matches,
 //       const ConditionT::MatchData&);
@@ -58,11 +63,16 @@ class DeclarativeConditionSet {
   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.
+  // Factory method that creates a WebRequestConditionSet according to the JSON
+  // array |conditions| passed by the extension API. Because 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. This allows

[Jeffrey Yasskin, 2013/01/22 08:21:06]

How? Using a single URLMatcher would guarantee this trivially, which is one
reason I suggested doing that.

+  // one to tell which attribute had a match in IsFulfilled.
   // 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 +83,24 @@ 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|.

[Jeffrey Yasskin, 2013/01/22 08:21:06]

s/request_data/match_data/

[vabr (Chromium), 2013/01/22 14:54:30]

Done.

+  // Therefore, if |match_data| contains URL matches from more matchers, the

[Jeffrey Yasskin, 2013/01/22 08:21:06]

"more matchers" than what?

[vabr (Chromium), 2013/01/22 14:54:30]

I meant "multiple", i.e., more than 1. Changed.

EDIT: "Multiple matchers" does not make sense any more. If we decide to get rid
of |url_matches|, then that sentence will be erased. Otherwise I'll change "URL
matches from more matchers" to "matches from multiple URLs".

+  // 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|.
-  void GetURLMatcherConditionSets(
-      URLMatcherConditionSet::Vector* condition_sets) const;
+  // Returns |index| + 1 if at least one condition's GetURLMatcherConditionSets
+  // returns |index| + 1, otherwise returns -1.
+  int GetURLMatcherConditionSets(

[Jeffrey Yasskin, 2013/01/22 08:21:06]

It looks like every use of this function just iterates over all of the indices
and unions up all of the sets. If that's true, I think the original signature
was correct.

+      URLMatcherConditionSet::Vector* condition_sets,
+      int index) const;
 
   // Returns whether there are some conditions without UrlFilter attributes.
   bool HasConditionsWithoutUrls() const {
@@ -223,7 +227,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 +292,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 more_condition_sets_exist = false;
   for (typename Conditions::const_iterator i = conditions_.begin();
        i != conditions_.end(); ++i) {
-    (*i)->GetURLMatcherConditionSets(condition_sets);
+    if ((*i)->GetURLMatcherConditionSets(condition_sets, index) >= 0)
+      more_condition_sets_exist = true;
   }
+  return (more_condition_sets_exist ? (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 +318,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 +331,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);
     if (condition_sets.empty()) {
       conditions_without_urls.push_back(i->get());
     } else {
@@ -430,7 +442,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 +452,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());