Make DriveFeedProcessor asynchronous.

Make DriveFeedProcessor asynchronous.

DriveResourceMetadata:
* Add RefreshEntryProto, which will eventually replace RefreshFile. It handles update of metadata, change of parents, and both files and directories.
* Introduce AddEntryToParent which adds a DriveEntryProto to the tree. It calls AddEntryToDirectoryInternal which contains common code for AddEntryToParent and AddEntryToDirectory.
* Both AddEntryToParent and RefreshEntryProto use GetParent, a helper to determine the parent. 
* GetChildDirectories recursively gets the list of all children directory paths.
* unit tests for these functions.
* update unit tests to remove dependence on drive_files.h

DriveFeedProcessor:
* ApplyEntryProto calls DriveResourceMetadata::GetEntryInfoByResourceId.
* ContinueApplyEntryProto calls RemoveEntryFromParent for deleted entries, RefreshEntryProto for existing entries, and AddEntryToParent for new entries.
* RefreshEntryProto calls DriveResourceMetadata::RefreshEntryProto.
* NotifyForRefreshEntryProto updates changed_dirs_ with the list of affected directories to be notified.
* AddEntryToParent calls DriveResourceMetadata::AddEntryToParent.
* NotifyForAddEntryToParent updates changed_dirs_ with affected directories to be notified.
* RemoveEntryFromParent first fetches all child directories that need to be notified.
* OnGetChildrenForRemove calls DriveResourceMetadata::RemoveEntryFromParent.
* NotifyForRemoveEntryFromParent updates changed_dirs_ with affected directories to be notified.

A number of functions are now unused. They have been marked with TODOs since this patch is rather large.

BUG=137374
TEST=unit tests.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=168653

[achuithb, 2012-11-13 12:35:40]

Satoru-san, I will start working on unit tests next, but the code is ready for
review.

You probably want to look at drive_resource_metadata.* first then
drive_feed_processor.*

Thanks!

[satorux1, 2012-11-13 22:56:45]

Took a quick look. The change looks good. I'll take a closer look later.

http://codereview.chromium.org/11293247/diff/8/chrome/browser/chromeos/drive/...
File chrome/browser/chromeos/drive/drive_resource_metadata.h (right):

http://codereview.chromium.org/11293247/diff/8/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_resource_metadata.h:264: // Returns root if
|parent_resource_id| is not set.
is not set -> is empty?

This also seems to return NULL. Please document when it can happen.

[achuithb, 2012-11-14 01:01:35]

Added unit tests as well.

http://codereview.chromium.org/11293247/diff/8/chrome/browser/chromeos/drive/...
File chrome/browser/chromeos/drive/drive_resource_metadata.h (right):

http://codereview.chromium.org/11293247/diff/8/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_resource_metadata.h:264: // Returns root if
|parent_resource_id| is not set.
On 2012/11/13 22:56:45, satorux1 wrote:
> is not set -> is empty?
> 
> This also seems to return NULL. Please document when it can happen.

Done.

[satorux1, 2012-11-14 04:21:03]

mtomasz is now ooo. adding hashimoto@ for extra eyes.

[achuithb, 2012-11-14 04:23:16]

On 2012/11/14 04:21:03, satorux1 wrote:
> mtomasz is now ooo. adding hashimoto@ for extra eyes.

Great idea. Thank you.

[hashimoto, 2012-11-14 10:15:56]

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
File chrome/browser/chromeos/drive/drive_feed_processor.h (right):

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_feed_processor.h:98: void
AddEntryToParent(const DriveEntryProto& entry_proto);
Our coding style discourages function overloading. Could you rename at least one
of these two AddEntryToParent?

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
File chrome/browser/chromeos/drive/drive_resource_metadata.cc (right):

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:623: DVLOG(1) <<
"AddEntryToParent" << added_entry->GetFilePath().value();
s/AddEntryToParent/AddEntryToDirectoryInternal/

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:664: 
nit: No need to have two blank lines.

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
File chrome/browser/chromeos/drive/drive_resource_metadata.h (right):

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_resource_metadata.h:254: void
AddEntryToParent(const DriveEntryProto& entry_proto,
Why are you adding this method instead of rewriting AddEntryToDirectory?

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_resource_metadata.h:259: void
GetChangedDirectories(
The term 'changed' seems not obvious in the context of DriveResourceMetadata.
How about moving this method to drive_feed_processor.cc?

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_resource_metadata.h:267: DriveDirectory*
GetParent(const std::string& parent_resource_id);
Does this method need to be public?

[achuithb, 2012-11-14 11:11:29]

Thanks for the comments. PTAL!

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
File chrome/browser/chromeos/drive/drive_feed_processor.h (right):

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_feed_processor.h:98: void
AddEntryToParent(const DriveEntryProto& entry_proto);
On 2012/11/14 10:15:56, hashimoto wrote:
> Our coding style discourages function overloading. Could you rename at least
one
> of these two AddEntryToParent?

Renamed to AddEntryToParentDeprecated. This will be gone in the next CL along
with other dependencies on drive_files.h.

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
File chrome/browser/chromeos/drive/drive_resource_metadata.cc (right):

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:623: DVLOG(1) <<
"AddEntryToParent" << added_entry->GetFilePath().value();
On 2012/11/14 10:15:56, hashimoto wrote:
> s/AddEntryToParent/AddEntryToDirectoryInternal/

Done.

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:664: 
On 2012/11/14 10:15:56, hashimoto wrote:
> nit: No need to have two blank lines.

Done.

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
File chrome/browser/chromeos/drive/drive_resource_metadata.h (right):

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_resource_metadata.h:254: void
AddEntryToParent(const DriveEntryProto& entry_proto,
On 2012/11/14 10:15:56, hashimoto wrote:
> Why are you adding this method instead of rewriting AddEntryToDirectory?

AddEntryToParent has a DriveEntryProto, but does not know the path of the parent
directory. AddEntryToDirectory has a directory path, but does not have access to
the directory resource id. It's convenient to have two functions for now. It's
not obvious that they can be combined, and if they can, it's probably not right
to do it in this CL.

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_resource_metadata.h:259: void
GetChangedDirectories(
On 2012/11/14 10:15:56, hashimoto wrote:
> The term 'changed' seems not obvious in the context of DriveResourceMetadata.
> How about moving this method to drive_feed_processor.cc?

Ah, I can't do that. The code necessary to determine the changed file paths
needs to touch DriveEntry objects, and we need to move that out of
DriveFeedProcessor. We want DriveFeedProcessor to only handle DriveEntryProto
objects (which is almost true after this CL lands).

We can probably add some more to the comments, but it seems like this logic will
have to belong to DriveResourceMetadata even in the future when we use a
database.

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_resource_metadata.h:267: DriveDirectory*
GetParent(const std::string& parent_resource_id);
On 2012/11/14 10:15:56, hashimoto wrote:
> Does this method need to be public?

Done.

[satorux1, 2012-11-15 01:49:10]

http://codereview.chromium.org/11293247/diff/6005/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/drive_feed_processor.cc (right):

http://codereview.chromium.org/11293247/diff/6005/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_feed_processor.cc:311: if (dir)
nit: please add back {}. The body is not one line.

http://codereview.chromium.org/11293247/diff/6005/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/drive_file_system_unittest.cc (right):

http://codereview.chromium.org/11293247/diff/6005/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:1047: .Times(1);
I'm confused. Here, two things are tested:

1) "Added file.gdoc" is added in "drive/Directory 1" by applying
"delta_file_added_in_directory.json"

2) "Added file.gdoc" is deleted in "drive/Directory 1" by applying
"delta_file_added_in_directory.json"

I think "drive/Directory 1/Sub Directory Folder" and "drive/Directory 1/Sub
Directory Folder/Sub Sub Directory Folder" should be unaffected here.

http://codereview.chromium.org/11293247/diff/6005/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:1126: .Times(1);
Ditto. Should these directories be affected?

[hashimoto, 2012-11-15 04:10:41]

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
File chrome/browser/chromeos/drive/drive_resource_metadata.h (right):

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_resource_metadata.h:259: void
GetChangedDirectories(
On 2012/11/14 11:11:29, achuith.bhandarkar wrote:
> On 2012/11/14 10:15:56, hashimoto wrote:
> > The term 'changed' seems not obvious in the context of
DriveResourceMetadata.
> > How about moving this method to drive_feed_processor.cc?
> 
> Ah, I can't do that. The code necessary to determine the changed file paths
> needs to touch DriveEntry objects, and we need to move that out of
> DriveFeedProcessor. We want DriveFeedProcessor to only handle DriveEntryProto
> objects (which is almost true after this CL lands).
> 
> We can probably add some more to the comments, but it seems like this logic
will
> have to belong to DriveResourceMetadata even in the future when we use a
> database.

The main problem here is that, to understand what "changed" means, it requires
to look at both DriveResourceMetadata and DriveFeedProcessor.
How about doing one of the following?
 (1) Rename this method to a more descriptive name like
"GetParentAndAllChildDirectoriesIfEntryIsDirectory".
 (2) Instead of adding this method, call ReadDirectoryByPath recursively from
DriveFeedProcessor.
 (3) Fix this method to just call DriveDirectory::GetChildDirectoryPaths() and
rename to "GetChildDirectoryPaths"

IIUC, the only reason why you need this method is
DriveDirectory::GetChildDirectoryPaths.
If so, I think (2) or (3) may be better than (1) because the current
implementation of this method seems too specialized.

[achuithb, 2012-11-15 23:39:53]

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
File chrome/browser/chromeos/drive/drive_resource_metadata.h (right):

http://codereview.chromium.org/11293247/diff/9/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_resource_metadata.h:259: void
GetChangedDirectories(
On 2012/11/15 04:10:41, hashimoto wrote:
> On 2012/11/14 11:11:29, achuith.bhandarkar wrote:
> > On 2012/11/14 10:15:56, hashimoto wrote:
> > > The term 'changed' seems not obvious in the context of
> DriveResourceMetadata.
> > > How about moving this method to drive_feed_processor.cc?
> > 
> > Ah, I can't do that. The code necessary to determine the changed file paths
> > needs to touch DriveEntry objects, and we need to move that out of
> > DriveFeedProcessor. We want DriveFeedProcessor to only handle
DriveEntryProto
> > objects (which is almost true after this CL lands).
> > 
> > We can probably add some more to the comments, but it seems like this logic
> will
> > have to belong to DriveResourceMetadata even in the future when we use a
> > database.
> 
> The main problem here is that, to understand what "changed" means, it requires
> to look at both DriveResourceMetadata and DriveFeedProcessor.
> How about doing one of the following?
>  (1) Rename this method to a more descriptive name like
> "GetParentAndAllChildDirectoriesIfEntryIsDirectory".
>  (2) Instead of adding this method, call ReadDirectoryByPath recursively from
> DriveFeedProcessor.
>  (3) Fix this method to just call DriveDirectory::GetChildDirectoryPaths() and
> rename to "GetChildDirectoryPaths"
> 
> IIUC, the only reason why you need this method is
> DriveDirectory::GetChildDirectoryPaths.
> If so, I think (2) or (3) may be better than (1) because the current
> implementation of this method seems too specialized.

I've decided to go with (3). The problem with (2) is that it will require a
number of async calls which, when leveldb is implemented, result in a lot of
unnecessary ping-pong with the ui and blocking thread. It's more efficient to
get all the children in one step.

http://codereview.chromium.org/11293247/diff/6005/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/drive_feed_processor.cc (right):

http://codereview.chromium.org/11293247/diff/6005/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_feed_processor.cc:311: if (dir)
On 2012/11/15 01:49:10, satorux1 wrote:
> nit: please add back {}. The body is not one line.

Made it one line; moved the comment. Removed the pronoun.

[achuithb, 2012-11-15 23:57:08]

http://codereview.chromium.org/11293247/diff/6005/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/drive_file_system_unittest.cc (right):

http://codereview.chromium.org/11293247/diff/6005/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:1047: .Times(1);
On 2012/11/15 01:49:10, satorux1 wrote:
> I'm confused. Here, two things are tested:
> 
> 1) "Added file.gdoc" is added in "drive/Directory 1" by applying
> "delta_file_added_in_directory.json"
> 
> 2) "Added file.gdoc" is deleted in "drive/Directory 1" by applying
> "delta_file_added_in_directory.json"
> 
> I think "drive/Directory 1/Sub Directory Folder" and "drive/Directory 1/Sub
> Directory Folder/Sub Sub Directory Folder" should be unaffected here.

Note that the delta json file actually has 2 document entries - one for the
newly added file, and one for the parent directory. 

If the parent directory is modified, all the children should be notified. This
is actually broken in TOT, and we are not sending all the correct notifications.
The code at fault is here:
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chromeos/dri...

Note that we move the existing children from old_entry to the new entry using
TakeOverEntries BEFORE calling RemoveEntryFromParent. RemoveEntryFromParent
attempts to collect the children of old_entry being removed, but there are never
any, because TakeOverEntries has already been called. The code should actually
call GetChildDirectoryPaths first, then TakeOverEntries, and finally
RemoveEntry.

http://codereview.chromium.org/11293247/diff/6005/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:1126: .Times(1);
On 2012/11/15 01:49:10, satorux1 wrote:
> Ditto. Should these directories be affected?

Same as above.

[achuithb, 2012-11-15 23:57:50]

I need to re-order the functions in the cc file to match the order in the
header. I'll do this after I get lgtms so as not to mess up the diff.

[satorux1, 2012-11-16 06:57:48]

http://codereview.chromium.org/11293247/diff/6005/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/drive_file_system_unittest.cc (right):

http://codereview.chromium.org/11293247/diff/6005/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:1047: .Times(1);
On 2012/11/15 23:57:08, achuith.bhandarkar wrote:
> On 2012/11/15 01:49:10, satorux1 wrote:
> > I'm confused. Here, two things are tested:
> > 
> > 1) "Added file.gdoc" is added in "drive/Directory 1" by applying
> > "delta_file_added_in_directory.json"
> > 
> > 2) "Added file.gdoc" is deleted in "drive/Directory 1" by applying
> > "delta_file_added_in_directory.json"
> > 
> > I think "drive/Directory 1/Sub Directory Folder" and "drive/Directory 1/Sub
> > Directory Folder/Sub Sub Directory Folder" should be unaffected here.
> 
> Note that the delta json file actually has 2 document entries - one for the
> newly added file, and one for the parent directory. 
> 
> If the parent directory is modified, all the children should be notified. This
> is actually broken in TOT, and we are not sending all the correct
notifications.

Let's not change the semantics in this patch.

1) The patch is a major refactoring, so changing the behaviors while refactoring
should usually be avoided

2) Notifying children looks wrong. The purpose of changed directories
notification is to update the contents of the current directory shown in
Files.app. There is no need to update the contents if something was changed in a
parent directory or above.

3) mtomasz filed crbug.com/158908 about inconsistency between file watching on 
the local disk and file watching on Drive. We should fix the inconsistency.


> The code at fault is here:
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chromeos/dri...
> 
> Note that we move the existing children from old_entry to the new entry using
> TakeOverEntries BEFORE calling RemoveEntryFromParent. RemoveEntryFromParent
> attempts to collect the children of old_entry being removed, but there are
never
> any, because TakeOverEntries has already been called. The code should actually
> call GetChildDirectoryPaths first, then TakeOverEntries, and finally
> RemoveEntry.

[achuithb, 2012-11-16 22:55:56]

Changes to drive_filesystem_unittest.cc have been reverted. For Refresh, we
don't notify children of directories as requested.

PTAL, Satoru-san and Ryo-san.

http://codereview.chromium.org/11293247/diff/6005/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/drive_file_system_unittest.cc (right):

http://codereview.chromium.org/11293247/diff/6005/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:1047: .Times(1);
On 2012/11/16 06:57:48, satorux1 wrote:
> On 2012/11/15 23:57:08, achuith.bhandarkar wrote:
> > On 2012/11/15 01:49:10, satorux1 wrote:
> > > I'm confused. Here, two things are tested:
> > > 
> > > 1) "Added file.gdoc" is added in "drive/Directory 1" by applying
> > > "delta_file_added_in_directory.json"
> > > 
> > > 2) "Added file.gdoc" is deleted in "drive/Directory 1" by applying
> > > "delta_file_added_in_directory.json"
> > > 
> > > I think "drive/Directory 1/Sub Directory Folder" and "drive/Directory
1/Sub
> > > Directory Folder/Sub Sub Directory Folder" should be unaffected here.
> > 
> > Note that the delta json file actually has 2 document entries - one for the
> > newly added file, and one for the parent directory. 
> > 
> > If the parent directory is modified, all the children should be notified.
This
> > is actually broken in TOT, and we are not sending all the correct
> notifications.
> 
> Let's not change the semantics in this patch.
> 
> 1) The patch is a major refactoring, so changing the behaviors while
refactoring
> should usually be avoided
> 
> 2) Notifying children looks wrong. The purpose of changed directories
> notification is to update the contents of the current directory shown in
> Files.app. There is no need to update the contents if something was changed in
a
> parent directory or above.
> 
> 3) mtomasz filed crbug.com/158908 about inconsistency between file watching on

> the local disk and file watching on Drive. We should fix the inconsistency.
> 
> 
> > The code at fault is here:
> >
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chromeos/dri...
> > 
> > Note that we move the existing children from old_entry to the new entry
using
> > TakeOverEntries BEFORE calling RemoveEntryFromParent. RemoveEntryFromParent
> > attempts to collect the children of old_entry being removed, but there are
> never
> > any, because TakeOverEntries has already been called. The code should
actually
> > call GetChildDirectoryPaths first, then TakeOverEntries, and finally
> > RemoveEntry.
> 

Done.

[achuithb, 2012-11-16 22:55:56]

Changes to drive_filesystem_unittest.cc have been reverted. For Refresh, we
don't notify children of directories as requested.

PTAL, Satoru-san and Ryo-san.

http://codereview.chromium.org/11293247/diff/6005/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/drive_file_system_unittest.cc (right):

http://codereview.chromium.org/11293247/diff/6005/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:1047: .Times(1);
On 2012/11/16 06:57:48, satorux1 wrote:
> On 2012/11/15 23:57:08, achuith.bhandarkar wrote:
> > On 2012/11/15 01:49:10, satorux1 wrote:
> > > I'm confused. Here, two things are tested:
> > > 
> > > 1) "Added file.gdoc" is added in "drive/Directory 1" by applying
> > > "delta_file_added_in_directory.json"
> > > 
> > > 2) "Added file.gdoc" is deleted in "drive/Directory 1" by applying
> > > "delta_file_added_in_directory.json"
> > > 
> > > I think "drive/Directory 1/Sub Directory Folder" and "drive/Directory
1/Sub
> > > Directory Folder/Sub Sub Directory Folder" should be unaffected here.
> > 
> > Note that the delta json file actually has 2 document entries - one for the
> > newly added file, and one for the parent directory. 
> > 
> > If the parent directory is modified, all the children should be notified.
This
> > is actually broken in TOT, and we are not sending all the correct
> notifications.
> 
> Let's not change the semantics in this patch.
> 
> 1) The patch is a major refactoring, so changing the behaviors while
refactoring
> should usually be avoided
> 
> 2) Notifying children looks wrong. The purpose of changed directories
> notification is to update the contents of the current directory shown in
> Files.app. There is no need to update the contents if something was changed in
a
> parent directory or above.
> 
> 3) mtomasz filed crbug.com/158908 about inconsistency between file watching on

> the local disk and file watching on Drive. We should fix the inconsistency.
> 
> 
> > The code at fault is here:
> >
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chromeos/dri...
> > 
> > Note that we move the existing children from old_entry to the new entry
using
> > TakeOverEntries BEFORE calling RemoveEntryFromParent. RemoveEntryFromParent
> > attempts to collect the children of old_entry being removed, but there are
> never
> > any, because TakeOverEntries has already been called. The code should
actually
> > call GetChildDirectoryPaths first, then TakeOverEntries, and finally
> > RemoveEntry.
> 

Done.

[satorux1, 2012-11-19 01:24:24]

LGTM with nits

http://codereview.chromium.org/11293247/diff/28002/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_feed_processor.cc (right):

http://codereview.chromium.org/11293247/diff/28002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_feed_processor.cc:202: if
(!entry_proto->has_file_specific_info()) {
The following would be a bit clearer:

entry_proto->file_info().is_directory()

http://codereview.chromium.org/11293247/diff/28002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_feed_processor.cc:219:
!entry_proto.has_file_specific_info()));
ditto. please check other places too.

http://codereview.chromium.org/11293247/diff/28002/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc (right):

http://codereview.chromium.org/11293247/diff/28002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc:53: // Add a
DriveEntryProto to the metadata tree.
Adds

http://codereview.chromium.org/11293247/diff/28002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc:143: }
Calling ASSERT_() doesn't seem to be good, as test code cannot stop when this
fails. I think this function should return a bool so that client code can do:

ASSERT_TRUE(AddDriveEntryProto(....));

http://codereview.chromium.org/11293247/diff/28002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc:386:
EXPECT_EQ(DRIVE_FILE_ERROR_NOT_FOUND, db_error);
ASSERT_EQ()? I think we shouldn't go further if this fails.

http://codereview.chromium.org/11293247/diff/28002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc:401:
EXPECT_EQ(DRIVE_FILE_ERROR_IN_USE, db_error);
ASSERT_EQ?

http://codereview.chromium.org/11293247/diff/28002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc:411:
EXPECT_EQ(DRIVE_FILE_OK, db_error);
ASSERT_EQ?

[hashimoto, 2012-11-19 10:18:02]

lgtm with nits

http://codereview.chromium.org/11293247/diff/28002/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_resource_metadata.h (right):

http://codereview.chromium.org/11293247/diff/28002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.h:95:
ChangedDirectoriesCallback;
Please rename this.

[achuithb, 2012-11-19 21:49:32]

Thanks for the reviews, Satoru-san and Ryo-san!

http://codereview.chromium.org/11293247/diff/28002/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_resource_metadata.h (right):

http://codereview.chromium.org/11293247/diff/28002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.h:95:
ChangedDirectoriesCallback;
On 2012/11/19 10:18:02, hashimoto wrote:
> Please rename this.

Done.

http://codereview.chromium.org/11293247/diff/28002/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc (right):

http://codereview.chromium.org/11293247/diff/28002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc:53: // Add a
DriveEntryProto to the metadata tree.
On 2012/11/19 01:24:24, satorux1 wrote:
> Adds

Done.

http://codereview.chromium.org/11293247/diff/28002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc:143: }
On 2012/11/19 01:24:24, satorux1 wrote:
> Calling ASSERT_() doesn't seem to be good, as test code cannot stop when this
> fails. I think this function should return a bool so that client code can do:
> 
> ASSERT_TRUE(AddDriveEntryProto(....));

Done.

http://codereview.chromium.org/11293247/diff/28002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc:386:
EXPECT_EQ(DRIVE_FILE_ERROR_NOT_FOUND, db_error);
On 2012/11/19 01:24:24, satorux1 wrote:
> ASSERT_EQ()? I think we shouldn't go further if this fails.

Done.

http://codereview.chromium.org/11293247/diff/28002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc:401:
EXPECT_EQ(DRIVE_FILE_ERROR_IN_USE, db_error);
On 2012/11/19 01:24:24, satorux1 wrote:
> ASSERT_EQ?

Done.

http://codereview.chromium.org/11293247/diff/28002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc:411:
EXPECT_EQ(DRIVE_FILE_OK, db_error);
On 2012/11/19 01:24:24, satorux1 wrote:
> ASSERT_EQ?

Done.

[commit-bot: I haz the power, 2012-11-19 21:56:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/achuith@chromium.org/11293247/16013