gdata: Move GDataFileSystem::AddUploadedFile() call out of uploader and into GDataDownloadObserver.

gdata: Move GDataFileSystem::AddUploadedFile() call out of uploader and into GDataDownloadObserver class.

The async GDataFileSystem::AddUploadedFile() call causes the file to be moved to the gdata cache. This must be invoked after DownloadObserver receives the COMPLETE status notification.

BUG=136435
TEST=unit_tests, lumpy

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146046

[hshi1, 2012-07-09 21:46:49]

Explanation why I'm doing this:

(1) This removes the GDataFileSystem::AddUploadedFile call from GDataUploader,
thus partially paves the road to solve the inter-dependency between
GDataFileSystem and GDataUploader. After this CL, the only remaining dependency
on GDataFileSystem in GDataUploader is the ReadDirectoryByPath() call.

(2) The old code is actually wrong. I have debugged several scenarios of
download+upload and found that the GDataUploader::MoveFileToCache() never
actually does anything. The reason behind this is:
GDataUploader::MoveFileToCache() will only call AddUploadedFile() upon
completion of download, and upload_file_info->entry() is valid. However,
upload_file_info->entry() is only set to a valid pointer in
GDataUploader::OnResumeUploadResponseReceived, where the uploader removes the
|upload_id| from its map and invokes the completion_callback. Therefore, the
most natural place to call GDataFileSystem::AddUploadedFile() should be in the
completion callback.

(3) We already have a case where AddUploadedFile() is invoked in the
completion_callback. That is, for the case of transferring file from local disk
to remote file system, we call AddUploadedFile in
GDataFileSystem::OnTransferCompleted().

[satorux1, 2012-07-09 22:07:53]

On 2012/07/09 21:46:49, hshi1 wrote:
> Explanation why I'm doing this:
> 
> (1) This removes the GDataFileSystem::AddUploadedFile call from GDataUploader,
> thus partially paves the road to solve the inter-dependency between
> GDataFileSystem and GDataUploader. After this CL, the only remaining
dependency
> on GDataFileSystem in GDataUploader is the ReadDirectoryByPath() call.
> 
> (2) The old code is actually wrong. I have debugged several scenarios of
> download+upload and found that the GDataUploader::MoveFileToCache() never
> actually does anything. The reason behind this is:
> GDataUploader::MoveFileToCache() will only call AddUploadedFile() upon
> completion of download, and upload_file_info->entry() is valid. However,
> upload_file_info->entry() is only set to a valid pointer in
> GDataUploader::OnResumeUploadResponseReceived, where the uploader removes the
> |upload_id| from its map and invokes the completion_callback. Therefore, the
> most natural place to call GDataFileSystem::AddUploadedFile() should be in the
> completion callback.

wow, this is subtle... Was this causing a real bug? If so, please file a bug,
and add it to BUG= line.

> 
> (3) We already have a case where AddUploadedFile() is invoked in the
> completion_callback. That is, for the case of transferring file from local
disk
> to remote file system, we call AddUploadedFile in
> GDataFileSystem::OnTransferCompleted().

[satorux1, 2012-07-09 22:08:16]

LGTM, but please wait for achuith.bhandarkar's comments

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

http://codereview.chromium.org/10759007/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:485:
upload_file_info_ptr->gdata_path.DirName(),
nit: indentation is off? please align parameters vertically

file_system_->AddUploadedFile(
    UPLOAD_NEW_FILE,
    ...);

[hshi1, 2012-07-09 22:11:48]

Patch Set #2 fixes the indentation and a typo (the action should be MOVE, not
COPY).

This is actually not causing any noticeable bugs. What happens is that for the
download->upload case, the file is never added to the gdata cache, so after
completion you won't immediately see the file on gdrive. But if you refresh the
file manager or wait for a minute or so it will be updated.

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

http://codereview.chromium.org/10759007/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:485:
upload_file_info_ptr->gdata_path.DirName(),
On 2012/07/09 22:08:16, satorux1 wrote:
> nit: indentation is off? please align parameters vertically
> 
> file_system_->AddUploadedFile(
>     UPLOAD_NEW_FILE,
>     ...);

Done.

[hshi1, 2012-07-09 22:13:15]

Patch Set #2 fixes the indentation and a typo (the action should be MOVE, not
COPY).

This is actually not causing any noticeable bugs. What happens is that for the
download->upload case, the file is never added to the gdata cache, so after
completion you won't immediately see the file on gdrive. But if you refresh the
file manager or wait for a minute or so it will be updated.

[achuithb, 2012-07-09 22:28:44]

On 2012/07/09 22:08:16, satorux1 wrote:
> LGTM, but please wait for achuith.bhandarkar's comments
> 
>
http://codereview.chromium.org/10759007/diff/1/chrome/browser/chromeos/gdata/...
> File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):
> 
>
http://codereview.chromium.org/10759007/diff/1/chrome/browser/chromeos/gdata/...
> chrome/browser/chromeos/gdata/gdata_download_observer.cc:485:
> upload_file_info_ptr->gdata_path.DirName(),
> nit: indentation is off? please align parameters vertically
> 
> file_system_->AddUploadedFile(
>     UPLOAD_NEW_FILE,
>     ...);

+1 to Satoru-san's comment about opening a new bug and describing the behavior.

So the file was not moved to the cache directory after the upload was finished?
I can see how that could fall through the cracks. It used to work at one point,
but with no test coverage, it's easy for this to regress and not be noticed. 

lgtm otherwise.

[achuithb, 2012-07-09 22:29:32]

On 2012/07/09 22:13:15, hshi1 wrote:
> Patch Set #2 fixes the indentation and a typo (the action should be MOVE, not
> COPY).
> 
> This is actually not causing any noticeable bugs. What happens is that for the
> download->upload case, the file is never added to the gdata cache, so after
> completion you won't immediately see the file on gdrive. But if you refresh
the
> file manager or wait for a minute or so it will be updated.

how does it get updated? Does the file get downloaded again?

[achuithb, 2012-07-09 22:30:12]

On 2012/07/09 22:13:15, hshi1 wrote:
> Patch Set #2 fixes the indentation and a typo (the action should be MOVE, not
> COPY).
> 
> This is actually not causing any noticeable bugs. What happens is that for the
> download->upload case, the file is never added to the gdata cache, so after
> completion you won't immediately see the file on gdrive. But if you refresh
the
> file manager or wait for a minute or so it will be updated.

How does it get updated? Does the file get downloaded again?

[achuithb, 2012-07-09 22:30:53]

On 2012/07/09 22:13:15, hshi1 wrote:
> Patch Set #2 fixes the indentation and a typo (the action should be MOVE, not
> COPY).
> 
> This is actually not causing any noticeable bugs. What happens is that for the
> download->upload case, the file is never added to the gdata cache, so after
> completion you won't immediately see the file on gdrive. But if you refresh
the
> file manager or wait for a minute or so it will be updated.

how does it get updated? Does the file get downloaded again?

[hshi1, 2012-07-09 22:32:50]

Filed bug 136435 which describes the problem.

>> how does it get updated? Does the file get downloaded again?

Yes exactly the file gets downloaded again. It is a waste of bandwidth and
frustrating when the file is large.

[hshi1, 2012-07-10 00:18:54]

Achuith can you review Patch #3 please? It takes care of a timing sensitive
issue -- namely, CompleteDownload() renames the downloaded file from
intermediate path (*.crdownload) to its final name, while AddUploadedFile()
moves the file at the same time.

After Patch #3, we always wait for CompleteDownload() to rename the path, then
use GetTargetFilePath() as the final file content path for the cache MOVE
operation.

[achuithb, 2012-07-10 16:57:34]

Adding Asanka for review.

[achuithb, 2012-07-10 16:58:43]

http://codereview.chromium.org/10759007/diff/4004/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://codereview.chromium.org/10759007/diff/4004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:498: const FilePath&
file_content_path = download_item->GetTargetFilePath();
Are we sure that the file has been renamed at this point? Otherwise
AddUploadedFile will fail, right? The file rename actually happens on the FILE
thread so this can actually be racy.

[hshi1, 2012-07-10 17:32:34]

Good point! Unfortunately it looks like neither Patch #2 nor #3 is safe.

We basically have two (async, posted) tasks running at the same time:
(1) upon download completion, rename current name (*.crdownload) to the final
"target" name (without the .crdownload suffix);
(2) move the file from download path to the gdata cache path
(tmp/file:<resource_id>);

I think we should really get rid of the rename step in (1) because it is totally
unnecessary; for uploads we should not need that file to exist in its final
target path.

On 2012/07/10 16:58:43, achuith.bhandarkar wrote:
>
http://codereview.chromium.org/10759007/diff/4004/chrome/browser/chromeos/gda...
> File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):
> 
>
http://codereview.chromium.org/10759007/diff/4004/chrome/browser/chromeos/gda...
> chrome/browser/chromeos/gdata/gdata_download_observer.cc:498: const FilePath&
> file_content_path = download_item->GetTargetFilePath();
> Are we sure that the file has been renamed at this point? Otherwise
> AddUploadedFile will fail, right? The file rename actually happens on the FILE
> thread so this can actually be racy.

[asanka, 2012-07-10 17:43:25]

On 2012/07/10 17:32:34, hshi1 wrote:
> Good point! Unfortunately it looks like neither Patch #2 nor #3 is safe.
> 
> We basically have two (async, posted) tasks running at the same time:
> (1) upon download completion, rename current name (*.crdownload) to the final
> "target" name (without the .crdownload suffix);
> (2) move the file from download path to the gdata cache path
> (tmp/file:<resource_id>);
> 
> I think we should really get rid of the rename step in (1) because it is
totally
> unnecessary; for uploads we should not need that file to exist in its final
> target path.

The downloads system assumes ownership of the downloaded file all the way until
the COMPLETED notification is sent out. So ideally we shouldn't move the file
out from under it until that happens.

> 
> On 2012/07/10 16:58:43, achuith.bhandarkar wrote:
> >
>
http://codereview.chromium.org/10759007/diff/4004/chrome/browser/chromeos/gda...
> > File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):
> > 
> >
>
http://codereview.chromium.org/10759007/diff/4004/chrome/browser/chromeos/gda...
> > chrome/browser/chromeos/gdata/gdata_download_observer.cc:498: const
FilePath&
> > file_content_path = download_item->GetTargetFilePath();
> > Are we sure that the file has been renamed at this point? Otherwise
> > AddUploadedFile will fail, right? The file rename actually happens on the
FILE
> > thread so this can actually be racy.

[achuithb, 2012-07-10 18:45:43]

On 2012/07/10 17:32:34, hshi1 wrote:
> Good point! Unfortunately it looks like neither Patch #2 nor #3 is safe.
> 
> We basically have two (async, posted) tasks running at the same time:
> (1) upon download completion, rename current name (*.crdownload) to the final
> "target" name (without the .crdownload suffix);
> (2) move the file from download path to the gdata cache path
> (tmp/file:<resource_id>);
> 
> I think we should really get rid of the rename step in (1) because it is
totally
> unnecessary; for uploads we should not need that file to exist in its final
> target path.

We've bolted on gdata uploads, so the downloads system is unaware that it
doesn't need to do the rename. Randy is working on eliminating this step. 

We should probably do the minimum work necessary to fix the code.

I think the original strategy (move the file to cache after the download is
complete by doing the download->IsComplete() test) was sound. We should figure
out why that's broken now.

[hshi1, 2012-07-10 18:55:23]

I agree that it is only safe to move file to gdata cache when
GDataDownloadObserver receives the COMPLETE notification. The old strategy was
working, but it got broken by an earlier change of mine
(https://chromiumcodereview.appspot.com/10540132) that causes the
|upload_file_info| to be removed in OnResumeUploadResponseReceived().

Conceptually, |upload_file_info|'s completion_callback is supposed to be invoked
when upload is indeed complete. This is the case for Local-to-Remote file
transfer where it is safe to move file to gdata cache (see
GDataFileSystem::OnTransferCompleted), but for download-upload path, the content
file path is still owned by the download system until observer receives
COMPLETE, thus strictly speaking when completion_callback is invoked we're not
really done yet.

I will have a CL uploaded in a minute.

[hshi1, 2012-07-10 19:02:12]

Achuith - please review Patch #4:

(1) In completion_callback, we cannot move the file to gdata cache immediately
due to the async renaming of downloaded file. Therefore we create an external
data struct with the necessary information needed to perform the move later.

(2) Upon receiving COMPLETE notification, call MoveFileToCache to perform the
move.

(3) We cannot remove the pending download item until the move is complete,
therefore make the removal wait for completion of the move.

(4) Move the DVLOG(1) << "Number of pending downloads=" out of
OnDownloadUpdated() and into Add/RemovePendingDownload because
RemovePendingDownload() may be called asynchronously for the COMPLETE
notification.

[hshi1, 2012-07-10 19:13:44]

Minor polish in Patch #5.

[hshi1, 2012-07-10 20:20:32]

Made another revision in Patch #7. Sometimes the observer receives multiple
COMPLETE notifications, thus we need to take care to ensure that the file is
only moved once (upon the first COMPLETE notification) to gdata cache; and that
the RemovePendingDownload is only called after completion of that move.

[hshi1, 2012-07-10 21:08:33]

Patch #8: for the case of "Save As" (to MHTML), it looks like the Download
system does not need to rename, and CompleteDownload() notifies observers of
COMPLETE immediately. Thus it is necessary to save the external struct first,
then call CompleteDownload().

[asanka, 2012-07-10 21:11:59]

http://codereview.chromium.org/10759007/diff/27006/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://codereview.chromium.org/10759007/diff/27006/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:538:
download_item->GetTargetFilePath(),
This doesn't affect GData uploads currently, but it is possible for the target
path to change during the completion stage (i.e. if there's a name collision for
some types of downloads). So if you want to read the downloaded file, you want
to look at GetTargetFilePath() when the download item is in the COMPLETE state.

In other words, you could just call GetTargetFilePath() directly in
MoveDownloadedFileToCache().

http://codereview.chromium.org/10759007/diff/27006/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:551: // The observer
may receive multiple COMPLETE notifications, so only
COMPLETE is a state. So you may receive multiple OnDownloadUpdated()
notifications while it is in this state. 

If possible, why not call RemovePendingDownload() after calling
AddUploadedFile()? That way you don't have to worry about multiple
notifications.

[hshi1, 2012-07-10 21:32:55]

Thanks asanka for the suggestions. PTAL at patch #9.

http://codereview.chromium.org/10759007/diff/27006/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://codereview.chromium.org/10759007/diff/27006/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:538:
download_item->GetTargetFilePath(),
I see, I've moved the GetTargetFilePath() call to inside
MoveDownloadedFileToCache().

On 2012/07/10 21:11:59, asanka wrote:
> This doesn't affect GData uploads currently, but it is possible for the target
> path to change during the completion stage (i.e. if there's a name collision
for
> some types of downloads). So if you want to read the downloaded file, you want
> to look at GetTargetFilePath() when the download item is in the COMPLETE
state.
> 
> In other words, you could just call GetTargetFilePath() directly in
> MoveDownloadedFileToCache().

http://codereview.chromium.org/10759007/diff/27006/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:551: // The observer
may receive multiple COMPLETE notifications, so only
Good point! Done.

On 2012/07/10 21:11:59, asanka wrote:
> COMPLETE is a state. So you may receive multiple OnDownloadUpdated()
> notifications while it is in this state. 
> 
> If possible, why not call RemovePendingDownload() after calling
> AddUploadedFile()? That way you don't have to worry about multiple
> notifications.
>

[hshi1, 2012-07-10 22:43:35]

Forgot to reply to an earlier comment by Achuith. This should be addressed by
the Patch #9. This has so far been manually tested, on lumpy, saving data/image
files and "save as" of mhtml fiels to gdrive (with or without default download
dir) and it appears working ok.

After the last patch, we've restored the old logic where AddUploadedFile() is
only called at COMPLETE notification, but I believe the new code is cleaner than
the old code before my refactor broke this.

http://codereview.chromium.org/10759007/diff/4004/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://codereview.chromium.org/10759007/diff/4004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:498: const FilePath&
file_content_path = download_item->GetTargetFilePath();
You're right, the renaming is asynchronous. Observer must wait for COMPLETE
notification then call AddUploadedFile.

On 2012/07/10 16:58:44, achuith.bhandarkar wrote:
> Are we sure that the file has been renamed at this point? Otherwise
> AddUploadedFile will fail, right? The file rename actually happens on the FILE
> thread so this can actually be racy.

[achuithb, 2012-07-10 23:04:35]

http://codereview.chromium.org/10759007/diff/19013/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://codereview.chromium.org/10759007/diff/19013/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:68: class
GDataCacheExternalData : public DownloadItem::ExternalData {
Why not add entry_ to UploadingExternalData? It's a bit hard to keep track of
the various ExternalData classes and when they get added and removed.

http://codereview.chromium.org/10759007/diff/19013/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:78: DocumentEntry*
entry() const { return entry_.get(); }
Is this used?

http://codereview.chromium.org/10759007/diff/19013/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_download_observer.h (right):

http://codereview.chromium.org/10759007/diff/19013/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_download_observer.h:126: void
MoveDownloadedFileToCache(content::DownloadItem* download);
Do you like this name? I feel like 'Downloaded' is redundant, and Cache should
be qualified, so I'd prefer MoveFileToGDataCache.

[hshi1, 2012-07-10 23:46:57]

PTAL Patch #10, thanks!

http://codereview.chromium.org/10759007/diff/19013/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://codereview.chromium.org/10759007/diff/19013/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:68: class
GDataCacheExternalData : public DownloadItem::ExternalData {
Sounds good, done.
On 2012/07/10 23:04:35, achuith.bhandarkar wrote:
> Why not add entry_ to UploadingExternalData? It's a bit hard to keep track of
> the various ExternalData classes and when they get added and removed.

http://codereview.chromium.org/10759007/diff/19013/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:78: DocumentEntry*
entry() const { return entry_.get(); }
On 2012/07/10 23:04:35, achuith.bhandarkar wrote:
> Is this used?

Yes it is used in a DCHECK in MoveFileToGDataCache. I see that it may be
redundant, so I've removed it.

http://codereview.chromium.org/10759007/diff/19013/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_download_observer.h (right):

http://codereview.chromium.org/10759007/diff/19013/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_download_observer.h:126: void
MoveDownloadedFileToCache(content::DownloadItem* download);
On 2012/07/10 23:04:35, achuith.bhandarkar wrote:
> Do you like this name? I feel like 'Downloaded' is redundant, and Cache should
> be qualified, so I'd prefer MoveFileToGDataCache.

Makes sense, done.

[achuithb, 2012-07-11 00:06:11]

http://codereview.chromium.org/10759007/diff/28028/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://codereview.chromium.org/10759007/diff/28028/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:50: void
set_entry(DocumentEntry* entry) { entry_.reset(entry); }
Why not
void set_entry(scoped_ptr<DocumentEntry> entry) { entry_ = entry.Pass(); }
I think that would make the transfer of ownership clearer rather than releasing
at caller site and recapturing here?

http://codereview.chromium.org/10759007/diff/28028/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:505: // destruction of
scoped pointer |upload_file_info|.
How about this:
Take ownership of DocumentEntry from UploadFileInfo. This is used by
GDataFileSystem::AddUploadedFile to add the entry to GDataCache after the upload
completes.

[hshi1, 2012-07-11 00:13:48]

PTAL @ Patch #11, thanks!

http://codereview.chromium.org/10759007/diff/28028/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://codereview.chromium.org/10759007/diff/28028/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:50: void
set_entry(DocumentEntry* entry) { entry_.reset(entry); }
On 2012/07/11 00:06:11, achuith.bhandarkar wrote:
> Why not
> void set_entry(scoped_ptr<DocumentEntry> entry) { entry_ = entry.Pass(); }
> I think that would make the transfer of ownership clearer rather than
releasing
> at caller site and recapturing here?

Done.

http://codereview.chromium.org/10759007/diff/28028/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:505: // destruction of
scoped pointer |upload_file_info|.
On 2012/07/11 00:06:11, achuith.bhandarkar wrote:
> How about this:
> Take ownership of DocumentEntry from UploadFileInfo. This is used by
> GDataFileSystem::AddUploadedFile to add the entry to GDataCache after the
upload
> completes.

Done.

[achuithb, 2012-07-11 00:16:19]

lgtm.

Not entirely sure if Asanka would like to review this as well. Perhaps you can
ping him?

[hshi1, 2012-07-11 00:18:43]

@asanka, do you have more comments?

I have addressed your earlier comments in diff Patch #6 vs Patch #7.

Thanks.

On 2012/07/11 00:16:19, achuith.bhandarkar wrote:
> lgtm.
> 
> Not entirely sure if Asanka would like to review this as well. Perhaps you can
> ping him?

[commit-bot: I haz the power, 2012-07-11 01:02:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/10759007/21014

[commit-bot: I haz the power, 2012-07-11 03:31:25]

Change committed as 146046

[asanka, 2012-07-11 15:56:52]

Sorry I had to run yesterday. Just a couple of post-commit comments.

https://chromiumcodereview.appspot.com/10759007/diff/21014/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

https://chromiumcodereview.appspot.com/10759007/diff/21014/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:523: // completion of
the AddUploadedFile() call below.
Is it? It looks like |entry| will leak.

https://chromiumcodereview.appspot.com/10759007/diff/21014/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_download_observer.h (right):

https://chromiumcodereview.appspot.com/10759007/diff/21014/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_download_observer.h:119: //
MaybeCompleteDownload() method on the DownloadItem to allow it to complete.
Nit: This doesn't call MaybeCompleteDownload() anymore. Instead it runs a
completion callback to allow the download item to complete.

[hshi1, 2012-07-11 16:17:39]

https://chromiumcodereview.appspot.com/10759007/diff/21014/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

https://chromiumcodereview.appspot.com/10759007/diff/21014/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:523: // completion of
the AddUploadedFile() call below.
On 2012/07/11 15:56:52, asanka wrote:
> Is it? It looks like |entry| will leak.

It won't leak. Upon completion of the AddUploadedFile() call below, the callback
specified at line 537 will be invoked to release the |entry|:
base::Bind(&base::DeletePointer<DocumentEntry>, entry)

[asanka, 2012-07-11 16:31:37]

https://chromiumcodereview.appspot.com/10759007/diff/21014/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

https://chromiumcodereview.appspot.com/10759007/diff/21014/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:523: // completion of
the AddUploadedFile() call below.
On 2012/07/11 16:17:40, hshi1 wrote:
> On 2012/07/11 15:56:52, asanka wrote:
> > Is it? It looks like |entry| will leak.
> 
> It won't leak. Upon completion of the AddUploadedFile() call below, the
callback
> specified at line 537 will be invoked to release the |entry|:
> base::Bind(&base::DeletePointer<DocumentEntry>, entry)

Ah. Sorry. You are right.