chromeos: Change DriveResourceMetadata's method arguemnts from DriveEntryProto* to resource ID when…

chromeos: Change DriveResourceMetadata's method arguemnts from DriveEntryProto* to resource ID when possible

We cannot modify an entry's state via raw pointers with DB.

Replace DriveEntryProto* with resource ID when possible.
Omit unnecessary arguments.
Add missing parent resource ID for test data.

BUG=127856
TEST=unit_tests

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

[kinaba, 2013-03-15 05:48:08]

https://codereview.chromium.org/12465012/diff/10001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_resource_metadata.cc (right):

https://codereview.chromium.org/12465012/diff/10001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:164: DriveEntryProto*
parent = entry->parent_resource_id().empty() ? NULL :
Now, empty parent_resource_id means it is the root entry,
and moving root entry to somewhere else is obviously invalid.
Could you check the case earlier and simplify the conditional here?

https://codereview.chromium.org/12465012/diff/10001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:166: if (parent &&
parent->file_info().is_directory())
Similarly, non directory parent is a bug.
I think this could be a DCHECK().

https://codereview.chromium.org/12465012/diff/10001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:416:
entry->set_parent_resource_id(new_parent->resource_id());
Is this set_parent necessary?
At this point |entry| has |entry_proto|'s parent_id, which should
point to |new_parent| already.

https://codereview.chromium.org/12465012/diff/10001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:781: // No need to care
about root missing case.
Inversely, do we need to care about non-root existing case?
We shouldn't load a proto into a non empty resource metadata.
I think we can DCHECK() the condition at the call site of ProtoToDirectory and
simplify the logic below.

https://codereview.chromium.org/12465012/diff/10001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:791:
AddEntryToDirectory(directory);  // Transfer ownership.
If the "Allocate a new DriveEntryProto when missing." is taken and this one not,
|directory| will leak.
Or, if |directory| entry has existed as a non-root entry and it had different
parent_resource_id, it'll become multi-rooted.

Both the situation won't happen under the assumptions
- there won't be an empty-parent entry in the proto, and
- this method is called only for empty (root only) Resource Metadata.
So practically this should be ok, but I'd like to see it clearly from the code.

[hashimoto, 2013-03-15 06:40:23]

https://codereview.chromium.org/12465012/diff/10001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_resource_metadata.cc (right):

https://codereview.chromium.org/12465012/diff/10001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:164: DriveEntryProto*
parent = entry->parent_resource_id().empty() ? NULL :
On 2013/03/15 05:48:08, kinaba wrote:
> Now, empty parent_resource_id means it is the root entry,
> and moving root entry to somewhere else is obviously invalid.
> Could you check the case earlier and simplify the conditional here?

Changed to return DRIVE_FILE_ERROR_INVALID_OPERATION for root.

https://codereview.chromium.org/12465012/diff/10001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:166: if (parent &&
parent->file_info().is_directory())
On 2013/03/15 05:48:08, kinaba wrote:
> Similarly, non directory parent is a bug.
> I think this could be a DCHECK().

Now we no longer need to get parent.
Removed.

https://codereview.chromium.org/12465012/diff/10001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:416:
entry->set_parent_resource_id(new_parent->resource_id());
On 2013/03/15 05:48:08, kinaba wrote:
> Is this set_parent necessary?
> At this point |entry| has |entry_proto|'s parent_id, which should
> point to |new_parent| already.

Excellent catch.
Removed.

https://codereview.chromium.org/12465012/diff/10001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:781: // No need to care
about root missing case.
On 2013/03/15 05:48:08, kinaba wrote:
> Inversely, do we need to care about non-root existing case?
> We shouldn't load a proto into a non empty resource metadata.
> I think we can DCHECK() the condition at the call site of ProtoToDirectory and
> simplify the logic below.

Added a DCHECK(child_maps_[proto.drive_entry().resource_id()].empty())

https://codereview.chromium.org/12465012/diff/10001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:791:
AddEntryToDirectory(directory);  // Transfer ownership.
On 2013/03/15 05:48:08, kinaba wrote:
> If the "Allocate a new DriveEntryProto when missing." is taken and this one
not,
> |directory| will leak.
> Or, if |directory| entry has existed as a non-root entry and it had different
> parent_resource_id, it'll become multi-rooted.
> 
> Both the situation won't happen under the assumptions
> - there won't be an empty-parent entry in the proto, and
> - this method is called only for empty (root only) Resource Metadata.
> So practically this should be ok, but I'd like to see it clearly from the
code.

Moved directory allocation to under the for loop for sub directories.
Renamed this method to AddDescendantsFromProto.

[kinaba, 2013-03-15 06:44:15]

lgtm

[hashimoto, 2013-03-15 07:08:32]

Committed patchset #3 manually as r188275 (presubmit successful).