Refactoring how conditions without URL attributes are handled.

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 31 matching lines @@
 //
 // WebRequestCondition
 //
 
 WebRequestCondition::WebRequestCondition(
     scoped_refptr<URLMatcherConditionSet> url_matcher_conditions,
     const WebRequestConditionAttributes& condition_attributes)
     : url_matcher_conditions_(url_matcher_conditions),
       condition_attributes_(condition_attributes),
       applicable_request_stages_(~0) {
-  CHECK(url_matcher_conditions.get());

[vabr (Chromium), 2012/12/13 12:18:06]

|url_matcher_conditions| being NULL is now legal and means that there were no
UrlFilter attributes in this condition.

[battre, 2012/12/18 08:20:44]

Can you please document this?

[vabr (Chromium), 2012/12/18 18:26:42]

Done, at the declaration of |url_matcher_conditions_|.

   for (WebRequestConditionAttributes::const_iterator i =
        condition_attributes_.begin(); i != condition_attributes_.end(); ++i) {
     applicable_request_stages_ &= (*i)->GetStages();
   }
 }
 
 WebRequestCondition::~WebRequestCondition() {}
 
 bool WebRequestCondition::IsFulfilled(

[Jeffrey Yasskin, 2012/12/18 02:24:32]

It would be nice to somehow DCHECK here that the URLMatcher was actually applied
and matches the right URLs.

[vabr (Chromium), 2012/12/18 18:26:42]

I'm not sure that I understand. Are you suggesting to move the check
  DCHECK_EQ(url_match, trigger->second->url_matcher_condition_set_id());
from WebRequestConditionSet::IsFulfilled to here?

EDIT: After the refactoring, there is no more that DCHECK in
WebRequestConditionSet::IsFulfilled. Still, do you want me to pass the iterator
to URL matches here to re-check?

     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;
 }
 
+bool WebRequestCondition::IsFulfilledIndependentlyOfURL(
+    const WebRequestRule::RequestData& request_data) const {
+  if (url_matcher_conditions_.get())

[Jeffrey Yasskin, 2012/12/18 02:24:32]

You don't need the .get() for use inside an if() anymore.

[vabr (Chromium), 2012/12/18 18:26:42]

That's true. Unfortunately there are 4 ways to write this
(https://groups.google.com/a/chromium.org/d/msg/chromium-dev/ASSnFysgpsg/hr6-w...),
and the ".get()" looks like locally consistent.

+    return false;
+  return IsFulfilled(request_data);
+}
+
 // static
 scoped_ptr<WebRequestCondition> WebRequestCondition::Create(
     URLMatcherConditionFactory* 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);
   }
@@ skipping 38 matching lines @@
       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);
-  }
   scoped_ptr<WebRequestCondition> result(
       new WebRequestCondition(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();
+    : conditions_(conditions),
+      has_untriggered_conditions_(
+          IsSomeConditionWithoutUrlMatcher(conditions)) {
+  for (Conditions::const_iterator i = conditions_.begin();
+       i != conditions_.end();

[battre, 2012/12/18 08:20:44]

nit: no new line for consistency.

[vabr (Chromium), 2012/12/18 18:26:42]

Done.

        ++i) {
+    if (!(*i)->url_matcher_condition_set().get())
+      continue;

[battre, 2012/12/18 08:20:44]

nit: I'd propose to invert the logic:
if ((*i)->url_matcher_condition_set()) {
  ...
}

[vabr (Chromium), 2012/12/18 18:26:42]

Done.

     URLMatcherConditionSet::ID trigger_id =
         (*i)->url_matcher_condition_set_id();
     match_triggers_[trigger_id] = i->get();
   }
 }
 
 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());
   return trigger->second->IsFulfilled(request_data);
 }
 
+bool WebRequestConditionSet::IsFulfilledWithoutURLMatcher(
+    const WebRequestRule::RequestData& request_data) const {
+  for (Conditions::const_iterator i = conditions_.begin();

[Jeffrey Yasskin, 2012/12/18 02:24:32]

Do you want to check all conditions here, or just the ones that don't have URL
attributes?

[vabr (Chromium), 2012/12/18 18:26:42]

Just the ones without URL attributes.
The for-cycle goes over all conditions, but IsFulfilledIndependentlyOfURL
evaluates to false quickly for each condition with a URL attribute.

EDIT: After the refactoring, this function is gone.

+       i != conditions_.end(); ++i) {
+    if ((*i)->IsFulfilledIndependentlyOfURL(request_data))
+      return true;
+  }
+  return false;
+}
+
 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());
+    scoped_refptr<URLMatcherConditionSet> set =
+        (*i)->url_matcher_condition_set();
+    if (set.get())
+      condition_sets->push_back(set);
   }
 }
 
 // static
+bool WebRequestConditionSet::IsSomeConditionWithoutUrlMatcher(

[Jeffrey Yasskin, 2012/12/18 02:24:32]

s/IsSome/Has/, I think.

[Jeffrey Yasskin, 2012/12/18 02:24:32]

Since this doesn't use any class-private data, I'd just make it a local function
in this .cc file inside an anonymous namespace.

[vabr (Chromium), 2012/12/18 18:26:42]

Done.

[vabr (Chromium), 2012/12/18 18:26:42]

Done.

+    const WebRequestConditionSet::Conditions& conditions) {
+  for (Conditions::const_iterator i = conditions.begin();
+       i != conditions.end(); ++i) {
+    if (!(*i)->url_matcher_condition_set().get())
+      return true;
+  }
+  return false;
+}
+
+// static
 scoped_ptr<WebRequestConditionSet> WebRequestConditionSet::Create(
     URLMatcherConditionFactory* 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,
                                     (*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