Add gdata content search

Add gdata content search

Search results will be shown under virtual path gdata/.search/<query>.
Content search will be triggered whenever we try to enumerate directory with
the mentioned file path format.
Entries returned by search results will have file names formatted as
<resource_id>.<real_file_name>

NOTE: the file manager part is in separate cl.

TEST = manual, unittests: *GData*
BUG=chromium-os:27539
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=135606

[tbarzic, 2012-05-03 00:56:14]

Guys,
can you, please, take a look :)

Toni

[satorux1, 2012-05-03 17:58:33]

Nice work! As usual, can I ask you to split this patch, so we can review the
code more effectively?  maybe:

1) gdata_util stuff is pretty isolated.
2) add |search_query| to DocumentService::GetDocuments()
3) implement the search feature.

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_documents_service.h (right):

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_documents_service.h:79: // used only if
|start_changestamp| is 0.
If we don't want to search, can we just pass an empty string? please document
about it.

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1234:
reinterpret_cast<GDataEntry*>(NULL));
indentation is off?

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1246:
reinterpret_cast<GDataEntry*>(NULL));
indentation is off?

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1429: FilePath dest_file_path
= original_dest_file_path;
If I understand right, you don't have to initialize them. just:

FilePath src_file_path;
FilePath dest_file_path;

because they will be set below.

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1453: dest_parent_path =
dest_parent->GetFilePath();
This code path is used for regular paths (not search results), right? I
originally thought that the code path is only used for the search results, per
the comment. could you clarify the comment a bit?

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1456: }
might want to add:

DCHECK(!src_file_path.empty());
DCHECK(!dest_file_path.empty());

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1803: base::Bind(callback,
base::PLATFORM_FILE_OK));
Why is it OK? The entry wasn't found, right? please add some comment

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1813: entry->GetFilePath()));
why is this changed? file_path == entry->GetFilePath(), right?

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1855: path_type ==
util::GDATA_SEARCH_PATH_RESULT) {
maybe:

 if (util::IsSearchDirectoryPath(directory_path)) {

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2851: false,
could you add comments to 0,0,false? they look cryptic.

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.h:208: // Does server side
content search for |search_query|. Search results will be
please move this function to where ReadDirectoryByPathAsync() is defined, as
they are similar

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.h:213: virtual void
SearchContent(const std::string& search_query,
maybe GetEntriesByQueryAsync()? (we'll drop Async from function names once all
synchronous public functions are gone)

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.h:575: bool
allow_multiple_feeds;
maybe should_fetch_multipe_feeds ?

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc (right):

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc:156: if
(util::GetSearchPathStatus(file_path) == util::GDATA_SEARCH_PATH_QUERY) {
please add some comment about what we are doing here.

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right):

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:3079: void
ContentSearchCallback(MessageLoop* message_loop,
function comment is missing.

please also move this to anonymous namespace above

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:3127: for (size_t i
= 0; i < ARRAYSIZE_UNSAFE(kSearchResultPairs); i++) {
why cannot you use arraysize()?

i++ -> ++i (as the latter is more common in our code)

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:3169:
EXPECT_TRUE(FindEntry(real_file_path) == NULL);
EXPECT_FALSE(FindEntry(real_file_path))

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:142: // E.g. |fake_search_directory|
below.
What about GDataRootDirectory? What parent does it have? might want to add some
comment.

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:514: GDataEntry** entry_to_return);
This function seems to do more than just modifying parameters. In fact,
|entry_to_return| isn't a FindEntryByPath parameter. Can you split into two
functions or so?

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:523: scoped_ptr<GDataDirectory>
fake_search_query_directory_;
What are the differences between the two directories?

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_util.h (right):

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.h:20: enum GDataSearchPathType {
Please add some comment about what is a search path.

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.h:58: // Checks if the path is under
(virtual) gdata earch directory, and returns its
earch -> search

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_util.h:70: void CrackSearchFileName(const
std::string& search_file_name,
Crack -> Parse? Crack sounds rather uncivilized. :)

Besides, we have ParseCachePath() below.

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/mock_gdata_documents_service.cc (right):

http://codereview.chromium.org/10274002/diff/12003/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/mock_gdata_documents_service.cc:74: //
TODO(tbarzic): rename the json file.
Any reason why cannot we do this now?

[Ben Chan, 2012-05-03 22:47:39]

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_documents_service_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_documents_service_browsertest.cc:114:
std::string(), // search string
nit: two spaces before comment

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_documents_service_browsertest.cc:138:
std::string(), // search string
nit: two spaces before comment

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1138: std::string() /* no
search query*/,
nit: one space before */

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1152: std::string() /* no
search query*/,
nit: one space before */

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1191: std::string() /* no
search query*/,
nit: one space before */

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.h:348: // Can be called from
UI/IO thread. |callback| is run on calling thread.
nit:  for consistency, "run on calling thread" -> "run on the calling thread"

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.h:636: // |status| was
|LoadFeedFromServer| successfull.
typo: successfull?

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_files.h:141: // True it the entry is not
bound to any file system (i.e. doesn't have a root
typo: True it -> True if

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_files.h:520: // should never  be used for
file operations or storing file entries.
nit: remove one extra space in "never  be"

[zel, 2012-05-03 22:57:56]

http://codereview.chromium.org/10274002/diff/22001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

http://codereview.chromium.org/10274002/diff/22001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.h:416: virtual void
GetEntriesByQueryAsync(
Get<Something>ByQuery is called Search :)

Please rename this method to SearchAsync instead.

http://codereview.chromium.org/10274002/diff/22001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10274002/diff/22001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:495:
util::ParseSearchFileName((*components)[3], &resource_id, &file_name);
DCHECK that |components| starts with [ "gdata", "search",...]

http://codereview.chromium.org/10274002/diff/22001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10274002/diff/22001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:521: scoped_ptr<GDataDirectory>
fake_search_directory_;
hmm, what if I make two search requests at the same time? if you want/need to
cache results of search you should probably hold the map<query, GDataDirectory>.
I am also not sure that GDataRootDirectory is the best place for it.

[tbarzic, 2012-05-03 23:56:17]

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_documents_service.h (right):

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_documents_service.h:79: // used only if
|start_changestamp| is 0.
On 2012/05/03 17:58:34, satorux1 wrote:
> If we don't want to search, can we just pass an empty string? please document
> about it.

Done.

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1234:
reinterpret_cast<GDataEntry*>(NULL));
On 2012/05/03 17:58:34, satorux1 wrote:
> indentation is off?

Done.

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1246:
reinterpret_cast<GDataEntry*>(NULL));
On 2012/05/03 17:58:34, satorux1 wrote:
> indentation is off?

Done.

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1429: FilePath dest_file_path
= original_dest_file_path;
On 2012/05/03 17:58:34, satorux1 wrote:
> If I understand right, you don't have to initialize them. just:
> 
> FilePath src_file_path;
> FilePath dest_file_path;
> 
> because they will be set below.

Done.

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1453: dest_parent_path =
dest_parent->GetFilePath();
On 2012/05/03 17:58:34, satorux1 wrote:
> This code path is used for regular paths (not search results), right? I
> originally thought that the code path is only used for the search results, per
> the comment. could you clarify the comment a bit?

Done.

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1456: }
On 2012/05/03 17:58:34, satorux1 wrote:
> might want to add:
> 
> DCHECK(!src_file_path.empty());
> DCHECK(!dest_file_path.empty());

Done.

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1803: base::Bind(callback,
base::PLATFORM_FILE_OK));
On 2012/05/03 17:58:34, satorux1 wrote:
> Why is it OK? The entry wasn't found, right? please add some comment

Done.

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1813: entry->GetFilePath()));
On 2012/05/03 17:58:34, satorux1 wrote:
> why is this changed? file_path == entry->GetFilePath(), right? 

not necessary.
if file_path is in gdata search result directory, those valued will be
different:
("gdata/.search/foo/resource_id.file_name" vs. "gdata/some_dirs/file_name")

we want to use the actual file system path

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1855: path_type ==
util::GDATA_SEARCH_PATH_RESULT) {
On 2012/05/03 17:58:34, satorux1 wrote:
> maybe:
> 
>  if (util::IsSearchDirectoryPath(directory_path)) {

Done.

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2851: false,
On 2012/05/03 17:58:34, satorux1 wrote:
> could you add comments to 0,0,false? they look cryptic.

yeah, I came to the same conclusion looking at the change last night :)

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.h:208: // Does server side
content search for |search_query|. Search results will be
On 2012/05/03 17:58:34, satorux1 wrote:
> please move this function to where ReadDirectoryByPathAsync() is defined, as
> they are similar

Done.

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.h:213: virtual void
SearchContent(const std::string& search_query,
On 2012/05/03 17:58:34, satorux1 wrote:
> maybe GetEntriesByQueryAsync()? (we'll drop Async from function names once all
> synchronous public functions are gone)

Done.

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.h:575: bool
allow_multiple_feeds;
On 2012/05/03 17:58:34, satorux1 wrote:
> maybe should_fetch_multipe_feeds ?

Done.

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc (right):

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc:156: if
(util::GetSearchPathStatus(file_path) == util::GDATA_SEARCH_PATH_QUERY) {
On 2012/05/03 17:58:34, satorux1 wrote:
> please add some comment about what we are doing here. 

Done.

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right):

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:3079: void
ContentSearchCallback(MessageLoop* message_loop,
On 2012/05/03 17:58:34, satorux1 wrote:
> function comment is missing.
> 
> please also move this to anonymous namespace above

Done.

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:3127: for (size_t i
= 0; i < ARRAYSIZE_UNSAFE(kSearchResultPairs); i++) {
struct SearchResultPair is defined in function.. will move it to namespace {}

On 2012/05/03 17:58:34, satorux1 wrote:
> why cannot you use arraysize()?
> 
> i++ -> ++i (as the latter is more common in our code)

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:3169:
EXPECT_TRUE(FindEntry(real_file_path) == NULL);
On 2012/05/03 17:58:34, satorux1 wrote:
> EXPECT_FALSE(FindEntry(real_file_path))

Done.

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_files.h:142: // E.g. |fake_search_directory|
below.
On 2012/05/03 17:58:34, satorux1 wrote:
> What about GDataRootDirectory? What parent does it have? might want to add
some
> comment.

it doesn't have parents, but it has root_ (itself)

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_files.h:514: GDataEntry** entry_to_return);
On 2012/05/03 17:58:34, satorux1 wrote:
> This function seems to do more than just modifying parameters. In fact,
> |entry_to_return| isn't a FindEntryByPath parameter. Can you split into two
> functions or so?

moved part of implementation to FindEntryByPath

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_files.h:523: scoped_ptr<GDataDirectory>
fake_search_query_directory_;
On 2012/05/03 17:58:34, satorux1 wrote:
> What are the differences between the two directories?

there's only semantic difference.. I'll get rid of fake_search_query_directory

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_util.h (right):

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_util.h:20: enum GDataSearchPathType {
On 2012/05/03 17:58:34, satorux1 wrote:
> Please add some comment about what is a search path.

Done.

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_util.h:58: // Checks if the path is under
(virtual) gdata earch directory, and returns its
On 2012/05/03 17:58:34, satorux1 wrote:
> earch -> search

Done.

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_util.h:70: void CrackSearchFileName(const
std::string& search_file_name,
On 2012/05/03 17:58:34, satorux1 wrote:
> Crack -> Parse? Crack sounds rather uncivilized. :)

Well, am I from Balkans or not :)
(Balkan people are known to be uncivilized)

> 
> Besides, we have ParseCachePath() below.

Done

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
File chrome/browser/chromeos/gdata/mock_gdata_documents_service.cc (right):

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/mock_gdata_documents_service.cc:74: //
TODO(tbarzic): rename the json file.
On 2012/05/03 17:58:34, satorux1 wrote:
> Any reason why cannot we do this now?

Well, I guess not..

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_documents_service_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_documents_service_browsertest.cc:114:
std::string(), // search string
On 2012/05/03 22:47:39, Ben Chan wrote:
> nit: two spaces before comment

Done.

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_documents_service_browsertest.cc:138:
std::string(), // search string
On 2012/05/03 22:47:39, Ben Chan wrote:
> nit: two spaces before comment

Done.

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1138: std::string() /* no
search query*/,
On 2012/05/03 22:47:39, Ben Chan wrote:
> nit: one space before */

Done.

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1152: std::string() /* no
search query*/,
On 2012/05/03 22:47:39, Ben Chan wrote:
> nit: one space before */

Done.

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1191: std::string() /* no
search query*/,
On 2012/05/03 22:47:39, Ben Chan wrote:
> nit: one space before */

Done.

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.h:348: // Can be called from
UI/IO thread. |callback| is run on calling thread.
On 2012/05/03 22:47:39, Ben Chan wrote:
> nit:  for consistency, "run on calling thread" -> "run on the calling thread"

Done.

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.h:416: virtual void
GetEntriesByQueryAsync(
On 2012/05/03 22:57:56, zel wrote:
> Get<Something>ByQuery is called Search :)
> 
> Please rename this method to SearchAsync instead.

Done.

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.h:636: // |status| was
|LoadFeedFromServer| successfull.
On 2012/05/03 22:47:39, Ben Chan wrote:
> typo: successfull?

Done.

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_files.cc:495:
util::ParseSearchFileName((*components)[3], &resource_id, &file_name);
On 2012/05/03 22:57:56, zel wrote:
> DCHECK that |components| starts with [ "gdata", "search",...]

Done.

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_files.cc:542:
fake_search_directory_->set_file_name(file_path.BaseName().value());
don't really need this

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_files.h:141: // True it the entry is not
bound to any file system (i.e. doesn't have a root
On 2012/05/03 22:47:39, Ben Chan wrote:
> typo: True it -> True if

Done.

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_files.h:520: // should never  be used for
file operations or storing file entries.
On 2012/05/03 22:47:39, Ben Chan wrote:
> nit: remove one extra space in "never  be"

Done.

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_files.h:521: scoped_ptr<GDataDirectory>
fake_search_directory_;
On 2012/05/03 22:57:56, zel wrote:
> hmm, what if I make two search requests at the same time? if you want/need to
> cache results of search you should probably hold the map<query,
GDataDirectory>.
> I am also not sure that GDataRootDirectory is the best place for it.

This is not supposed to keep any data. We only need this so we have something to
return when file manager tries to resolve "gdata/.search/foo" or "gdata/.search"
paths (which don't really exist in actual gdata file system).

But, yeah, for caching results, we'll need a map.

https://chromiumcodereview.appspot.com/10274002/diff/30001/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

https://chromiumcodereview.appspot.com/10274002/diff/30001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.h:1315: void
SearchAsyncOnUIThread(const std::string& search_query,
indent!

[zel, 2012-05-04 00:00:07]

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_files.h:521: scoped_ptr<GDataDirectory>
fake_search_directory_;
On 2012/05/03 23:56:17, tbarzic wrote:
> On 2012/05/03 22:57:56, zel wrote:
> > hmm, what if I make two search requests at the same time? if you want/need
to
> > cache results of search you should probably hold the map<query,
> GDataDirectory>.
> > I am also not sure that GDataRootDirectory is the best place for it.
> 
> This is not supposed to keep any data. We only need this so we have something
to
> return when file manager tries to resolve "gdata/.search/foo" or
"gdata/.search"
> paths (which don't really exist in actual gdata file system).
> 
> But, yeah, for caching results, we'll need a map.

I am confused. Why would we care about this is there is no search data inside?

[tbarzic, 2012-05-04 00:10:01]

On 2012/05/04 00:00:07, zel wrote:
>
https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
> File chrome/browser/chromeos/gdata/gdata_files.h (right):
> 
>
https://chromiumcodereview.appspot.com/10274002/diff/22001/chrome/browser/chr...
> chrome/browser/chromeos/gdata/gdata_files.h:521: scoped_ptr<GDataDirectory>
> fake_search_directory_;
> On 2012/05/03 23:56:17, tbarzic wrote:
> > On 2012/05/03 22:57:56, zel wrote:
> > > hmm, what if I make two search requests at the same time? if you want/need
> to
> > > cache results of search you should probably hold the map<query,
> > GDataDirectory>.
> > > I am also not sure that GDataRootDirectory is the best place for it.
> > 
> > This is not supposed to keep any data. We only need this so we have
something
> to
> > return when file manager tries to resolve "gdata/.search/foo" or
> "gdata/.search"
> > paths (which don't really exist in actual gdata file system).
> > 
> > But, yeah, for caching results, we'll need a map.
> 
> I am confused. Why would we care about this is there is no search data inside?

It's just to make file browser happy.
When we are changing directory in file browser file browser does something like
(in short version):

filesystem.getDirectory(newPath,{},function(entry){
  readDirectory(entry);
});

so when file browser tries to get the entry, we return this fake entry  . when
readDirectory for 'gdata/.search/foo' is called, we intercept it in
file_system_proxy, do the search and return entry containing search results.

[zel, 2012-05-04 00:21:03]

i see

LGTM

[satorux1, 2012-05-04 01:48:39]

LGTM with nits: please consider addressing them before submit.

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

https://chromiumcodereview.appspot.com/10274002/diff/12003/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1813: entry->GetFilePath()));
On 2012/05/03 23:56:17, tbarzic wrote:
> On 2012/05/03 17:58:34, satorux1 wrote:
> > why is this changed? file_path == entry->GetFilePath(), right? 
> 
> not necessary.
> if file_path is in gdata search result directory, those valued will be
> different:
> ("gdata/.search/foo/resource_id.file_name" vs. "gdata/some_dirs/file_name")
> 
> we want to use the actual file system path

I see then, could you add it as a comment?

https://chromiumcodereview.appspot.com/10274002/diff/25002/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

https://chromiumcodereview.appspot.com/10274002/diff/25002/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1136: true,
could you add /* should_fetch_multiple_feeds */

https://chromiumcodereview.appspot.com/10274002/diff/25002/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1150: true,
ditto.

https://chromiumcodereview.appspot.com/10274002/diff/25002/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1189: true,
ditto.

https://chromiumcodereview.appspot.com/10274002/diff/25002/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:3695:
NotifyDirectoryChanged(entry->GetFilePath());
you might also want to document why we should use GetFilePath() rather than
directory_path here.

https://chromiumcodereview.appspot.com/10274002/diff/25002/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4971: }
you can probably move this to anonymous namespace. One less thing to have in .h
file is generally preferable.

https://chromiumcodereview.appspot.com/10274002/diff/25002/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

https://chromiumcodereview.appspot.com/10274002/diff/25002/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_files.cc:491: DCHECK(components->size() >
3);
why not DCHEC_EQ(4, components->size())? can this be larger than 4? if so,
please document about it.

https://chromiumcodereview.appspot.com/10274002/diff/25002/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_files.cc:519: // Remove "gdata/.search" from
path.
if we know compoentns->size() == 4, then we could

components->clear();
components->push_back(...);
components->push_back(...);

https://chromiumcodereview.appspot.com/10274002/diff/25002/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_files.cc:552: if
(!ModifyFindEntryParamsIfSearchPath(file_path, &components, &current_dir,
Maybe IfSearchPath -> ForSearchPath()? The function is now called only if the
path is a search path.

https://chromiumcodereview.appspot.com/10274002/diff/25002/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

https://chromiumcodereview.appspot.com/10274002/diff/25002/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_files.h:506: // pointing to a search result
path. The find path parameters should be
find path parameters -> find entry parameters

https://chromiumcodereview.appspot.com/10274002/diff/25002/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_files.h:508: // result.
this function is rather tricky. could you illustrate how this works with some
example?

https://chromiumcodereview.appspot.com/10274002/diff/25002/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_util.h (right):

https://chromiumcodereview.appspot.com/10274002/diff/25002/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_util.h:73: void ParseSearchFileName(const
std::string& search_file_name,
shouldn't this return true/false that indicates success/failure?

[satorux1, 2012-05-04 01:53:28]

http://codereview.chromium.org/10274002/diff/25002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10274002/diff/25002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:504: // If |file_path| is indeed
search file path, we should continue search from
is this comment still relevant? I thought now |file_path| is guaranteed to be a
search path? please revise as needed.