Add support for matching query parameters in URLMatcher

components/url_matcher/url_matcher.cc

Index: components/url_matcher/url_matcher.cc
diff --git a/components/url_matcher/url_matcher.cc b/components/url_matcher/url_matcher.cc
index de8632d730e4aa408515118b2b026f7ef0c79b3c..8eaab343a83cdb87cea6bafe3d8739369b3e0d92 100644
--- a/components/url_matcher/url_matcher.cc
+++ b/components/url_matcher/url_matcher.cc
@@ -196,7 +196,6 @@ bool URLMatcherCondition::IsFullURLCondition() const {
   switch (criterion_) {
     case HOST_CONTAINS:
     case PATH_CONTAINS:
-    case QUERY_CONTAINS:
     case URL_PREFIX:
     case URL_SUFFIX:
     case URL_CONTAINS:
@@ -222,9 +221,9 @@ bool URLMatcherCondition::IsMatch(
   DCHECK(string_pattern_);
   if (!ContainsKey(matching_patterns, string_pattern_->id()))
     return false;
-  // The criteria HOST_CONTAINS, PATH_CONTAINS, QUERY_CONTAINS are based on
-  // a substring match on the raw URL. In case of a match, we need to verify
-  // that the match was found in the correct component of the URL.
+  // The criteria HOST_CONTAINS, PATH_CONTAINS are based on a substring match on
+  // the raw URL. In case of a match, we need to verify that the match was found
+  // in the correct component of the URL.
   switch (criterion_) {
     case HOST_CONTAINS:
       return url.host().find(string_pattern_->pattern()) !=
@@ -232,9 +231,6 @@ bool URLMatcherCondition::IsMatch(
     case PATH_CONTAINS:
       return url.path().find(string_pattern_->pattern()) !=
           std::string::npos;
-    case QUERY_CONTAINS:
-      return url.query().find(string_pattern_->pattern()) !=
-          std::string::npos;
     default:
       break;
   }
@@ -250,7 +246,17 @@ namespace {
 const char kBeginningOfURL[] = {static_cast<char>(-1), 0};
 const char kEndOfDomain[] = {static_cast<char>(-2), 0};
 const char kEndOfPath[] = {static_cast<char>(-3), 0};
-const char kEndOfURL[] = {static_cast<char>(-4), 0};
+const char kBeginningOfQueryComponent[] = {static_cast<char>(-4), 0};
+const char kEndOfURL[] = {static_cast<char>(-5), 0};
+const char kQuerySeperator = '&';
+
+std::string PrepareQuery(std::string query, bool prepend_query_start = true) {

[battre, 2014/04/01 08:23:00]

Please add a description of the function.

Please add a comment and describe that you intentionally pass |query| via the
stack instead of a const-ref.

I think that prepend_query_start is not an adequate description of what the
parameter does. How about prepend_beginning_of_query_component?

[Joao da Silva, 2014/04/01 09:18:47]

Default parameters are not allowed by the style guide:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments

[battre, 2014/04/01 09:29:40]

I thought so as well and then removed the comment. They are allowed for
functions in the anonymous namespace.

[kaliamoorthi, 2014/04/01 13:11:17]

Done.

[kaliamoorthi, 2014/04/01 13:11:17]

I have removed the default parameter anyway.

+  for (std::string::iterator it = query.begin(); it != query.end(); ++it) {
+    if (*it == kQuerySeperator)
+      *it = kBeginningOfQueryComponent[0];
+  }
+  return prepend_query_start ? kBeginningOfQueryComponent + query : query;
+}
 }  // namespace
 
 URLMatcherConditionFactory::URLMatcherConditionFactory() : id_counter_(0) {}
@@ -265,7 +271,8 @@ std::string URLMatcherConditionFactory::CanonicalizeURLForComponentSearches(
     const GURL& url) const {
   return kBeginningOfURL + CanonicalizeHostname(url.host()) + kEndOfDomain +
          url.path() + kEndOfPath +
-         (url.has_query() ? "?" + url.query() : std::string()) + kEndOfURL;
+         (url.has_query() ? PrepareQuery(url.query()) : std::string()) +
+         kEndOfURL;
 }
 
 URLMatcherCondition URLMatcherConditionFactory::CreateHostPrefixCondition(
@@ -316,10 +323,10 @@ URLMatcherCondition URLMatcherConditionFactory::CreatePathEqualsCondition(
 URLMatcherCondition URLMatcherConditionFactory::CreateQueryPrefixCondition(
     const std::string& prefix) {
   std::string pattern;
-  if (!prefix.empty() && prefix[0] == '?')
-    pattern = kEndOfPath + prefix;
-  else
-    pattern = kEndOfPath + ('?' + prefix);
+  pattern =

[battre, 2014/04/01 08:23:00]

Either the assignment should become part of the constructor, or we we may
re-introduce the if statement. I think the latter may help for readability.

[kaliamoorthi, 2014/04/01 13:11:17]

I have reintroduced the if else.

+      kEndOfPath + PrepareQuery((!prefix.empty() && prefix[0] == '?')
+                                    ? prefix.substr(1, prefix.length() - 1)

[battre, 2014/04/01 08:23:00]

nit: prefix.substr(1) should be sufficient, right?

[kaliamoorthi, 2014/04/01 13:11:17]

That simplifies the expression. Fixed it.

+                                    : prefix);
 
   return CreateCondition(URLMatcherCondition::QUERY_PREFIX, pattern);
 }
@@ -330,25 +337,26 @@ URLMatcherCondition URLMatcherConditionFactory::CreateQuerySuffixCondition(
     return CreateQueryEqualsCondition(suffix);
   } else {
     return CreateCondition(URLMatcherCondition::QUERY_SUFFIX,
-                           suffix + kEndOfURL);
+                           PrepareQuery(suffix, false) + kEndOfURL);
   }
 }
 
 URLMatcherCondition URLMatcherConditionFactory::CreateQueryContainsCondition(
     const std::string& str) {
-  if (!str.empty() && str[0] == '?')
-    return CreateQueryPrefixCondition(str);
-  else
-    return CreateCondition(URLMatcherCondition::QUERY_CONTAINS, str);
+  std::string query_condition;
+  query_condition =
+      (!str.empty() && str[0] == '?') ? str.substr(1, str.length() - 1) : str;

[battre, 2014/04/01 08:23:00]

Can you re-introduce the if statement (also below)? I think that is easier to
read.

[kaliamoorthi, 2014/04/01 13:11:17]

Done.

+  return CreateCondition(URLMatcherCondition::QUERY_CONTAINS,
+                         PrepareQuery(query_condition));
 }
 
 URLMatcherCondition URLMatcherConditionFactory::CreateQueryEqualsCondition(
     const std::string& str) {
   std::string pattern;
-  if (!str.empty() && str[0] == '?')
-    pattern = kEndOfPath + str + kEndOfURL;
-  else
-    pattern = kEndOfPath + ('?' + str) + kEndOfURL;
+  pattern = kEndOfPath + PrepareQuery((!str.empty() && str[0] == '?')
+                                          ? str.substr(1, str.length() - 1)
+                                          : str) +
+            kEndOfURL;
 
   return CreateCondition(URLMatcherCondition::QUERY_EQUALS, pattern);
 }