child_files_ and child_directories_ have resource_ids instead of GDataFile* and GDataDirectory*.

child_files_ and child_directories_ have resource_ids instead of GDataFile* and GDataDirectory*.

* GDataFileCollection and GDataDirectoryCollection are replaced by GDataChildMap which has resource_id as value instead of GDataFile* and GDataDirectory*.
* FindChild returns a resource_id instead of GDataEntry*
* Get rid of AddChild as it's only being used in one place.
* TakeOverEntries return type is now void instead of bool.
* Add helper TakeOverEntry to avoid cut n paste.

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

[achuithb, 2012-08-20 10:09:07]

Please review.

[satorux1, 2012-08-20 16:40:08]

http://codereview.chromium.org/10855243/diff/6004/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_directory_service.cc (right):

http://codereview.chromium.org/10855243/diff/6004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_directory_service.cc:304:
GetEntryByResourceId(current_dir->FindChild(components[i]));
looks a bit scary. Maybe:

const std::string resource_id = current_dir->FindChild(components[i]);
if (resource_id.empty())
  return NULL;
GDataEntry* entry = GetEntryByResourceId(resource_id);

http://codereview.chromium.org/10855243/diff/6004/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10855243/diff/6004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.cc:255: iter !=
dir->child_files_.end(); ++iter) {
maybe add:

  const std::string& resource_id = iter->second;

to make it clear iter->second is a resource id.

http://codereview.chromium.org/10855243/diff/6004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.cc:282: const std::string&
GDataDirectory::FindChild(
I'd just return const std::string rather than a reference. Less efficient, but
less tricky.

http://codereview.chromium.org/10855243/diff/6004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.cc:320: GDataEntry* child =
directory_service_->GetEntryByResourceId(iter->second);
DCHECK(child); ?

http://codereview.chromium.org/10855243/diff/6004/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10855243/diff/6004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.h:257: // Find a child's resource_id
by its name.
Please document what's returned if the child is not found. Empty string?

[achuithb, 2012-08-20 21:14:57]

PTAL

http://codereview.chromium.org/10855243/diff/6004/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_directory_service.cc (right):

http://codereview.chromium.org/10855243/diff/6004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_directory_service.cc:304:
GetEntryByResourceId(current_dir->FindChild(components[i]));
On 2012/08/20 16:40:08, satorux1 wrote:
> looks a bit scary. Maybe:
> 
> const std::string resource_id = current_dir->FindChild(components[i]);
> if (resource_id.empty())
>   return NULL;
> GDataEntry* entry = GetEntryByResourceId(resource_id);

Done. I've added a DCHECK for entry instead of a runtime check.

http://codereview.chromium.org/10855243/diff/6004/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10855243/diff/6004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.cc:255: iter !=
dir->child_files_.end(); ++iter) {
On 2012/08/20 16:40:08, satorux1 wrote:
> maybe add:
> 
>   const std::string& resource_id = iter->second;
> 
> to make it clear iter->second is a resource id.

I've added a method TakeOverEntry instead to avoid a chunk of cut 'n paste code.

http://codereview.chromium.org/10855243/diff/6004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.cc:282: const std::string&
GDataDirectory::FindChild(
On 2012/08/20 16:40:08, satorux1 wrote:
> I'd just return const std::string rather than a reference. Less efficient, but
> less tricky.

Done.

http://codereview.chromium.org/10855243/diff/6004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.cc:320: GDataEntry* child =
directory_service_->GetEntryByResourceId(iter->second);
On 2012/08/20 16:40:08, satorux1 wrote:
> DCHECK(child); ?

Done.

http://codereview.chromium.org/10855243/diff/6004/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10855243/diff/6004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.h:257: // Find a child's resource_id
by its name.
On 2012/08/20 16:40:08, satorux1 wrote:
> Please document what's returned if the child is not found. Empty string?

Done.

[satorux1, 2012-08-20 21:36:36]

LGTM

[achuithb, 2012-08-20 21:40:24]

On 2012/08/20 21:36:36, satorux1 wrote:
> LGTM

Thank you!