Files.app: split query into AND conditioned keywords in metadata search.

components/drive/search_metadata.cc

 // Copyright (c) 2013 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 "components/drive/search_metadata.h"
 
 #include <algorithm>
 #include <queue>
 
 #include "base/bind.h"
 #include "base/i18n/string_search.h"
 #include "base/metrics/histogram.h"
+#include "base/strings/string_piece.h"
+#include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "components/drive/drive_api_util.h"
 #include "components/drive/file_system_core_util.h"
 #include "net/base/escape.h"
 
 namespace drive {
 namespace internal {
 
 namespace {
@@ skipping 111 matching lines @@
 
  private:
   ResourceMetadata* metadata_;
 
   // local ID to is_hidden map.
   std::map<std::string, bool> is_hiding_child_;
 };
 
 // Used to implement SearchMetadata.
 // Adds entry to the result when appropriate.
-// In particular, if |query| is non-null, only adds files with the name matching
-// the query.
+// In particular, if size of |queries| is larger than 0, only adds files with
+// the name matching the query.
 FileError MaybeAddEntryToResult(
     ResourceMetadata* resource_metadata,
     ResourceMetadata::Iterator* it,
-    base::i18n::FixedPatternStringSearchIgnoringCaseAndAccents* query,
+    const ScopedVector<
+        base::i18n::FixedPatternStringSearchIgnoringCaseAndAccents>& queries,
     const SearchMetadataPredicate& predicate,
     size_t at_most_num_matches,
     HiddenEntryClassifier* hidden_entry_classifier,
     ScopedPriorityQueue<ResultCandidate, ResultCandidateComparator>*
         result_candidates) {
   DCHECK_GE(at_most_num_matches, result_candidates->size());
 
   const ResourceEntry& entry = it->GetValue();
 
   // If the candidate set is already full, and this |entry| is old, do nothing.
   // We perform this check first in order to avoid the costly find-and-highlight
   // or FilePath lookup as much as possible.
   if (result_candidates->size() == at_most_num_matches &&
       !CompareByTimestamp(entry, result_candidates->top()->entry))
     return FILE_ERROR_OK;
 
   // Add |entry| to the result if the entry is eligible for the given
   // |options| and matches the query. The base name of the entry must
   // contain |query| to match the query.
   std::string highlighted;
   if (!predicate.Run(entry) ||
-      (query && !FindAndHighlight(entry.base_name(), query, &highlighted)))
+      (queries.size() > 0 &&
+       !FindAndHighlight(entry.base_name(), queries, &highlighted)))
     return FILE_ERROR_OK;
 
   // Hidden entry should not be returned.
   bool hidden = false;
   FileError error = hidden_entry_classifier->IsHidden(entry, &hidden);
   if (error != FILE_ERROR_OK || hidden)
     return error;
 
   // Make space for |entry| when appropriate.
   if (result_candidates->size() == at_most_num_matches)
     result_candidates->pop();
   result_candidates->push(new ResultCandidate(it->GetID(), entry, highlighted));
   return FILE_ERROR_OK;
 }
 
 // Implements SearchMetadata().
 FileError SearchMetadataOnBlockingPool(ResourceMetadata* resource_metadata,
                                        const std::string& query_text,
                                        const SearchMetadataPredicate& predicate,
                                        int at_most_num_matches,
                                        MetadataSearchResultVector* results) {
   ScopedPriorityQueue<ResultCandidate,
                       ResultCandidateComparator> result_candidates;
 
   // Prepare data structure for searching.
-  base::i18n::FixedPatternStringSearchIgnoringCaseAndAccents query(
-      base::UTF8ToUTF16(query_text));
+  std::vector<base::string16> keywords =
+      base::SplitString(base::UTF8ToUTF16(query_text),
+                        base::StringPiece16(base::kWhitespaceUTF16),
+                        base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+
+  ScopedVector<base::i18n::FixedPatternStringSearchIgnoringCaseAndAccents>
+      queries;
+  for (const auto& keyword : keywords) {
+    queries.push_back(
+        new base::i18n::FixedPatternStringSearchIgnoringCaseAndAccents(
+            keyword));
+  }
 
   // Prepare an object to filter out hidden entries.
   ResourceEntry mydrive;
   FileError error = resource_metadata->GetResourceEntryByPath(
       util::GetDriveMyDriveRootPath(), &mydrive);
   if (error != FILE_ERROR_OK)
     return error;
   HiddenEntryClassifier hidden_entry_classifier(resource_metadata,
                                                 mydrive.local_id());
 
   // Iterate over entries.
   scoped_ptr<ResourceMetadata::Iterator> it = resource_metadata->GetIterator();
   for (; !it->IsAtEnd(); it->Advance()) {
     FileError error = MaybeAddEntryToResult(
-        resource_metadata, it.get(), query_text.empty() ? NULL : &query,
-        predicate, at_most_num_matches, &hidden_entry_classifier,
-        &result_candidates);
+        resource_metadata, it.get(), queries, predicate, at_most_num_matches,
+        &hidden_entry_classifier, &result_candidates);
     if (error != FILE_ERROR_OK)
       return error;
   }
 
   // Prepare the result.
   for (; !result_candidates.empty(); result_candidates.pop()) {
     const ResultCandidate& candidate = *result_candidates.top();
     // The path field of entries in result_candidates are empty at this point,
     // because we don't want to run the expensive metadata DB look up except for
     // the final results. Hence, here we fill the part.
@@ skipping 75 matching lines @@
     } else {
       return entry.file_specific_info().cache_state().is_present();
     }
   }
 
   return true;
 }
 
 bool FindAndHighlight(
     const std::string& text,
-    base::i18n::FixedPatternStringSearchIgnoringCaseAndAccents* query,
+    const ScopedVector<
+        base::i18n::FixedPatternStringSearchIgnoringCaseAndAccents>& queries,
     std::string* highlighted_text) {
-  DCHECK(query);
   DCHECK(highlighted_text);
   highlighted_text->clear();
 
-  base::string16 text16 = base::UTF8ToUTF16(text);
+  // Empty query does not match with anything.
+  if (text.empty())

[hashimoto, 2015/09/07 06:17:09]

The comment says "empty query", but here the emptiness of |text| is checked.
Why do we need to perform this check?

[yawano, 2015/09/08 10:27:21]

We should have checked empty query. Fixed and added test case for this.

If we don't check empty query here, this method will return true.

+    return false;
+
+  // Check text matches with all queries.
   size_t match_start = 0;
   size_t match_length = 0;
-  if (!query->Search(text16, &match_start, &match_length))
-    return false;
 
-  base::string16 pre = text16.substr(0, match_start);
-  base::string16 match = text16.substr(match_start, match_length);
-  base::string16 post = text16.substr(match_start + match_length);
-  highlighted_text->append(net::EscapeForHTML(base::UTF16ToUTF8(pre)));
-  highlighted_text->append("<b>");
-  highlighted_text->append(net::EscapeForHTML(base::UTF16ToUTF8(match)));
-  highlighted_text->append("</b>");
-  highlighted_text->append(net::EscapeForHTML(base::UTF16ToUTF8(post)));
+  base::string16 text16 = base::UTF8ToUTF16(text);
+  std::vector<bool> highlights(text16.size(), false);
+  for (auto* query : queries) {
+    if (!query->Search(text16, &match_start, &match_length))
+      return false;
+
+    size_t match_end = match_start + match_length;
+    for (size_t i = match_start; i < match_end; ++i) {
+      highlights[i] = true;
+    }

[hashimoto, 2015/09/07 06:17:09]

nit: You can do std::fill(highlights.begin() + match_start, highlights.begin() +
match_end, true);
(http://www.cplusplus.com/reference/algorithm/fill/)

[yawano, 2015/09/08 10:27:21]

Done.

+  }
+
+  // Generate highlighted text.
+  bool highlighting = false;
+  size_t start = 0;

[hashimoto, 2015/09/07 06:17:09]

nit: |start| sounds a bit ambiguous.
How about a more specific name like "start_current_segment"?
("segment" can be "section", "part", "fragment" or whatever word which makes
sense)

[yawano, 2015/09/08 10:27:21]

Done.

+
+  for (size_t i = 0; i < text16.size(); ++i) {
+    if (highlighting == highlights[i])
+      continue;
+
+    // Append [start, i).
+    if (i > start) {

[hashimoto, 2015/09/07 06:17:09]

Do we need this check?
IIUC this is only needed if highlights[0]==true.
How about removing |highlighting| and using highlights[start] instead?

[yawano, 2015/09/08 10:27:21]

Done.

+      highlighted_text->append(net::EscapeForHTML(
+          base::UTF16ToUTF8(text16.substr(start, i - start))));
+    }
+
+    highlighted_text->append(highlighting ? "</b>" : "<b>");

[hashimoto, 2015/09/07 06:17:09]

I found this line a bit difficult to understand.
How about adding <b> and </b> at the same time as the highlighted string?

Like:
  if (highlighting)
    highlighted_text->append("<b>");
  highlighted_text->append(net::EscapeForHTML(...));
  if (highlighting)
    highlighted_text->append("</b>");

[yawano, 2015/09/08 10:27:21]

Done.

+
+    start = i;
+    highlighting = highlights[i];
+  }
+
+  DCHECK_GE(text16.size(), start);
+  highlighted_text->append(net::EscapeForHTML(
+      base::UTF16ToUTF8(text16.substr(start, text16.size() - start))));
+
+  if (highlighting)
+    highlighted_text->append("</b>");
+
   return true;
 }
 
 }  // namespace internal
 }  // namespace drive