gdata: Add GetEntryInfoByPath() and ReadDirectoryByPath() to GDataDirectoryService

gdata: Add GetEntryInfoByPath() and ReadDirectoryByPath() to GDataDirectoryService

These are asynchronous functions that'll replace FindEntryByPathSync().

BUG=137160
TEST=added unit tests; no behavior change. these functions are unused yet.

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

[achuithb, 2012-08-07 20:46:06]

https://chromiumcodereview.appspot.com/10837148/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

https://chromiumcodereview.appspot.com/10837148/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/gdata/gdata_files.cc:697: if (!callback.is_null()) {
Should this be a DCHECK? You should probably bail at the top of the function if
callback is null.

https://chromiumcodereview.appspot.com/10837148/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/gdata/gdata_files.cc:713: if (entry &&
entry->AsGDataDirectory()) {
I feel like this would be easier to read if you used nested if statements.
if (entry) {
  GDataDirectory* directory = entry->AsGDataDirectory();
  if (directory) {
    ...
  } else {
    error = GDATA_FILE_ERROR_NOT_A_DIRECTORY;
  }
}

But I guess it's fine as it is.

https://chromiumcodereview.appspot.com/10837148/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/gdata/gdata_files.cc:720: static_cast<const
GDataEntry*>(iter->second)->ToProtoFull(&proto);
Please add a comment as to why this static_cast is necessary.

https://chromiumcodereview.appspot.com/10837148/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/gdata/gdata_files.cc:734: error =
GDATA_FILE_ERROR_NOT_FOUND;
Don't really need this but I suppose if you want to be explicit.

https://chromiumcodereview.appspot.com/10837148/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/gdata/gdata_files.cc:737: if (!callback.is_null()) {
Same as above. Don't think there's a point to this function if callback is null.

https://chromiumcodereview.appspot.com/10837148/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

https://chromiumcodereview.appspot.com/10837148/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/gdata/gdata_files.h:84: // If |error| is not
PLATFORM_FILE_OK, |entries| is set to NULL.
Isn't the error GDATA_FILE_OK?

https://chromiumcodereview.appspot.com/10837148/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/gdata/gdata_files.h:420: const GetEntryInfoCallback&
callback);
indentation.

[satorux1, 2012-08-07 21:13:20]

http://codereview.chromium.org/10837148/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10837148/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:697: if (!callback.is_null()) {
On 2012/08/07 20:46:07, achuith.bhandarkar wrote:
> Should this be a DCHECK? You should probably bail at the top of the function
if
> callback is null.

Made it a DCHECK. There is no point of calling this function without a callback.

http://codereview.chromium.org/10837148/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:713: if (entry &&
entry->AsGDataDirectory()) {
On 2012/08/07 20:46:07, achuith.bhandarkar wrote:
> I feel like this would be easier to read if you used nested if statements.
> if (entry) {
>   GDataDirectory* directory = entry->AsGDataDirectory();
>   if (directory) {
>     ...
>   } else {
>     error = GDATA_FILE_ERROR_NOT_A_DIRECTORY;
>   }
> }
> 
> But I guess it's fine as it is.

Wanted to save one indentation level, so keep it as is.

http://codereview.chromium.org/10837148/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:720: static_cast<const
GDataEntry*>(iter->second)->ToProtoFull(&proto);
On 2012/08/07 20:46:07, achuith.bhandarkar wrote:
> Please add a comment as to why this static_cast is necessary.

Done. Turned out static_cast was not needed here. To be a good citizen, made it
a function, and removed the duplicate code elsewhere :)

http://codereview.chromium.org/10837148/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:734: error =
GDATA_FILE_ERROR_NOT_FOUND;
On 2012/08/07 20:46:07, achuith.bhandarkar wrote:
> Don't really need this but I suppose if you want to be explicit.

Yes

http://codereview.chromium.org/10837148/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:737: if (!callback.is_null()) {
On 2012/08/07 20:46:07, achuith.bhandarkar wrote:
> Same as above. Don't think there's a point to this function if callback is
null.

Done.

http://codereview.chromium.org/10837148/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10837148/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.h:84: // If |error| is not
PLATFORM_FILE_OK, |entries| is set to NULL.
On 2012/08/07 20:46:07, achuith.bhandarkar wrote:
> Isn't the error GDATA_FILE_OK?

Done.

http://codereview.chromium.org/10837148/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.h:420: const GetEntryInfoCallback&
callback);
On 2012/08/07 20:46:07, achuith.bhandarkar wrote:
> indentation.

Done.

[achuithb, 2012-08-07 21:39:20]

lgtm

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

http://codereview.chromium.org/10837148/diff/6003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.cc:714: GDataDirectory* directory =
entry->AsGDataDirectory();
nit: Is this temporary necessary?

[satorux1, 2012-08-07 22:57:05]

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

http://codereview.chromium.org/10837148/diff/6003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.cc:714: GDataDirectory* directory =
entry->AsGDataDirectory();
On 2012/08/07 21:39:20, achuith.bhandarkar wrote:
> nit: Is this temporary necessary?

Good catch. no longer needed.