Declarative Web Request: firstPartyForCookiesUrl

chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc

 // 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.
 
 #include "chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/stringprintf.h"
 #include "base/values.h"
@@ skipping 27 matching lines @@
 namespace extensions {
 
 namespace keys = declarative_webrequest_constants;
 
 //
 // WebRequestCondition
 //
 
 WebRequestCondition::WebRequestCondition(
     scoped_refptr<URLMatcherConditionSet> url_matcher_conditions,
+    scoped_refptr<URLMatcherConditionSet> first_party_url_matcher_conditions,
     const WebRequestConditionAttributes& condition_attributes)
     : url_matcher_conditions_(url_matcher_conditions),
+      first_party_url_matcher_conditions_(first_party_url_matcher_conditions),
       condition_attributes_(condition_attributes),
       applicable_request_stages_(~0) {
   CHECK(url_matcher_conditions.get());
   for (WebRequestConditionAttributes::const_iterator i =
        condition_attributes_.begin(); i != condition_attributes_.end(); ++i) {
     applicable_request_stages_ &= (*i)->GetStages();
   }
 }
 
 WebRequestCondition::~WebRequestCondition() {}
 
 bool WebRequestCondition::IsFulfilled(
     const WebRequestRule::RequestData& request_data) const {
   // All condition attributes must be fulfilled for a fulfilled condition.
   if (!(request_data.stage & applicable_request_stages_)) {
     // A condition that cannot be evaluated is considered as violated.
     return false;
   }
 
   for (WebRequestConditionAttributes::const_iterator i =
        condition_attributes_.begin(); i != condition_attributes_.end(); ++i) {
     if (!(*i)->IsFulfilled(request_data))
       return false;
   }
   return true;
 }
 
+// Creates a URLMatcherConditionSet with a single, always true condition.
+// Helper function for WebRequestCondition::Create.
+scoped_refptr<URLMatcherConditionSet> CreateAlwaysTrueUrlCondition(

[battre, 2012/12/06 19:53:26]

How about
1) Renaming this to CreateAlwaysTrueConditionSet (drop Url, add Set)
2) Moving this to URLMatcherConditionFactory (as a non-static function)
?

+    URLMatcherConditionFactory* factory) {
+  URLMatcherConditionSet::Conditions conditions;
+  conditions.insert(factory->CreateHostPrefixCondition(""));
+  return new URLMatcherConditionSet(++g_next_id, conditions);
+}
+
 // static
 scoped_ptr<WebRequestCondition> WebRequestCondition::Create(
     URLMatcherConditionFactory* url_matcher_condition_factory,
+    URLMatcherConditionFactory* first_party_url_matcher_condition_factory,
     const base::Value& condition,
     std::string* error) {
   const base::DictionaryValue* condition_dict = NULL;
   if (!condition.GetAsDictionary(&condition_dict)) {
     *error = kExpectedDictionary;
     return scoped_ptr<WebRequestCondition>(NULL);
   }
 
   // Verify that we are dealing with a Condition whose type we understand.
   std::string instance_type;
   if (!condition_dict->GetString(keys::kInstanceTypeKey, &instance_type)) {
     *error = kConditionWithoutInstanceType;
     return scoped_ptr<WebRequestCondition>(NULL);
   }
   if (instance_type != keys::kRequestMatcherType) {
     *error = kExpectedOtherConditionType;
     return scoped_ptr<WebRequestCondition>(NULL);
   }
 
   WebRequestConditionAttributes attributes;
   scoped_refptr<URLMatcherConditionSet> url_matcher_condition_set;
+  scoped_refptr<URLMatcherConditionSet> first_party_url_matcher_condition_set;
 
   for (base::DictionaryValue::Iterator iter(*condition_dict);
        iter.HasNext(); iter.Advance()) {
     const std::string& condition_attribute_name = iter.key();
     const Value& condition_attribute_value = iter.value();
+    const bool name_is_url = condition_attribute_name == keys::kUrlKey;

[battre, 2012/12/06 19:53:26]

Nit: I would prefer if you left this to have the structure
} else if (condition_attribute_name == keys::kUrlKey) {
...
} else if (condition_attribute_name == keys::kFirstPartyForCookiesUrlKey) {
...
}

I think this is easier to read. The amount of code duplication is not that big
in my opinion.

     if (condition_attribute_name == keys::kInstanceTypeKey) {
       // Skip this.
-    } else if (condition_attribute_name == keys::kUrlKey) {
+    } else if (name_is_url ||
+               condition_attribute_name == keys::kFirstPartyForCookiesUrlKey) {
       const base::DictionaryValue* dict = NULL;
       if (!condition_attribute_value.GetAsDictionary(&dict)) {
         *error = base::StringPrintf(kInvalidTypeOfParamter,
                                     condition_attribute_name.c_str());
       } else {
-        url_matcher_condition_set =
-            URLMatcherFactory::CreateFromURLFilterDictionary(
-                url_matcher_condition_factory, dict, ++g_next_id, error);
+        if (!name_is_url) {
+          first_party_url_matcher_condition_set =
+              URLMatcherFactory::CreateFromURLFilterDictionary(
+                  first_party_url_matcher_condition_factory,
+                  dict, ++g_next_id, error);
+        } else {
+          url_matcher_condition_set =
+              URLMatcherFactory::CreateFromURLFilterDictionary(
+                  url_matcher_condition_factory, dict, ++g_next_id, error);
+        }
       }
     } else if (WebRequestConditionAttribute::IsKnownType(
         condition_attribute_name)) {
       scoped_ptr<WebRequestConditionAttribute> attribute =
           WebRequestConditionAttribute::Create(
               condition_attribute_name,
               &condition_attribute_value,
               error);
       if (attribute.get())
         attributes.push_back(make_linked_ptr(attribute.release()));
     } else {
       *error = base::StringPrintf(kUnknownConditionAttribute,
                                   condition_attribute_name.c_str());
     }
     if (!error->empty())
       return scoped_ptr<WebRequestCondition>(NULL);
   }
 
   if (!url_matcher_condition_set) {
-    URLMatcherConditionSet::Conditions url_matcher_conditions;
-    url_matcher_conditions.insert(
-        url_matcher_condition_factory->CreateHostPrefixCondition(""));
-    url_matcher_condition_set =
-        new URLMatcherConditionSet(++g_next_id, url_matcher_conditions);
+    url_matcher_condition_set = CreateAlwaysTrueUrlCondition(
+        url_matcher_condition_factory);
   }
+  if (!first_party_url_matcher_condition_set) {
+    first_party_url_matcher_condition_set = CreateAlwaysTrueUrlCondition(
+        first_party_url_matcher_condition_factory);
+  }
+
   scoped_ptr<WebRequestCondition> result(
-      new WebRequestCondition(url_matcher_condition_set, attributes));
+      new WebRequestCondition(url_matcher_condition_set,
+                              first_party_url_matcher_condition_set,
+                              attributes));
 
   if (!result->stages()) {
     *error = kConditionCannotBeFulfilled;
     return scoped_ptr<WebRequestCondition>(NULL);
   }
 
   return result.Pass();
 }
 
 
 //
 // WebRequestConditionSet
 //
 
 WebRequestConditionSet::WebRequestConditionSet(
     const WebRequestConditionSet::Conditions& conditions)
     : conditions_(conditions) {
   for (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();
+    trigger_id = (*i)->first_party_url_matcher_condition_set_id();
+    match_triggers_[trigger_id] = i->get();

[battre, 2012/12/06 19:53:26]

This is a serious performance regression as we would trigger the
WebRequetConditionSet for all URLs that don't have a first_party attribute.

The is what the logic should be:

- if exactly one condition set is non-trivial: add it as a trigger
- if both condition sets are non-trivial: add the more specific one because both
need to be fulfilled (I would use the url_matcher_condition_set as a rule of
thumb)
- if both condition sets are always true: add a single one, does not matter
which one.

   }
 }
 
 WebRequestConditionSet::~WebRequestConditionSet() {}
 
 bool WebRequestConditionSet::IsFulfilled(
     URLMatcherConditionSet::ID url_match,
     const WebRequestRule::RequestData& request_data) const {
   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());
+  DCHECK(url_match == trigger->second->url_matcher_condition_set_id() ||
+         url_match ==
+             trigger->second->first_party_url_matcher_condition_set_id());
   return trigger->second->IsFulfilled(request_data);

[battre, 2012/12/06 19:53:26]

This is not sufficient in my opinion.  I think here we need to return whether
both are fulfilled.

 }
 
 void WebRequestConditionSet::GetURLMatcherConditionSets(
     URLMatcherConditionSet::Vector* condition_sets) const {
   for (Conditions::const_iterator i = conditions_.begin();
        i != conditions_.end(); ++i) {
     condition_sets->push_back((*i)->url_matcher_condition_set());
   }
 }
 
+void WebRequestConditionSet::GetFirstPartyURLMatcherConditionSets(
+    URLMatcherConditionSet::Vector* condition_sets) const {
+  for (Conditions::const_iterator i = conditions_.begin();
+       i != conditions_.end(); ++i) {
+    condition_sets->push_back((*i)->first_party_url_matcher_condition_set());
+  }
+}
+
 // static
 scoped_ptr<WebRequestConditionSet> WebRequestConditionSet::Create(
     URLMatcherConditionFactory* url_matcher_condition_factory,
+    URLMatcherConditionFactory* first_party_url_matcher_condition_factory,
     const AnyVector& conditions,
     std::string* error) {
   WebRequestConditionSet::Conditions result;
 
   for (AnyVector::const_iterator i = conditions.begin();
        i != conditions.end(); ++i) {
     CHECK(i->get());
     scoped_ptr<WebRequestCondition> condition =
         WebRequestCondition::Create(url_matcher_condition_factory,
+                                    first_party_url_matcher_condition_factory,
                                     (*i)->value(), error);
     if (!error->empty())
       return scoped_ptr<WebRequestConditionSet>(NULL);
     result.push_back(make_linked_ptr(condition.release()));
   }
 
   return scoped_ptr<WebRequestConditionSet>(new WebRequestConditionSet(result));
 }
 
 }  // namespace extensions