Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17140009:
Support contents upload on FakeDriveService (Closed)
|
Created:
7 years, 6 months ago by tzik Modified:
7 years, 6 months ago Reviewers:
hashimoto CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSupport contents upload on FakeDriveService
* Add support for file content upload,
* Fill MD5 on contents update.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207836
Patch Set 1 #Patch Set 2 : +ResumeUpload change #Patch Set 3 : style fix #
Total comments: 10
Patch Set 4 : nitpick #Patch Set 5 : +UploadSession #Patch Set 6 : split out indentation fix #
Total comments: 18
Patch Set 7 : address comments #Patch Set 8 : s/upload_session_/upload_sessions_/, drop weak_factory_ #Patch Set 9 : drop GetUploadStatus change #
Total comments: 3
Patch Set 10 : +completion callback #
Total comments: 6
Patch Set 11 : drop uploaded_entry #
Total comments: 2
Patch Set 12 : drop unused line #
Messages
Total messages: 16 (0 generated)
PTL
https://codereview.chromium.org/17140009/diff/6001/chrome/browser/drive/fake_... File chrome/browser/drive/fake_drive_service.cc (right): https://codereview.chromium.org/17140009/diff/6001/chrome/browser/drive/fake_... chrome/browser/drive/fake_drive_service.cc:1056: entry->Set("test$data", new base::BinaryValue(buffer.Pass(), end_position)); Do we need to update the entry's state before the whole part of the file gets uploaded? I expect cancelling upload to result in no-op. I think we should move this part and SetString("docs$size.$t", ~) above to after the content_length != end_position check below. WDYT? https://codereview.chromium.org/17140009/diff/6001/chrome/browser/drive/fake_... File chrome/browser/drive/fake_drive_service_unittest.cc (right): https://codereview.chromium.org/17140009/diff/6001/chrome/browser/drive/fake_... chrome/browser/drive/fake_drive_service_unittest.cc:1719: test_util::WriteStringToFile(local_file_path, contents); nit: Should better EXPECT_TRUE/ASSERT_TRUE the result? https://codereview.chromium.org/17140009/diff/6001/chrome/browser/drive/fake_... chrome/browser/drive/fake_drive_service_unittest.cc:1888: EXPECT_EQ(old_largest_change_id + 1, GetLargestChangeByAboutResource()); Could you add MD5 checks for AddNewFile()? https://codereview.chromium.org/17140009/diff/6001/chrome/browser/google_apis... File chrome/browser/google_apis/test_util.h (right): https://codereview.chromium.org/17140009/diff/6001/chrome/browser/google_apis... chrome/browser/google_apis/test_util.h:77: Please add a function comment. BTW, do we need to add this function? I think using file_util::ReadFileToString and discarding uninterested part form the result would suit for our purpose.
https://codereview.chromium.org/17140009/diff/6001/chrome/browser/drive/fake_... File chrome/browser/drive/fake_drive_service.cc (right): https://codereview.chromium.org/17140009/diff/6001/chrome/browser/drive/fake_... chrome/browser/drive/fake_drive_service.cc:1056: entry->Set("test$data", new base::BinaryValue(buffer.Pass(), end_position)); On 2013/06/20 10:17:42, hashimoto wrote: > Do we need to update the entry's state before the whole part of the file gets > uploaded? > I expect cancelling upload to result in no-op. > > I think we should move this part and SetString("docs$size.$t", ~) above to after > the content_length != end_position check below. > WDYT? It needs a bit more work to achieve no-op on cancel, since InitiateUpload{New,Existing}File alse have side effect. Should I do it in this CL? For latter part of your comment. Reading whole file contents at once makes me uneasy, since we'll be reading other part of the file than [start_position, end_position) at the point. https://codereview.chromium.org/17140009/diff/6001/chrome/browser/drive/fake_... File chrome/browser/drive/fake_drive_service_unittest.cc (right): https://codereview.chromium.org/17140009/diff/6001/chrome/browser/drive/fake_... chrome/browser/drive/fake_drive_service_unittest.cc:1719: test_util::WriteStringToFile(local_file_path, contents); On 2013/06/20 10:17:42, hashimoto wrote: > nit: Should better EXPECT_TRUE/ASSERT_TRUE the result? Done. https://codereview.chromium.org/17140009/diff/6001/chrome/browser/drive/fake_... chrome/browser/drive/fake_drive_service_unittest.cc:1888: EXPECT_EQ(old_largest_change_id + 1, GetLargestChangeByAboutResource()); On 2013/06/20 10:17:42, hashimoto wrote: > Could you add MD5 checks for AddNewFile()? Done. https://codereview.chromium.org/17140009/diff/6001/chrome/browser/google_apis... File chrome/browser/google_apis/test_util.h (right): https://codereview.chromium.org/17140009/diff/6001/chrome/browser/google_apis... chrome/browser/google_apis/test_util.h:77: On 2013/06/20 10:17:42, hashimoto wrote: > Please add a function comment. > > BTW, do we need to add this function? > I think using file_util::ReadFileToString and discarding uninterested part form > the result would suit for our purpose. It seems to work. OK, I'll drop this.
https://codereview.chromium.org/17140009/diff/6001/chrome/browser/drive/fake_... File chrome/browser/drive/fake_drive_service.cc (right): https://codereview.chromium.org/17140009/diff/6001/chrome/browser/drive/fake_... chrome/browser/drive/fake_drive_service.cc:1056: entry->Set("test$data", new base::BinaryValue(buffer.Pass(), end_position)); On 2013/06/20 13:37:22, tzik wrote: > On 2013/06/20 10:17:42, hashimoto wrote: > > Do we need to update the entry's state before the whole part of the file gets > > uploaded? > > I expect cancelling upload to result in no-op. > > > > I think we should move this part and SetString("docs$size.$t", ~) above to > after > > the content_length != end_position check below. > > WDYT? > > It needs a bit more work to achieve no-op on cancel, since > InitiateUpload{New,Existing}File alse have side effect. > Should I do it in this CL? Good catch, setting "docs$size.$t" to 0 while leaving "test$data" looks wrong. Please fix them too. IIRC InitiateUploadNew(Existing)File without following ResumeUpload calls results in no-op with the real server, even when content_length==0. > > For latter part of your comment. > Reading whole file contents at once makes me uneasy, since we'll be reading > other part of the file than [start_position, end_position) at the point. I cannot come up with real use cases where this difference might be a problem, and reading the entire file at once would be simpler than reading each part separately. Anyways, it's up to you.
Updated. Could you take another look? https://codereview.chromium.org/17140009/diff/6001/chrome/browser/drive/fake_... File chrome/browser/drive/fake_drive_service.cc (right): https://codereview.chromium.org/17140009/diff/6001/chrome/browser/drive/fake_... chrome/browser/drive/fake_drive_service.cc:1056: entry->Set("test$data", new base::BinaryValue(buffer.Pass(), end_position)); On 2013/06/20 14:36:44, hashimoto wrote: > On 2013/06/20 13:37:22, tzik wrote: > > On 2013/06/20 10:17:42, hashimoto wrote: > > > Do we need to update the entry's state before the whole part of the file > gets > > > uploaded? > > > I expect cancelling upload to result in no-op. > > > > > > I think we should move this part and SetString("docs$size.$t", ~) above to > > after > > > the content_length != end_position check below. > > > WDYT? > > > > It needs a bit more work to achieve no-op on cancel, since > > InitiateUpload{New,Existing}File alse have side effect. > > Should I do it in this CL? > Good catch, setting "docs$size.$t" to 0 while leaving "test$data" looks wrong. > Please fix them too. > > IIRC InitiateUploadNew(Existing)File without following ResumeUpload calls > results in no-op with the real server, even when content_length==0. Done. I introduced UploadSession to make abondand upload no-op. > > > > For latter part of your comment. > > Reading whole file contents at once makes me uneasy, since we'll be reading > > other part of the file than [start_position, end_position) at the point. > I cannot come up with real use cases where this difference might be a problem, > and reading the entire file at once would be simpler than reading each part > separately. > Anyways, it's up to you. OK, let me go simpler way. Reading them at once will be enough for fake implementation.
https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... File chrome/browser/drive/fake_drive_service.cc (right): https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:987: if (!ContainsKey(upload_session_, upload_url)) { nit: Hmn, I didn't know that we had this function in stl_util.h. We usually use std::map::count to do this kind of check. What is the merit of this function over upload_session_.count(upload_url) > 0? https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:1001: base::Passed(scoped_ptr<ResourceEntry>()))); Why are you returning null pointers even in success cases? BTW, is this part needed to be in this change? Please add tests for this method if you change the implementation. https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:1060: int64 current_size = session->uploaded_size; nit: Do we need this variable? https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:1111: if (session->resource_id.empty()) { resource_id.empty() is true iff the upload is for a new file, right? I think having a comment about it would help future readers. BTW, do we still need to append mode=newfile|existing parameter for URLs? https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:1175: UploadRangeResponse(HTTP_SUCCESS, Please return HTTP_CREATED for new files. https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:1327: void FakeDriveService::AddNewChangestamp(base::DictionaryValue* entry) { How about renaming this method to something like AddNewChangestampAndETag? Please update the method comment too. https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... File chrome/browser/drive/fake_drive_service.h (right): https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.h:263: std::map<GURL, UploadSession> upload_session_; nit: How about renaming this variable to something like |upload_sessions_|? This variable can remember multiple sessions. https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.h:276: base::WeakPtrFactory<FakeDriveService> weak_factory_; nit: In this directory, we usually name WeakPtrFactory "weak_ptr_factory_".
https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... File chrome/browser/drive/fake_drive_service.cc (right): https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:987: if (!ContainsKey(upload_session_, upload_url)) { On 2013/06/21 04:16:21, hashimoto wrote: > nit: Hmn, I didn't know that we had this function in stl_util.h. > We usually use std::map::count to do this kind of check. > What is the merit of this function over upload_session_.count(upload_url) > 0? Changed this to count(). They are exactly same in their functionality or performance. https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:1001: base::Passed(scoped_ptr<ResourceEntry>()))); On 2013/06/21 04:16:21, hashimoto wrote: > Why are you returning null pointers even in success cases? > > BTW, is this part needed to be in this change? > Please add tests for this method if you change the implementation. IIUC, the server doesn't return resource entry for this request. Let me check it. https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:1060: int64 current_size = session->uploaded_size; On 2013/06/21 04:16:21, hashimoto wrote: > nit: Do we need this variable? Done. https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:1111: if (session->resource_id.empty()) { On 2013/06/21 04:16:21, hashimoto wrote: > resource_id.empty() is true iff the upload is for a new file, right? > I think having a comment about it would help future readers. > > BTW, do we still need to append mode=newfile|existing parameter for URLs? Done. https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:1175: UploadRangeResponse(HTTP_SUCCESS, On 2013/06/21 04:16:21, hashimoto wrote: > Please return HTTP_CREATED for new files. Uploads for new file are handled around 1111, so this is existing file case. https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:1327: void FakeDriveService::AddNewChangestamp(base::DictionaryValue* entry) { On 2013/06/21 04:16:21, hashimoto wrote: > How about renaming this method to something like AddNewChangestampAndETag? > Please update the method comment too. Done. https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... File chrome/browser/drive/fake_drive_service.h (right): https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.h:263: std::map<GURL, UploadSession> upload_session_; On 2013/06/21 04:16:21, hashimoto wrote: > nit: How about renaming this variable to something like |upload_sessions_|? This > variable can remember multiple sessions. Done. https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.h:276: base::WeakPtrFactory<FakeDriveService> weak_factory_; On 2013/06/21 04:16:21, hashimoto wrote: > nit: In this directory, we usually name WeakPtrFactory "weak_ptr_factory_". Let me drop this. The class no longer uses WeakPtr now.
https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... File chrome/browser/drive/fake_drive_service.cc (right): https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:1001: base::Passed(scoped_ptr<ResourceEntry>()))); On 2013/06/21 05:06:52, tzik wrote: > On 2013/06/21 04:16:21, hashimoto wrote: > > Why are you returning null pointers even in success cases? > > > > BTW, is this part needed to be in this change? > > Please add tests for this method if you change the implementation. > > IIUC, the server doesn't return resource entry for this request. Let me check > it. Done. I move this part of change to another CL.
lgtm https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... File chrome/browser/drive/fake_drive_service.cc (right): https://codereview.chromium.org/17140009/diff/19004/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:1175: UploadRangeResponse(HTTP_SUCCESS, On 2013/06/21 05:06:52, tzik wrote: > On 2013/06/21 04:16:21, hashimoto wrote: > > Please return HTTP_CREATED for new files. > > Uploads for new file are handled around 1111, so this is existing file case. You're right, thanks. https://codereview.chromium.org/17140009/diff/31001/chrome/browser/drive/fake... File chrome/browser/drive/fake_drive_service.cc (right): https://codereview.chromium.org/17140009/diff/31001/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:1087: base::MessageLoop::current()->PostTask( nit: The only difference between PostTask here and below is HTTP status code and ResourceEntry. Can we reduce the code duplication?
https://codereview.chromium.org/17140009/diff/31001/chrome/browser/drive/fake... File chrome/browser/drive/fake_drive_service.cc (right): https://codereview.chromium.org/17140009/diff/31001/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:1087: base::MessageLoop::current()->PostTask( On 2013/06/21 05:41:24, hashimoto wrote: > nit: The only difference between PostTask here and below is HTTP status code and > ResourceEntry. > Can we reduce the code duplication? Did you mean a reduction like this? https://codereview.chromium.org/17140009/diff2/31001:37001/chrome/browser/dri...
https://codereview.chromium.org/17140009/diff/31001/chrome/browser/drive/fake... File chrome/browser/drive/fake_drive_service.cc (right): https://codereview.chromium.org/17140009/diff/31001/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:1087: base::MessageLoop::current()->PostTask( On 2013/06/21 07:54:02, tzik wrote: > On 2013/06/21 05:41:24, hashimoto wrote: > > nit: The only difference between PostTask here and below is HTTP status code > and > > ResourceEntry. > > Can we reduce the code duplication? > > Did you mean a reduction like this? > https://codereview.chromium.org/17140009/diff2/31001:37001/chrome/browser/dri... Wow, I was only expecting the calls for HTTP_NOT_FOUND and HTTP_CREATED here and below getting merged, but your new code also looks fine. https://codereview.chromium.org/17140009/diff/37001/chrome/browser/drive/fake... File chrome/browser/drive/fake_drive_service.cc (right): https://codereview.chromium.org/17140009/diff/37001/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:116: UploadRangeResponse(GDATA_NO_CONNECTION, Shouldn't this be |error|? https://codereview.chromium.org/17140009/diff/37001/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:1017: scoped_ptr<ResourceEntry> uploaded_entry; nit: It seems there is no need to have this variable? scoped_ptr<ResourceEntry>() can be passed directly as an argument for NULL cases and ResourceEntry::CreateFrom(*entry) can be passed for non-NULL cases. https://codereview.chromium.org/17140009/diff/37001/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:1025: completion_callback.Run(HTTP_NOT_FOUND, nit: The next line can be merged into this line without violating 80 column rule? The same goes for other places.
Thanks! Updated. https://codereview.chromium.org/17140009/diff/37001/chrome/browser/drive/fake... File chrome/browser/drive/fake_drive_service.cc (right): https://codereview.chromium.org/17140009/diff/37001/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:116: UploadRangeResponse(GDATA_NO_CONNECTION, On 2013/06/21 08:05:32, hashimoto wrote: > Shouldn't this be |error|? Done. https://codereview.chromium.org/17140009/diff/37001/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:1017: scoped_ptr<ResourceEntry> uploaded_entry; On 2013/06/21 08:05:32, hashimoto wrote: > nit: It seems there is no need to have this variable? > scoped_ptr<ResourceEntry>() can be passed directly as an argument for NULL cases > and ResourceEntry::CreateFrom(*entry) can be passed for non-NULL cases. Done. https://codereview.chromium.org/17140009/diff/37001/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:1025: completion_callback.Run(HTTP_NOT_FOUND, On 2013/06/21 08:05:32, hashimoto wrote: > nit: The next line can be merged into this line without violating 80 column > rule? The same goes for other places. Done.
lgtm with a nit. thanks https://codereview.chromium.org/17140009/diff/42001/chrome/browser/drive/fake... File chrome/browser/drive/fake_drive_service.cc (right): https://codereview.chromium.org/17140009/diff/42001/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:1012: base::Passed(scoped_ptr<ResourceEntry>()); nit: Remove this line.
https://codereview.chromium.org/17140009/diff/42001/chrome/browser/drive/fake... File chrome/browser/drive/fake_drive_service.cc (right): https://codereview.chromium.org/17140009/diff/42001/chrome/browser/drive/fake... chrome/browser/drive/fake_drive_service.cc:1012: base::Passed(scoped_ptr<ResourceEntry>()); On 2013/06/21 11:10:46, hashimoto wrote: > nit: Remove this line. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tzik@chromium.org/17140009/47002
Message was sent while issue was closed.
Change committed as 207836 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
