Don't strip punctuation inside quotes in history search queries.

chrome/browser/history/query_parser.cc

 // Copyright (c) 2011 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/history/query_parser.h"
 
 #include <algorithm>
 
 #include "base/compiler_specific.h"
 #include "base/i18n/break_iterator.h"
@@ skipping 62 matching lines @@
 
 // A QueryNodeWord is a single word in the query.
 class QueryNodeWord : public QueryNode {
  public:
   explicit QueryNodeWord(const string16& word);
   virtual ~QueryNodeWord();
 
   const string16& word() const { return word_; }
 
   void set_literal(bool literal) { literal_ = literal; }
+  void AppendText(const string16& text) { word_ += text; }

[mrossetti, 2012/02/07 01:05:57]

We generally only inline simple getters and setters. This should not be inlined.

 
   // QueryNode:
   virtual int AppendToSQLiteQuery(string16* query) const OVERRIDE;
   virtual bool IsWord() const OVERRIDE;
   virtual bool Matches(const string16& word, bool exact) const OVERRIDE;
   virtual bool HasMatchIn(
       const std::vector<QueryWord>& words,
       Snippet::MatchPositions* match_positions) const OVERRIDE;
   virtual void AppendWords(std::vector<string16>* words) const OVERRIDE;
 
@@ skipping 265 matching lines @@
   // TODO(evanm): support a locale here
   if (!iter.Init())
     return false;
 
   // To handle nesting, we maintain a stack of QueryNodeLists.
   // The last element (back) of the stack contains the current, deepest node.
   std::vector<QueryNodeList*> query_stack;
   query_stack.push_back(root);
 
   bool in_quotes = false;  // whether we're currently in a quoted phrase
+  QueryNodeWord* current_word = NULL;
   while (iter.Advance()) {
     // Just found a span between 'prev' (inclusive) and 'pos' (exclusive). It
     // is not necessarily a word, but could also be a sequence of punctuation
     // or whitespace.
-    if (iter.IsWord()) {
-      QueryNodeWord* word_node = new QueryNodeWord(iter.GetString());
+    if (IsQueryQuote(query[iter.prev()])) {
+      if (!in_quotes) {
+        QueryNodeList* quotes_node = new QueryNodePhrase;
+        query_stack.back()->AddChild(quotes_node);
+        query_stack.push_back(quotes_node);
+        in_quotes = true;
+      } else {
+        query_stack.pop_back();  // Stop adding to the quoted phrase.
+        in_quotes = false;
+      }
+      current_word = NULL;
+    } else if (iter.IsWord() || in_quotes) {

[mrossetti, 2012/02/07 01:05:57]

Okay, as you can see from the unit tests, trying to handle both an open quote
run AND just a regular word here (at line 385) causes the parser results for
string containing special characters to be different than with the current code.

I suggest only handling the |in_quotes| case on this branch, and handling the
unquoted words down below in those empty braces starting on line 394.

+      if (current_word) {
+        current_word->AppendText(iter.GetString());
+      } else {
+        current_word = new QueryNodeWord(iter.GetString());
+        query_stack.back()->AddChild(current_word);

[mrossetti, 2012/02/07 01:05:57]

Do the set_literal here...

+      }
       if (in_quotes)

[mrossetti, 2012/02/07 01:05:57]

...not here.

-        word_node->set_literal(true);
-      query_stack.back()->AddChild(word_node);
-    } else {  // Punctuation.
-      if (IsQueryQuote(query[iter.prev()])) {
-        if (!in_quotes) {
-          QueryNodeList* quotes_node = new QueryNodePhrase;
-          query_stack.back()->AddChild(quotes_node);
-          query_stack.push_back(quotes_node);
-          in_quotes = true;
-        } else {
-          query_stack.pop_back();  // Stop adding to the quoted phrase.
-          in_quotes = false;
-        }
-      }
+        current_word->set_literal(true);
+    } else {  // Punctuation or whitespace outside quotes.

[mrossetti, 2012/02/07 01:05:57]

These empty braces are just screaming for something! ;^) I believe if you change
this to:

    } else if (iter.IsWord()) {
      QueryNodeWord* word_node = new QueryNodeWord(iter.GetString());
      query_stack.back()->AddChild(word_node);
    }

it will restore the old, unquoted functionality and please the unit tests.

     }
   }
 
   root->RemoveEmptySubnodes();
   return true;
 }
 
 void QueryParser::ExtractQueryWords(const string16& text,
                                     std::vector<QueryWord>* words) {
   base::i18n::BreakIterator iter(text, base::i18n::BreakIterator::BREAK_WORD);
   // TODO(evanm): support a locale here
   if (!iter.Init())
     return;
 
   while (iter.Advance()) {
     // Just found a span between 'prev' (inclusive) and 'pos' (exclusive). It
     // is not necessarily a word, but could also be a sequence of punctuation
     // or whitespace.
     if (iter.IsWord()) {
       string16 word = iter.GetString();
       if (!word.empty()) {
         words->push_back(QueryWord());
         words->back().word = word;
         words->back().position = iter.prev();
       }
     }
   }
 }