Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12246002:
Implement GetUploadStatusOperation on GData WAPI. (Closed)
|
Created:
7 years, 10 months ago by hidehiko Modified:
7 years, 10 months ago CC:
chromium-reviews, achuith+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@b148632_create_base_operation Visibility:
Public. |
DescriptionImplement GetUploadStatusOperation on GData WAPI.
As a preparation of 5xx error handling in DriveUploader,
this CL implements the GDataWapiService::GetUploadStatus.
BUG=148632
TEST=Ran unit_tests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184617
Patch Set 1 : #
Total comments: 4
Patch Set 2 : Rebase #Patch Set 3 : Update comments. #
Total comments: 18
Patch Set 4 : Update comments and add explicit initialization for text fixture. #Patch Set 5 : Rebase #Patch Set 6 : Remove unknown content length support, which is not used. #
Total comments: 4
Patch Set 7 : Fix mock server's behavior, as well as empty Range header handling. #
Total comments: 5
Patch Set 8 : Fix unittests. #Patch Set 9 : Rebase #
Messages
Total messages: 28 (0 generated)
This CL depends on https://codereview.chromium.org/12224026/ and https://codereview.chromium.org/12226008/. Also, I'll send a following CL to the code review soon. Thank you for your review in advance, - hidehiko
https://codereview.chromium.org/12246002/diff/11001/chrome/browser/google_api... File chrome/browser/google_apis/gdata_wapi_operations.h (right): https://codereview.chromium.org/12246002/diff/11001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations.h:567: // of a file. Could you be more specific? What data we can get from this operation? https://codereview.chromium.org/12246002/diff/11001/chrome/browser/google_api... File chrome/browser/google_apis/gdata_wapi_operations_unittest.cc (right): https://codereview.chromium.org/12246002/diff/11001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:885: // of ResumeUploadOperation. Please also mention that GetUploadStatusOperation is also tested in the test case.
Thank you for your review. Could you take another look? https://codereview.chromium.org/12246002/diff/11001/chrome/browser/google_api... File chrome/browser/google_apis/gdata_wapi_operations.h (right): https://codereview.chromium.org/12246002/diff/11001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations.h:567: // of a file. On 2013/02/07 05:38:57, satorux1 wrote: > Could you be more specific? What data we can get from this operation? Done. https://codereview.chromium.org/12246002/diff/11001/chrome/browser/google_api... File chrome/browser/google_apis/gdata_wapi_operations_unittest.cc (right): https://codereview.chromium.org/12246002/diff/11001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:885: // of ResumeUploadOperation. On 2013/02/07 05:38:57, satorux1 wrote: > Please also mention that GetUploadStatusOperation is also tested in the test > case. Done.
https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... File chrome/browser/google_apis/gdata_wapi_operations.h (right): https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations.h:572: // of a file. This operation calls |callback| given via the constructor Let's drop " given via the constructor" as it's obvious. https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations.h:573: // with the range of the successfully uploaded data, or HTTP_SUCCESS or maybe it's cleaner to describe the two case separately like: This result is passed to |callback| as follows: - If a file has been completely uploaded to |upload_url|, |callback| is called with HTTP_CREATED. The range parameters are unused. - If a file has been partially uploaded to |upload_url|, |callback| is called with HTTP_SUCCESS, with the range of the previously uploaded data. https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations.h:584: const UploadRangeCallback& callback, BTW, UploadRangeCallback takes scoped_ptr<ResourceEntry>, which we don't care for this operation, right? If so, I think we should not use UploadRangeCallback. We should instead define a new callback type. What do you think?
https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... File chrome/browser/google_apis/gdata_wapi_operations_unittest.cc (right): https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:332: int64 content_length_; Do we need to keep the values of start_position and end_position? nit: Please initialize these variables in the ctor. https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:885: // of ResumeUploadOperation. GetUploadOperation is also testes in this test nit: s/testes/tested/? https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:1021: kUploadContent.size()); Could you also test the content_length==1 case?
Thank you for your review. Could you take another look? https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... File chrome/browser/google_apis/gdata_wapi_operations.h (right): https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations.h:572: // of a file. This operation calls |callback| given via the constructor On 2013/02/07 06:54:13, satorux1 wrote: > Let's drop " given via the constructor" as it's obvious. Done. https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations.h:573: // with the range of the successfully uploaded data, or HTTP_SUCCESS or On 2013/02/07 06:54:13, satorux1 wrote: > maybe it's cleaner to describe the two case separately like: > > This result is passed to |callback| as follows: > > - If a file has been completely uploaded to |upload_url|, |callback| is called > with HTTP_CREATED. The range parameters are unused. > > - If a file has been partially uploaded to |upload_url|, |callback| is called > with HTTP_SUCCESS, with the range of the previously uploaded data. Updated a bit more. How about this? https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations.h:584: const UploadRangeCallback& callback, On 2013/02/07 06:54:13, satorux1 wrote: > BTW, UploadRangeCallback takes scoped_ptr<ResourceEntry>, which we don't care > for this operation, right? > > If so, I think we should not use UploadRangeCallback. We should instead define > a new callback type. What do you think? The ResourceEntry is used actually. Please see also UploadRangeOperationBase::ProcessURLFetchResults and OnDataParsed. https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... File chrome/browser/google_apis/gdata_wapi_operations_unittest.cc (right): https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:332: int64 content_length_; On 2013/02/07 06:56:13, hashimoto wrote: > Do we need to keep the values of start_position and end_position? > nit: Please initialize these variables in the ctor. Yes, it is necessary to keep them. This class is not only text fixture but also the http test server. The request from ResumeUploadOperation updates these fields, and it is necessary to include these "previously updated" values to the response for GetUploadOperation to emulate the server's behavior. Added a bit more comment. Initialized in SetUp(). https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:885: // of ResumeUploadOperation. GetUploadOperation is also testes in this test On 2013/02/07 06:56:13, hashimoto wrote: > nit: s/testes/tested/? Good catch. Fixed. https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:1021: kUploadContent.size()); On 2013/02/07 06:56:13, hashimoto wrote: > Could you also test the content_length==1 case? Sorry, I don't understand your request now... The content_length must be shared with the one for InitiateUploadParams. Here, it is kUploadContent.size().
https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... File chrome/browser/google_apis/gdata_wapi_operations_unittest.cc (right): https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:332: int64 content_length_; On 2013/02/07 07:44:07, hidehiko wrote: > On 2013/02/07 06:56:13, hashimoto wrote: > > Do we need to keep the values of start_position and end_position? > > nit: Please initialize these variables in the ctor. > > Yes, it is necessary to keep them. > This class is not only text fixture but also the http test server. The request > from ResumeUploadOperation updates these fields, and it is necessary to include > these "previously updated" values to the response for GetUploadOperation to > emulate the server's behavior. Added a bit more comment. I see, thanks. > > > Initialized in SetUp(). https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:1021: kUploadContent.size()); On 2013/02/07 07:44:07, hidehiko wrote: > On 2013/02/07 06:56:13, hashimoto wrote: > > Could you also test the content_length==1 case? Sorry, not 1, -1. > > Sorry, I don't understand your request now... > The content_length must be shared with the one for InitiateUploadParams. Here, > it is kUploadContent.size(). In gdata_wapi_operations.h, you added a comment "|content_length| is the whole data size to be uploaded, or -1 if it is unknown." And it seems there is no test for the -1 case.
https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... File chrome/browser/google_apis/gdata_wapi_operations.h (right): https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations.h:584: const UploadRangeCallback& callback, On 2013/02/07 07:44:07, hidehiko wrote: > On 2013/02/07 06:54:13, satorux1 wrote: > > BTW, UploadRangeCallback takes scoped_ptr<ResourceEntry>, which we don't care > > for this operation, right? > > > > If so, I think we should not use UploadRangeCallback. We should instead > define > > a new callback type. What do you think? > > The ResourceEntry is used actually. Please see also > UploadRangeOperationBase::ProcessURLFetchResults and OnDataParsed. I thought ResourceEntry was necessary for ResumeUploadOperation, but not GetUploadStatusOperation, as we only care about the status code and the range here.
https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... File chrome/browser/google_apis/gdata_wapi_operations_unittest.cc (right): https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:1021: kUploadContent.size()); The case has gone away when the streamed uploading (via DownloadObserver) is removed. We can (and should) erase mentions to the -1 case.
On 2013/02/07 07:58:59, satorux1 wrote: > https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... > File chrome/browser/google_apis/gdata_wapi_operations.h (right): > > https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... > chrome/browser/google_apis/gdata_wapi_operations.h:584: const > UploadRangeCallback& callback, > On 2013/02/07 07:44:07, hidehiko wrote: > > On 2013/02/07 06:54:13, satorux1 wrote: > > > BTW, UploadRangeCallback takes scoped_ptr<ResourceEntry>, which we don't > care > > > for this operation, right? > > > > > > If so, I think we should not use UploadRangeCallback. We should instead > > define > > > a new callback type. What do you think? > > > > The ResourceEntry is used actually. Please see also > > UploadRangeOperationBase::ProcessURLFetchResults and OnDataParsed. > > I thought ResourceEntry was necessary for ResumeUploadOperation, but not > GetUploadStatusOperation, as we only care about the status code and the range > here. Chatted offline. I now understand that ResourceEntry was needed for GetUploadStatusOperation too.
I defer the review to kinaba@ and hashimoto@. leaving now.
Thank you for your review. Could you take another look? https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... File chrome/browser/google_apis/gdata_wapi_operations.h (right): https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations.h:584: const UploadRangeCallback& callback, On 2013/02/07 07:58:59, satorux1 wrote: > On 2013/02/07 07:44:07, hidehiko wrote: > > On 2013/02/07 06:54:13, satorux1 wrote: > > > BTW, UploadRangeCallback takes scoped_ptr<ResourceEntry>, which we don't > care > > > for this operation, right? > > > > > > If so, I think we should not use UploadRangeCallback. We should instead > > define > > > a new callback type. What do you think? > > > > The ResourceEntry is used actually. Please see also > > UploadRangeOperationBase::ProcessURLFetchResults and OnDataParsed. > > I thought ResourceEntry was necessary for ResumeUploadOperation, but not > GetUploadStatusOperation, as we only care about the status code and the range > here. Acknowledged. https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... File chrome/browser/google_apis/gdata_wapi_operations_unittest.cc (right): https://codereview.chromium.org/12246002/diff/14001/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:1021: kUploadContent.size()); On 2013/02/07 08:01:10, kinaba wrote: > The case has gone away when the streamed uploading (via DownloadObserver) is > removed. > We can (and should) erase mentions to the -1 case. So, I've just removed -1 support from ResumeUploadOperation and GetUploadStatusOperation.
https://codereview.chromium.org/12246002/diff/15004/chrome/browser/google_api... File chrome/browser/google_apis/gdata_wapi_operations_unittest.cc (right): https://codereview.chromium.org/12246002/diff/15004/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:250: end_position_ = 0; Theoretically, it should be end_position_ = -1, since we are dealing with [start,end] range rather than [start,end). By the way, what the real server returns in case no bytes are uploaded yet? It looks it can indeed happen when the first chunk uploading returned 5xx. https://codereview.chromium.org/12246002/diff/15004/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:312: base::Int64ToString(start_position_) + "-" + Correct me if I'm wrong, When data is uploaded, the "Range"'s left hand side value is the start index of the uploaded data (i.e., the value of Content-Range request header). On the other hand, for GetUploadStatusOperation case, https://developers.google.com/drive/manage-uploads says "the Range header in its response to specify which bytes it has received so far", so to me the left value is always 0. If that's the case, two different things are mixed here. Could you clarify this?
# Sorry, rebase and my edit is mixed... Thank you for your review. Could you take another look? https://codereview.chromium.org/12246002/diff/15004/chrome/browser/google_api... File chrome/browser/google_apis/gdata_wapi_operations_unittest.cc (right): https://codereview.chromium.org/12246002/diff/15004/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:250: end_position_ = 0; On 2013/02/07 09:06:12, kinaba wrote: > Theoretically, it should be end_position_ = -1, since we are dealing with > [start,end] range rather than [start,end). By the way, what the real server > returns in case no bytes are uploaded yet? It looks it can indeed happen when > the first chunk uploading returned 5xx. Changed to use received_bytes_. https://codereview.chromium.org/12246002/diff/15004/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:312: base::Int64ToString(start_position_) + "-" + On 2013/02/07 09:06:12, kinaba wrote: > Correct me if I'm wrong, > > When data is uploaded, the "Range"'s left hand side value is the start index of > the uploaded data (i.e., the value of Content-Range request header). On the > other hand, for GetUploadStatusOperation case, > https://developers.google.com/drive/manage-uploads says "the Range header in its > response to specify which bytes it has received so far", so to me the left value > is always 0. > > If that's the case, two different things are mixed here. Could you clarify this? The server will return no Range header, if there is no received bytes. Changed the this server's behavior, its handling in gdata_wapi_operations.cc and added its test case (see below). Thank you for catching it! https://codereview.chromium.org/12246002/diff/13005/chrome/browser/google_api... File chrome/browser/google_apis/gdata_wapi_operations_unittest.cc (right): https://codereview.chromium.org/12246002/diff/13005/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:948: // 2) Before sending any data, check the current status. Hmm, the diff looks not good. What I did is inserting the 2) block here to test no Range header case, and re-number 2) to 3).
LGTM Please wait for kinaba's review
https://codereview.chromium.org/12246002/diff/13005/chrome/browser/google_api... File chrome/browser/google_apis/gdata_wapi_operations_unittest.cc (right): https://codereview.chromium.org/12246002/diff/13005/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:316: "bytes=0-" + base::Int64ToString(received_bytes_ - 1)); Is 0- the expected response when we have non-empty request content? (I think it is ok for the case when the content is empty and just querying the current status.) At least, the expectation below > EXPECT_EQ(static_cast<int64>(start_position), > response.start_position_received); doesn't look so. I'm unsure about the real behavior of the production server, but either here or the expectation looks requiring modification. https://codereview.chromium.org/12246002/diff/13005/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:901: const std::string kUploadContent(kMaxNumBytes + 1, 'a'); In order to catch the above commented case, could you change the case here to 2 * kMaxNumBytes + 1? Then we can exercise the non-start and non-end case.
Sorry it seems I had something mistake... # Probably, local test looks not working well... And also sorry that the patch is mixed with rebasing... I've fixed the unittests with following your comments. Could you take another look? Thank you for your review in advance, - hidehiko https://codereview.chromium.org/12246002/diff/13005/chrome/browser/google_api... File chrome/browser/google_apis/gdata_wapi_operations_unittest.cc (right): https://codereview.chromium.org/12246002/diff/13005/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:316: "bytes=0-" + base::Int64ToString(received_bytes_ - 1)); On 2013/02/08 10:19:43, kinaba wrote: > Is 0- the expected response when we have non-empty request content? (I think it > is ok for the case when the content is empty and just querying the current > status.) > > At least, the expectation below > > EXPECT_EQ(static_cast<int64>(start_position), > > response.start_position_received); > doesn't look so. I'm unsure about the real behavior of the production server, > but either here or the expectation looks requiring modification. The actual server returns 0-end if there is (one or more) received bytes. Fixed tests below. https://codereview.chromium.org/12246002/diff/13005/chrome/browser/google_api... chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:901: const std::string kUploadContent(kMaxNumBytes + 1, 'a'); On 2013/02/08 10:19:43, kinaba wrote: > In order to catch the above commented case, could you change the case here to 2 > * kMaxNumBytes + 1? Then we can exercise the non-start and non-end case. Done.
On 2013/02/08 11:53:10, hidehiko wrote: > Sorry it seems I had something mistake... > # Probably, local test looks not working well... > And also sorry that the patch is mixed with rebasing... > > I've fixed the unittests with following your comments. > Could you take another look? > > Thank you for your review in advance, > - hidehiko > > https://codereview.chromium.org/12246002/diff/13005/chrome/browser/google_api... > File chrome/browser/google_apis/gdata_wapi_operations_unittest.cc (right): > > https://codereview.chromium.org/12246002/diff/13005/chrome/browser/google_api... > chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:316: "bytes=0-" + > base::Int64ToString(received_bytes_ - 1)); > On 2013/02/08 10:19:43, kinaba wrote: > > Is 0- the expected response when we have non-empty request content? (I think > it > > is ok for the case when the content is empty and just querying the current > > status.) > > > > At least, the expectation below > > > EXPECT_EQ(static_cast<int64>(start_position), > > > response.start_position_received); > > doesn't look so. I'm unsure about the real behavior of the production server, > > but either here or the expectation looks requiring modification. > > The actual server returns 0-end if there is (one or more) received bytes. > Fixed tests below. > > https://codereview.chromium.org/12246002/diff/13005/chrome/browser/google_api... > chrome/browser/google_apis/gdata_wapi_operations_unittest.cc:901: const > std::string kUploadContent(kMaxNumBytes + 1, 'a'); > On 2013/02/08 10:19:43, kinaba wrote: > > In order to catch the above commented case, could you change the case here to > 2 > > * kMaxNumBytes + 1? Then we can exercise the non-start and non-end case. > > Done. lgtm
Rebased. Unfortunately, this CL got old so rebasing needed a bit task more than just merge. Would you mind to take another look? Best regards, - hidehiko
lgtm
still looks lgtm to me
Thank you for your review. Enqueueing to CQ.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hidehiko@chromium.org/12246002/19001
Retried try job too often on mac_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hidehiko@chromium.org/12246002/19001
Retried try job too often on mac_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hidehiko@chromium.org/12246002/19001
Message was sent while issue was closed.
Change committed as 184617 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
