Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10540132:
gdata: Convert GDataFileSystem::AddUploadFile() to asynchronous. (Closed)
|
Created:
8 years, 6 months ago by hshi1 Modified:
8 years, 6 months ago CC:
chromium-reviews, achuith+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptiongdata: Convert GDataFileSystem::AddUploadFile() to asynchronous.
This is part 3 of converting public synchronous functions in GDataFileSystem to asynchronous.
Convert GDataFileSystem::AddUploadFile() to an asynchronous function. Upon completion it will invoke the callback on the UI thread.
Pass scoped_ptr<UploadFileInfo> instead of a UploadFileInfo* pointer to GDataUploader::UploadFailed, UploadCompletedCallback callbacks, and to the callback function for GDataFileSystem::AddUploadedFile, to signify transfer of ownership.
Decouple the UploadFileInfo entry removal in UploadFileInfoMap |pending_uploads_| from the deletion of the UploadFileInfo object.
Rename GDataUploader::DeleteUpload() to GDataUploader::RemoveUpload() and make it private. This function only removes entry from |pending_uploads_| but no longer deletes the UploadFileInfo object.
Defer deletion of UploadFileInfo object until the asynchronous function call is complete. Object is automatically deleted when the scoped pointer is no longer passed on.
BUG=132389
TEST=unit_tests, browser_tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142515
Patch Set 1 #Patch Set 2 : Decouple the removal of UploadFileInfo from UploadFileInfoMap and object deletion #Patch Set 3 : Rebase svn:trunk/src@142185 and resolve conflicts. #
Total comments: 6
Patch Set 4 : Address Satoru's comments. #
Total comments: 3
Patch Set 5 : Change RemoveUpload to use upload_id instead of UploadFileInfo*. #Patch Set 6 : Get rid of the lock as suggested by Satoru. #
Total comments: 19
Patch Set 7 : Use scoped_ptr for cleaner ownership transfers and automatic deletion. #
Total comments: 4
Patch Set 8 : Call 'RemoveUpload' before the async function to ensure the map does not have deleted pointers. #Patch Set 9 : Rebase svn:trunk/src@142413 and resolve merge conflicts with Satoru's change. #Patch Set 10 : Rebase svn:trunk/src@142436 and resolve merge conflicts with Satoru's and Toni's changes this morni… #Patch Set 11 : Fix a possible NULL pointer due to base::Passed. #
Total comments: 4
Patch Set 12 : Add comments to explain why Patch Set #11 is necesary. #Messages
Total messages: 40 (0 generated)
Achuith: this CL is the 3rd and last part of the planned sync-to-async conversion of public functions in GDataFileSystem. The CL at its current form compiles and passes tests but in my opinion is fairly awkward, with long chains of callback passing along the path from GDatDownloadObserver::OnDownloadUpdated() to GDataUploader::UpdateUpload() to GDataFileSystem::AddUploadFile(). Any suggestions or comments are welcome. Thanks!
My apologies, this CL is causing problems on real devices (when attempting to "save as" a file to gdrive). I'm taking this CL offline to fix the problem first.
On 2012/06/12 23:40:27, hshi1 wrote: > My apologies, this CL is causing problems on real devices (when attempting to > "save as" a file to gdrive). I'm taking this CL offline to fix the problem > first. k, ping me when it's ready.
This looks tricky. The async AddUploadFile() is re-entrant. Suppose the DownloadObserver receives an IN_PROGRESS update immediately followed by a COMPLETE, both would call UploadDownloadItem -> UpdateUpload -> GDataUploader::MoveFileToCache -> GDataFileSystem::AddUploadedFile. Problem is, GDataUploader::MoveFileToCache deletes the upload_file_info*.
Hi Achuith, It appears that the current Patch Set, which is a simple mechanical conversion of AddUploadFile() from synchronous to asynchronous, is not safe. Fundamentally the problem is that GDataDownloadObserver::OnDownloadUpdated() implements the virtual function defined in base class content::DownloadItem which is not an async interface. Functions such as GDataDownloadObserver::ShouldUpload, GDataDownloadObserver::UpdateUpload and GDataUploader::MoveFileToCache all assume they are not re-entrant. I wonder if I need to use a base::Autolock object in GDataUploader to ensure that calls are sequential, but this sort of defeats the purpose of this refactoring work. What do you think? Thanks, Haixia On Tue, Jun 12, 2012 at 5:05 PM, <achuith@chromium.org> wrote: > On 2012/06/12 23:40:27, hshi1 wrote: > >> My apologies, this CL is causing problems on real devices (when >> attempting to >> "save as" a file to gdrive). I'm taking this CL offline to fix the problem >> first. >> > > k, ping me when it's ready. > > http://codereview.chromium.**org/10540132/<http://codereview.chromium.org/105... >
Hi Satoru and Achuith, can you please review Patch Set #2. In order to make AddUploadFile asynchronous, I believe it is necessary to serialize access to GDataUploader's UploadFileInfoMap member |pending_uploads_|, and decouple the removal of UploadFileInfo objects from the map from the actual deletion of the UploadFileInfo object (the latter cannot occur until the async AddUploadFile no longer needs to use the object, thus should be done in a callback invoked upon completion of AddUploadFile).
Sorry please ignore Patch Set #2 and review Patch Set #3 instead. This is rebased at svn@142185 and resolved merge conflicts. It looks like there has been some refactoring work that moved some stuff to the GDataCache class.
http://codereview.chromium.org/10540132/diff/12004/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10540132/diff/12004/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1186: } else { maybe: else if (upload_file_info->entry.get()) { http://codereview.chromium.org/10540132/diff/12004/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:3609: callback.Run(); Should we run the callback here? StoreToCache() is asynchronous hence returns immediately. Shouldn't we run |callback| upon the completion of StoreToCache()? http://codereview.chromium.org/10540132/diff/12004/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): http://codereview.chromium.org/10540132/diff/12004/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.h:377: // function is called. maybe: |callback| will be called on the UI thread upon completion of the operation. BTW, the callback does not take any parameter. Does this mean that the caller is not interested in the result was successful or not?
http://codereview.chromium.org/10540132/diff/12004/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10540132/diff/12004/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1186: } else { In the original code there is no such check; even if upload_file_info->entry.get() == NULL the code would still execute the callback. On 2012/06/14 20:30:06, satorux1 wrote: > maybe: > > else if (upload_file_info->entry.get()) { http://codereview.chromium.org/10540132/diff/12004/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:3609: callback.Run(); You're right, does the new patch set work? On 2012/06/14 20:30:06, satorux1 wrote: > Should we run the callback here? StoreToCache() is asynchronous hence returns > immediately. Shouldn't we run |callback| upon the completion of StoreToCache()? http://codereview.chromium.org/10540132/diff/12004/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): http://codereview.chromium.org/10540132/diff/12004/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.h:377: // function is called. Done. Yes the only thing callback does is to delete the UploadFileInfo* object. On 2012/06/14 20:30:06, satorux1 wrote: > maybe: > > |callback| will be called on the UI thread upon completion of the operation. > > BTW, the callback does not take any parameter. Does this mean that the caller is > not interested in the result was successful or not?
Please get this reviewed by achuith. He's more familiar with the code. http://codereview.chromium.org/10540132/diff/7015/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_uploader.cc (right): http://codereview.chromium.org/10540132/diff/7015/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_uploader.cc:77: content::DownloadItem* download) { on what thread is this function called? In other places, we are mostly on UI thread, but we don't have a DCHECK here could you add DCHECK(BrowserThread::CurrentlyOn(...))?
Patch Set #5: I think it is safer for RemoveUpload() to take an int upload_id instead of the UploadFileInfo* pointer. The latter may not be valid if the AddUploadedFile invokes the callback to delete the pointer before RemoveUpload() is called. http://codereview.chromium.org/10540132/diff/7015/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_uploader.cc (right): http://codereview.chromium.org/10540132/diff/7015/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_uploader.cc:77: content::DownloadItem* download) { On 2012/06/14 21:32:34, satorux1 wrote: > on what thread is this function called? In other places, we are mostly on UI > thread, but we don't have a DCHECK here > > could you add DCHECK(BrowserThread::CurrentlyOn(...))? Done.
http://codereview.chromium.org/10540132/diff/7015/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_uploader.cc (right): http://codereview.chromium.org/10540132/diff/7015/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_uploader.cc:77: content::DownloadItem* download) { On 2012/06/14 21:40:17, hshi1 wrote: > On 2012/06/14 21:32:34, satorux1 wrote: > > on what thread is this function called? In other places, we are mostly on UI > > thread, but we don't have a DCHECK here > > > > could you add DCHECK(BrowserThread::CurrentlyOn(...))? > > Done. Hmm, then seems to me that all functions run on UI thread. If so, we don't need a lock?
Aha, I think you're correct. Previously I had to introduce the lock because the DeleteUpload() function ties the two things together, namely (1) removal of UploadFileInfo from the map, and (2) the deletion of the UploadFileInfo object. But if I always have UploadFileInfoMap operations on the UI thread then I won't need the lock. On 2012/06/14 21:47:25, satorux1 wrote: > http://codereview.chromium.org/10540132/diff/7015/chrome/browser/chromeos/gda... > File chrome/browser/chromeos/gdata/gdata_uploader.cc (right): > > http://codereview.chromium.org/10540132/diff/7015/chrome/browser/chromeos/gda... > chrome/browser/chromeos/gdata/gdata_uploader.cc:77: content::DownloadItem* > download) { > On 2012/06/14 21:40:17, hshi1 wrote: > > On 2012/06/14 21:32:34, satorux1 wrote: > > > on what thread is this function called? In other places, we are mostly on UI > > > thread, but we don't have a DCHECK here > > > > > > could you add DCHECK(BrowserThread::CurrentlyOn(...))? > > > > Done. > > Hmm, then seems to me that all functions run on UI thread. If so, we don't need > a lock?
Please check Patch Set #6, removed all locks as suggested. To clarify, the reason why locks are not necessary is because all operations on the UploadFileInfoMap |pending_uploads_| are all executed immediately on the UI thread, so they are serialized by definition. The deletion of the UploadFileInfo object is deferred to until completion of the AddUploadedFile().
Could you please update the CL description so we describe everything that's going on in this patch? I think it's really risky to land a patch this big with no unit test coverage. This is not in M21 scope, right? http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:276: if (!callback.is_null()) could we add a DCHECK for UI thread here? http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1184: UploadFileInfo* upload_file_info) { We are getting ownership of upload_file_info here. Could we use scoped_ptr instead? http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1204: UploadFileInfo* upload_file_info) { could we used scoped_ptr instead? http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1210: // In case of error upload_file_info will be deleted by the uploader. This is really weird (deletion by uploader in case of error, and by us in case of success). Is there a way to fix this? If not, add a TODO and just release? http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_uploader.cc (right): http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_uploader.cc:364: int upload_id = upload_file_info->upload_id; nit: could we make this const? http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_uploader.cc:389: int upload_id = upload_file_info->upload_id; nit: make this const http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_uploader.h (right): http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_uploader.h:79: void UploadFailed(UploadFileInfo* upload_file_info, Can we use a scoped_ptr here too? http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_uploader.h:89: void OnAddUploadFileComplete(UploadFileInfo* upload_file_info); Why does this need to be a member function? I think it should be in anonymous scope. Also, can we have the argument be scoped_ptr instead?
http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:3558: // Post a task to the same thread, rather than calling it here, as I'm confused. Why are we doing this?
On 2012/06/14 22:27:17, achuith.bhandarkar wrote: > Could you please update the CL description so we describe everything that's > going on in this patch? > > I think it's really risky to land a patch this big with no unit test coverage. > This is not in M21 scope, right? I'd rather want to see this checked in before M22 branch cut. Otherwise, we need to wait for two weeks or so before checking in refactoring patches like this. What would make you feel comfortable about checking this in? > > http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... > File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): > > http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/gdata_file_system.cc:276: if (!callback.is_null()) > could we add a DCHECK for UI thread here? > > http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/gdata_file_system.cc:1184: UploadFileInfo* > upload_file_info) { > We are getting ownership of upload_file_info here. Could we use scoped_ptr > instead? > > http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/gdata_file_system.cc:1204: UploadFileInfo* > upload_file_info) { > could we used scoped_ptr instead? > > http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/gdata_file_system.cc:1210: // In case of error > upload_file_info will be deleted by the uploader. > This is really weird (deletion by uploader in case of error, and by us in case > of success). Is there a way to fix this? If not, add a TODO and just release? > > http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... > File chrome/browser/chromeos/gdata/gdata_uploader.cc (right): > > http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/gdata_uploader.cc:364: int upload_id = > upload_file_info->upload_id; > nit: could we make this const? > > http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/gdata_uploader.cc:389: int upload_id = > upload_file_info->upload_id; > nit: make this const > > http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... > File chrome/browser/chromeos/gdata/gdata_uploader.h (right): > > http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/gdata_uploader.h:79: void > UploadFailed(UploadFileInfo* upload_file_info, > Can we use a scoped_ptr here too? > > http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... > chrome/browser/chromeos/gdata/gdata_uploader.h:89: void > OnAddUploadFileComplete(UploadFileInfo* upload_file_info); > Why does this need to be a member function? I think it should be in anonymous > scope. > > Also, can we have the argument be scoped_ptr instead?
On 2012/06/14 22:38:01, satorux1 wrote: > On 2012/06/14 22:27:17, achuith.bhandarkar wrote: > > Could you please update the CL description so we describe everything that's > > going on in this patch? > > > > I think it's really risky to land a patch this big with no unit test coverage. > > This is not in M21 scope, right? > > I'd rather want to see this checked in before M22 branch cut. Otherwise, we need > to wait for two weeks or so before checking in refactoring patches like this. > > What would make you feel comfortable about checking this in? There are a number of manual test cases - small/fast downloads, large downloads, paused downloads, throttled downloads. There are a number of download paths - default download to drive, save html file, save with file picker. There are also a number of failure scenarios, which I'm especially concerned about since UploadFailed is being changed with this patch. This patch is pure code refactor, so there's no user-visible change, but it has a risk of regressions. I see no benefit in landing this now. I'd rather wait until a week or two after the branch point. We could easily use that time to develop good unit tests.
Achuith, can you review Patch Set #7? I think by using scoped_ptr we won't need to have to explicitly delete the pointer any more. As soon as the chain of callbacks stop passing the scoped_ptr along, it will be deleted automatically. As for risk, I can help run tests manually. But it shouldn't be too risky to checky in for M21 as it is a pure refactor. If we wait until M22 branches it will be many weeks after, and conflicts would be hard to merge. http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:276: if (!callback.is_null()) On 2012/06/14 22:27:18, achuith.bhandarkar wrote: > could we add a DCHECK for UI thread here? Done. http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1184: UploadFileInfo* upload_file_info) { On 2012/06/14 22:27:18, achuith.bhandarkar wrote: > We are getting ownership of upload_file_info here. Could we use scoped_ptr > instead? Done. http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1204: UploadFileInfo* upload_file_info) { On 2012/06/14 22:27:18, achuith.bhandarkar wrote: > could we used scoped_ptr instead? Done. http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1210: // In case of error upload_file_info will be deleted by the uploader. This is no longer necessary as the pointer gets automatically deleted when the scoped_ptr is no longer passed along. http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:3558: // Post a task to the same thread, rather than calling it here, as On 2012/06/14 22:33:33, achuith.bhandarkar wrote: > I'm confused. Why are we doing this? This is the purpose of this CL and this bug - to make AddUploadedFile an asynchronous function. By posting a task to AddUploadedFileOnUIThread(), we are able to return immediately while the task can run in the background. http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_uploader.cc (right): http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_uploader.cc:364: int upload_id = upload_file_info->upload_id; On 2012/06/14 22:27:18, achuith.bhandarkar wrote: > nit: could we make this const? Done. http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_uploader.cc:389: int upload_id = upload_file_info->upload_id; On 2012/06/14 22:27:18, achuith.bhandarkar wrote: > nit: make this const Done. http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_uploader.h (right): http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_uploader.h:79: void UploadFailed(UploadFileInfo* upload_file_info, On 2012/06/14 22:27:18, achuith.bhandarkar wrote: > Can we use a scoped_ptr here too? Done. http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_uploader.h:89: void OnAddUploadFileComplete(UploadFileInfo* upload_file_info); On 2012/06/14 22:27:18, achuith.bhandarkar wrote: > Why does this need to be a member function? I think it should be in anonymous > scope. > > Also, can we have the argument be scoped_ptr instead? Done.
On 2012/06/14 22:56:52, achuith.bhandarkar wrote: > There are a number of manual test cases - small/fast downloads, large downloads, > paused downloads, throttled downloads. There are a number of download paths - > default download to drive, save html file, save with file picker. There are also > a number of failure scenarios, which I'm especially concerned about since > UploadFailed is being changed with this patch. > > This patch is pure code refactor, so there's no user-visible change, but it has > a risk of regressions. I see no benefit in landing this now. I'd rather wait > until a week or two after the branch point. We could easily use that time to > develop good unit tests. The downside with postponing landing this is that there will be a 2-3 week gap before M21 can stabilize, and it is a huge pain to keep resolving change conflicts. As a general rule of thumb it is always better to get refactoring changes landed before a branch. We will get better unit test coverage but the download+upload-to-gdrive is such a complicated process that I doubt how much confidence unit tests can gain us, in the end we still would need to run the manual test cases you described here. I have tested this patch on a lumpy a bit and haven't noticed anything unusual so far, but if you could recommend some specific edge case to verify I would be happy to run them manually.
Could you please update the patch description to describe all changes in this patch?
> > The downside with postponing landing this is that there will be a 2-3 week gap > before M21 can stabilize, and it is a huge pain to keep resolving change > conflicts. As a general rule of thumb it is always better to get refactoring > changes landed before a branch. > > We will get better unit test coverage but the download+upload-to-gdrive is such > a complicated process that I doubt how much confidence unit tests can gain us, > in the end we still would need to run the manual test cases you described here. I agree that unit tests don't help download+upload case, but they do help the upload-only case. You can easily manually kick off an upload and with a mock doc service and mock file system simulate various failures. This CL doesn't really affect the download stuff. > I have tested this patch on a lumpy a bit and haven't noticed anything unusual > so far, but if you could recommend some specific edge case to verify I would be > happy to run them manually. Ok, that's fine. Please test both large and small files. Use the download file picker (save link as), the html page file picker (save as), as well as setting the default download location to drive.
http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10540132/diff/11005/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:3558: // Post a task to the same thread, rather than calling it here, as On 2012/06/15 00:16:28, hshi1 wrote: > On 2012/06/14 22:33:33, achuith.bhandarkar wrote: > > I'm confused. Why are we doing this? > > This is the purpose of this CL and this bug - to make AddUploadedFile an > asynchronous function. By posting a task to AddUploadedFileOnUIThread(), we are > able to return immediately while the task can run in the background. I've lost sight of the big picture on why we're doing this?
Achuith - I have updated the changelist description. On 2012/06/15 01:27:33, achuith.bhandarkar wrote: > I've lost sight of the big picture on why we're doing this? This was originally part of bug 127048 filed by Satoru. The goal is to convert all public, synchronous functions in GDataFileSystem to synchronous, and the AddUploadedFile happens to be the last one to be converted. As Satoru puts it, having synchronous access to |root_| in the public functions of GDataFileSystem is a "roadblock for us to switch to the on-disk metadata storage system". I am not entirely sure what this means but guess it has something to do with the levelDB changes you're working on.
http://codereview.chromium.org/10540132/diff/6021/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_uploader.cc (right): http://codereview.chromium.org/10540132/diff/6021/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_uploader.cc:331: RemoveUpload(upload_id); RemoveUpload first before passing upload_file_info to the callback. http://codereview.chromium.org/10540132/diff/6021/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_uploader.cc:395: RemoveUpload(upload_id); I think we want to RemoveUpload first. Otherwise the map has a deleted UploadFileInfo*.
Done with Patch Set #8. Thanks! http://codereview.chromium.org/10540132/diff/6021/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_uploader.cc (right): http://codereview.chromium.org/10540132/diff/6021/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_uploader.cc:331: RemoveUpload(upload_id); On 2012/06/15 01:53:10, achuith.bhandarkar wrote: > RemoveUpload first before passing upload_file_info to the callback. Done. http://codereview.chromium.org/10540132/diff/6021/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_uploader.cc:395: RemoveUpload(upload_id); On 2012/06/15 01:53:10, achuith.bhandarkar wrote: > I think we want to RemoveUpload first. Otherwise the map has a deleted > UploadFileInfo*. Done.
lgtm. Please wait for Satoru-san, in case he wants to review this again. Please run the manual tests as well before you land this.
LGTM. The latest version looks a lot simpler than the original patch, thanks to achuith's comments!
Thanks again for doing all the cleanup. I think I left the code in a bit of a messy state. I'm especially happy with the clarity in memory management now. We really do need to get around to having more test coverage for this code though!
I resolved some merge conflicts with TOT this morning and re-ran manual tests with different size test files (1MB, 5MB, 20MB, 100MB, 500MB), pause/resume/cancel, download file picker and html page file picker, as well as setting default download location to a subdirectory under gdrive. (It seems I still can't set default download location to /drive/ root directory from the Settings page.)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hshi@chromium.org/10540132/20001
Failed to apply patch for chrome/browser/chromeos/gdata/gdata_file_system.cc: While running patch -p1 --forward --force; patching file chrome/browser/chromeos/gdata/gdata_file_system.cc Hunk #2 succeeded at 1173 (offset -11 lines). Hunk #3 succeeded at 3499 (offset -49 lines). Hunk #4 FAILED at 3585. Hunk #5 succeeded at 3553 (offset -53 lines). 1 out of 5 hunks FAILED -- saving rejects to file chrome/browser/chromeos/gdata/gdata_file_system.cc.rej
Looks like two more changes got landed in gdata_file_system.cc this morning after my previous rebase. I'll resolve merge conflicts and test again before submitting this.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hshi@chromium.org/10540132/21015
Satoru can you review this (Patch Set #11)? Looks like the scoped_ptr upload_file_info may be invalidated by base::Passed before evaluation of upload_file_info->gdata_path etc., depending on compiler's param evaluation order.
http://codereview.chromium.org/10540132/diff/31003/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10540132/diff/31003/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1181: const UploadFileInfo* upload_file_info_ptr = upload_file_info.get(); Please add a comment.
http://codereview.chromium.org/10540132/diff/31003/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10540132/diff/31003/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1181: const UploadFileInfo* upload_file_info_ptr = upload_file_info.get(); On 2012/06/15 21:53:58, achuith.bhandarkar wrote: > Please add a comment. yeah, this is subtle. glad you caught this before submitting. :)
http://codereview.chromium.org/10540132/diff/31003/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10540132/diff/31003/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1181: const UploadFileInfo* upload_file_info_ptr = upload_file_info.get(); On 2012/06/15 21:53:58, achuith.bhandarkar wrote: > Please add a comment. Done. http://codereview.chromium.org/10540132/diff/31003/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1181: const UploadFileInfo* upload_file_info_ptr = upload_file_info.get(); Yes, thanks for mentioning a similar issue before submitting... fortunately I was able to abort this from CQ, it was down to the last test item. On 2012/06/15 21:59:15, satorux1 wrote: > On 2012/06/15 21:53:58, achuith.bhandarkar wrote: > > Please add a comment. > > yeah, this is subtle. glad you caught this before submitting. :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hshi@chromium.org/10540132/27006
Change committed as 142515 |
