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"
 #include "base/i18n/case_conversion.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
+#include "base/string_util.h"
 
 namespace {
 
 // Returns true if |mp1.first| is less than |mp2.first|. This is used to
 // sort match positions.
 int CompareMatchPosition(const Snippet::MatchPosition& mp1,
                          const Snippet::MatchPosition& mp2) {
   return mp1.first < mp2.first;
 }
 
@@ skipping 15 matching lines @@
       mp.second = std::max(mp.second, i->second);
       i = matches->erase(i);
     } else {
       return;
     }
   }
 }
 
 // Sorts the match positions in |matches| by their first index, then coalesces
 // any match positions that intersect each other.
-void CoalseAndSortMatchPositions(Snippet::MatchPositions* matches) {
+void CoalesceAndSortMatchPositions(Snippet::MatchPositions* matches) {
   std::sort(matches->begin(), matches->end(), &CompareMatchPosition);
   // WARNING: we don't use iterator here as CoalesceMatchesFrom may remove
   // from matches.
   for (size_t i = 0; i < matches->size(); ++i)
     CoalesceMatchesFrom(i, matches);
 }
 
 // Returns true if the character is considered a quote.
 bool IsQueryQuote(wchar_t ch) {
   return ch == '"' ||
          ch == 0xab ||    // left pointing double angle bracket
          ch == 0xbb ||    // right pointing double angle bracket
          ch == 0x201c ||  // left double quotation mark
          ch == 0x201d ||  // right double quotation mark
          ch == 0x201e;    // double low-9 quotation mark
 }
 
+// Returns true if the character is considered non-breaking when it appears in
+// the middle of a word. This can be used to prevent an URL-like query from
+// being broken into multiple words.
+bool IsNonBreakingSymbol(wchar_t ch) {
+  return ch == '.' || ch == '-';
+}
+
 }  // namespace
 
 // Inheritance structure:
 // Queries are represented as trees of QueryNodes.
 // QueryNodes are either a collection of subnodes (a QueryNodeList)
 // or a single word (a QueryNodeWord).
 
-// A QueryNodeWord is a single word in the query.
+// A QueryNodeWord is a sequence of consecutive characters in a query.
+// It can be an actual word or an URL-like sequence of characters.
 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 Append(const string16& word);

[Scott Hess - ex-Googler, 2013/04/17 18:14:02]

I think this sense of "Append" is enough different from the sense of "Append"
inherited from QueryNode that maybe it would be worth changing to something
else.  Accumulate?  And I prefer the |str| usage over |word|, or some other term
implying bag-o-chars.

 
   // 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;
 
  private:
   string16 word_;
   bool literal_;
 
   DISALLOW_COPY_AND_ASSIGN(QueryNodeWord);
 };
 
 QueryNodeWord::QueryNodeWord(const string16& word)
     : word_(word),
       literal_(false) {}
 
 QueryNodeWord::~QueryNodeWord() {}
 
+void QueryNodeWord::Append(const string16& str) {
+  word_ += str;
+}
+
 int QueryNodeWord::AppendToSQLiteQuery(string16* query) const {
   query->append(word_);
 
   // Use prefix search if we're not literal and long enough.
   if (!literal_ && QueryParser::IsWordLongEnoughForPrefixSearch(word_))
     *query += L'*';
   return 1;
 }
 
 bool QueryNodeWord::IsWord() const {
@@ skipping 225 matching lines @@
     if (!query_nodes[i]->HasMatchIn(query_words, &matches))
       return false;
   }
   if (lower_text.length() != text.length()) {
     // The lower case string differs from the original string. The matches are
     // meaningless.
     // TODO(sky): we need a better way to align the positions so that we don't
     // completely punt here.
     match_positions->clear();
   } else {
-    CoalseAndSortMatchPositions(&matches);
+    CoalesceAndSortMatchPositions(&matches);
     match_positions->swap(matches);
   }
   return true;
 }
 
 bool QueryParser::ParseQueryImpl(const string16& query, QueryNodeList* root) {
   base::i18n::BreakIterator iter(query, base::i18n::BreakIterator::BREAK_WORD);
   // 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 (in_quotes)
-        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;
-        }
+    // is not necessarily a word, it could also be a punctuation or whitespace
+    // character. Punctuation is preserved inside quotes, and otherwise removed
+    // except if it is a non-breaking character in the middle of a word.
+
+    wchar_t first_char = query[iter.prev()];
+    if (IsQueryQuote(first_char)) {
+      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 && !IsWhitespace(first_char))) {

[Scott Hess - ex-Googler, 2013/04/17 18:14:02]

Previously, if there were a sequence of IsWord() tokens without intervening
!IsWord() tokens, it would become a sequence of QueryNodeWord children.  Now
they will be appended together into a single QueryNodeWord.

+      // Append to the current word if the new token is a word or a non-
+      // whitespace token inside quotes.
+      if (current_word) {
+        current_word->Append(iter.GetString());
+      } else {
+        current_word = new QueryNodeWord(iter.GetString());
+        current_word->set_literal(in_quotes);
+        query_stack.back()->AddChild(current_word);
+      }
+    } else if (current_word != NULL) {
+      // Allow non-breaking symbols inside a word.
+      // Any other punctuation or whitespace character ends the current word.
+
+      // TODO(dubroy): Consider sharing code with the omnibox to allow a more
+      // accurate "best guess" of whether a sequence of characters is a URL.
+      if (IsNonBreakingSymbol(first_char))
+        current_word->Append(iter.GetString());

[Scott Hess - ex-Googler, 2013/04/17 18:14:02]

I believe this won't back out the symbol(s) if the next token is a non-word. 
Intentional to allow training non-breaking symbols?  If so, the comment should
be modified.

I'm trying to think of how this could just accumulate the word using prev() and
pos(), then capture it once, but AFAICT it would require too many points to
commit the word.  You could accumulate the non-breaking symbols in a string16,
and change the above to accumulate them on the end of the previous word before
the new word, then clear the symbol accumulator when opening a new current_word.

I think that could also handle the adjacent IsWord() problem mentioned above. 
Append if there is a current_word _and_ the non-breaking accumulator has
content, otherwise push a new QueryNodeWord.

+      else
+        current_word = NULL;
     }
   }
 
   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();
       }
     }
   }
 }