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

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

BUG=514853
TEST=unit_tests:SearchMetadataTest.*;SearchMetadataSimpleTest.*

Committed: https://crrev.com/c6c0abef8ce6a72ddddca7889ef5f14149fff230
Cr-Commit-Position: refs/heads/master@{#347862}

[yawano, 2015-09-01 12:18:06]

yawano@chromium.org changed reviewers:
+ hashimoto@chromium.org

[yawano, 2015-09-01 12:18:06]

PTAL. Thank you!

[hashimoto, 2015-09-02 03:24:11]

qq: How much does this change affect the performance for users with a lot of
files stored in Drive?
You can measure the elapsed time with the "Event Log" section of
chrome://drive-internal.

https://codereview.chromium.org/1321553003/diff/1/components/drive/search_met...
File components/drive/search_metadata.cc (right):

https://codereview.chromium.org/1321553003/diff/1/components/drive/search_met...
components/drive/search_metadata.cc:202:
base::SplitString(base::UTF8ToUTF16(query_text), base::string16(1, ' '),
IIUC this code doesn't support whitespace characters other than ASCII space,
like IDEOGRAPHIC SPACE.
(Chromium has a list of whitespace characters
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...)

Is this intended behavior?
Do we have a (future) plan to support other whitespace characters?

https://codereview.chromium.org/1321553003/diff/1/components/drive/search_met...
components/drive/search_metadata.cc:338: int match_end =
static_cast<int>(match_start + match_length);
You don't have to static_cast if you use size_t instead of int.

https://codereview.chromium.org/1321553003/diff/1/components/drive/search_met...
components/drive/search_metadata.cc:339: for (int i =
static_cast<int>(match_start); i < match_end; i++) {
++i instead of i++

https://codereview.chromium.org/1321553003/diff/1/components/drive/search_met...
components/drive/search_metadata.cc:346: for (int i = 0; i <
static_cast<int>(text16.size()); i++) {
Use size_t, ++i

https://codereview.chromium.org/1321553003/diff/1/components/drive/search_met...
components/drive/search_metadata.cc:354:
net::EscapeForHTML(base::UTF16ToUTF8(text16.substr(i, 1))));
Does this code work with a string which contains surrogate pairs?
Please add a test for the case if we haven't had one yet.

https://codereview.chromium.org/1321553003/diff/1/components/drive/search_met...
File components/drive/search_metadata.h (right):

https://codereview.chromium.org/1321553003/diff/1/components/drive/search_met...
components/drive/search_metadata.h:10: #include
"components/drive/file_system_interface.h"
#include "base/memory/scoped_vector.h" should be here.
("You should include all the headers that define the symbols you rely upon"
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Or...)

[yawano, 2015-09-04 09:33:55]

PTAL again. Thank you!

https://codereview.chromium.org/1321553003/diff/1/components/drive/search_met...
File components/drive/search_metadata.cc (right):

https://codereview.chromium.org/1321553003/diff/1/components/drive/search_met...
components/drive/search_metadata.cc:202:
base::SplitString(base::UTF8ToUTF16(query_text), base::string16(1, ' '),
No. This should be able to handle other types of whitespace characters. I fixed
it. Thank you!

https://codereview.chromium.org/1321553003/diff/1/components/drive/search_met...
components/drive/search_metadata.cc:338: int match_end =
static_cast<int>(match_start + match_length);
On 2015/09/02 03:24:11, hashimoto wrote:
> You don't have to static_cast if you use size_t instead of int.

Done.

https://codereview.chromium.org/1321553003/diff/1/components/drive/search_met...
components/drive/search_metadata.cc:339: for (int i =
static_cast<int>(match_start); i < match_end; i++) {
On 2015/09/02 03:24:11, hashimoto wrote:
> ++i instead of i++

Done.

https://codereview.chromium.org/1321553003/diff/1/components/drive/search_met...
components/drive/search_metadata.cc:346: for (int i = 0; i <
static_cast<int>(text16.size()); i++) {
On 2015/09/02 03:24:11, hashimoto wrote:
> Use size_t, ++i

Done.

https://codereview.chromium.org/1321553003/diff/1/components/drive/search_met...
components/drive/search_metadata.cc:354:
net::EscapeForHTML(base::UTF16ToUTF8(text16.substr(i, 1))));
No, this hasn't worked. Fixed and added unit test.

https://codereview.chromium.org/1321553003/diff/1/components/drive/search_met...
File components/drive/search_metadata.h (right):

https://codereview.chromium.org/1321553003/diff/1/components/drive/search_met...
components/drive/search_metadata.h:10: #include
"components/drive/file_system_interface.h"
On 2015/09/02 03:24:11, hashimoto wrote:
> #include "base/memory/scoped_vector.h" should be here.
> ("You should include all the headers that define the symbols you rely upon"
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Or...)

Done.

[yawano, 2015-09-04 10:13:07]

For performance, I did a try and checked the result in chrome://drive-internal.
I haven't counted correct number of files on this test set, I think it will be
around 100s. Query was "t e s jpg" for patched version and "test.jpg" for
not-patched version.

As far as I see the log, I couldn't find a big performance difference.

The response time seems to become long when the query is short. This is
reasonable given that many results will hit with short query.

The range of response times are roughly:
Patched version: ~700ms - ~20ms
Not-patched version: ~700ms - ~10ms

While I can see more lower response times (around ~10ms) with not-patched
version, the range is almost similar with two versions. Sometime, patched
version is faster and sometime it's not.

Given that the response time becomes short when query becomes large, I don't
think matching query part is not a dominant part of response time.

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

lgtm with nits

https://codereview.chromium.org/1321553003/diff/40001/components/drive/search...
File components/drive/search_metadata.cc (right):

https://codereview.chromium.org/1321553003/diff/40001/components/drive/search...
components/drive/search_metadata.cc:332: if (text.empty())
The comment says "empty query", but here the emptiness of |text| is checked.
Why do we need to perform this check?

https://codereview.chromium.org/1321553003/diff/40001/components/drive/search...
components/drive/search_metadata.cc:348: }
nit: You can do std::fill(highlights.begin() + match_start, highlights.begin() +
match_end, true);
(http://www.cplusplus.com/reference/algorithm/fill/)

https://codereview.chromium.org/1321553003/diff/40001/components/drive/search...
components/drive/search_metadata.cc:353: size_t start = 0;
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)

https://codereview.chromium.org/1321553003/diff/40001/components/drive/search...
components/drive/search_metadata.cc:360: if (i > start) {
Do we need this check?
IIUC this is only needed if highlights[0]==true.
How about removing |highlighting| and using highlights[start] instead?

https://codereview.chromium.org/1321553003/diff/40001/components/drive/search...
components/drive/search_metadata.cc:365: highlighted_text->append(highlighting ?
"</b>" : "<b>");
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>");

https://codereview.chromium.org/1321553003/diff/40001/components/drive/search...
File components/drive/search_metadata_unittest.cc (right):

https://codereview.chromium.org/1321553003/diff/40001/components/drive/search...
components/drive/search_metadata_unittest.cc:437: "Directory\xE3\x80\x80"
nit: Why is a newline here?
It's a bit confusing as it looks like "Directory\xE3\x80\x80" and "1" are
different arguments.

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

Since I've added significant amount of change, PTAL again. Thank you!

https://codereview.chromium.org/1321553003/diff/40001/components/drive/search...
File components/drive/search_metadata.cc (right):

https://codereview.chromium.org/1321553003/diff/40001/components/drive/search...
components/drive/search_metadata.cc:332: if (text.empty())
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.

https://codereview.chromium.org/1321553003/diff/40001/components/drive/search...
components/drive/search_metadata.cc:348: }
On 2015/09/07 06:17:09, hashimoto wrote:
> nit: You can do std::fill(highlights.begin() + match_start, highlights.begin()
+
> match_end, true);
> (http://www.cplusplus.com/reference/algorithm/fill/)

Done.

https://codereview.chromium.org/1321553003/diff/40001/components/drive/search...
components/drive/search_metadata.cc:353: size_t start = 0;
On 2015/09/07 06:17:09, hashimoto wrote:
> 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)

Done.

https://codereview.chromium.org/1321553003/diff/40001/components/drive/search...
components/drive/search_metadata.cc:360: if (i > start) {
On 2015/09/07 06:17:09, hashimoto wrote:
> Do we need this check?
> IIUC this is only needed if highlights[0]==true.
> How about removing |highlighting| and using highlights[start] instead?

Done.

https://codereview.chromium.org/1321553003/diff/40001/components/drive/search...
components/drive/search_metadata.cc:365: highlighted_text->append(highlighting ?
"</b>" : "<b>");
On 2015/09/07 06:17:09, hashimoto wrote:
> 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>");

Done.

https://codereview.chromium.org/1321553003/diff/40001/components/drive/search...
File components/drive/search_metadata_unittest.cc (right):

https://codereview.chromium.org/1321553003/diff/40001/components/drive/search...
components/drive/search_metadata_unittest.cc:437: "Directory\xE3\x80\x80"
"git cl format" has formatted in this way.

[hashimoto, 2015-09-08 10:39:05]

lgtm

https://codereview.chromium.org/1321553003/diff/80001/components/drive/search...
File components/drive/search_metadata.cc (right):

https://codereview.chromium.org/1321553003/diff/80001/components/drive/search...
components/drive/search_metadata.cc:175: (queries.size() > 0 &&
nit: Since queries' emptiness is checked in FindAndHighlight, you don't need to
double-check it here.

https://codereview.chromium.org/1321553003/diff/80001/components/drive/search...
components/drive/search_metadata.cc:348: if (queries.size() == 0)
nit: empty() should be more common in our codebase than size()==0.

[yawano, 2015-09-08 11:19:09]

Changed FindAndHighlight so that empty query matches with anything. PTAL. Sorry
for asking review many times. Thank you!

https://codereview.chromium.org/1321553003/diff/80001/components/drive/search...
File components/drive/search_metadata.cc (right):

https://codereview.chromium.org/1321553003/diff/80001/components/drive/search...
components/drive/search_metadata.cc:175: (queries.size() > 0 &&
Simply removing queries.size() > 0 has not worked.

For empty queries, these values become

queries.size() > 0 : false
FindAndHighlight: false

and conditions would be

queries.size() > 0 && !FindAndHighlight: false
!FindAndHighlight: true

To make this simple, I've changed FindAndHighlight so that empty query matches
with anything.

https://codereview.chromium.org/1321553003/diff/80001/components/drive/search...
components/drive/search_metadata.cc:348: if (queries.size() == 0)
On 2015/09/08 10:39:05, hashimoto wrote:
> nit: empty() should be more common in our codebase than size()==0.

Acknowledged.

[hashimoto, 2015-09-09 02:26:19]

lgtm

[yawano, 2015-09-09 02:30:15]

On 2015/09/09 02:26:19, hashimoto wrote:
> lgtm

Thank you!

[yawano, 2015-09-09 02:30:20]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2015-09-09 02:31:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1321553003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1321553003/100001

[commit-bot: I haz the power, 2015-09-09 09:01:05]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-09 09:02:28]

Patchset 6 (id:??) landed as
https://crrev.com/c6c0abef8ce6a72ddddca7889ef5f14149fff230
Cr-Commit-Position: refs/heads/master@{#347862}