GData downloads cleanup

GData downloads cleanup.

* Fix a leak in GDataFileSystem::OnTransferCompleted. 
* UploadCompletionCallback's returns an UploadFileInfo* (to enable it to be deleted) instead of DocumentEntry* 
* Split GDataUploader::RemovePendingUpload into UploadFailed and DeleteUpload. Make DeleteUpload public for use by GDataFileSystem::OnTransferCompleted.
* GDataUploader::UploadFile takes a scoped_ptr to make the transfer of ownership clearer.
* Rename GDataUploader::UploadComplete to GDataUploader::MoveFileToCache.
* Rename GDataDownloadObserver::temp_download_path_ to gdata_tmp_download_path_ for consistency with code elsewhere.
* Rearrange args in GDataDownloadObserver::Initialize
* Some other renames
* Get rid of gdata:: in gdata_file_system.cc
* pragma once in all header files.

BUG=NONE
TEST=NONE
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=129595

[achuithb, 2012-03-27 23:08:55]

Please take a look, Ben and Asanka!

[Ben Chan, 2012-03-28 05:29:36]

http://chromiumcodereview.appspot.com/9844006/diff/1023/chrome/browser/chrome...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://chromiumcodereview.appspot.com/9844006/diff/1023/chrome/browser/chrome...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:95: gdata_uploader_ =
gdata_uploader;
nit: Seems like |gdata_uploader_| is not expected to be NULL.  perhaps adding
DCHECK(gdata_uploader_)

http://chromiumcodereview.appspot.com/9844006/diff/1023/chrome/browser/chrome...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://chromiumcodereview.appspot.com/9844006/diff/1023/chrome/browser/chrome...
chrome/browser/chromeos/gdata/gdata_file_system.cc:754: UploadFileInfo*
upload_file_info) {
nit: DCHECK(upload_file_info)

http://chromiumcodereview.appspot.com/9844006/diff/1023/chrome/browser/chrome...
File chrome/browser/chromeos/gdata/gdata_uploader.cc (right):

http://chromiumcodereview.appspot.com/9844006/diff/1023/chrome/browser/chrome...
chrome/browser/chromeos/gdata/gdata_uploader.cc:359: NOTREACHED();
NOTREACHED() ???

[achuithb, 2012-03-28 09:14:04]

Fixed. PTAL.

http://chromiumcodereview.appspot.com/9844006/diff/1023/chrome/browser/chrome...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://chromiumcodereview.appspot.com/9844006/diff/1023/chrome/browser/chrome...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:95: gdata_uploader_ =
gdata_uploader;
On 2012/03/28 05:29:36, Ben Chan wrote:
> nit: Seems like |gdata_uploader_| is not expected to be NULL.  perhaps adding
> DCHECK(gdata_uploader_)

Done.

http://chromiumcodereview.appspot.com/9844006/diff/1023/chrome/browser/chrome...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://chromiumcodereview.appspot.com/9844006/diff/1023/chrome/browser/chrome...
chrome/browser/chromeos/gdata/gdata_file_system.cc:754: UploadFileInfo*
upload_file_info) {
On 2012/03/28 05:29:36, Ben Chan wrote:
> nit: DCHECK(upload_file_info)

Done.

http://chromiumcodereview.appspot.com/9844006/diff/1023/chrome/browser/chrome...
File chrome/browser/chromeos/gdata/gdata_uploader.cc (right):

http://chromiumcodereview.appspot.com/9844006/diff/1023/chrome/browser/chrome...
chrome/browser/chromeos/gdata/gdata_uploader.cc:359: NOTREACHED();
On 2012/03/28 05:29:36, Ben Chan wrote:
> NOTREACHED() ???

You're right - bad idea. Replaced with LOG(ERROR).

[asanka, 2012-03-28 22:02:33]

LGTM with nits.

Although some files have changed on trunk.

http://codereview.chromium.org/9844006/diff/7006/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/9844006/diff/7006/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:721: UploadFileInfo*
upload_file_info) {
Nit: Might be clearer if this took a scoped_ptr<> as well. That way it would be
clear from the binding site in TransferRegularFile() that ownership is being
transferred.

http://codereview.chromium.org/9844006/diff/7006/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_uploader.cc (right):

http://codereview.chromium.org/9844006/diff/7006/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_uploader.cc:303: upload_file_info);
Minor Nit: Perhaps have a way to assert that we only invoke the callback once?

[achuithb, 2012-03-28 23:27:11]

Thanks for the reviews, Ben and Asanka!

http://codereview.chromium.org/9844006/diff/7006/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/9844006/diff/7006/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:721: UploadFileInfo*
upload_file_info) {
On 2012/03/28 22:02:33, asanka wrote:
> Nit: Might be clearer if this took a scoped_ptr<> as well. That way it would
be
> clear from the binding site in TransferRegularFile() that ownership is being
> transferred.

scoped_ptr can't be used with PostTaskAndReply properly, or couldn't be.

https://groups.google.com/a/chromium.org/group/chromium-dev/msg/ddd7942f93b8ed4b
Not sure if this is fixed now...

http://codereview.chromium.org/9844006/diff/7006/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_uploader.cc (right):

http://codereview.chromium.org/9844006/diff/7006/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_uploader.cc:303: upload_file_info);
On 2012/03/28 22:02:33, asanka wrote:
> Minor Nit: Perhaps have a way to assert that we only invoke the callback once?

I've put the Reset back in.