Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1081313002:
Drive: Add response handling to BatchUploadRequst class. (Closed)
DescriptionDrive: Add response handling to BatchUploadRequst class.
BUG=451917
TEST=DriveApiRequestTest.BatchUploadRequest
Committed: https://crrev.com/35d010b6e1351dfad5a7c8872946d2fef6ad8545
Cr-Commit-Position: refs/heads/master@{#327646}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Fixed. #
Total comments: 16
Patch Set 3 : Fixed. #
Total comments: 14
Patch Set 4 : Fixed. #
Total comments: 2
Patch Set 5 : . #Patch Set 6 : Fix compile error on windows. #
Messages
Total messages: 21 (7 generated)
hirono@chromium.org changed reviewers: + kinaba@chromium.org
PTAL, thanks!
https://codereview.chromium.org/1081313002/diff/1/google_apis/drive/drive_api... File google_apis/drive/drive_api_requests.cc (right): https://codereview.chromium.org/1081313002/diff/1/google_apis/drive/drive_api... google_apis/drive/drive_api_requests.cc:139: // Splits multipart |response| into |parts|. Returns true on succcess. I think these util functions are complex enough for adding specific unit tests (not just as an integrated test within the response parsing.) Could you add such tests? https://codereview.chromium.org/1081313002/diff/1/google_apis/drive/drive_api... google_apis/drive/drive_api_requests.cc:139: // Splits multipart |response| into |parts|. Returns true on succcess. Link to the spec (and preferably a short summary of it as the comment) of multipart response will be very useful. https://codereview.chromium.org/1081313002/diff/1/google_apis/drive/drive_api... google_apis/drive/drive_api_requests.cc:148: boundary = response.substr(0, pos); Can't you get the boundary from HTTP response header (Content-Type: multipart/midex; boundary=HERE) instead of the first line? I thought there can be a preamble before the first boundary line, so the current code may not be valid in such case. https://codereview.chromium.org/1081313002/diff/1/google_apis/drive/drive_api... google_apis/drive/drive_api_requests.cc:161: const size_t next_pos = response.find(boundary, pos); IIUC boundary may still appear in content body (while \r\n--boundary\r\n must not), so you probably need additional care. https://codereview.chromium.org/1081313002/diff/1/google_apis/drive/drive_api... google_apis/drive/drive_api_requests.cc:181: pos += kHttpBr.size() * 2; Btw, I guess it is in general easy to split all the response first by kHttpBr into vector<string>, and then apply the multipart related jobs in the line-oriented manner. If you feel anxious about performance on splitting and re-concatenating, you can use vector<string::iterator> or maybe StringPiece. https://codereview.chromium.org/1081313002/diff/1/google_apis/drive/drive_api... google_apis/drive/drive_api_requests.cc:1244: } Maybe I should have asked this in the previous CL? Where is |this| deleted in this successful branch?
Patchset #2 (id:20001) has been deleted
Thank you! https://codereview.chromium.org/1081313002/diff/1/google_apis/drive/drive_api... File google_apis/drive/drive_api_requests.cc (right): https://codereview.chromium.org/1081313002/diff/1/google_apis/drive/drive_api... google_apis/drive/drive_api_requests.cc:139: // Splits multipart |response| into |parts|. Returns true on succcess. On 2015/04/14 06:25:26, kinaba wrote: > Link to the spec (and preferably a short summary of it as the comment) of > multipart response will be very useful. Done. https://codereview.chromium.org/1081313002/diff/1/google_apis/drive/drive_api... google_apis/drive/drive_api_requests.cc:148: boundary = response.substr(0, pos); On 2015/04/14 06:25:25, kinaba wrote: > Can't you get the boundary from HTTP response header (Content-Type: > multipart/midex; boundary=HERE) instead of the first line? I thought there can > be a preamble before the first boundary line, so the current code may not be > valid in such case. Done. https://codereview.chromium.org/1081313002/diff/1/google_apis/drive/drive_api... google_apis/drive/drive_api_requests.cc:161: const size_t next_pos = response.find(boundary, pos); On 2015/04/14 06:25:26, kinaba wrote: > IIUC boundary may still appear in content body (while \r\n--boundary\r\n must > not), so you probably need additional care. Done. https://codereview.chromium.org/1081313002/diff/1/google_apis/drive/drive_api... google_apis/drive/drive_api_requests.cc:181: pos += kHttpBr.size() * 2; On 2015/04/14 06:25:26, kinaba wrote: > Btw, I guess it is in general easy to split all the response first by kHttpBr > into vector<string>, and then apply the multipart related jobs in the > line-oriented manner. > > If you feel anxious about performance on splitting and re-concatenating, you can > use vector<string::iterator> or maybe StringPiece. Done. https://codereview.chromium.org/1081313002/diff/1/google_apis/drive/drive_api... google_apis/drive/drive_api_requests.cc:1244: } On 2015/04/14 06:25:25, kinaba wrote: > Maybe I should have asked this in the previous CL? > Where is |this| deleted in this successful branch? Done.
https://codereview.chromium.org/1081313002/diff/40001/google_apis/drive/drive... File google_apis/drive/drive_api_requests.cc (right): https://codereview.chromium.org/1081313002/diff/40001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:160: std::string::const_iterator output_begin = begin + prefix.size(); strictly speaking this addition is not legal if output_begin > end. Maybe: if (end - begin > prefix.size()) return false; if (!std::equal(prefix.begin(), prefix.end(), begin)) return false; output->assign(begin + prefix.size(), end); return true; Or see my comment below. I guess uses of this function can be replaced with StringPiece and string_util.h. https://codereview.chromium.org/1081313002/diff/40001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:195: } nit: this loop can be factored out as a function, something like SplitByHttpBr(). https://codereview.chromium.org/1081313002/diff/40001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:212: for (size_t i = 0; i < lines.size() - 1; ++i) { You can utilize base/strings/string_piece.h. Let current_line = StringPiece(lines[i], line[i + 1]) with kHttpBr removed, then IsLineEqual can be a simple comparison and GetAfterPrefix can be starts_with followed by substr(). https://codereview.chromium.org/1081313002/diff/40001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:239: const bool is_new_part = IsLineEqual(lines[i], lines[i + 1], header); I'm not sure how much we need to be pedantic, but according to RFC, you need to accept a line with header followed by spaces. ("transport-padding" rule in RFC) Probably we can ignore such cases. https://codereview.chromium.org/1081313002/diff/40001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:257: responses.back().body = std::string(body_begin, lines[i]); then body will include the last CRLF (i.e., two chars before lines[i]). Is it ok? (I'm guessing not.) https://codereview.chromium.org/1081313002/diff/40001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:265: break; nit: I slightly prefer these 4 lines to be put inside if(...||...) block, which makes clearer that something is done here only when (is_new_part || was_new_part) but is's up to your preference. https://codereview.chromium.org/1081313002/diff/40001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:1293: kMultipartMixedMimeTypePrefix, &boundary)) { According to the RFC boundary field can be quoted (search "WARNING TO IMPLEMENTORS") https://codereview.chromium.org/1081313002/diff/40001/google_apis/drive/drive... File google_apis/drive/drive_api_requests.h (right): https://codereview.chromium.org/1081313002/diff/40001/google_apis/drive/drive... google_apis/drive/drive_api_requests.h:80: // string. It looks like: In addition to the RFC, you are assuming that the "Content" part includes the HTTP response header. It'll be better noted. I also think that those detailed comments can be in the .cc file. Here in .h, it is enough if it is mentioned that |response| is the HTTP response body from Drive server as the reply of multipart request, and |parts| will be the corresponding result of split.
Thanks! https://codereview.chromium.org/1081313002/diff/40001/google_apis/drive/drive... File google_apis/drive/drive_api_requests.cc (right): https://codereview.chromium.org/1081313002/diff/40001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:160: std::string::const_iterator output_begin = begin + prefix.size(); On 2015/04/27 08:09:37, kinaba wrote: > strictly speaking this addition is not legal if output_begin > end. Maybe: > > if (end - begin > prefix.size()) > return false; > if (!std::equal(prefix.begin(), prefix.end(), begin)) > return false; > output->assign(begin + prefix.size(), end); > return true; > > > Or see my comment below. I guess uses of this function can be replaced with > StringPiece and string_util.h. I turned to use StringPiece and just removed this function. https://codereview.chromium.org/1081313002/diff/40001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:195: } On 2015/04/27 08:09:37, kinaba wrote: > nit: this loop can be factored out as a function, something like > SplitByHttpBr(). Done. https://codereview.chromium.org/1081313002/diff/40001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:212: for (size_t i = 0; i < lines.size() - 1; ++i) { On 2015/04/27 08:09:37, kinaba wrote: > You can utilize base/strings/string_piece.h. > > Let current_line = StringPiece(lines[i], line[i + 1]) with kHttpBr removed, then > IsLineEqual can be a simple comparison and GetAfterPrefix can be starts_with > followed by substr(). Done. https://codereview.chromium.org/1081313002/diff/40001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:239: const bool is_new_part = IsLineEqual(lines[i], lines[i + 1], header); On 2015/04/27 08:09:37, kinaba wrote: > I'm not sure how much we need to be pedantic, but according to RFC, you need to > accept a line with header followed by spaces. ("transport-padding" rule in RFC) > Probably we can ignore such cases. Let me handle this. Done https://codereview.chromium.org/1081313002/diff/40001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:257: responses.back().body = std::string(body_begin, lines[i]); On 2015/04/27 08:09:37, kinaba wrote: > then body will include the last CRLF (i.e., two chars before lines[i]). Is it > ok? (I'm guessing not.) Yes, it should not include CRLF. Thank you for catching it. https://codereview.chromium.org/1081313002/diff/40001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:265: break; On 2015/04/27 08:09:37, kinaba wrote: > nit: I slightly prefer these 4 lines to be put inside if(...||...) block, which > makes clearer that something is done here only when (is_new_part || > was_new_part) but is's up to your preference. Done. https://codereview.chromium.org/1081313002/diff/40001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:1293: kMultipartMixedMimeTypePrefix, &boundary)) { On 2015/04/27 08:09:37, kinaba wrote: > According to the RFC boundary field can be quoted (search "WARNING TO > IMPLEMENTORS") Done. https://codereview.chromium.org/1081313002/diff/40001/google_apis/drive/drive... File google_apis/drive/drive_api_requests.h (right): https://codereview.chromium.org/1081313002/diff/40001/google_apis/drive/drive... google_apis/drive/drive_api_requests.h:80: // string. It looks like: On 2015/04/27 08:09:37, kinaba wrote: > In addition to the RFC, you are assuming that the "Content" part includes the > HTTP response header. It'll be better noted. > > I also think that those detailed comments can be in the .cc file. > Here in .h, it is enough if it is mentioned that |response| is the HTTP response > body from Drive server as the reply of multipart request, and |parts| will be > the corresponding result of split. Done.
https://codereview.chromium.org/1081313002/diff/60001/google_apis/drive/drive... File google_apis/drive/drive_api_requests.cc (right): https://codereview.chromium.org/1081313002/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:162: piece[piece.size() - 1 - trim_size] == '\t')) { while(true){if(X){Y;continue;} break;} is equivalent to while(X){Y;} https://codereview.chromium.org/1081313002/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:211: if (content_type_piece[0] == '"' && content_type_piece.size() >= 2 && (or move the size check below before this [0] and [size-1] dereference https://codereview.chromium.org/1081313002/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:240: if (state == STATE_PART_HEADER && line == "") { line.empty() https://codereview.chromium.org/1081313002/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:262: if (state == STATE_PART_HTTP_HEADER && line == "") { line.empty() https://codereview.chromium.org/1081313002/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:264: body = ""; body.clear() or body = std::string() (in Chromium std::string() is preferred over "" if it is meant to be a std::string object) https://codereview.chromium.org/1081313002/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:294: if (state == STATE_PART_HTTP_BODY) { else { if(...) {} } => else if(...) {} ? https://codereview.chromium.org/1081313002/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:296: body.append(kHttpBr); If the body begins with multiple empty lines, this code will drop the lines.
Thanks again! https://codereview.chromium.org/1081313002/diff/60001/google_apis/drive/drive... File google_apis/drive/drive_api_requests.cc (right): https://codereview.chromium.org/1081313002/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:162: piece[piece.size() - 1 - trim_size] == '\t')) { On 2015/04/28 03:54:54, kinaba wrote: > while(true){if(X){Y;continue;} break;} > > is equivalent to while(X){Y;} Done. https://codereview.chromium.org/1081313002/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:211: if (content_type_piece[0] == '"' && On 2015/04/28 03:54:54, kinaba wrote: > content_type_piece.size() >= 2 && > > (or move the size check below before this [0] and [size-1] dereference Done. https://codereview.chromium.org/1081313002/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:240: if (state == STATE_PART_HEADER && line == "") { On 2015/04/28 03:54:54, kinaba wrote: > line.empty() Done. https://codereview.chromium.org/1081313002/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:262: if (state == STATE_PART_HTTP_HEADER && line == "") { On 2015/04/28 03:54:54, kinaba wrote: > line.empty() Done. https://codereview.chromium.org/1081313002/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:264: body = ""; On 2015/04/28 03:54:54, kinaba wrote: > body.clear() or body = std::string() > (in Chromium std::string() is preferred over "" if it is meant to be a > std::string object) Done. https://codereview.chromium.org/1081313002/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:294: if (state == STATE_PART_HTTP_BODY) { On 2015/04/28 03:54:54, kinaba wrote: > else { if(...) {} } => else if(...) {} ? Done. https://codereview.chromium.org/1081313002/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:296: body.append(kHttpBr); On 2015/04/28 03:54:54, kinaba wrote: > If the body begins with multiple empty lines, this code will drop the lines. Done.
lgtm https://codereview.chromium.org/1081313002/diff/80001/google_apis/drive/drive... File google_apis/drive/drive_api_requests.cc (right): https://codereview.chromium.org/1081313002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:225: const std::string terminator = "--" + boundary + "--"; nit: move the declaration of |header| and |terminator before |lines| (and after setting |boundary|.) Since theses two are strongly related to the boundary, the connection will be more clearer.
Thank you! https://codereview.chromium.org/1081313002/diff/80001/google_apis/drive/drive... File google_apis/drive/drive_api_requests.cc (right): https://codereview.chromium.org/1081313002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_api_requests.cc:225: const std::string terminator = "--" + boundary + "--"; On 2015/04/28 05:47:49, kinaba wrote: > nit: move the declaration of |header| and |terminator before |lines| (and after > setting |boundary|.) Since theses two are strongly related to the boundary, the > connection will be more clearer. Done.
The CQ bit was checked by hirono@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinaba@chromium.org Link to the patchset: https://codereview.chromium.org/1081313002/#ps100001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1081313002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_rel on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by hirono@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinaba@chromium.org Link to the patchset: https://codereview.chromium.org/1081313002/#ps120001 (title: "Fix compile error on windows.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1081313002/120001
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/35d010b6e1351dfad5a7c8872946d2fef6ad8545 Cr-Commit-Position: refs/heads/master@{#327646} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
