Implement query translation from GData WAPI to Drive API v2.

Implement query translation from GData WAPI to Drive API v2.

There are many gaps between search query for GData WAPI and one for Drive API
v2. To fill the gap, TranslateQuery parses the query for GData WAPI and builds
the one for Drive API. Unfortunately, there are missing query patterns on
Drive API v2, so it only supports limited patterns.

BUG=232352
TEST=Ran unit_tests and tested manually.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195147

[hidehiko, 2013-04-18 05:36:18]

Notes: unfortunately we cannot use base::StringTokenizer because it is unknown
if the result token is quoted string or not from it so that we cannot support
the exclusion query pattern.
Also, SearchInDirectory is out of scope. The only client would be sync file
system, but it is not yet used actually, and will be replaced by a new method to
support title-exact match both on GData WAPI and Drive API v2 (cc'ed kinuko@ and
tzik@, for FYI).

Thank you for your review in advance,
- hidehiko

[kinaba, 2013-04-18 06:59:48]

https://codereview.chromium.org/14341002/diff/1/chrome/browser/google_apis/dr...
File chrome/browser/google_apis/drive_api_service.cc (right):

https://codereview.chromium.org/14341002/diff/1/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_api_service.cc:219: //   Quoted query (e.g.
"dog cat").
Could you add the URLs for the search syntax of two APIs as a comment?

https://codereview.chromium.org/14341002/diff/1/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_api_service.cc:239: // whole remaining string
as a token as a fallback.
How about:
end_token = query.length();
query += '"';

and share the "token = ...; index = ...;" part with the correct case.
I remember discussion with you about npos, but I'm still worrying :).
Future reader might feel anxious as well about passing npos.

https://codereview.chromium.org/14341002/diff/1/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_api_service.cc:267: base::StringAppendF(
Please add comment on what "fullText" includes, like
https://code.google.com/p/chromium/issues/detail?id=232352#c5.

[hashimoto, 2013-04-18 07:23:50]

https://codereview.chromium.org/14341002/diff/1/chrome/browser/google_apis/dr...
File chrome/browser/google_apis/drive_api_service.cc (right):

https://codereview.chromium.org/14341002/diff/1/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_api_service.cc:218: //   Exclusion query (e.g.
-cat).
Please add output examples.

https://codereview.chromium.org/14341002/diff/1/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_api_service.cc:220: std::string
TranslateQuery(const std::string& original_query) {
Can't we have a test for this function or DriveAPIService::Search?

https://codereview.chromium.org/14341002/diff/1/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_api_service.cc:221: base::string16 query =
UTF8ToUTF16(original_query);
Why do we need to convert the string to UTF16, not using UTF8 directly or using
UTF8ToWide?

[hidehiko, 2013-04-18 09:04:32]

Thank you for your review.

To add unittests in a sane manner, moved the function to drive_api_util. Also
rebased on 14013005, which moves the EscapeQueryStringValue to drive_api_util.

PTAL?
- hidehiko

https://codereview.chromium.org/14341002/diff/1/chrome/browser/google_apis/dr...
File chrome/browser/google_apis/drive_api_service.cc (right):

https://codereview.chromium.org/14341002/diff/1/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_api_service.cc:218: //   Exclusion query (e.g.
-cat).
On 2013/04/18 07:23:50, hashimoto wrote:
> Please add output examples.

Done.

https://codereview.chromium.org/14341002/diff/1/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_api_service.cc:219: //   Quoted query (e.g.
"dog cat").
On 2013/04/18 06:59:48, kinaba wrote:
> Could you add the URLs for the search syntax of two APIs as a comment?

Done.

https://codereview.chromium.org/14341002/diff/1/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_api_service.cc:220: std::string
TranslateQuery(const std::string& original_query) {
On 2013/04/18 07:23:50, hashimoto wrote:
> Can't we have a test for this function or DriveAPIService::Search?

Done.

https://codereview.chromium.org/14341002/diff/1/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_api_service.cc:221: base::string16 query =
UTF8ToUTF16(original_query);
On 2013/04/18 07:23:50, hashimoto wrote:
> Why do we need to convert the string to UTF16, not using UTF8 directly or
using
> UTF8ToWide?

To handle non-ascii white space correctly. Added comments.

https://codereview.chromium.org/14341002/diff/1/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_api_service.cc:239: // whole remaining string
as a token as a fallback.
On 2013/04/18 06:59:48, kinaba wrote:
> How about:
> end_token = query.length();
> query += '"';
> 
> and share the "token = ...; index = ...;" part with the correct case.
> I remember discussion with you about npos, but I'm still worrying :).
> Future reader might feel anxious as well about passing npos.

Indeed. Done.

https://codereview.chromium.org/14341002/diff/1/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_api_service.cc:267: base::StringAppendF(
On 2013/04/18 06:59:48, kinaba wrote:
> Please add comment on what "fullText" includes, like
> https://code.google.com/p/chromium/issues/detail?id=232352#c5.

Done.

[kinaba, 2013-04-18 09:57:05]

lgtm

[hashimoto, 2013-04-18 10:37:15]

https://codereview.chromium.org/14341002/diff/10001/chrome/browser/google_api...
File chrome/browser/google_apis/drive_api_service.cc (right):

https://codereview.chromium.org/14341002/diff/10001/chrome/browser/google_api...
chrome/browser/google_apis/drive_api_service.cc:17: #include
"base/utf_string_conversions.h"
nit: No need to include this?

https://codereview.chromium.org/14341002/diff/10001/chrome/browser/google_api...
File chrome/browser/google_apis/drive_api_util_unittest.cc (right):

https://codereview.chromium.org/14341002/diff/10001/chrome/browser/google_api...
chrome/browser/google_apis/drive_api_util_unittest.cc:35: EXPECT_EQ("fullText
contains 'dog cat'", TranslateQuery("\"dog cat"));
What are the desired output for the following cases? (quotes are not to clarify)
 - => not fullText contains '', or <empty string>
 "" => fullText contains '', or <empty string>

Also, can we have test cases for the following cases?
 -"dog cat" => not fullText contains 'dog cat'
 -"" => not fullText contains '', or <empty string>

[hidehiko, 2013-04-18 12:05:54]

Thank you for your review. PTAL?

https://codereview.chromium.org/14341002/diff/10001/chrome/browser/google_api...
File chrome/browser/google_apis/drive_api_service.cc (right):

https://codereview.chromium.org/14341002/diff/10001/chrome/browser/google_api...
chrome/browser/google_apis/drive_api_service.cc:17: #include
"base/utf_string_conversions.h"
On 2013/04/18 10:37:16, hashimoto wrote:
> nit: No need to include this?

Done.

https://codereview.chromium.org/14341002/diff/10001/chrome/browser/google_api...
File chrome/browser/google_apis/drive_api_util_unittest.cc (right):

https://codereview.chromium.org/14341002/diff/10001/chrome/browser/google_api...
chrome/browser/google_apis/drive_api_util_unittest.cc:35: EXPECT_EQ("fullText
contains 'dog cat'", TranslateQuery("\"dog cat"));
On 2013/04/18 10:37:16, hashimoto wrote:
> What are the desired output for the following cases? (quotes are not to
clarify)
>  - => not fullText contains '', or <empty string>
>  "" => fullText contains '', or <empty string>
> 
> Also, can we have test cases for the following cases?
>  -"dog cat" => not fullText contains 'dog cat'
>  -"" => not fullText contains '', or <empty string>

Good point. Handled quoted text with a leading hyphen-minus, and ignored empty
tokens.

[hashimoto, 2013-04-18 12:35:39]

lgtm

[commit-bot: I haz the power, 2013-04-18 16:25:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/14341002/22001

[commit-bot: I haz the power, 2013-04-18 18:38:06]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-04-18 18:51:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/14341002/22001

[commit-bot: I haz the power, 2013-04-18 23:41:09]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-04-19 01:28:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/14341002/22001

[commit-bot: I haz the power, 2013-04-19 12:57:23]

Change committed as 195147